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

Add animation_started/finished signals to AnimationTree and fix time accuracy in StateMachine #70278

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Dec 19, 2022

In the past, many times we received reports of problems with AnimationTree not firing signals for AnimationPlayer, since AnimationTree only gets the AnimationPlayer's animation list and the AnimationPlayer does not process anything.

However, there seemed to be a demand for signals, and a common workaround was to add a MethodTrack at the end of the Animation. This has caused also some reported problems in the past with MethodTracks at the end not being processed, but this should be fixed with the recent overhaul of the animation process.

But it is not wise to leave workarounds in place, so adding a signal to the AnimationTree eliminates the need for such workarounds.

I am not sure if only the animation name is sufficient as a signal argument. This means that it is currently impossible to identify which NodeAnimation processed the Animation.

The reason for this is that it is the NodeAnimation that can fire the signal, and the NodeAnimation itself does not have a name, its label is held by the NodeBlendTree or NodeStateMachine map. Well, considering that we are replacing a workaround that has a MethodTrack at the end of the animation, I guess this is sufficient.

Closes #28311 Fixes #70318

animsignal.mp4

animation_tree_signal_test.zip

@TokageItLab TokageItLab added this to the 4.0 milestone Dec 19, 2022
@TokageItLab TokageItLab requested review from a team as code owners December 19, 2022 00:23
@TokageItLab TokageItLab force-pushed the add-animation-started-finished-signal-to-tree branch 7 times, most recently from 7ad7315 to 0377bf9 Compare December 19, 2022 01:18
@TokageItLab TokageItLab marked this pull request as draft December 19, 2022 02:06
@TokageItLab
Copy link
Member Author

Work in progress as there seems to be a problem with the StateMachine transition process.

@TokageItLab TokageItLab modified the milestones: 4.0, 4.1 Dec 19, 2022
@lufog
Copy link
Contributor

lufog commented Dec 19, 2022

Perhaps related #55423 (comment) (probably not 🤷‍♂️)

@TokageItLab
Copy link
Member Author

TokageItLab commented Dec 19, 2022

I expect this is probably due to the hack use of the context blend time = -1 in the StateMachine algorithm... Not related, see comment below.

@TokageItLab TokageItLab force-pushed the add-animation-started-finished-signal-to-tree branch 2 times, most recently from 3b9cdd3 to 24ccbfa Compare December 19, 2022 22:31
@TokageItLab TokageItLab marked this pull request as ready for review December 19, 2022 22:31
@TokageItLab
Copy link
Member Author

TokageItLab commented Dec 19, 2022

Okay, fixed. The problem was that the time in the StateMachine was not a double but a float, so it was not precise enough, and the fade end was determined too early1.

animation_tree_signal_test_statemachine.zip

By the way, there is a issue with the "animation_started" signal not firing in travel mode with no connection, which is the same problem as in #70318. Currently, travel mode transitions with no connection do not do a 0-seek, so start cannot be detected. We will need to pick up it later.

Footnotes

  1. When fade_blend == 1.0 in crossfades, the old animation must play with a blend value of 0 (CMP_EPSILON) in order for the end of the animation to be processed.

@TokageItLab TokageItLab modified the milestones: 4.1, 4.0 Dec 19, 2022
@TokageItLab TokageItLab changed the title Add animation_started/finished signals to AnimationTree Add animation_started/finished signals to AnimationTree and fix time accuracy in StateMachine Dec 19, 2022
@TokageItLab TokageItLab force-pushed the add-animation-started-finished-signal-to-tree branch 4 times, most recently from df37b5c to b8d97a9 Compare December 20, 2022 01:00
@TokageItLab
Copy link
Member Author

TokageItLab commented Dec 20, 2022

Included fix for #70318. Force playback from the head when teleporting travel (with no connection) from another state.

@TokageItLab TokageItLab force-pushed the add-animation-started-finished-signal-to-tree branch from ff5c61e to 4cd144d Compare December 20, 2022 16:20
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

Yes, this makes sense so as not have to rely on the MethodTrack workaround, and while I was a little concerned with the implication of issues surrounding floating point precision, moving to double makes sense from a consistency standpoint nonetheless, so this all seems fine to me.

@akien-mga akien-mga merged commit ecd895a into godotengine:master Dec 23, 2022
@akien-mga
Copy link
Member

Thanks!

@Valeryn4
Copy link
Contributor

I've been waiting for this for four years! Sorry...

@sirmike
Copy link

sirmike commented Jan 11, 2023

@TokageItLab Have you checked if this code can also be backported to 3.x?

@TokageItLab
Copy link
Member Author

TokageItLab commented Jan 11, 2023

Unfortunately, this depends on a bit large refactoring done in #69336, so it would be difficult to backport.

@TokageItLab TokageItLab deleted the add-animation-started-finished-signal-to-tree branch February 14, 2024 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants