Fix StopUsingBeatmapClock() applying adjustments to track it was supposed to stop using
#25253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pposed to stop using - Closes ppy#25248 - Possibly also closes ppy#20475 Regressed in e33486a. `StopUsingBeatmapClock()` intends to, as the name says, stop operating on the working beatmap clock to yield its usage to other components on exit. As part of that it tries to unapply audio adjustments so that other screens can apply theirs freely instead. However, the aforementioned commit introduced a bug in this. Previously to it, `track` was an alias for the `SourceClock`, which could be mutated in an indirect way via `ChangeSource()` calls. The aforementioned commit made `track` a `readonly` field, initialised in constructor, which would _never_ change value. In particular, it would _always_ be the beatmap track, which meant that `StopUsingBeatmapClock()` would remove the adjustments from the beatmap track, but then at the end of the method, _apply them onto that same track again_. This was only saved by the fact that clock adjustments are removed again on disposal of the `MasterGameplayClockContainer()`. This - due to async disposal pressure - could explain infrequently reported cases wherein the track would just continue to speed up ad infinitum. To fix, fully substitute the beatmap track for a virtual track at the point of calling `StopUsingBeatmapClock()`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Regressed in e33486a.
StopUsingBeatmapClock()intends to, as the name says, stop operating on the working beatmap clock to yield its usage to other components on exit. As part of that it tries to unapply audio adjustments so that other screens can apply theirs freely instead.However, the aforementioned commit introduced a bug in this. Previously to it,
trackwas an alias for theSourceClock, which could be mutated in an indirect way viaChangeSource()calls. The aforementioned commit madetrackareadonlyfield, initialised in constructor, which would never change value. In particular, it wouldalways be the beatmap track, which meant that
StopUsingBeatmapClock()would remove the adjustments from the beatmap track, but then at the end of the method, apply them onto that same track again.This was only saved by the fact that clock adjustments are removed again on disposal of the
MasterGameplayClockContainer. This - due to async disposal pressure - could explain infrequently reported cases wherein the track would just continue to speed up ad infinitum.To fix, fully substitute the beatmap track for a virtual track at the point of calling
StopUsingBeatmapClock().