AnimationPlayer play() does not play anymore when the same animation is playing.

[golddotasksquestions]

Godot version: 3.1.2 stable

OS/device including version: Win7

Issue description:
The AnimationPlayer won't interrupt itself anymore to start playing the same animation anew on play(). When another animation than the current one is asked to be played, interrupting works as usual.

This is highly problematic as it breaks a lot of code in action games or anything that needs immediate user feedback.
This completely breaks my main project.

Steps to reproduce:
Make an animation that lasts for a few seconds to easily see the effect, trigger the animation in your process or physics_process through input, run and try to make the animation play again while it is still playing.

This used to work, but does not anymore:

func _physics_process(_delta):
	if Input.is_action_just_pressed("LMB"):
		$AnimationPlayer.play("default")

Minimal Reproduction Project:
AnimationPlayer_cannot_interrupt.zip

[akien-mga]

The AnimationPlayer won't interrupt itself anymore

"anymore" compared to which version?

[akien-mga]

I tested 3.1, 3.1.1, 3.1.2 and the current master branch, they all give the same behavior described above.

If you want to reset the animation to restart it, you can use:

$AnimationPlayer.stop(true)  # restart = true is the default, so you can also omit it
$AnimationPlayer.play("default")

[golddotasksquestions]

Yes, those give me the same results as well. It's definitely working in 3.0.6:

AnimationPlayer_interrupt.zip

Damnyou Githup UI! Sorry for the accidental close, I was trying to close the comment I was writing, not the whole issue.

[akien-mga]

This was changed on purpose in 3.1, I'll push an update to the docs to clarify.

[golddotasksquestions]

@akien-mga What exactly is the purpose behind this?
If I call play() I would expect the animation to start playing.

[KoBeWi]

What exactly is the purpose behind this?

Probably code like this

func process(delta):
    if velocity.x != 0:
        $AnimationPlayer.play("walk")
    else:
        $AnimationPlayer.play("stand")

I haven't written code like this for a long time, so I didn't even notice this now works. Lots of people had problems with animations resetting.

[golddotasksquestions]

It defies logic. We take a lot of effort to teach people how _process() is called every frame and _physics_process() is called every physics frame and then ... this??!?
If play() is the "command" to start the AnimationPlayer playing an animation, and I call it in a function that is called ever frame, it SHOULD be triggered every frame.

Having to add stop every time you want The Animationplayer to start playing is completely illogical on the other hand and adds completely unnecessary code.

Don't get me started on stop (true). That makes about as much sense as the playback GUI godotengine/godot-proposals#167
Absolutely none at all.

This is so bad. And here I thought I already saw rock bottom with the AnimationPlayer "improvements" in 3.1.

[KoBeWi]

From my experience, you more often have to check if animation isn't played yet than want to restart it.

[bojidar-bg]

I remember making it so that $AnimationPlayer.current_animation = "walk" would play "walk" only if "walk" is not already playing in #15611 (and it worked). Seems like someone had a similar idea for .play("walk") in #25776.

Not sure what has to be done here. I feel that either play's behavior should be reverted to always reset or that current_animation could be removed / replaced with assigned_animation.

[KoBeWi]

play's behavior should be reverted

Pls no. The resetting could be made optional behavior if there's demand for it (which brings Godot proposals here). Either as an argument for play() or a new method.

[golddotasksquestions]

Play() worked perfectly fine before 3.1

If you call play(), it plays

If you run play in either _process() or _physics_process() you immediately learned what either of those functions do.

Now, you learn nothing and play() is behaving differently from any other function you would call in either of those processes.
Now you have to apparently stop() the AnimationPlayer before playing. What does .stop(false) even mean? Logically stop(false) means "play". But in 3.1 it's telling the AnimationPlayer to "pause" while stop(true) mean to actually stop!? Why would stop(false) then not be named .pause() then? Is the goal here to confuse people? Why would we need to tell something to stop anyway if we want to play it?? It's completely redundant!
It's not like it teaches you nothing anymore, it teaches the wrong things now.

Besides where is the discussion for this? This is so elemental to the AnimaitonPlayer behavior and it seems like it has been implemented without any prior discussion.

This should be reverted.

[AttackButton]

I use this a lot and I don't care that much. but the first time I tried to use it I found this a little bit confusing too. Maybe this could be a good choice to make a pause function instead of a stop(false) one, the new godot users could reap the benefits of this. And the play() could stay as the 'play' and the 'resume" actions.

Maybe changing the play("same_as_before") to play("resume_animation") (same as play()).

[akien-mga]

The compatibility breakage was done on purpose in #25776 for 3.1, and while I agree that it could have used more discussion, it doesn't make sense to backtrack and break compatibility again for 3.1 users.

The current behavior is consistent and #34128 documents it well. play and stop are used for both pause/resume and stop/restart actions. The previous 3.0.x behavior did not allow resuming an animation, so the current API provides a lot more flexibility. You can now even pause an animation half-way through, and resume playing in the reverse direction.

As mentioned, the previous behavior can be obtained by simply calling stop() (or seek(0)) before restarting with play().

For 4.0, we can discuss (in a proposal) changing the API again to make it more explicit. I think it would be worth having start(), pause(), resume(), stop() as explicit methods instead of the multi-behavioral play() and stop(). play() should maybe be removed altogether as it's ambiguous.

There should also be a cleanup around the current_animation and assigned_animation properties, the API could be overall much clearer if we redesign it from the ground up to properly cater to the different playback use cases (including solving the awkwardness of play_backwards()).

[akien-mga]

Maybe changing the play("same_as_before") to play("resume_animation") (same as play()).

That's a misunderstanding due to unclear documentation (fixed in #34128). "same_as_before" is not a keyword, it was just meant as a placeholder where you should write the same animation name as the currently playing animation, e.g.:

play("run")
... # some frames pass
stop(false)  # restart = false means that the current position is not reset to 0, so it's paused, not reset
... # do some stuff
play("run")  # either
play()  # or
# "run" animation resumes where it stopped

[Feniks-Gaming]

Listen @akien-mga and @KoBeWi I think you are focusing on a wrong problem here without seeing a bigger picture.

I agree with @golddotasksquestions the issue is not how often play() is used but how it defines the logic of the engine. If engine calls every function every frame inside the physics process. It should call every function every frame inside physic process INCLUDING play() function. It is vital that engine behaves in a predictable way that user can understand without spending couple of hours on discord/reddit and docs to find out that something is yet another exception to how you would expect engine to behave.

The issue isn't the fact that you want to use play() one way or the other the issue is the fact that docs lie to you when they say "process function calls each function every frame". The more exceptions we have the harder to intuitively use. The moment docs mislead you like this is the moment you stop trusting documentation and this is the moment when you may as well simply not have it.

The primary issue is not what is easier to use but what should be expected from the engine. If something is slightly less practical but promotes consistency and intuitive use it should stay that way. Logic of how the process function works demands that play() is called every frame just like any other function is.

[akien-mga]

@Feniks-Gaming You misunderstand the issue. play() is called each frame if you call it in _process or _physics_process. It's just that it doesn't do what you think it does. What it does is what the documentation now states, i.e. start the animation from where it was stopped, which can be:

• The first animation frame if it was never played, or finished playing, or was reset manually
• The last animation frame if it was never played and you use from_end = true (or play_backwards)
• The previous animation frame if it had been paused

If it's already playing, then it keeps playing. That doesn't mean that play() does not run, but its logic is that "the show must go on". What you want is restart(), and that's not what it is, as discussed and documented above. Currently there's no builtin restart behavior, so you have to stop or seek(0) if you want to restart from the first frame on play().

[Feniks-Gaming]

It makes sense but if that is a case play() should be renamed to something else. When call play() on AudioStreamPlayer it doesn't act the same way than calling play() on AnimationPlayer either every node that has play() function should behave consistently or if one of the nodes doesn't then it's function should be renamed.

[akien-mga]

@Feniks-Gaming Hence:

For 4.0, we can discuss (in a proposal) changing the API again to make it more explicit. I think it would be worth having start(), pause(), resume(), stop() as explicit methods instead of the multi-behavioral play() and stop(). play() should maybe be removed altogether as it's ambiguous.

[Feniks-Gaming]

I that would work spot on in my opinion. Why wait till 4.0 is it not something we could bring with 3.2.1 for example?

Is the issue created in proposal for this or do we need to make one?

[akien-mga]

I that would work spot on in my opinion. Why wait till 4.0 is it not something we could bring with 3.2.1 for example?

That would break compatibility, which should not happen in maintenance releases like 3.2.1. And there's little justification for breaking compatibility in 3.2 itself with regard to the new behavior in 3.1, as the new behavior is working properly and used in production by many.

Is the issue created in proposal for this or do we need to make one?

I don't think there is one yet. It should be opened in https://github.com/godotengine/godot-proposals, ideally with a proposal for what the new public API should be (i.e. the methods and properties that AnimationPlayer would have in 4.0, especially the ones that would be added/removed/renamed/modified). Edit: Having a full proposal is not a requirement for opening the issue though, I just mean that it's what the issue should eventually lead to, so that we have something concrete and well designed to implement.

[Feniks-Gaming]

I am happy to work on proposal maybe collaborate with @golddotasksquestions to make a draft of one.

[golddotasksquestions]

I that would work spot on in my opinion. Why wait till 4.0 is it not something we could bring with 3.2.1 for example?

That would break compatibility, which should not happen in maintenance releases like 3.2.1.

Sorry @akien-mga , but that's BS: This change also broke compatibility from 3.0.6 to 3.1. There was literally no discussion about this what so ever. Neither #25745 nor #17423 have a discussion about changing the play("animation") behavior to "only play when not playing". As far as I know there isn't even a issue requesting this change. The only trace related to play("animation") not starting to play this animation from the beginning is this comment:
#17423 (comment)
which however had no reaction and also only talks about the empty play(), not play("animation").

Added functionality is always good. Breaking things, or forcing confusing workflow onto people for everyday-use basic things, is not!

I'm all for discussing start(), restart(), pause(), resume(), stop() in a proposal, but until this discussion happened, this breaking change on play("animation") should be immediately reverted.

The added functionality of the absurd stop(true), stop(false) would be unaffected by reverting play("animation").

[golddotasksquestions]

As mentioned, the previous behavior can be obtained by simply calling stop() (or seek(0)) before restarting with play().

Who would want this and especially ... why???

Why would you want this:

AnimationPlayer.stop()
AnimationPlayer.seek(0)
AnimationPlayer.play("animation")

instead of this:
AnimationPlayer.play("animation")

It's great that we can pause animation now with one line of code instead of 3. But this has nothing to do with asking the AnimationPlayer to start playing an animation.

[akien-mga]

Sorry @akien-mga , but that's BS: This change also broke compatibility from 3.0.6 to 3.1.

Yes, and that's OK when justified. We can argue all you want about whether this was justified (and I called it in #25776 (comment), but it seems it was not added to the changelog in the end). Breaking compatibility between two minor version (3.0 and 3.1) is something we do when fixing bugs, but not in maintenance release, as in my comment that you're reacting to (3.2 to 3.2.1, or 3.0 to 3.0.6).

It is normal that when moving between major (3.x to 4.x) or minor (3.0 to 3.1) versions, some changes may be necessary to your projects to keep a behavior that has been modified in a compatibility breaking way in a later version. You may disagree with the justification for breaking compatibility, but it's done, and thousands of users have started projects with Godot 3.1 or later and are relying on the current behavior. Reverting it would break compatibility again, so it should only be done if there's a very strong reason to do so. I don't see one myself, as mentioned I think the current behavior makes sense and provides more features than what 3.0 offered. I agree that the API is confusing, but it's working as designed, so its design could be reviewed in the next big compatibility breakage (4.0).

Neither #25745 nor #17423 have a discussion about changing the play("animation") behavior to "only play when not playing".

You're still conflating play() with your expectation of it to only be a restart(). That's not what it's meant to be, and it being its behavior in 3.0 was considered a bug, and fixed. We want to support a workflow where users can pause and resume animations, which was not possible in 3.0.

Again, it's fine to disagree with the decision taken to use play() for both restart (existing) and resume (new feature), but it's not wrong per se.

AnimationPlayer.stop()
AnimationPlayer.seek(0)
AnimationPlayer.play("animation")

I wrote or, not and. You can use either stop() or seek(0).

But we're running around in circles here, I explained things very clearly already above on the comments you downvoted.

And BTW if you're here to call my explanations "bullshit", I'd prefer not to engage further with you.

[akien-mga]

You can try this on your example project to understand what is now possible and was not in 3.0:

	if Input.is_action_just_pressed("LMB"):
		# Restart animation
		$AnimationPlayer.stop()
		$AnimationPlayer.play("default")
	
	if Input.is_action_just_pressed("RMB"):
		# Pause/resume animation
		if $AnimationPlayer.is_playing():
			$AnimationPlayer.stop(false)
		else:
			$AnimationPlayer.play("default")

LMB will restart as you wanted, and RMB will let you pause and resume the animation, and restart it too once it finishes.

[Feniks-Gaming]

It may work but it's confusing as hell what does a a $AnimationPlayer.stop(false) even mean to any sane person it basically means opposite of stop which is play(). It's not clear at first glance of what is expected behaviour of this function.

[akien-mga]

It may work but it's confusing as hell what does a a $AnimationPlayer.stop(false) even mean to any sane person it basically means opposite of stop which is play(). It's not clear at first glance of what is expected behaviour of this function.

Again, I agree, which is why I suggest refactoring this in 4.0. Until then, reading the documentation or the auto-completion tooltip will tell you that the boolean argument is named "reset", which gives a good hint as to what stop(reset=false) might do.

[Feniks-Gaming]

I think we agree on functionality we disagree on a time frame proposed. 3.0 ->3.1 was happy to break functionality why is 3.1->3.2 breaking functionality a problem?

[golddotasksquestions]

I'm calling things "BS" when I feel like they are an expression of a double standard.
To me, having this breaking change of an elemental feature from 3.0.6 to 3.1 signed off without any prior discussion or even request and then asking us to file a proposal, have a discussion and then maaaaybe if someone kind enough finds time and energy to submit a PR, would fix things in a future version of Godot that who-knows-comes-when, is a double standard to me.
Closing this issue here without consensus makes it worse imho.

If you @akien-mga would also see the change of play("animation") as a unintended mishap, it would be not problem to revert it in 3.2. No change to the added functionality of stop() necessary.

The 3.1 play("animation") behavior is a lot harder to explain. It makes the process and physics_process harder to explain because it seemingly adds an exception to their behavior for those who cannot or did not read the source code. Explaining the AnimationPlayerand process/physics_process is something I do on a daily basis in the Godot community channels.

3.0.6 had seek() as well. There was nothing stopping anyone from using it as "resume from where you left of". It's even in the name.

This just adds another nail to the coffin that is a number of failed "improvements" to the AnimaitonPlayer in 3.1. It makes me sad. Because the AnimationPlayer in 3.0.6 though not perfect, was a great tool, and it has become more and more downgraded. :(