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 Deterministic blending with Dominant/Recessive doesn't have initial value even if there is no Discrete track #92126

Merged
merged 1 commit into from
May 29, 2024

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented May 19, 2024

Follow up #89389. Fixes #92127.

I listed the behaviors, some of which were problematic:

Deterministic RESET Track CallbackModeDiscrete Is init value appeared in animation with lacking some tracks
true None (but there is track in another animation) Dominant No, init value may be discrete
true None (but there is track in another animation) Recessive No, init value may be discrete

Indeed, the Reset track can be Discrete, but if there is no Discrete track in the other animations, it should be considered Continuous. This causes problems in Deterministic blending, where the results may be inconsistent as #92127.

With this PR, the Dominant/Recessive mode will first check for the presence of a Reset track, and if there is none, it will check for the presence of blended Discrete track. So the list will be:

Deterministic RESET Track CallbackModeDiscrete Is init value appeared in animation with lacking some tracks
true None (but there is track in another animation) Dominant Yes, if "there is no other discrete track blended" and "continuous track is blended or blend amount is zero" / But not so, it is No since discrete value should be take precedence
true None (but there is track in another animation) Recessive Yes, if "there is no other discrete track blended" and "continuous track is blended or blend amount is zero" / But not so, it is No since discrete value should be take precedence

@TokageItLab TokageItLab added this to the 4.3 milestone May 19, 2024
@TokageItLab TokageItLab requested a review from a team as a code owner May 19, 2024 16:32
@TokageItLab TokageItLab marked this pull request as draft May 20, 2024 02:10
@TokageItLab TokageItLab force-pushed the reset-dominant branch 3 times, most recently from 7edbb55 to 17a20e8 Compare May 20, 2024 04:10
@TokageItLab
Copy link
Member Author

TokageItLab commented May 20, 2024

I was organizing the documentation on Discrete mode, and since the #89389 was invading the role of the Deterministic option and generating confusion, I have reorganized the behavior.

This causes #89163 to recur, but that is the intended behavior to ensure Deterministic option result consistency. So #89163 needs to be controlled by turning off the Deterministic option.

@TokageItLab TokageItLab marked this pull request as ready for review May 20, 2024 04:14
@TokageItLab TokageItLab marked this pull request as draft May 20, 2024 04:19
@TokageItLab TokageItLab force-pushed the reset-dominant branch 2 times, most recently from e211abf to a827eb2 Compare May 20, 2024 05:30
@TokageItLab
Copy link
Member Author

TokageItLab commented May 20, 2024

Adding the is_init flag used by Dominant and Recessive instead of init_use_continuous to solve the issue overall.

I remember conversing with reduz a bit about this at the beginning of 4.0, but the problem with #92127 in the first place is that the reset animation does not play when the blend amount is 0.

The workaround is to either apply the reset animation even if the blend amount is 0, or to use a flag to apply the reset animation only once. This PR will use the former for Force Continuous and the latter for Dominant and Recessive. This will make the Dominant mode reset occur only once, so the problem in #89163 should not recur.

However, that init flag may not be very stable, since it changes depending on the timing of cache deletion in the absence of Continuous tracks. If real stability is needed, the timing of applying the Discrete value itself would need to be changed, but at this stage that change is too large.

@TokageItLab TokageItLab marked this pull request as ready for review May 20, 2024 05:34
@TokageItLab TokageItLab requested a review from lyuma May 20, 2024 05:41
@TokageItLab TokageItLab marked this pull request as draft May 20, 2024 15:08
@TokageItLab TokageItLab marked this pull request as ready for review May 20, 2024 15:53
@TokageItLab TokageItLab requested a review from a team as a code owner May 20, 2024 15:53
@TokageItLab
Copy link
Member Author

TokageItLab commented May 20, 2024

Now I make Recessive the default due to reevaluating the overall behavior.

In the past, we changed it once in #88492 because there was a problem with the Reset track taking precedence, but this should not be a problem now, and Recessive should be a better default because it does not have the flicker that can occur with Dominant, and it gives the same results as 3.x.

In there are two animations:

continuous_and_discrete.mp4

Force Continuous:

force_continuous.mp4

Dominant:

dominant.mp4

Recessive:

recessive.mp4

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

We're not able to do a good review. We trust Tokage.

@TokageItLab
Copy link
Member Author

TokageItLab commented May 28, 2024

I would like to include this in beta1 as this changes the default value, also consider the consistency with the blog post godotengine/godot-website#851.

@akien-mga akien-mga merged commit 20ad681 into godotengine:master May 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

In AnimationTree and AnimationPlayer, rapid crossfading in Dominant mode can make inconsistent results.
4 participants