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.

Sign up for GitHub

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

Jump to bottom

Add p_keep_state to AnimationPlayer::stop() #71619

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Add p_keep_state to AnimationPlayer::stop() #71619

merged 1 commit into from
Jan 18, 2023

Conversation

TokageItLab
Copy link
Member

Follow up #33733.

Add an option p_keep_state to AnimationPlayer::stop() to avoid state updates for 3.x compatibility and for versatility.

@TokageItLab TokageItLab added this to the 4.0 milestone Jan 18, 2023
@TokageItLab TokageItLab requested review from a team as code owners January 18, 2023 13:04
@TokageItLab TokageItLab changed the title Add p_keep_state to AnimationPlayer::stop() Add p_keep_state to AnimationPlayer::stop() Jan 18, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jan 18, 2023

Maybe the description could be updated to mention that stop() does not run method and audio tracks, so it might be preferred over seeking.

@TokageItLab TokageItLab force-pushed the add-keep-state-to-anim-stop branch from d4bc17a to adb300b Compare January 18, 2023 14:11
@TokageItLab
Copy link
Member Author

updated docs

@TokageItLab TokageItLab force-pushed the add-keep-state-to-anim-stop branch from adb300b to 156f3f7 Compare January 18, 2023 14:14
@TokageItLab TokageItLab force-pushed the add-keep-state-to-anim-stop branch from 156f3f7 to d16004f Compare January 18, 2023 16:05
KoBeWi
KoBeWi approved these changes Jan 18, 2023
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I feel like the docs could be clarified a bit more, like explicitly mention that play() after stop(true) will restart animation and there could be some code samples etc.

The PR looks ok though.

@TokageItLab TokageItLab requested a review from reduz January 18, 2023 18:14
@akien-mga
Copy link
Member

Seems fine to me, but does that mean that we practically invert the semantics for the stop() boolean flag between 3.x and 4.0? If so many users might get issues porting their projects and having their stop() and stop(true) calls misbehaving.

It's not blocking (and the breakage already happened in the previous PR), but I wonder what we could do to ease the transition.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 18, 2023

No, the stop(true) in this PR is the same as stop(true) in 3.x

@akien-mga akien-mga merged commit acd6244 into godotengine:master Jan 18, 2023
@akien-mga
Copy link
Member

Thanks!

@TokageItLab TokageItLab mentioned this pull request Jan 23, 2023
Merged
@TokageItLab TokageItLab deleted the add-keep-state-to-anim-stop branch February 14, 2024 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi approved these changes

@akien-mga akien-mga akien-mga approved these changes

@reduz reduz Awaiting requested review from reduz

Assignees
No one assigned
Labels
enhancement topic:animation
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@TokageItLab @KoBeWi @akien-mga