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

Apply "Remove Immutable Tracks" after post-import. #90064

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Mar 31, 2024

Reimplements "Remove Immutable" in ResourceImportScene::_post_fix_animations by comparing to the skeleton rest.

The old implementation took a shortcut, effectively optimizing animation tracks before the animation tracks were even added to the AnimationPlayer. However, this optimization led to incorrect results in cases where the skeleton bone rest was not equal to the initial bone pose. Therefore, it is necessary to delay removing animation tracks until after the correct rest pose is calculated in rest-fixer.

I pass false into GLTFDocument / FBXDocument APIs in order to preserve the original implementation (which is available at runtime) for compatibility. The codepaths are too different to offer a replacement with the new implementation. The old implementation of "remove_immutable_tracks" rather chose which tracks to keep before adding them in the first place (a shortcut), while the new implementation detects which tracks are immutable and removes them in the post import flow. While it is sort of duplicate functionality in two places, the code is not duplicated, and after basically a day of thinking about it, I was unable to come up with an approach for reconciling the two features of the same name.

Fixes #90031 (See the test files I uploaded to that issue)
Note that the position offset is incorrect on the imported animation tracks. I chose to split that off into a separate issue. See also

CC @TokageItLab

Reimplements "Remove Immutable" by comparing to the skeleton rest.
It is necessary to delay removing animation tracks until after the correct rest pose is calculated in rest-fixer.
Preserves the original implementation in the GLTFDocument / FBXDocument API for compatibility.
@akien-mga akien-mga merged commit 96a75d9 into godotengine:master Apr 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

#else
return gltf->generate_scene(state, (float)p_options["animation/fps"], (bool)p_options["animation/trimming"], (bool)p_options["animation/remove_immutable_tracks"]);
return gltf->generate_scene(state, (float)p_options["animation/fps"], (bool)p_options["animation/trimming"], false);
Copy link
Member

@TokageItLab TokageItLab May 14, 2024

Choose a reason for hiding this comment

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

I suspect that the original implementation of remove_immutable_tracks is completely ignored due to enforcing false here and causing the issue in #91893. I wonder if it needs to be enabled depending on the options as (bool)p_options["animation/remove_immutable_tracks"], in this case such as if the external pose is not being used as a reference for rest. Thoughts? @lyuma @fire

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That argument enables the old option which applies before silhouette fixer and hence will remove tracks inappropriately before silhouette fixer changes the pose. When doing retargeting, it is important for this to be false, and it can always be false.

The remove immutable tracks code in resource_importer_scene has code that should remove scale tracks if they match the rest pose.

Copy link
Member

Choose a reason for hiding this comment

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

But has this PR also changed the order of ApplyNodeScale and RemoveImmutableTrack when you changed the order?

@yythlj
Copy link

yythlj commented May 24, 2024

how to fix this problem on godot 4.3.dev6 or godot4.2?my project can not import any new mixamo skeleton now...

@yythlj
Copy link

yythlj commented May 24, 2024

when I click the import the animationplayer node on the import scene instance, the model disappear.And never occur on the scene

@lyuma
Copy link
Contributor Author

lyuma commented May 24, 2024

@yythlj My apologies for introducing the regression in this PR. We have solved the issue you describe in #92012

The fix was already merged and will be included in 4.3 beta which should be out shortly. Please be patient ;-)

@yythlj
Copy link

yythlj commented May 24, 2024

such a big bug how to continue project's work ...
cannot wait a minutes
image

@yythlj
Copy link

yythlj commented May 24, 2024

luckyly I has the backup.The bug will make all glb.import be fail and no fix forever

@yythlj
Copy link

yythlj commented May 29, 2024

It's incredible, after two months, the fix for this severe bug still hasn't been packaged.

@akien-mga
Copy link
Member

akien-mga commented May 29, 2024

@yythlj You were already asked to be patient. If you can't use 4.3-dev6 due to this, stay on 4.2.2. Or build from the master branch to have the fix until a new snapshot is published. You can even download latest artifacts for your platform from our CI.

Dev snapshots are what the name implies - snapshots from the development branch. If you start using them, you accept the risk that they might have bugs.

Please refer to the Code of Conduct for an overview of what kind of behavior is expected on this platform.

@yythlj
Copy link

yythlj commented May 29, 2024 via email

@akien-mga
Copy link
Member

According to the bug report, that shouldn't be the case. The bug was a regression in 4.3-dev and didn't affect 4.2.1.

Can you share a reproduction project?

@yythlj
Copy link

yythlj commented May 29, 2024

new proj on 4.2.1.stable maybe no problem.But my project now both error on the 4.2 and 4.3, because alreay use 4.3 open the proj and the tscn had changed.this state go back to 4.2 version take no use

@lyuma
Copy link
Contributor Author

lyuma commented Jun 1, 2024

@yythlj 4.3 beta is out. I would be interested to know if updating to 4.3 beta solves the issue.
(Note that if you have an existing .tscn, it might remember old values so double check with a new instanced scene or replay the animation.)

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.

Remove Immutable Tracks import option breaks some character animations
6 participants