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 regression in FBX import caused by Skeleton3D #41497

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Aug 25, 2020

A change in commit f7fdc87 changed the Skeleton3D "pose" property from PROPERTY_USAGE_EDITOR to PROPERTY_USAGE_NOEDITOR.
This should have had no effect, however it turns out assimp was assigning to the pose property.

This change adjusts the FBX import to only get_bone_rest/set_bone_rest, not set_bone_pose

This PR supersedes #41490 - In other words, rather than undoing the cause of the regression, this attempts to fix the root cause in the old assimp FBX importer.

I have tested this on a poorly hand animated FBX, and can confirm that the second part of this patch appears to fix animation issues.

A change in commit f7fdc87 changed the Skeleton3D "pose" property from PROPERTY_USAGE_EDITOR to PROPERTY_USAGE_NOEDITOR.
This should have had no effect, however it turns out assimp was assigning to the pose property.

This change adjusts the FBX import to only get_bone_rest/set_bone_rest, not set_bone_pose.
@lyuma
Copy link
Contributor Author

lyuma commented Aug 25, 2020

Some comments for @RevoluPowered : This change is on Master because this regression that caused this issue is not yet present in the 3.2 branch. I understand that a fix has a chance of conflicting with a forward-port of #39418 to master, but if the bug is left unfixed, we both risk having a starting state that cannot be tested or compared against... The other thing, is that #39418 still assigns to pose (skeleton->set_bone_pose on modules/fbx/data/fbx_skeleton.cpp line 107:

print_verbose("working on bone: " + itos(bone_index) + " bone name:" + bone->bone_name);
skeleton->set_bone_rest(bone->godot_bone_id, get_unscaled_transform(bone->pivot_xform->LocalTransform, state.scale));
skeleton->set_bone_pose(bone->godot_bone_id, get_unscaled_transform(bone->pose_node, state.scale));

In order to test the effect of this on the FBX rewrite, I started with 3.2 backport by @fire of the Skeleton Inspector (available at https://github.com/fire/godot/tree/skeleton-inspector-3-2 ), and then merged @RevoluPowered #39418 (from https://github.com/RevoluPowered/godot/tree/feature/fbx_final_stage ).

After testing this change has on the FBX rewrite + new Skeleton class, I can confirm that the bug is present in the above code, and again, the fix is to remove the "set_bone_pose" function call. Here are the some screenshots against the same (proprietary) FBX file I have been using to test:

Before:
image

After:
image

Hence, this work should still be relevant for the FBX rewrite when it comes time to port to master. I do not think this needs to be addressed in the 3.2 branch, at least not yet.

Copy link
Contributor

@RevoluPowered RevoluPowered left a comment

Choose a reason for hiding this comment

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

we should really look how this works with the new importer too but this is OK for the assimp one

@RevoluPowered
Copy link
Contributor

#40707 <-- 4.0 port of the FBX importer is here, but it's not got a lot of production fixes it needs from the 3.2 branch, I will update it soon once we resolve a bug found in the document parser.

@akien-mga akien-mga merged commit e968109 into godotengine:master Aug 25, 2020
@akien-mga
Copy link
Member

Thanks!

@lyuma lyuma deleted the assimp_set_pose_fix branch May 19, 2023 10:48
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