Skip to content

Change NodeOneShot fading to uses self delta instead of input delta #101792

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

Merged
merged 1 commit into from
May 19, 2025

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jan 19, 2025

Predict the time scale from the delta difference so that the fading time does not depend on the input AnimationNode's delta.

This breaks compatibility a bit because it changes the behavior of the existing fading time scaled by the downstream time scale. However, since the scaling is specific to NodeOneShot, this should bring it closer to the behavior of other AnimationNodes.

It is possible that a strange state may be produced if the timescale is changed during a fade, but since NodeTransiton and StateMachine should have the same problem, I judge that it is not a problem. The case where the timescale is changed during a transition in AtEnd with a fade is quite niche, IMO.

The problem of the fade time and end time being out of sync as in #87009, should no longer occur, but the fade duration will change, as explained below (bit change old behavior).

@AThousandShips AThousandShips changed the title Changed NodeOneShot fading so that it does not depend on input delta Change NodeOneShot fading so that it does not depend on input delta Jan 19, 2025
@TokageItLab TokageItLab changed the title Change NodeOneShot fading so that it does not depend on input delta Change NodeOneShot fading to uses self delta instead of input delta Jan 20, 2025
@TokageItLab TokageItLab requested a review from SaracenOne March 15, 2025 11:25
@TokageItLab TokageItLab force-pushed the oneshot-scale branch 2 times, most recently from f7abf18 to c075353 Compare May 17, 2025 22:53
@TokageItLab TokageItLab requested a review from lyuma May 17, 2025 22:58
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

Might be a small change in compatibility for specific users, but I would prefer to fix the jank.

If someone has problems with this change that can't be addressed by tweaking the fadein/fadeout time, then they can file a new issue and we can consider adding a setting to bring back the old behavior.

@lyuma
Copy link
Contributor

lyuma commented May 17, 2025

Discussed in a call with Saracen and Tokage and iFire, and we also reviewed the code, so we agree to merge this.

@TokageItLab
Copy link
Member Author

TokageItLab commented May 18, 2025

Supplemental:

This changing affects the case where there is a TimeScale downstream of the OneShot.

image

Until now, the OneShot's fade time was scaled by the downstream (left) TimeScale. This means that NodeOneShot had the problem of inverting the parent-child / master-slave relationship.

Transition and StateMachine do not have this problem.

image

This means that only OneShot was inconsistent, which is basically the change that should have been made as the bugfix.

In the case of the TimeScale is placed downstream of the OneShot, there are several possible ways to preserve the old behavior.

  • Explicitly divide the fade time by the downstream TimeScale
  • Change the insertion position of the TimeScale to upstream (right) of the OneShot

image

If someone has problems with this change that can't be addressed by tweaking the fadein/fadeout time, then they can file a new issue and we can consider adding a setting to bring back the old behavior.

If there are too many past projects that this would affect, we would consider this, but if this is to be made permanently optional, we need to consider whether to include a master-slave inversion in Transition and StateMachine as well, but at least we should not do that. If it were to be made optional, it would be an option with the deprecated flag like bool legacy_mode.

@TokageItLab TokageItLab moved this from Ready for review to Approved, Waiting for Production in Animation Team Issue Triage May 18, 2025
@Repiteo Repiteo merged commit 70fa7fb into godotengine:master May 19, 2025
20 checks passed
@github-project-automation github-project-automation bot moved this from Approved, Waiting for Production to Done in Animation Team Issue Triage May 19, 2025
@Repiteo
Copy link
Contributor

Repiteo commented May 19, 2025

Thanks!

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

Successfully merging this pull request may close these issues.

OneShots can't fade out StateMachine shots with no running animation
3 participants