Random repro: Godot 4: AnimatedSprite2D: Animation plays multiple times but calling animation_finished signal only once

[fuzhouch]

Godot version

4.1.2.stable.flathub [399c9dc39]

System information

Godot v4.1.2.stable (399c9dc39) - Freedesktop SDK 23.08 (Flatpak runtime) - Wayland - Vulkan (Compatibility) - Mesa Intel(R) Xe Graphics (TGL GT2) () - 11th Gen Intel(R) Core(TM) i5-11320H @ 3.20GHz (8 Threads)

Issue description

I find Godot 4 AnimatedSprite2D can repeatedly play an animation more than once, then calls animation_finished handler function once. It can be demonstrated with the following code sample below. A summary of how to trigger it:

1. $AnimatedSprite2D.play() is called in _process().
2. The nimation name (a variable) is changed outside _process().
3. I use animation_finished to capture the end of animation playback.

A set of gif files are attached below to compare the results. They are from same code but different runs. Note that the issue is not stably reproduced. When I got bug report, I ran 29 times, and got 5 case flashing twice, and 2 extemely wrong case that flashes 5 times.

Please pay attention to the flashes of gold (once vs multiple times) and printed message "STATUS_COLLECTED animation played ...". The message always play once.

The same behavior is identified on both Linux and macOS.

Good case demo - Animation is played only once.

Wrong case demo - Animation is played twice

Extremely wrong case demo - Animation is played multiple times.

A simplified code sample

func _process(delta: float) -> void:
    $AnimatedSprite2D.play(animation[action_status])

func _on_self_destroy_timer_timeout() -> void:
    action_status = STATUS_COLLECTED  # Wait for 3 seconds to trigger animation change (static -> flash)

func _on_animated_sprite_2d_animation_finished() -> void:
    if action_status == STATUS_COLLECTED:
        print("STATUS_COLLECTED animation played once - can't be guranteed")
        queue_free() # Call queue_free() when animation is finished.

Context

This issue is reported during the same migration project with godotengine/godot-docs#8372 from Godot 3 to 4. An experiment shows, that Godot 3 can guarantee to play STATUS_COLLECTED animation once and then call animation_finished handler once, but Godot 4 does not guarantee this: it can play animation multiple times, then call animation_finished handler once. Again, it seems an inconsistency between Godot 3 and Godot 4.

Impact to player: Test players felt confused because they thought the repeated animation some different/special kind of gold, so they raised a question to me that whether this is a speical update in game.

Steps to reproduce

1. Checkout project https://github.com/fuzhouch/godot-3to4-comparison/ to local, say ./godot-3to4-comparison.
2. Launch Godot 4.1.2. Open godot 4 project, ./godot-3to4-comparison/godot4/project.godot
3. From Godot 4 editor, Launch scene, demo_animation_sprite_2d_play_animation_bad.tscn

NOTE: The project is kinds of as minimal as possible. It contains multiple scenes, while each of which are presenting a single bug. Every scene is self-contained, and do not depends on each other.

The scene uses a 3-second timer to tigger problematic animation. So open the scene, wait for 3 seconds, then we can see the gold flashes and disappears.

Result:

• In good case, It should flash only once, but it randomly flashes multiple times.
• No matter it flashes once or multiple times, the animation_finished signal handler is called once. As expected. (Proof: the print() message is written to debug window only once)

A workaround:

Simplify move $AnimatedSprite.play() out from _process(), but call it in different signal callbacks.

As a comparison of Godot 3, use step below:

1. Launch Godot 3.5.3, then open Godot 3 project, ./godot-3to4-comparison/godot3/project.godot
2. From Godot 3 editor, Launch scene, Demo_AnimatedSprite2D_Play_Animation.tscn

In my experiment, Godot 3.5.3, the gold always flashes once, then animation_finished handler is called once. The same code also works on my production game.

Minimal reproduction project

• Project repository: https://github.com/fuzhouch/godot-3to4-comparison/

• Godot 4 project path (for reproducing issue): godot4/

• Godot 3 project path (for behavior comparison): godot3/

• The scene we use to reproduce the bug: demo_animation_sprite_2d_play_animation_bad.tscn

• The scene that shows workaround: demo_animation_sprite_2d_play_animation_good.tscn

• The scene (under project godot3, requires Godot 3.5.3) for behavior comparison: Demo_AnimatedSprite2D_Play_Animation.tscn

[fuzhouch]

Just to clarify: this is not super critical issue since I have workaround, but a question may be like this: Does this issue indicate it may lead to unexpected results when calling $AnimatedSprite.play() in _process() or _physics_process()? Note that the current tutorials also put $AnimatedSprite.play() in _process().

[AThousandShips]

My bad got it backwards, will take a look

[fuzhouch]

No problem my friend. Please kindly check my example project to get details.

[AThousandShips]

This is probably due to a bad sync between the frame time and the processing of the animation, if the processing happens in the wrong frame

Unsure how to approach this though

[fuzhouch]

Is it possible we compare the behavior of Godot 3 and 4 of AnimatedSprite code? In my experience, this code pattern works perfectly since March 2023 in my game project, which seems pretty stable.

[AThousandShips]

The behaviors aren't comparable as the 3.x doesn't restart like the 4.x code does, but there is a possible bug with the 4.x code, I am looking into it, but the 3.x code isn't relevant to this

[fuzhouch]

Sure understand. Please kindly let me know if more information is needed.

[AThousandShips]

For this I'm not exactly sure how to continue, there is a sync issue here that might be a bug, there might be the simple limitation that the signals are unreliable when calling play, or there might be a fix here

CC @TokageItLab

[TokageItLab]

Please summarize the issue. Is the problem "AnimatedSprite2D::animation_finished is not firing correctly", right?

[kleonc]

I couldn't reproduce the bug but indeed seems there's a logic problem. Specifically (the same issue for backwards animating): https://github.com/godotengine/godot/blob/4363ae34fa3f52223ae78ff2f2c187ab99827e3a/scene/2d/animated_sprite_2d.cpp#L196-L218 If during last frame processing frame_progress < 1.0 and to_process == remaining == (1.0 - frame_progress) / abs_speed then we'll end up with frame_progress == 1.0 (assuming no float error) and remaining == 0.0. Because of this, the iteration will break: https://github.com/godotengine/godot/blob/4363ae34fa3f52223ae78ff2f2c187ab99827e3a/scene/2d/animated_sprite_2d.cpp#L182 without handling the frame_progress >= 1.0 part. Such state would be handled on the next NOTIFICATION_INTERNAL_PROCESS.

But the issue is, in the meantime (until the next NOTIFICATION_INTERNAL_PROCESS) frame_progress can be changed. E.g. by calling play from _process(), like in the MRP. Because of the specific internal state mentioned above it can result in restarting the current animation: https://github.com/godotengine/godot/blob/4363ae34fa3f52223ae78ff2f2c187ab99827e3a/scene/2d/animated_sprite_2d.cpp#L473-L475 In such case e.g. animation_finished signal would be not emitted even though the end of a non-looped animation was reached while processing (frame == last_frame && frame_progress >= 1.0 was true).

[fuzhouch]

Hey folks, I may have a related question on a proper workaround before we have a fix.

As a developer who focus on action game, I highly rely on animation+status shift pattern to perform a sequence of actions combining animation and processing logic (example code below: imagine a "throw fireball -> backswing -> idle" cycle). It relies on stable animation_finished handler called in one animation playback, so I can shift status in signal handler.

var action_status: int = STATUS_IDLE
func _physics_process(_delta):
    $AnimatedSprite.play(status_to_animation(action_status))  # Rely on stable animation_finished handler called
     # More logic to handle attack interaction

func _on_AnimatedSprite_animation_finished() -> void:
    if action_status == STATUS_ATTACK_FIREBALL: # Status shifting in animation_finished
        action_status = STATUS_BACKSWING
        $SwordAttack_Area2D.set_collision_mask_value(CONSTANT_ENEMY, false)  # shifting both animation and action 
    elif action_status == STATUS_BACKSWING
        action_status = STATUS_IDLE

When this bug takes effect, the pattern above should be unstable. In my project, I can see multiple STATUS_ATTACK_FIREBALL animation performed quickly in my machine.

I'm now trying to use a new coding pattern, that I remove $AnimatedSprite2D.play() from _physics_process(), but use $AnimatedSprite.call_deferred("play", ....) in animation_finished handler. So far I haven't seen significant problems, but I'm not sure whether it's safe from engine layer.

Do you think the new pattern a proper workaround?

func _physics_process(_delta):
    # Don't call $AnimatedSprite.play() here. Only logic to handle attack interaction.

func _on_AnimatedSprite_animation_finished() -> void:
    if action_status == STATUS_ATTACK_FIREBALL: # Status shifting in animation_finished
        action_status = STATUS_BACKSWING
        $SwordAttack_Area2D.set_collision_mask_value(CONSTANT_ENEMY, false)  # shifting both animation and action 
        $AnimatedSprite.call_deferred("play", status_to_animation(action_status)) 
    elif action_status == STATUS_BACKSWING
        action_status = STATUS_IDLE
        $AnimatedSprite.call_deferred("play", status_to_animation(action_status)) 

[kleonc]

@fuzhouch Regarding the logic issue I've mentioned in https://github.com/godotengine/godot/issues/84250#issuecomment-1787503034 what's problemtic is calling AnimatedSprite2D.play when the animation reached its end but the animation_finished signal for it was not yet emitted. Meaning if you're already reacting to animation_finished signal then you shouldn't need to defer the call to play any further.

So as a workaround this should be fine:

func _physics_process(_delta):
    # Don't call $AnimatedSprite.play() here. Only logic to handle attack interaction.

func _on_AnimatedSprite_animation_finished() -> void:
    if action_status == STATUS_ATTACK_FIREBALL: # Status shifting in animation_finished
        action_status = STATUS_BACKSWING
        $SwordAttack_Area2D.set_collision_mask_value(CONSTANT_ENEMY, false)  # shifting both animation and action 
        $AnimatedSprite.play(status_to_animation(action_status)) 
    elif action_status == STATUS_BACKSWING
        action_status = STATUS_IDLE
        $AnimatedSprite.play(status_to_animation(action_status)) 

If this snippet still results in some issues that would mean there's also some other problem than what I've mentioned in https://github.com/godotengine/godot/issues/84250#issuecomment-1787503034.

[fuzhouch]

Thank you @kleonc!

I'm moving all my code to call_deferred in the past days. Luckily the code is not that big. Let me try to move some characters to direct calls and compare the results. Will let you know if I seen any differences.