Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add per-bone meta to Skeleton3D #87150

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Add per-bone meta to Skeleton3D #87150

merged 1 commit into from
Sep 17, 2024

Conversation

demolke
Copy link
Contributor

@demolke demolke commented Jan 13, 2024

  • 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
Copy link
Contributor Author

demolke commented Jan 13, 2024

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
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
Copy link
Contributor

lyuma commented Apr 27, 2024

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
@TokageItLab TokageItLab added this to the 4.4 milestone Aug 31, 2024
@demolke
Copy link
Contributor Author

demolke commented Sep 1, 2024

Actually playing with this it will need some more work, I'll switch it to draft.

@demolke demolke marked this pull request as draft September 1, 2024 07:48
@demolke demolke force-pushed the bones branch 3 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
Copy link
Contributor Author

demolke commented Sep 16, 2024

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 review from a team as code owners September 16, 2024 14:05
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM

editor/plugins/skeleton_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
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
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@demolke demolke deleted the bones branch September 17, 2024 15:15
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