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

Reset on Save doesn't work if there is no RESET animation especially for imported glTF #85936

Closed
ydeltastar opened this issue Dec 8, 2023 · 11 comments

Comments

@ydeltastar
Copy link
Contributor

ydeltastar commented Dec 8, 2023

Tested versions

v4.2.stable.official [46dc277]

System information

Windows 10.0.22621

Issue description

Skeleton3D bones constantly change transforms in the editor while working with animations and states in an AnimationTree/AnimationPlayer.
These changes are only for preview but they are saved in the scene's file every time adding unnecessary diffs to source control.

Sometimes the Skeleton3D pose breaks after editing externally and reimporting assets because the bone transforms weren't reset. I have to disable "EditableChildren" and reload the scene to fix the bones. (Separated issue #91211)

AnimationMixer's "Reset on Save" feature should affect Skeleton3D so only the rest pose is saved.

Steps to reproduce

  • Open main.tscn on the attached MRP.
  • AnimationTree should be playing the "walk_forward" state.
  • Save and make a commit.
  • Let the animation play and save the scene again.
  • Check the source control's diff to see that bone transforms changed in main.tscn.
  • The transforms will change every time you save the scene even if there aren't any changes.

Minimal reproduction project (MRP)

skeleton3d_mrp.zip

@xix-xeaon
Copy link

According to the documentation, it seems the "Reset on Save" feature requires a RESET animation. Your model doesn't have one. Neither do any of the models I have for my project, and I don't think it should be a requirement.

It could simply switch to the first animation for saving if there's no RESET animation. The correct animation will be selected by an AnimationTree (or possibly code) anyway. If one wants it to play the selected animation, "Reset on Save" wouldn't be used anyway.

Even without a RESET animation, it should at least reset the selected animation to the 0 time position.

@ydeltastar
Copy link
Contributor Author

ydeltastar commented Dec 23, 2023

It could simply switch to the first animation for saving if there's no RESET animation. The correct animation will be selected by an AnimationTree (or possibly code) anyway. If one wants it to play the selected animation, "Reset on Save" wouldn't be used anyway.

There is no need to rely on animations because Skeleton3D has a rest pose. It serves the same purpose as a reset animation storing the initial transformations for all bones.

@xix-xeaon
Copy link

@AThousandShips I can confirm that "reset on save" doesn't work for the MRP nor with models in my own project.

@Illauriel
Copy link
Contributor

There is no need to rely on animations because Skeleton3D has a rest pose. It serves the same purpose as a reset animation storing the initial transformations for all bones.

Btw, if you select the skeleton and click the Reset All Bone Poses button, does it return the skeleton to your desired state?
Screenshot 2023-12-25 170356

@xix-xeaon
Copy link

xix-xeaon commented Dec 25, 2023

@Illauriel If you first deactivate the AnimationTree, then go to the skeleton, and click the Reset All Bone Poses button then it does return the skeleton to a consistent state which is version control friendly. It does, however, not rest animations that aren't bones, which remain unfriendly. In the MRP the model is moved forward in the animation as well.

If you first deactivate the AnimationTree, then go to the models AnimationPlayer, and then pull on the animation timeline so it sets the 0 time then it returns the entire model to a consistent state. I don't think it completely works though since there may be animations with different tracks that have modified different state, since it would only reset its own tracks.

I suppose a solution could be to loop through the animations and set the 0 time for each track which hasn't been reset yet. Actually, I'm thinking animated state shouldn't even be saved at all when "Reset on Save" is used, but maybe that will have side effects, or is more complicated. The most important thing is that it doesn't change all the time.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 6, 2024

I think this is a documentation issue not a bug, since Godot does not currently have the ability to reset the animation without "RESET" animation.

I believe we can make it is possible to reset the value of an animation without RESET, but this is done for all trackpaths in the animation list. This can be more confusing since it is also done for animations that you are not using.

For example, if you have an object "Chair", and you mistakenly keyed it in the animation "forgotten_anim" in the AnimationList, then when you edit the "Chair" and reload the scene after saving, the poltergeist phenomenon will occur.

So if anyone write a proposal related to this, it might be better to have a property like force_reset_all_animations_in_the_list, which defaults to false.

@TokageItLab TokageItLab changed the title "Reset on Save" doesn't work for Skeleton3D's animations adding unnecessary diffs and breaking poses on reimport Reset on Save doesn't work if there is no RESET animation Jan 6, 2024
@xix-xeaon
Copy link

xix-xeaon commented Jan 6, 2024

I think we could look at this issue from a different perspective. If you create your own animations in Godot you can always add a RESET animation. But the issue OP is having, and that I'm having, refers to imported models. In this case you can't edit the animations (and you don't want to), and thus you can't add a RESET animation.

The issue would then have the title "Imported animations don't have a RESET animation causing the 'reset on save' feature to fail.". Now the problem is easy to solve, without conflicting with your example use case.

Add an option to generate a RESET animation when importing the model, resetting all tracks. A second PR (an enhancement and not necessary to solve this issue) could allow you to select which tracks to include in the reset, or even use a specific animation as the RESET.

Without a fix for this issue the "reset on save" feature really is broken in this case - it's not an issue with the documentation. In my project I'm currently holding off on adding AnimationTrees to imported models (basically every single character, monster etc) because of the mess it creates with diffs and the manual work required to sift through them or constantly reset everything manually.

@TokageItLab
Copy link
Member

I remember having a few conversations with Lyuma before about the need for the ability to generate RESET and Rest animations on import, but I don't remember there being a proposal yet. cc @lyuma

@TokageItLab TokageItLab changed the title Reset on Save doesn't work if there is no RESET animation Reset on Save doesn't work if there is no RESET animation especially imported glTF Jan 6, 2024
@TokageItLab TokageItLab changed the title Reset on Save doesn't work if there is no RESET animation especially imported glTF Reset on Save doesn't work if there is no RESET animation especially for imported glTF Jan 6, 2024
@and-rad
Copy link
Contributor

and-rad commented Jan 10, 2024

Something that could pass as a workaround for now is to just create an animation in the DCC with a single key keyed to the rest pose and name it RESET. I've successfully done that in Blender for character animations and the imported animation was correctly picked up as the RESET anim.

@lyuma
Copy link
Contributor

lyuma commented May 18, 2024

@ydeltastar Does the feature to import skeleton rest as RESET solve your usecase?

We implemented it at #89629 and it should be part of 4.3dev6

@ydeltastar
Copy link
Contributor Author

@ydeltastar Does the feature to import skeleton rest as RESET solve your usecase?

We implemented it at #89629 and it should be part of 4.3dev6

It solves the issue of bone transformations saving to file.
The other problem seems unrelated to animation and I created another issue for it (#91211) so this one is resolved.

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

No branches or pull requests

8 participants