Implement AnimationPlayer pause() and resume()

[madmiraal]

Salvage of #44345. #44345 was closed, because it was considered "redundant given #46191 has been merged". However, #46191 does not implement godotengine/godot-proposals#287, fix #27499 or fix #36279. Furthermore, pausing (and unpausing) the entire SceneTree is not a convenient or even realistic way of pausing and resuming an AnimationPlayer's playing animation.

As originally identified in #34125 and elaborated on in godotengine/godot-proposals#287, in AnimationPlayer:

Currently play() method doesn't adhere to single responsibility principle. If user calls play() while there is no animation playing it acts as start_animation() where as when user calls it when animation is already playing it acts as resume_animation(). This dual behaviour is not desired by users as it requires a condition statement in order to use start_animation functionality of play(). It is also inconstant across the engine as for example AudioStreamPlayer method play() method has only singular functionality of start_playing(). This is confusing to understand and breaks intuitiveness of the engine.

Similarly, AnimationPlayer:stop() has a parameter that defaults to true to either stop and reset the animation or just pause the animation. However, as identified in #27499, although stop(true) resets the playback position it doesn't reset the animation.

This PR:

• Implements two new functions in AnimationPlayer: pause() and resume(). pause() will pause a currently playing animation, and resume() will resume a paused() animation.
• Removes the redundant AnimationPlayer playback_active property.
• Renames AnimationPlayer::play() to start(), and also removes the play_backwards() method and the from_end parameter (uses a negative speed to determine this).
• Ensures that start() always plays the specified (or current animation) from the beginning regardless of whether or not the requested animation is the same as the current animation.
• Ensures that stop() also resets the animation.
• Updates the relevant documentation.

Closes godotengine/godot-proposals#287
Fixes #27499
Fixes #36279
Supersedes #33733

[goblinJoel]

I like splitting stop and play into stop, pause, start, and resume, but this does leave a question. After this change, what will be the best way to replay an animation if it's currently finished, resume an animation if paused, switch to and start an animation if it's not the current one, and do nothing if it's already current and playing?

In 3.4's docs, it's unclear to me if current_animation is cleared when the animation pauses or is stopped (but I think it must be cleared when the animation ends, otherwise it would be the same as assigned_animation). is_playing() is also ambiguous in 3.4, but your doc updates show it means the animation is actively moving forward. Would it be...

var name = "animation_i_want"
if player.assigned_animation != name:
  # switch and start
  player.start(name)
elif player.is_playing():
  # currently playing the anim already, so do nothing
  pass
elif player.current_animation_position <= 0:
  # stopped, but already on the correct anim, so start it
  # but would this even work, or would current_animation_position be null since is_playing() is false?
  player.start(name)
else:
  # we're not assigned a different anim, not playing, and not at the start, so we must be paused on the correct anim
  player.resume()

I'm actually having a hard time figuring out how you would do this. There's got to be a better way that I'm missing.

[madmiraal]

After this change, what will be the best way to replay an animation if it's currently finished, resume an animation if paused, switch to and start an animation if it's not the current one, and do nothing if it's already current and playing?

Desired Action	Command
Replay an animation if it's currently finished	player.start()
Resume an animation if paused	player.resume()
Switch to and start an animation if it's not the current one	player.start("animation_i_want")
Do nothing if it's already current and playing	pass

In 3.4's docs, it's unclear to me if current_animation is cleared when the animation pauses or is stopped (but I think it must be cleared when the animation ends, otherwise it would be the same as assigned_animation). is_playing() is also ambiguous in 3.4, but your doc updates show it means the animation is actively moving forward.

current_animation and assigned_animation are not changed by this PR. current_animation is a runtime parameter that is controlled and used by the Godot engine. If an animation is playing, it contains the name of the animation. When the animation is playing, it has the same value as assigned_animation. Ignore this property. Use assigned_animation to control the current animation assigned to the player. assigned_animation is a user parameter that can be used to assign the desired animation without changing whether or not the player is currently playing. It's normally used in the editor to set which animation to play.

Note:

player.start("animation_i_want")

is the same as

player.assigned_animation = "animation_i_want"
player.start()

Would it be...

I would suggest:

var name = "animation_i_want"
# Ensure player is not paused
player.resume()
if player.assigned_animation != name or not player.is_playing():
  # Start desired animation
  player.start(name)

[goblinJoel]

Thanks for the thorough response! That technique you suggest doesn't seem too bad. Would you mind adding info to your docs about what happens if you pause an animation that is already paused or resume one that is already playing, never started, or finished?

[madmiraal]

Updated the documentation for pause to:

Pauses the currently playing animation. To resume the animation again use [method resume]. To stop and reset an animation use [method stop].
[b]Note:[/b] Has no effect if the animation is not playing. See [method is_playing]

and resume to:

Resumes an animation that was paused. See [method pause].
[b]Note:[/b] Has no effect if the animation is not paused. It will not start an animation that was not playing or is finished. See [method start].

[lyuma]

I'd also be interested to hear the response to feedback that "you can now pause any node, so this may be excessive".

Specifically, what does this provide that node pausing does not?

If the node is paused, does it also freeze animation playback? (Do some forms of pausing cause animations to skip frames?)

[goblinJoel]

As more of a Godot novice, I would feel like using a generic node-pausing function (that I assume is like turning off processing?) is a hack for pausing an animation. I would expect that to have some sort of consequence besides pausing playback, since I'm not really thinking about pausing the player node so much as the animation it's currently playing. So having a pause method on the player to pause the animation is more reassuring. Plus I'm more likely to find it when looking through the animation player docs wondering how to pause.

[YuriSizov]

Plus I'm more likely to find it when looking through the animation player docs wondering how to pause.

This can simply be addressed in the description section of the node's documentation.

[lyuma]

As for "I would expect that to have some sort of consequence besides pausing playback," if the docs specifically state that it doesn't, would that assuage your concern?

Is pausing recursive?

[goblinJoel]

From the PR description, the original poster probably has some thoughts on that and has thought about it more. But as for myself...

I took a quick look at #46191, and it sounds like pausing in that context is basically the same as disabling processing. I didn't see at a glance whether pausing is applied to a single node (and any children, based on their process mode), or if it's global. I guess it must be per-node, otherwise the settings don't make much sense?

If the correct way to pause an animation was to stop processing on the AnimationPlayer (via the generic pause functionality), it would definitely have to be documented inside AnimationPlayer. Otherwise, why would one think that the right way to pause playback on an animation was to pause processing of the player and all its children (unless individually set not to inherit)?

If the docs specifically state that there aren't any consequences of pausing the AnimationPlayer node aside from pausing playback, and that that's the way you're supposed to do it, that would help my concern. But I do still think the concept of pausing an animation should be separate from pausing gameplay or node processing. It does sound like it's recursive by default, too, and whether that's a bug or a feature for someone who wants to pause an animation will vary.

[akien-mga]

We discussed this briefly in a PR review meeting.

We want to make sure we get this properly reviewed by all folks currently active on animation APIs (CC @godotengine/animation). There's also a lot of movement around AnimatedSprite right now from @Mickeon @dalexeev, and a lot of conflicting proposals for different parts of the various animation components.

I'm generally in favor of this proposal (splitting the current play and stop with boolean traps into four methods), but we need to make sure that we have a clear agreement and a clean API for 4.0 without remaining design issues that will lead to tons more proposal to change specific behaviors (e.g., without having looked at details, how does the new resume() work with backwards playing?).

[KoBeWi]

Seems like #44345 (comment) is still valid here.

[Mickeon]

Thanks for bringing this to my attention. Not a fan of changing play()'s behaviour, not a bit. Its current behaviour reduces code redundancy, by making sure the same animation isn't reset every time. It makes it much simpler and tidier to set up animation code, even if it's technically "improper".

[TokageItLab]

I am rather in favor of implementing pause(). Perhaps a stop() after pause() would reset the playback position, which is the expected behavior, but I think we should make some mockup diagrams and do a poll on how to combine this with reverse playback.

[Mickeon]

stop() is a very strong verb, and typically encompasses pause() already. It should imply pausing and resetting the animation back to 0 outright, which is typically the more common behavior, too.

I am in favour of adding pause(), which would simply be a tidy equivalent of setting playback_active to false (like CanvasItem's hide()). I do not see the point of introducing resume(). It makes things needlessly verbose, because as I've mentioned, I'm not in favour of changing play()'s current behavior, and if that were to be the case, with no parameter passed, it would behave exactly like a theoretical resume() would.

The last dilemma to figure out for me, and honestly this one surprises me, is the fact that AnimationPlayer's play() allows setting custom_speed, but why? playback_speed already exists, what is the necessity for it, is it a shorthand?

[dalexeev]

I also think that pause() should be the equivalent of playing = false, which is point 1 in the picture. But there is room for discussion about stop(). Which of the options (2 or 3) should be the default?

resume() looks unnecessary since play() can be called with no arguments. But it seems to me that we need to either remove the backwards parameter altogether (since speed_scale can be negative), or split the method into play() and play_backwards() (like in AnimationPlayer), since calling play() with no arguments means calling with backwards = false.

[Mickeon]

But there is room for discussion about stop(). Which of the options (2 or 3) should be the default?

I don't see any room for discussion, myself. If I could go back in time, I would set it like option 3. There's no reason it should behave like option 2, and if there is, it feels like an exception to the norm.

Think of the way your typical playback software (and hardware) works. There is Play/Pause, and then there is Stop:

• Play plays the playback;
• Pause pauses playback, if it is playing;
• Stop pauses playback and restarts it from the beginning.

Here's Vegas Pro's GUI:

Here's FLStudio12's GUI (and I just tried, it works exactly as I've described above).

Here's a stock image from a generic remote:

Aseprite does something interesting: by default, it toggles between Play/Pause, but it is possible to enable toggling between Play/Stop, instead. So, even with a layout and naming mix-up, the behaviour is essentially the same.

Mind everyone, it is fine to stray away from conventions, but the further you do, and the more spread these conventions are, the more difficult it becomes for users to adapt.

[dalexeev]

What if we make:

1, 2: pause(reset_frame_timeout: bool = false);
3: stop()?

It makes more sense to me.

[akien-mga]

Partially superseded by #71218.
For the rest, we'll have to assess each element individually after the 4.0 release and see what can be improved without breaking compatibility if possible.