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

Ensure methods skipped by AnimationPlayer::seek are not called #80708

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

garychia
Copy link
Contributor

Fixes #80640
The original code simply picked the key closest to the current time after the animation was seeked. This might accidentally call a method that had been skipped.

@garychia garychia requested a review from a team as a code owner August 17, 2023 10:12
@AThousandShips AThousandShips added this to the 4.x milestone Aug 17, 2023
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 17, 2023
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

This entire code will be refactored and gone soon, but it will be good for 4.2

@akien-mga akien-mga changed the title Ensure methods skipped by AnimationPlayer::seek are not called Ensure methods skipped by AnimationPlayer::seek are not called Aug 17, 2023
@akien-mga akien-mga merged commit 0511f9d into godotengine:master Aug 17, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 17, 2023
@golddotasksquestions
Copy link

golddotasksquestions commented Sep 12, 2023

I tested this with the latest dev build (Godot 4.2 dev4) and it $AnimationPlayer.seek(4.0) worked as expected, so did $AnimationPlayer.advance(4.0) Thanks a lot for the fix!

However one thing I noticed was that
$AnimationPlayer.seek(4.0, true)
seems to have the exact same effect as
$AnimationPlayer.seek(4.0, false) or
$AnimationPlayer.seek(4.0)

The documentation says:

If update is true, the animation updates too, otherwise it updates at process time. Events between the current frame and seconds are skipped.

At first I thought $AnimationPlayer.seek(4.0, true) therefore must be the equivalent of $AnimationPlayer.advance(4.0), meaning in my test project I should see "hi" being printed in the output. But that's not the case.
Then I thought maybe the animation is supposed to be visually different (updated), but that's also not the case.

So I don't know if this is a documentation issue which needs more clarification, or if $AnimationPlayer.seek(4.0, true) should actually behave like advance(), in which case there still is a bug.

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

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.

AnimationPlayer seek() does not skip Call Method Track Keys
6 participants