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 alters parameters during animation tree transition #82135

Closed
forager-pixel opened this issue Sep 22, 2023 · 9 comments · Fixed by #80813
Closed

RESET alters parameters during animation tree transition #82135

forager-pixel opened this issue Sep 22, 2023 · 9 comments · Fixed by #80813

Comments

@forager-pixel
Copy link

forager-pixel commented Sep 22, 2023

Godot version

4.1.1-stable

System information

Windows 10

Issue description

I have a simple animation tree that goes from a fade in (alpha modulation) on a tooltip to a bounce animation (y position translation). My reset track sets the alpha to zero. When the animation tree automatically transitions from the end of the "enter" animation to the "bounce" animation, it is resetting the Tooltip:modulate:alpha property based on the reset track.

It was my understanding that the reset track is played after saving your project, or loading the scene.. is it supposed to be played between animations? Perhaps I've misunderstood, but given that it uses the reset value for the modulate:alpha property even if I set it explicitly in the bounce animation, something is amiss.

I also get these errors in the editor about using call_deferred:
call_deferred_error

Steps to reproduce

Extract the attached reproduction project. Open animation_tree_bug.tscn. Click on "AnimationTree". Go to the AnimationTree editor, play "Enter" animation. It should then transition to the "Bounce" animation, and the alpha should immediately revert back to zero.

Minimal reproduction project

animation_tree_bug.zip

@bitsawer
Copy link
Member

At least the errors which might have caused this seem to be fixed in 4.2 dev5 and master at fe5b1c8. Can you try the latest dev5 and see if it fixes the issue for you?

@forager-pixel
Copy link
Author

forager-pixel commented Sep 22, 2023

Just built master to take a look myself, and can confirm the issue with the reset animation overriding the modulate parameter is present in both 4.2.dev5 and master.

I actually cannot consistently reproduce the errors I screenshotted (if that's what you're referring to), it seemed to happen when I was constructing the reproduction scene itself, modifying the reset track. At least so far on master I am not seeing those particular errors.

@forager-pixel
Copy link
Author

forager-pixel commented Sep 23, 2023

Looks related to code added in this commit: 17ce879#diff-47d98bbc16960ace8510b2cfa795da0e87601cc3f22a6f2e5155cfe01c750952.

Each track's value from the track cache is initialized per _process_graph call. The bezier value for tooltip:modulate is taken from the init_value stored in the cache that was generated at the start of the tree's playback. Init_value is never changed again since the cache remains the same. Given the second animation does not contain that modulation track to perform bezier interpolation, that initial cache value ends up being the applied value for the track.

Could there be some way to skip the track value initialization under certain conditions, or to ensure that we are not applying value updates to tracks that have not changed? Admittedly I don't know how this behaved before Godot 4 and this is my first foray into the codebase.. so I could be way off!

@forager-pixel
Copy link
Author

Made a band-aid fix, not sure of the best approach, so I'm not creating a PR until someone familiar with the animation tree can set eyes on it. I am not fond of adding a state variable to the track cache, but I am also not fond of iterating over animation states to ensure we only initialize the tracks used. forager-pixel@59763a6

@forager-pixel
Copy link
Author

I'm seeing at least one issue with this fix.. if you have xfade then suddenly it exposes a major issue with how the blend values are calculated. When blending to a value, if the "from" animation doesn't contain a value for that track, you then are only calculating the value from whatever fraction of the blend factor the "to" animation has - leading to erroneous values.

Perhaps philosophically there was an intent here for people to keep track of all animated values per animation state they create, but it puts burden on them to create more verbose animations than may be necessary, so I'm not too sold on that idea. In my mind why not keep states simple where possible, and allow the engine to transition using the actual previous values?

@bitsawer
Copy link
Member

I'm not very familiar with the animation system. @TokageItLab might be able to say if this is intentional behavior or a bug and if your linked fix makes sense.

@TokageItLab
Copy link
Member

TokageItLab commented Sep 25, 2023

Disabling the Deterministic option will be added by #80813(comment) should solve this problem (I saw that it means the same thing as the Write Default option in Unity).

However, there is a tradeoff: some AnimationNodes such as NodeAdd2/Add3/Sub2 will no longer be useful in Non-Deterministic blending, because the total blend weight will be forced to be normalized to 0 or 1; Deterministic blending accumulates total weight from RESET animation without normalization so can handle them correctly1, but Non-Deterministic blending does not2.

Footnotes

  1. For example, when the total blend weight is 0.1, it can use the [RESET animation (means init value)] + [Blended animation] * 0.1 as the result. However, doing nothing when the value is 0 is never allowed here; The result will be very different when the blend total of the previous frame is 1.0 and when it is 0.5. This means that the results will be inconsistent "depending on the manipulation speed of blend amount", it is too odd. https://github.com/forager-pixel/godot/commit/59763a6dbe9cb0c043476a4bbb42ffdd3440a2d2 is same with the code which was previously eliminated by us for that reason. See also #57675(comment)

  2. When the total blend weight is 0 does nothing to the object value, but if the blend weight is equal or greater than 0.001, it should be treated as 1.0 because the init value is not defined, the. And for consistency, total blend weights equal or greater than 1.001 should also be normalized by total weight to 1.0. e.g) 0.1 + 0.2 -> 0.1 / 0.3 + 0.2 / 0.3 = 1.0, 0.001 + 1.0 -> 0.001 / 1.001 + 1.0 / 1.001 = 1.0, -1.0 + 1.0 -> DIV/0 = do nothing (object values will not be changed)

@forager-pixel
Copy link
Author

forager-pixel commented Sep 25, 2023

Appreciate the responses @bitsawer @TokageItLab.

Does this mean that xfade will also be a useless property in non-deterministic blending? If I gather what you're saying it sounds like it will always set the blend factor to 0 or 1 when tracks dont match up. But, I dont fully understand why at least in a simple xfade case we cannot infer the initial value from the current value at the end of the "from" animation. I may be missing all the edge cases here, since animations nest in not so obvious ways sometimes.

@TokageItLab
Copy link
Member

TokageItLab commented Sep 25, 2023

Crossfade with missing tracks in Non-Deterministic blending is not performed. It stops working the moment it reaches 0 and when the value is 0.001 or greater, it is treated as 1.0. This is the same behavior as in Godot3.

This is because, as explained above, it has no default value, and although if you use some value as the default value, the result with and without crossfade (means that depending on the manipulation speed of blend amount) will be very different, and it will look like a complete bug.

If there are same tracks between the animations to be blended, xfade is done correctly. Normalization is done after calculating the individual blend weight ratios. For example, 0.001 + 0.001 will be normalized as 0.001 / 0.002 + 0.001 / 0.002 = 0.5 + 0.5 = 1 not 0.001 / 0.001 + 0.001 / 0.001 = 1.0 + 1.0 = 2.

If there is nesting, this will still get things right. So we can say this is a correct revision of Godot3's blending in that it correctly calculates the individual blend ratios.

@akien-mga akien-mga added this to the 4.2 milestone Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants