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

Fix gameplay skipping forward during resume operation #20014

Merged
merged 6 commits into from
Aug 31, 2022

Conversation

peppy
Copy link
Member

@peppy peppy commented Aug 29, 2022

Closes #19975.

I'm not precisely sure where this regressed in the changes, but it did. Rather than trying to figure out where the issue lies, I've just explicitly added this flow so we are aware of what is going on where and when.

Inline comment should explain this quite well.

@frenzibyte
Copy link
Member

I've took the time to debug through why this is occurring with the refactor and it turns out this is how things are supposed to work w.r.t. pause and resume:

  1. GameplayClock.Stop() is called due to user pause, setting IsPaused.Value = true and transforming the frequency to zero
  2. While the frequency is being adjusted, the clocks stop getting processed on Update
  3. GameplayClock.Start() is called due to user resume, seeking the track to the last processed frame time, which was at the moment Stop() was called.

This led to the realisation that FramedBeatmapClock actually always calls ProcessFrame regardless of the paused state, unlike the previous GameplayClockContainer version which processes the clocks only when in running state:

CleanShot 2022-08-29 at 15 07 36@2x

So while the solution here is valid, the new behaviour with FramedBeatmapClock makes me feel slightly uneasy about it potentially regressing other scenarios, but I can't think of any in my head, so we'll see how things go.

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

Also there seems to be leftover/debugging changes pushed in this PR.

Comment on lines 118 to 120
// Seeking the decoupled clock to its current time ensures that its source clock will be seeked to the same time
// This accounts for the clock source potentially taking time to enter a completely stopped state
Seek(GameplayClock.CurrentTime);
Copy link
Member

@frenzibyte frenzibyte Aug 29, 2022

Choose a reason for hiding this comment

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

As per my comment above, this is now pretty much pointless since the clock is always processed regardless of pause state.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed this for now.

@peppy peppy force-pushed the fix-resume-skip-forward branch from c4fd5c8 to 1bff540 Compare August 29, 2022 13:14
@peppy
Copy link
Member Author

peppy commented Aug 29, 2022

I figured that's how things were, although for whatever reason couldn't find the code in your screenshot.

peppy added 3 commits August 30, 2022 01:21
If we are going to continue to let the underlying clock process frames,
there needs to be a bit of lenience to allow the backwards seek on
resume (to play back over the freq ramp period).

The test is meant to be ensuring we don't skip the full offset amount,
so div10 seems pretty safe.
@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Aug 31, 2022
@smoogipoo smoogipoo merged commit 000412c into ppy:master Aug 31, 2022
@peppy peppy deleted the fix-resume-skip-forward branch September 5, 2022 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:gameplay next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unpausing gameplay skips forwards a little bit
3 participants