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

Fix FBX and glTF when root nodes are skeleton bones #90789

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Apr 17, 2024

This affects some FBX models in which "Import as skeleton bones" option is used. However, it theoretically can affect glTF or models without "Import as skeleton bone" enabled, but it would need the root of the model to be something that would generate a bone attachment. I'll try to file an issue, but I'm able to reproduce it with this suzanne model: suzanne_multimaterial.fbx.zip if the Import As Skeleton Bones is selected.

Fix two issues

  1. Set p_scene_parent to the skeleton to guarantee BoneAttachment3D nodes are added as a child of the active skeleton.
  2. Use get_owner() to go all the way up when calculating the root node in generate_scene

After this change, such models may look like:
image
image
Note how the skeleton is immediate under the root node, and contains bone attachment children.

@lyuma lyuma requested a review from a team as a code owner April 17, 2024 11:31
@akien-mga akien-mga added this to the 4.3 milestone Apr 17, 2024
modules/fbx/fbx_document.cpp Outdated Show resolved Hide resolved
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

The feature is good. Haven't fully reviewed the code implications. Added some code formatting nitpicks.

Set p_scene_parent to the skeleton to guarantee BoneAttachment3D nodes are added as a child of the active skeleton.
Use get_owner() to go all the way up when calculating the root node in generate_scene
@akien-mga akien-mga merged commit c295f18 into godotengine:master Apr 18, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants