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

Implement AnimationPlayer pause() and resume() #44345

Closed

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Dec 13, 2020

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.
  • Ensures that play() 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

@akien-mga
Copy link
Member

We discussed this in a PR review meeting and the general consensus is that it's good, but there are a few things to look further into:

  • AnimationPlayer has an active property, as well as is_playing method, which sound like they might also be related to attempts at pausing playback. This needs to be looked into some more to clarify.
  • AnimatedSprite also has a playing property which is now a duplicate for the pause and resume methods (in Implement AnimatedSprite pause() and resume() #44369).

@madmiraal
Copy link
Contributor Author

AnimationPlayer has an active property, as well as is_playing method, which sound like they might also be related to attempts at pausing playback. This needs to be looked into some more to clarify.

The active property prevents the AnimationPlayer from iterating, but it does not pause the playing caches.
The is_playing() method returns whether or not an animation has started, but not finished or (with this PR) been paused.

AnimatedSprite also has a playing property which is now a duplicate for the pause and resume methods (in #44369).

This is one of the issues raised in godotengine/godot-proposals#1994: the need for standardising the naming of bool properties and their getters and setters. Specifically, part 5 of the recommendation:

  1. If the bool property has a natural setter and "unsetter", they can be created as helper functions that map to set_(true) and set_(false). For example enabled should have enable() mapped to set_enabled(true) and and disable() mapped to set_enabled(false), paused should have pause mapped to set_paused(true) and resume() mapped to set_paused(false).

@madmiraal madmiraal force-pushed the implement-animation-pause-resume branch from 3ff5bdb to c4f7828 Compare December 31, 2020 09:44
@madmiraal
Copy link
Contributor Author

Updated to include two new commits:

  1. Removes AnimationPlayer playback_active property and updates the is_playing() documentation.
  2. Renames AnimationPlayer::play() to start(), and also removes the play_backwards() method and the from_end parameter (uses a negative speed to determine this).

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44630.

@akien-mga akien-mga requested a review from a team February 25, 2021 16:21
Copy link
Contributor

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

It looks fine, just few notes and a question (non stopping).

scene/animation/animation_player.cpp Show resolved Hide resolved
scene/animation/animation_player.cpp Show resolved Hide resolved
scene/animation/animation_player.cpp Show resolved Hide resolved
@madmiraal madmiraal force-pushed the implement-animation-pause-resume branch from 1ac8610 to 4b2982a Compare March 20, 2021 10:23
@madmiraal madmiraal requested review from a team as code owners March 20, 2021 10:23
@madmiraal
Copy link
Contributor Author

Rebased following merge of #47140.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 29, 2021

The removal of play() means that we no longer can continuously play an animation, i.e. you can't

func _process(delta):
    animationplayer.play("walk")

This is useful sometimes even if it's not really "correct" way to do animations (it reduces code complexity for simple objects).

@madmiraal madmiraal force-pushed the implement-animation-pause-resume branch from 4b2982a to ab7999b Compare April 5, 2021 17:32
Ensures that play() always plays the specified or current animation from
the beginning.

Also fixes stop() not resetting the animation.
- Removes play_backwards()
- Removes start() from_end parameter. If custom_speed is negative,
  it will automatically play the animation backwards from the end.
@madmiraal madmiraal force-pushed the implement-animation-pause-resume branch from ab7999b to 6667d91 Compare April 5, 2021 18:04
@madmiraal
Copy link
Contributor Author

Updated documentation using --doctool ..

@reduz
Copy link
Member

reduz commented Aug 30, 2021

This PR is now redundant given #46191 has been merged, so I am going to close it.

@madmiraal
Copy link
Contributor Author

#56645 is a salvage of this PR, because #46191 does not implement godotengine/godot-proposals#287, fix #27499 or fix #36279. Furthermore, pausing (and unpausing) the entire SceneTree is not a convenient way of pausing and resuming an AnimationPlayer's playing animation.

@Calinou Calinou removed the topic:core label Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants