Introduce FramedBeatmapClock (and use in gameplay flow)
#19828
Conversation
|
Test failures relevant, am investigating. |
Expose `IsCoupled` in `FramedBeatmapClock` for now to provide editor compatibility
…ets to the new time
peppy
force-pushed
the
no-gameplay-clock-gameplay-offset
branch
from
06ddb0d
to
b9e3397
Compare
|
I've rebased and pushed this with a heap of follow-up fixes, but a lot of them can be moved to their own PRs as they are not fixing issues on this branch, but issues that exist in Will wait to see how test failures look before moving forward. |
I've gone through these in detail and can't find an issue with the actual flow of things. For whatever reason, the new structure has a slightly higher delay, likely due to performing less `Seek` calls (previously a `Seek` was called after the clock start which may have been making this more accurate on the first `Player.Update`). I don't think it really matters that this is slightly off, but we'll see how this plays out.
Of note, I'm not sure whether the `IsPaused` check was meaningful, but it's not reimplemented in the new `FramedBeatmapClock`.
peppy
force-pushed
the
no-gameplay-clock-gameplay-offset
branch
from
37188ca
to
3eb1cda
Compare
|
Windows builds failed two times here: |
|
Latest commit should fix the windows test failure. I haven't tested against other tests for potential regressions, so we'll see how that plays out. Also appreciate manual testing against the latest change. Again, trying to get things working and passing tests as a first step just to set a baseline. Will probably still refactor a lot of this (in a hope to simplify) further down the track. |
| // Delay the start operation to ensure all children components get the initial seek time. | ||
| // Without this, children may get a start time beyond StartTime without seeing the time has elapsed. | ||
| // This can manifest as events which should be fired at the precise StartTime not firing. | ||
| SchedulerAfterChildren.Add(GameplayClock.Start); |
There was a problem hiding this comment.
So as far as I've debugged the issue myself, I understand the issue here to two-part:
GameplayClock.Start()processes the clock after starting the source.GameplayClock.Update()which runs before the content is updated, also processes the clock.
Since there is a non-zero time between the Start() call and either of the two ProcessFrame() calls, by the time children are updated the current time will always be non-zero. Some amount of time will have elapsed even if that amount is 0.0001ms.
FrameStabilityContainer immediately takes on this current time for the first frame and always lists zero elapsed time for this frame. And then of particular importance, samples care about if their actuation point occurred somewhere between the current frame and the last frame, based on elapsed time; which is zero; so there's no "last frame"; so there was no actuation.
I think this comment needs to explain it a bit better. In particular how "children may get a start time beyond StartTime" - which is those ProcessFrame() calls.
There was a problem hiding this comment.
Yeah, that's a pretty good summary. I've rewritten the inline comment, see if it reads better.
The tests are only meant to ensure that gameplay eventually starts. The case where failures can occur is where the master clock is behind the player clock (due to being in lead-in time). Because the test is running in real-time, it can take arbitrary amounts of time to catch up. If it took too long, the test would fail.
…ocks getting too far ahead
|
All test failures should be resolved. I've bumped framework here because the changes in this PR (especially the test amendments) are required for things to function correctly with the decoupled seek revert. Including conversation from discord discussing the remaining test issues resulting in fix at a546aa2.
|
osu.Game.Tests/Visual/Multiplayer/TestSceneMultiSpectatorScreen.cs
Outdated
Show resolved
Hide resolved
|
|
||
| #region Delegation of IFrameBasedClock to clock with all offsets applied | ||
|
|
||
| public double CurrentTime => finalClockSource.CurrentTime; |
There was a problem hiding this comment.
I think my confusion stems from "Seek operating in pre-offset time", @bdach is that referring to Seek internally seeking the track with time - totalAppliedOffset, which makes it pre-offset? or is it meant to say "accepting in pre-offset time"?
This is with the fact that I suppose GameplayClock.CurrentTime means post-offset time, while Track.CurrentTime means pre-offset time.
peppy
added
the
next release
|
AAAAArrrgh I forgot to actually test pausing/unpausing since it was previously "fixed" in this PR. But it's broken again... |
First piece of getting offsets more global. This just moves the handling to an isolated class which can be used in more contexts.
I originally had this working with a single global instance, but that turns out to be hard to make work with existing tests so I am now just adding this to the flow locally (with the editor to come next once this direction is agreed upon).
GameplayClock#19779OffsetCorrectionClockout ofMasterGameplayClockContainer#19836