Skip to content

Add per-bone meta to Skeleton3D - #87150

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
demolke:bones
Sep 17, 2024
Merged

akien-mga merged 1 commit into
godotengine:masterfrom
demolke:bones

Conversation

@demolke

@demolke demolke commented Jan 13, 2024 •

Copy link
Copy Markdown
Contributor
  • Bones are not represented as Nodes and don't inherit get/set meta functionality from Object, so the skeleton has to carry the meta information similarly to how other per-bone properties are handled.
  • Adds support for GLTF import/export

This was split off of #86183 which only deals with import/export of metadata for nodes, meshes and materials - all of which do inherit from Object and therefore support metadata already.

Supporting the same for bones requires changes to Skeleton3D datastructure, so it's better to have the discussion separate.

image

@demolke

demolke commented Jan 13, 2024

Copy link
Copy Markdown
Contributor Author

I would like to get #86183 merged first, so that this can be rebased and will be only one commit. Did not figure out how to tell github that this PRs diffbase is #86183

@AThousandShips

Copy link
Copy Markdown
Member

There's no such system AFAIK, just make it clear in the PR description that it depends on that PR and the production team and reviewers will keep that in mind

@fire
fire requested a review from lyuma January 13, 2024 23:44
@demolke
demolke force-pushed the bones branch 4 times, most recently from 0a8c475 to 437d183 Compare January 22, 2024 21:07
@demolke
demolke force-pushed the bones branch 2 times, most recently from 24d5aed to a9153b9 Compare January 29, 2024 07:02
@demolke
demolke force-pushed the bones branch 4 times, most recently from 675783f to d1f18eb Compare February 9, 2024 20:22
@demolke
demolke force-pushed the bones branch 2 times, most recently from f66f248 to 8db5072 Compare February 18, 2024 17:47
@lyuma

lyuma commented Apr 27, 2024

Copy link
Copy Markdown
Contributor

I like this idea because it enables round-tripping of GLTF extras.

Because we have heavily modified the skeleton class late in the release cycle already, I'm a bit hesitant to add another big change to the skeleton (adding another property per-bone). I think @TokageItLab would need to review the skeleton changes to make sure both the serialization and the API changes ok, but I don't want to put more on the plate for 4.3 this late in the cycle.

I'd feel more comfortable trying the original PR #86183 in for this release cycle, understanding that we won't have full round tripping of skeleton bone extras for now... and depending on feedback and when we have time, we can also add the skeleton bone support in the future.

That said, the code looks reasonably simple if I read only the second commit (since it's stacked on the other PR)... if Tokage approves it and we think the timing works, I think it's ok.

@demolke
demolke force-pushed the bones branch 4 times, most recently from 7387b86 to 19d5c94 Compare August 30, 2024 20:56
@demolke
demolke force-pushed the bones branch 2 times, most recently from d95fdfe to 7e9b40a Compare September 1, 2024 10:04
@demolke
demolke force-pushed the bones branch 4 times, most recently from 2169c9b to 7a27247 Compare September 16, 2024 12:49
@AThousandShips

This comment was marked as resolved.

@demolke
demolke force-pushed the bones branch 6 times, most recently from 6867c87 to 6ca053f Compare September 16, 2024 14:03
@demolke

demolke commented Sep 16, 2024

Copy link
Copy Markdown
Contributor Author

I've uploaded a new version which fixes deleting of meta and also contains the UI portion of the change. I've separated the "Add Metadata" dialog logic from Node into a separate Dialog and linked it to both node meta and bone meta.

image

@demolke
demolke marked this pull request as ready for review September 16, 2024 14:05
@demolke
demolke requested a review from a team as a code owner September 16, 2024 14:05
@demolke
demolke requested a review from a team September 16, 2024 14:05
Comment thread editor/plugins/skeleton_3d_editor_plugin.cpp Outdated
Comment thread scene/3d/skeleton_3d.cpp Outdated

@AThousandShips AThousandShips left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread editor/plugins/skeleton_3d_editor_plugin.cpp Outdated
Individual bones are not represented as `Node`s in Godot, in order to support meta functionality for them the skeleton has to carry the information similarly to how other per-bone properties are handled.
- Also adds support for GLTF import/export
@akien-mga
akien-mga merged commit e72a70d into godotengine:master Sep 17, 2024
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

@demolke
demolke deleted the bones branch September 17, 2024 15:15
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
wangshucheng pushed a commit to wangshucheng/godot that referenced this pull request Aug 27, 2026
Shane-Gadsby pushed a commit to Shane-Gadsby/godotwebgpu that referenced this pull request Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants