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 AnimatedSprite pause() and resume() #44369

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Dec 14, 2020

As originally identified by in #31168,

Right now, the method stop() in AnimatedSprite is working as a pause (it is even stated in the documentation saying it doesn't reset the frame counter)

As suggested in godotengine/godot-proposals/issues/#287, to make AnimatedSprite2D and AnimatedSprite3D interfaces the same as AnimationPlayer (and ideally AudioStreamPlayer, VideoPlayer and Tween) this PR implements pause() and resume(), and updates the play() and resume() methods in AnimatedSprite2D and AnimatedSprite3D to be consistent with the changes made to AnimationPlayer in #44345 #56645 including:

  • Ensuring that play() always plays the specified (or current animation) from the beginning (or end if playing backwards) regardless of whether or not the requested animation is the same as the current animation
  • Ensuring that stop() also resets the animation
  • Updating the relevant documentation

Part of #16863

@madmiraal
Copy link
Contributor Author

Updated to include renaming play() to start() to align with the changes made to #44345.

@madmiraal madmiraal force-pushed the implement-animatedsprite-pause-resume branch from 572dfd3 to 71d514b Compare March 20, 2021 10:20
@madmiraal madmiraal requested review from a team as code owners March 20, 2021 10:20
@madmiraal
Copy link
Contributor Author

madmiraal commented Mar 20, 2021

Rebased following merge of #47064.

@madmiraal madmiraal force-pushed the implement-animatedsprite-pause-resume branch from 71d514b to 6df9e6a Compare April 5, 2021 17:26
@madmiraal
Copy link
Contributor Author

Rebased following merge of #47645.

@reduz
Copy link
Member

reduz commented Jan 6, 2022

Give the new process modes in master, I think there is probably not a lot of merit for having this? other than maybe usability.

@madmiraal
Copy link
Contributor Author

Pausing (and unpausing) the entire SceneTree is not a convenient or even a realistic way of pausing and resuming an AnimatedSprite's playing animation.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 9, 2022

Pausing (and unpausing) the entire SceneTree is not a convenient or even a realistic way of pausing and resuming an AnimatedSprite's playing animation.

That was the point of the pause rework. You don't need to pause entire scene tree, just set process_mode of a node to PROCESS_MODE_DISABLED to pause it. It does affect the children too, but AnimationPlayer with child nodes is rather rare.

@madmiraal madmiraal force-pushed the implement-animatedsprite-pause-resume branch from 6df9e6a to ca49be3 Compare January 9, 2022 21:48
@madmiraal
Copy link
Contributor Author

It's worth stressing that, as detailed in the OP this PR is not about creating pause functionality (stop already does that). It's about having four methods: start(), stop(), pause() and resume() that behave as expected and consistently across all players. See #56645 for the AnimationPlayer version.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 10, 2022

I understand stop() and pause(), but what is the point of start() and resume()? Both methods are covered by play(), it just depends on whether you stopped or paused the animation before playing.

@madmiraal
Copy link
Contributor Author

  • start(): Always plays the animation from the beginning (or the end if backwards is true).
  • resume() Continues playing the animation from where it was paused.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 10, 2022

I know what they do, but if you want to restart animation, you can just do stop() and then play(). Or pause() play() to resume from a point. start() basically duplicates stop(). It's redundant.

@clayjohn
Copy link
Member

clayjohn commented Aug 9, 2022

Discussed in PR review. It appears there is no consensus that the proposed API is any easier for users than the old API. Accordingly, the current approach should be documented more clearly to describe when it will restart vs just start/stop the animation.

@clayjohn clayjohn closed this Aug 9, 2022
@clayjohn clayjohn removed this from the 4.0 milestone Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants