Add new DecouplingClock and obsolete DecoupleableInterpolatingFramedClock
#6001
Conversation
…y with new assertions
…urce while stopped
|
Generally this seems okay aside from the above review, which is mostly minor quibbles, but I'll highlight one point in particular:
I'm not sure if it's just me, but in a way, to me, reading this implementation, I have this vague feeling that I'm not necessarily making value judgements on that yet, but it feels like a potential rake one may step on. |
| decouplingClock.ChangeSource(secondSource); | ||
| decouplingClock.ProcessFrame(); | ||
|
|
||
| Assert.That(secondSource.CurrentTime, Is.EqualTo(second_source_time)); |
There was a problem hiding this comment.
Was expecting this to be Assert.That(decouplingClock.CurrentTime, ...). Maybe both should be asserted here?
There was a problem hiding this comment.
Good catch, this was a typo. Also regressed at some point in my changes, so I've fixed it by transferring the current time value.
| [Test] | ||
| public void TestSeekPositiveWhileDecoupling() | ||
| { | ||
| decouplingClock.AllowDecoupling = true; | ||
| Assert.That(decouplingClock.Seek(1000), Is.True); | ||
| decouplingClock.ProcessFrame(); | ||
|
|
||
| Assert.That(source.CurrentTime, Is.EqualTo(1000)); | ||
| Assert.That(decouplingClock.CurrentTime, Is.EqualTo(1000)); | ||
| } |
There was a problem hiding this comment.
DecouplingClock states it allows seeking beyond the seekable range, but the source clock in this case has a range (0, +inf) so it isn't testing that case like TestSeekNegativeWhileDecoupling is.
There was a problem hiding this comment.
I'm not sure why I didn't notice nothing ever actually sets TestClockWithRange.MaxTime to anything non-default...
| } | ||
|
|
||
| [Test] | ||
| public void TestForwardPlaybackOverZeroBoundary() |
There was a problem hiding this comment.
There should also be a test that shows the decoupled clock is able to keep running beyond the seekable range - the inverse of TestBackwardPlaybackOverZeroBoundary.
| public bool IsRunning | ||
| { | ||
| get | ||
| { | ||
| // Always use the source clock's running state if it's running. | ||
| if (Source.IsRunning) | ||
| return isRunning = true; | ||
|
|
||
| // If we're allowed to be decoupled, maintain our internal running state. | ||
| if (AllowDecoupling) | ||
| return isRunning; | ||
|
|
||
| // Otherwise we're definitely not running. | ||
| Debug.Assert(!Source.IsRunning); | ||
| return isRunning = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe just me, but it weirds me out that a getter is mutating the value of isRunning.
Is this code particularly important in that it needs to be referenced every frame for the clock to be in a good state?
For instance, and I'll say ahead of time I haven't tested this, but suppose:
- Decoupling is not allowed
- The decoupling clock is
.Start()ed (at 0) - The clock moves beyond
trackLengthat which point the source clock stops - From here I assume
ProcessFrame()becomes a no-op, butisRunningremainstruein an internally-decoupled state. - A
.Seek()to 0 on the decoupling clock would then re-start the track. This operation should (if my theory is correct) change behaviour depending on whether theIsRunningproperty has been referenced.
Is this expected behaviour?
There was a problem hiding this comment.
Do we want to change the implementation to match InterpolatingFramedClock for consistency?
diff --git a/osu.Framework.Tests/Clocks/DecouplingFramedClockTest.cs b/osu.Framework.Tests/Clocks/DecouplingFramedClockTest.cs
index a77817f0c..9025b1cd2 100644
--- a/osu.Framework.Tests/Clocks/DecouplingFramedClockTest.cs
+++ b/osu.Framework.Tests/Clocks/DecouplingFramedClockTest.cs
@@ -35,6 +35,7 @@ public void TestStartFromDecoupling(bool allowDecoupling)
Assert.That(decouplingClock.IsRunning, Is.False);
decouplingClock.Start();
+ decouplingClock.ProcessFrame();
Assert.That(source.IsRunning, Is.True);
Assert.That(decouplingClock.IsRunning, Is.True);
@@ -50,6 +51,7 @@ public void TestStartFromSource(bool allowDecoupling)
Assert.That(decouplingClock.IsRunning, Is.False);
source.Start();
+ decouplingClock.ProcessFrame();
Assert.That(source.IsRunning, Is.True);
Assert.That(decouplingClock.IsRunning, Is.True);
@@ -126,14 +128,17 @@ public void TestChangeSourceUpdatesToCorrectSourceState(bool allowDecoupling)
decouplingClock.AllowDecoupling = allowDecoupling;
source.Start();
+ decouplingClock.ProcessFrame();
Assert.That(decouplingClock.IsRunning, Is.True);
var secondSource = new TestClock();
decouplingClock.ChangeSource(secondSource);
+ decouplingClock.ProcessFrame();
Assert.That(decouplingClock.IsRunning, Is.False);
decouplingClock.ChangeSource(source);
+ decouplingClock.ProcessFrame();
Assert.That(decouplingClock.IsRunning, Is.True);
}
@@ -169,11 +174,13 @@ public void TestSourceStoppedWhileNotDecoupling()
{
decouplingClock.AllowDecoupling = false;
decouplingClock.Start();
+ decouplingClock.ProcessFrame();
Assert.That(source.IsRunning, Is.True);
Assert.That(decouplingClock.IsRunning, Is.True);
source.Stop();
+ decouplingClock.ProcessFrame();
Assert.That(source.IsRunning, Is.False);
Assert.That(decouplingClock.IsRunning, Is.False);
@@ -210,6 +217,7 @@ public void TestSourceStoppedWhileDecoupling()
{
decouplingClock.AllowDecoupling = true;
decouplingClock.Start();
+ decouplingClock.ProcessFrame();
Assert.That(source.IsRunning, Is.True);
Assert.That(decouplingClock.IsRunning, Is.True);
diff --git a/osu.Framework/Timing/DecouplingFramedClock.cs b/osu.Framework/Timing/DecouplingFramedClock.cs
index 898866642..5fc61a415 100644
--- a/osu.Framework/Timing/DecouplingFramedClock.cs
+++ b/osu.Framework/Timing/DecouplingFramedClock.cs
@@ -34,23 +34,7 @@ public sealed class DecouplingFramedClock : ISourceChangeableClock, IAdjustableC
/// </remarks>
public bool AllowDecoupling { get; set; } = true;
- public bool IsRunning
- {
- get
- {
- // Always use the source clock's running state if it's running.
- if (Source.IsRunning)
- return isRunning = true;
-
- // If we're allowed to be decoupled, maintain our internal running state.
- if (AllowDecoupling)
- return isRunning;
-
- // Otherwise we're definitely not running.
- Debug.Assert(!Source.IsRunning);
- return isRunning = false;
- }
- }
+ public bool IsRunning { get; private set; }
public double CurrentTime { get; private set; }
@@ -107,6 +91,7 @@ public void ProcessFrame()
if (Source.IsRunning)
{
currentTime = Source.CurrentTime;
+ isRunning = true;
return;
}
@@ -114,6 +99,7 @@ public void ProcessFrame()
if (!AllowDecoupling)
{
currentTime = Source.CurrentTime;
+ isRunning = false;
return;
}
@@ -141,6 +127,7 @@ public void ProcessFrame()
}
finally
{
+ IsRunning = isRunning;
lastReferenceTime = realtimeReferenceClock.CurrentTime;
CurrentTime = currentTime;
ElapsedFrameTime = CurrentTime - lastTime;
There was a problem hiding this comment.
Yeah this is sort of a continuation of my previous point about "implicit clock processing". Although I will say that it feels a lot more dicey to require ProcessFrame() calls for running state to propagate from the source clock to the decoupled, which as far as I can tell the diff above is trying to do.
That said, unless it incurs damage to any other pending adjustments game-side, I'd probably agree with applying the diff above.
There was a problem hiding this comment.
I can say that tests do pass osu! side with this change.
There was a problem hiding this comment.
Conceptually I'm fine with that change/the ones committed since this comment, and we can take if it ever goes wrong as a future learning lesson.
The other suggestion I had is that I only observed Seek() being affected by this, so simply changing its conditional to reference the property would also resolve it.
…ing source stopped
|
I've pushed a small fix for an intermittent failure I got after several hundred runs. |
|
Given that we have:
I wanted to also write diff --git a/osu.Framework.Tests/Clocks/DecouplingFramedClockTest.cs b/osu.Framework.Tests/Clocks/DecouplingFramedClockTest.cs
index 5eb211377..b9079d3a7 100644
--- a/osu.Framework.Tests/Clocks/DecouplingFramedClockTest.cs
+++ b/osu.Framework.Tests/Clocks/DecouplingFramedClockTest.cs
@@ -445,6 +445,53 @@ public void TestForwardPlaybackOverLengthBoundary()
Assert.That(source.IsRunning, Is.False);
}
+ [Test]
+ public void TestBackwardPlaybackOverLengthBoundary()
+ {
+ source = new TestStopwatchClockWithRangeLimit
+ {
+ MaxTime = 10000
+ };
+
+ decouplingClock.ChangeSource(source);
+ decouplingClock.AllowDecoupling = true;
+
+ decouplingClock.Seek(10200);
+ decouplingClock.Rate = -1;
+ decouplingClock.Start();
+
+ decouplingClock.ProcessFrame();
+
+ double time = decouplingClock.CurrentTime;
+
+ while (decouplingClock.CurrentTime > 10000)
+ {
+ Assert.That(source.IsRunning, Is.False);
+ Assert.That(source.CurrentTime, Is.EqualTo(10000));
+ Assert.That(decouplingClock.CurrentTime, Is.LessThanOrEqualTo(time));
+ time = decouplingClock.CurrentTime;
+
+ decouplingClock.ProcessFrame();
+ }
+
+ while (!source.IsRunning)
+ decouplingClock.ProcessFrame();
+
+ Assert.That(source.IsRunning, Is.True);
+ Assert.That(decouplingClock.CurrentTime, Is.LessThan(9900));
+
+ while (decouplingClock.CurrentTime > 9800)
+ {
+ Assert.That(decouplingClock.IsRunning, Is.True);
+ Assert.That(decouplingClock.CurrentTime, Is.LessThanOrEqualTo(time));
+ time = decouplingClock.CurrentTime;
+
+ decouplingClock.ProcessFrame();
+ }
+
+ Assert.That(source.IsRunning, Is.True);
+ }
+
#endregion
private class TestClockWithRange : TestClock
but it fails (more precisely, gets stuck at the while loop). Moreover, if I attempt to test manually via the other test scene: diff --git a/osu.Framework.Tests/Visual/Clocks/TestSceneClocks.cs b/osu.Framework.Tests/Visual/Clocks/TestSceneClocks.cs
index 32735e014..02460a8d8 100644
--- a/osu.Framework.Tests/Visual/Clocks/TestSceneClocks.cs
+++ b/osu.Framework.Tests/Visual/Clocks/TestSceneClocks.cs
@@ -1,6 +1,7 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.
+using System.Linq;
using NUnit.Framework;
using osu.Framework.Testing;
using osu.Framework.Tests.Clocks;
@@ -21,7 +22,7 @@ public void TestInterpolationWithLaggyClock()
[Test]
public void TestDecouplingWithRangeLimited()
{
- AddStep("add non-negative stopwatch", () => AddClock(new TestStopwatchClockWithRangeLimit()));
+ AddStep("add non-negative stopwatch", () => AddClock(new TestStopwatchClockWithRangeLimit { MaxTime = 15000 }));
AddStep("add decoupling", () => AddClock(new DecouplingFramedClock { AllowDecoupling = true }));
AddStep("seek decoupling to -10000", () =>
@@ -35,6 +36,17 @@ public void TestDecouplingWithRangeLimited()
foreach (var c in this.ChildrenOfType<VisualClock>())
(c.TrackingClock as DecouplingFramedClock)?.Seek(10000);
});
+
+ AddStep("seek decoupling to 20000", () =>
+ {
+ foreach (var c in this.ChildrenOfType<VisualClock>())
+ (c.TrackingClock as DecouplingFramedClock)?.Seek(20000);
+ });
+
+ AddToggleStep("toggle direction", b =>
+ {
+ (this.ChildrenOfType<VisualClock>().Last().TrackingClock as DecouplingFramedClock)!.Rate = b ? -1 : 1;
+ });
}
}
}
it also appears that the source clock is not restarted when decoupled clock crosses the length boundary backwards. Is my testing wrong, or is this a real issue? |
|
Your test is fine I think. There's no logic to support this case because we don't really use it. Moreso, we don't know where the "length" boundary is. I touch on this in the OP, suggesting we may want to expose the "valid range" of a clock and use that in decoupling. But without this change, I'd propose documenting this as a caveat and only adding support if we ever require it. |
|
You could theoretically use the last source time as a stopgap: diff --git a/osu.Framework/Timing/DecouplingFramedClock.cs b/osu.Framework/Timing/DecouplingFramedClock.cs
index 0d179459d..cdffdb132 100644
--- a/osu.Framework/Timing/DecouplingFramedClock.cs
+++ b/osu.Framework/Timing/DecouplingFramedClock.cs
@@ -116,7 +116,7 @@ public void ProcessFrame()
currentTime += elapsedReferenceTime;
// When crossing the zero time boundary, we should start and use the source clock.
- if (lastTime < 0 && currentTime >= 0)
+ if ((lastTime < 0 && currentTime >= 0) || (lastTime > Source.CurrentTime && currentTime <= Source.CurrentTime))
{
adjustableSourceClock.Seek(currentTime);
adjustableSourceClock.Start();
but you're right that the correctness of this value is anything but guaranteed. Probably fine to document and move on - shall I grab that so that we can move forward? |
|
You raise a very good point about using the source time. I'd be okay with considering that if it works. Since you've already got the tests and proposal handy, want to give that a try first? |
|
Yeah sure, I'll run that past both test suites and we'll see how it goes. |
|
Some failures, unfortunately:
The audio track test fails without my change, but the rest is firmly the fault of it. I'll have a look at both, but if I can't get my change to work in like 30 minutes, I'll bail. |
Came up as a failure when locally running tests for ppy/osu-framework#6001 - but the test is actually a previously-known flaky that I couldn't reproduce the failure of until the aforementioned PR. This appears to be a simple race; the test scene queries the track length from update thread, but the length is actually set on the audio thread. So it's not unreasonable that given unlucky timing, the length will not be set by `TrackBass` before it is queried. To fix, switch assert to until step. I'm generally not really willing to give this more time of day until this change is proven insufficient.
|
For the unrelated failure, see PR link above. For the related failures, yeah, my patch breaks things quite badly. While it is true that I'll leave a comment and let this be. |
Clocks, explained
Clocks have been painful, but they were never intended to be. The system should conceptually be simple:
CurrentTime.IFrameBasedClock) is processed (viaProcessFrame) by the consumer every frame and can therefore provide elapsed time.IAdjustableClock) exposes the ability to beStarted,Stopped,Seeked andReset. This is most commonly used to bring audio tracks into the clock system.ISourceChangeableClock), to allow nesting clocks inside each other. This can be used to add more complex behaviours to existing clocks via composition.These are the basic governing rules. Let's look at the two main cases for nesting clocks:
InterpolatingFramedClock(aka allowingCurrentTimeto be more precise than the underlying audio subsystem via interpolation). This one is pretty simple, and has been recently cleaned up via Tidy upInterpolatingFramedClock#5992.DecoupleableInterpolatingFramedClock. This is the thorn in the clock system.What was wrong with
DecoupleableInterpolatingFramedClock?InterpolatingFramedClock, overriding some pieces but also adding its own complexity on top. It was nigh impossible to follow the cross-interactions between these two implementations. It should have been implemented as its own stand-alone implementation which only decoupledSo really, the only thing that needs to be fixed (from the framework perspective) when I've said "clocks are fucked" many times over the years is this one implementation. I've rewritten it from scratch with everything I've learned over the years and with the above mistakes in mind. The new class is called 'DecouplingClock'.
Behaviour differences
Is now a stand-alone implementation
DecouplingClockno longer inherits fromInterpolatingClock. This reduces about half of the original complexity.Existing usages of
DecoupleableInterpolatingFramedClockwill need to nest aDecouplingClockinside of anInterpolatingClock. Note the order here –DecouplingClockshould directly encapsulate theITrackor similar source for best behaviour. Introducing decoupling in the middle of a framed hierarchy will likely not work well.Not frame based
Update: now is
DecouplingClockis intentionally re-implemented as a non-frame-based clock for simplicity. My main reason for this was to ensure that we are exposing "instantaneous" values to keep complexity to a minimum. ie. we don't need to unnecessarily track and compare time values.To make this work, time tracking for the case where time is decoupled is done whenCurrentTimeis called. I can understand if this may be seen as a bit weird, but I do think it's better than making it anIFrameBasedClock– at least until we can better define how things likeSeek(should a seek on a framed clock immediately updateCurrentTimeor only after aProcessFrame?) andIsRunnning(should running state of the nested clock immediately update or wait for next frame?) work.In practice, this seems to work quite well. It also hints to consumers to put theDecoupledClockinside any further adjustments like interpolation.A potential caveat here is that if a framed clock is provided as the decoupling source (as is done in some osu!-side usages such as tests and spectator), the source will need to be externally processed.Less mutations
When
AllowDecouplingis set tofalse, the new implementation will operate in a completely transparent way. No extra operations will be performed on theSource.When
AllowDecouplingis set totrue, there are only two cases whereDecouplingClockwill perform operations on theSource:DecouplingClock.Seekis requested by the user and the source returnsfalse(aka "cannot seek"),Stopwill be called on the source to ensure it stops. This is to cover the scenario where callingSeek(-1000)on a bass track will seek it to0and continue playing.Source, at the boundary ofCurrentTime==0, the source will beStarted once, hinting that it is likely able to take over.Touching on the
CurrentTime==0special caseTo keep things simple, the implementation assumes that if the
Sourcecould not seek to a negative time value, it will likely be able to again after reaching zero. This is the only time aDecouplingClockwillStartthe underlying source.This is a very simplistic approach to handle the main use case of decoupling that we have. If there a new use case where the
Sourcecan say, only track time from1000..6000and we perform a seek to0, theSourcewill not be hinted to start when reaching1000.Fixing this would require some extra thought from a performance perspective (we likely don't want to call
Start()on theSourceevery timeCurrentTimeis retrieved, for instance) without further hinting at what range a clock can actually handle.A further refinement could be doing precisely this –
DecouplingClockcan only operation on a source which implements a new interfaceIRangeLimitedClock { (double, double) ValidRange { get ; } }.Source must be
IAdjustableClockThis is another arbitrary limitation to keep complexity down.
DecouplingClockneeds to beIAdjustableClockin order to track operations performed against the source. Without this we would have a very hard time knowing what state we are in.If necessary in the future, we could allow sources to be non-adjustable, but methods like
Start/Stop/Seekwould need tothrowas we rationally cannot make them work.All adjustable operations should be performed on
DecouplingClockAs touched upon, all
IAdjustableClockoperations should be performed directly on theDecouplingClock, not the source. Operations performed directly on the source could lead to incorrect behaviour, such asSeeks being undone when in a decoupled state and stopped.Current state
I'm publishing this as a draft to get eyes on it and make sure the overall direction is agreeable. I'd estimate there's another day's work to get this ready for prime time, and I'll probably push ahead even if this doesn't get any eyes on it immediately.
I've left my full commit history intact, which means some of the earlier commits may be taking things in a slightly different direction. I want these around in case I need to backtrack based on feedback. And who knows, maybe it'll explain the direction I took better.
osu! integration
Works against osu! with some changes.
Note that there are a few straggler tests that need fixing, but the hard ones are handled.I've tried to explain changes in commit messages, but will continue to refine things.One cool thing is that it seems like a few hacks we had in place can be removed. Needs some more rigorous testing, though.
Test coverage
There's a few tests missing (see inline
TODOs) which I will add before marking this as non-draft. These came up as I adjusted the implementation to pass osu!-side tests, for what it's worth.Regarding obsoleting the old implementation
I've searched across github and cannot find any consumers using the old implementation (phew). We could potentially just nuke it from existence once osu! has switched across.
Prerequisite for ppy/osu#24811.
Breaking changes
DecoupleableInterpolatingFramedClockis obsoleted. UseDecouplingClockinsteadSee #6001 for more details on a migration path.