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

gltf: Always add animation tracks matching rest pose. #72007

Closed
wants to merge 1 commit into from

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Jan 24, 2023

Addresses issues where an animation glTF file has an animation track with only one keyframe. Fixes #71525
As a side-effect, this could cause many unwanted position, rotation and scale tracks.

I understand this change is likely to be unpopular.
Furthermore, the import setting on AnimationPlayer to change position and scale import to "Never" seems to not work right now.

But I'm creating this PR to open up the discussion on the best way to solve the issue where the model comes posed in the first frame of the animation and the animation only has that one keyframe.

@lyuma lyuma requested a review from a team as a code owner January 24, 2023 23:03
Addresses issues where an animation glTF file has an animation track with only one keyframe.
Fixes godotengine#71525
As a side-effect, this could cause many unwanted position, rotation and scale tracks.
@TokageItLab
Copy link
Member

Can this be made optional? Or can it be implemented as an additional option in Optimizer?

@fire
Copy link
Member

fire commented Jan 26, 2023

If this is considered 4.x, it might not be able to be merged in 4 series, since this changes the layout of animations from sometimes TRS to always TRS for each path.

TRS means translation, rotation and scale.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 26, 2023

I recommend implementing it as an option like "Ignore Needless Tracks = false" in the gltf import option, like the "Trimming" option.

@TokageItLab TokageItLab modified the milestones: 4.x, 4.0 Jan 26, 2023
@lyuma
Copy link
Contributor Author

lyuma commented Jan 26, 2023

@TokageItLab the default of false will cause all tracks to be imported. and then change it to true to save a bunch of tracks but potentially run into issues like 71525

By the way, we couldn't get position and scale tracks to be excluded when setting them to Never in the AnimationPlayer import options. Do you have any insight into why the Never option has no effect? I think users will probably want a way to exclude most position tracks (though I might want to keep hips)

Perhaps the "ignore needless" can be by track type? so I can keep needless rotation tracks but ignore them on position and scale?

@TokageItLab
Copy link
Member

"ignore needless" simply imports a track using the previous code (determines if the track has the same value as rest).

As for the IfPresent option, #63465 may be a hint. Well, it would be fine to have it in all TRS for consistency.

@lyuma
Copy link
Contributor Author

lyuma commented Jan 26, 2023

@TokageItLab the issue I have is setting them to import "Never" still imports them. Do you know why that is?

@TokageItLab
Copy link
Member

TokageItLab commented Jan 26, 2023

ANIMATION_IMPORT_TRACKS_IF_PRESENT_FOR_ALL is supposed to be an option that will fill in all tracks with each other so that no tracks are missing.

Isn't the problem that removes the tracks which match the rest before post import process?

@TokageItLab
Copy link
Member

If my understanding is correct, the track optimizations are done in 3 places, GLTFImporter / ResourceImporter / Retargeter.

This PR problem is to completely remove the GLTFImporter optimization. So, you need to either allow the ResourceImporter process to do this rest-match optimization, or make this PR itself optional.

@fire
Copy link
Member

fire commented Jan 26, 2023

I don't think it's the job of the importers to do the optimization. The job of importers is to get perfect output.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 26, 2023

I don't think it's the job of the importers to do the optimization. The job of importers is to get perfect output.

Make sense.

@fire @lyuma Can you port this rest-match optimization into ResourceImporter?

@TokageItLab
Copy link
Member

Hmmm, I looked into it, but I think it is still necessary to add an RemoveImmutableTracks option to the glTF importer, as well as the Trimming option.

This is because SceneImporter (PostImporter) has a lot of work to do to associate animations with skeletons.

I have done it with Retarget, but this Rest Match Optimization needs to be available not only to Humanoid, so the most reasonable solution is to add the option to the glTF Importer.

@akien-mga
Copy link
Member

Superseded by #72342.

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.

Importing Animation to Identical Rig Breaks It
5 participants