Adjust clock usage in line with framework changes #24885
Conversation
Except `FramedBeatmapClock`, which is the high-effort one.
The new implementation of `DecouplingClock` will not mutate the underlying clock in any way (unless attempting to start it when approaching from a negative time value). This should be quite safe as a result.
This avoids it ever mutating the underlying track (aka attempting to start it). Resolves the one caveat mentioned in aeef92f.
It was causing issues with the new implementation.
This isn't required, but avoids creating a temporary `StopwatchClock` and generally just makes debug easier with less state changes.
See inline comment for reasoning. It's a bit complicated.
…lockContainer` source
peppy
force-pushed
the
clock-fix-attempt-2
branch
from
84e4e3f to
251a85d
Compare
|
Something is not right with playback on some maps. Testing with both PRs up to date as of writing and local framework. 2023-09-22.15-39-20.mp4https://osu.ppy.sh/beatmapsets/1601364#osu/3315708 |
|
Thanks, can reproduce. Of note, this doesn't happen when retrying. |
|
As usual, this was a regression from an "innocent" code refactor. I've fixed this (framework side) and will push in a bit after I add test coverage. |
|
Test failures look relevant here, I'll investigate. |
peppy
added
the
next release
I don't know what this test was trying to do, but it was wrong. Any offset which is applied should be invisible to the clock's final `CurrentTime` (and to the user).
There was a problem hiding this comment.
I've read the diff somewhat carefully, tested the workarounds removed mentioned in the OP, playtested some more. I can't see a reason not to merge this at present.
I am heavily leaning on the test coverage, and won't pretend to rule out the possibility of future issues. But this seems to be pretty good as is.
| @@ -110,6 +95,7 @@ protected override void LoadComplete() | |||
| userAudioOffset = config.GetBindable<double>(OsuSetting.AudioOffset); | |||
| userAudioOffset.BindValueChanged(offset => userGlobalOffsetClock.Offset = offset.NewValue, true); | |||
|
|
|||
| // TODO: this doesn't update when using ChangeSource() to change beatmap. | |||
There was a problem hiding this comment.
This appears to be an observation about the current state of affairs made in passing, rather than a TODO added after realising something in this pull broke this, so I'm ignoring it.
DecouplingClockand obsoleteDecoupleableInterpolatingFramedClockosu-framework#6001.Very much a draft, PRing for visibility and because all tests pass and game behaves roughly as expected.
Hacks removed which should be manually tested: