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

AnimationPlayer doesn't discards track cache immediately after changing the animation #91904

Open
reimgrab opened this issue May 13, 2024 · 4 comments

Comments

@reimgrab
Copy link

Tested versions

  • Reproducible in 4.2.2.stable
  • Not reproducible in: 3.x, 4.1.4.stable

System information

Godot v4.1.4.stable - Ubuntu 22.04.4 LTS 22.04 - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1050 Ti (nvidia; 535.171.04) - Intel(R) Core(TM) i5-6600 CPU @ 3.30GHz (4 Threads)

Issue description

Playing an animation ("Anim2") in a method called via call method track ("Anim1") and using advance(0) (like described in the docs to update immediately) still plays the calling animation ("Anim1"). This results in an endless loop causing a stack overflow.
This worked fine in 3.x and 4.1.4.

Steps to reproduce

Create 2 animations ("Anim1" and "Anim2")
Add a call method track to the first and call the following method:

func call_track() -> void:
	$AnimationPlayer.play("Anim2")
	$AnimationPlayer.advance(0)

Minimal reproduction project (MRP)

animationplayer_bug.zip

@reimgrab reimgrab changed the title Animationplayer does not immediately change tracks on .play() Animationplayer does not immediately change tracks on .play() (regression) May 13, 2024
@TokageItLab
Copy link
Member

TokageItLab commented May 14, 2024

Can't reproduce in 4.2.2, the print results look like follows, I think it is correct:

1 called
2 called

I remember early 4.2 had similar bug but I fixed it in #85221 so please make sure your version again.

@reimgrab
Copy link
Author

Sorry, I somehow missed that it works correctly for the 'deferred' call method mode. It only breaks for the 'immediate' mode.

Godot v4.2.2.stable.mono - Ubuntu 22.04.4 LTS 22.04 - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1050 Ti (nvidia; 535.171.04) - Intel(R) Core(TM) i5-6600 CPU @ 3.30GHz (4 Threads)

@TokageItLab
Copy link
Member

TokageItLab commented May 15, 2024

I could confirm this, but I think this is a fairly rare use case and extremely near by specification changing instead of bug, so I don't know if we can support this. Also I remember making some fixes to the process immediately after playback in AnimationPlayer, so it might be no longer be needed the advance(0) hack after 4.2. Please check if you realy need to do that.

The reason for this issue is that since 4.2, it keeps a track cache for all animations, so to reproduce the past behavior, just discard the cached method once, but this may corrupt the crossfade.

I will look into whether play() can destroy the in-process track cache if there is no fade there, but the workaround available to you now is to explicitly destroy the cache by firing stop() before the animation changes, as shown in the code below.

func call_track() -> void:
	print("1 called")
	$AnimationPlayer.stop(false)
	$AnimationPlayer.play("Anim2")
	$AnimationPlayer.advance(0)

BTW, it is not recommended to do this in immediate mode in the first place. The reason is that if there are other keys in the same position as the method track, they may or may not be processed, depending on the order in which they are iterated within the method track core, which is out of the user's control.

@TokageItLab TokageItLab changed the title Animationplayer does not immediately change tracks on .play() (regression) AnimationPlayer doesn't discards track cache immediately after changing the animation May 15, 2024
@reimgrab
Copy link
Author

The real project is more complicated, the called method does a lot more and the change to a new animation is of course conditional, depending on intersection tests which must be run during physics frames.
Perhaps I do not understand immediate mode correctly but I use it with process mode physics to make sure that everything is happening during physics frames when needed.
This is a port of a Godot 3.5 2D pixel art project. I cannot recall the details but I had to use this technique to avoid incorrectly failing intersection tests or a wrong sprite frame being shown or both.
I do not think that this is a rare use case but perhaps it is.
I, too, found the workaround with calling stop() and it's good enough.
Nonetheless it is unfortunate, that the user needs to know about internals when deciding to use deferred or immediate mode.
That other keys may or may not be processed should seldom be a problem as you want immediate change to a new animation anyways.
I understand that it may be difficult to make this work with the new animation system so if it cannot be done, it cannot be done.
But at least it should be mentioned in the documentation that advance(0) is not safe in conjunction with immediate mode and that you manually must destroy the cache. And while we are at it perhaps an explanation for the deferred mode if "after events are processed" means during physics frames if process mode is physics.
Thanks for your time

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

No branches or pull requests

3 participants