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

Avoid infinite recursion between AnimationPlayer and AnimationMixer #98704

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matheusmdx
Copy link
Contributor

In theory AnimationPlayer.current_animation shouldn't be setted directly but in normal cases that don't cause any problem unless the animation key set current_animation to an empty string, in this case will start an infinite recursion between AnimationPlayer calling AnimationMixer::_blend_process and AnimationMixer setting the current animation to a empty string which will never stop until the engine crashes. This fix makes AnimationMixer ignore these animation keys (that would do nothing anyways) and avoid the crash.

Fix: #98076

@matheusmdx matheusmdx requested a review from a team as a code owner October 31, 2024 16:35
@matheusmdx matheusmdx force-pushed the avoid-anim-player-infinite-recursion branch from 8dcc11a to 004a24a Compare October 31, 2024 16:38
@TokageItLab
Copy link
Member

It looks like the patch is too specialized for special cases IMO. How about separating set_current_animation into internal and exposed methods, and not allowing the exposed method to have an empty animation name?

@matheusmdx
Copy link
Contributor Author

@TokageItLab I modified the pr to apply the changes u suggested, can you check again?

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

I believe this is a clean way of fixing the issue.

I do have one concern. Looking at the summary of the issue #98076, does it only occur with an empty character and not when “[stop]” is set? If we can confirm that, I think this can be merged.

@matheusmdx matheusmdx force-pushed the avoid-anim-player-infinite-recursion branch from dd9d0b0 to ee9d0cc Compare November 1, 2024 17:49
@matheusmdx
Copy link
Contributor Author

Indeed the ["stop"] also triggers the issue, updated again and now i think is ready to merge

@@ -942,6 +936,19 @@ void AnimationPlayer::_rename_animation(const StringName &p_from_name, const Str
}
}

void AnimationPlayer::_set_current_animation(const String &p_animation) {
if (p_animation == "[stop]" || p_animation.is_empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (p_animation == "[stop]" || p_animation.is_empty()) {
if (p_animation.is_empty() || p_animation == "[stop]") {

Prefer putting the cheaper condition first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@matheusmdx matheusmdx force-pushed the avoid-anim-player-infinite-recursion branch from ee9d0cc to 4f386a7 Compare November 1, 2024 18:02
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

Indeed the ["stop"] also triggers the issue

In that case, we need to think about a different approach. This is because a set of "[stop]" could be called from the inspector.

BTW, is it a problem if stop() is called in a method track? How about a different approach, such as calling clear_cache() with deferred inside stop()?

@matheusmdx
Copy link
Contributor Author

BTW, is it a problem if stop() is called in a method track?

Nothing happens, no crash but also the animation don't stop.

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.

Track of Current Animation Crash
3 participants