From d91456dc2993705d44a30ce74c87bfe1595c973c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 29 Oct 2020 14:25:47 +0900 Subject: [PATCH 1/8] Move initial validity check out of loop for clarity --- osu.Game/Rulesets/UI/FrameStabilityContainer.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Rulesets/UI/FrameStabilityContainer.cs b/osu.Game/Rulesets/UI/FrameStabilityContainer.cs index f8156164c1..8edeaf851d 100644 --- a/osu.Game/Rulesets/UI/FrameStabilityContainer.cs +++ b/osu.Game/Rulesets/UI/FrameStabilityContainer.cs @@ -96,7 +96,10 @@ namespace osu.Game.Rulesets.UI int loops = MaxCatchUpFrames; - while (state != PlaybackState.NotValid && loops-- > 0) + if (state == PlaybackState.NotValid) + return true; + + while (loops-- > 0) { updateClock(); From db2b00068f024589f26b4ab247b824f030a88b2b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 29 Oct 2020 14:42:16 +0900 Subject: [PATCH 2/8] Avoid sourcing parent clock when in a paused state --- .../Rulesets/UI/FrameStabilityContainer.cs | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/osu.Game/Rulesets/UI/FrameStabilityContainer.cs b/osu.Game/Rulesets/UI/FrameStabilityContainer.cs index 8edeaf851d..75f3aa90ee 100644 --- a/osu.Game/Rulesets/UI/FrameStabilityContainer.cs +++ b/osu.Game/Rulesets/UI/FrameStabilityContainer.cs @@ -85,23 +85,31 @@ namespace osu.Game.Rulesets.UI public override bool UpdateSubTree() { - state = frameStableClock.IsPaused.Value ? PlaybackState.NotValid : PlaybackState.Valid; + double proposedTime = manualClock.CurrentTime; if (frameStableClock.WaitingOnFrames.Value) { - // for now, force one update loop to check if frames have arrived - // this may have to change in the future where we want stable user pausing during replay playback. + // when waiting on frames, the update loop still needs to be run (at least once) to check for newly arrived frames. + // time should not be sourced from the parent clock in this case. state = PlaybackState.Valid; } + else if (!frameStableClock.IsPaused.Value) + { + state = PlaybackState.Valid; + proposedTime = parentGameplayClock.CurrentTime; + } + else + { + // time should not advance while paused, not should anything run. + state = PlaybackState.NotValid; + return true; + } int loops = MaxCatchUpFrames; - if (state == PlaybackState.NotValid) - return true; - while (loops-- > 0) { - updateClock(); + updateClock(ref proposedTime); if (state == PlaybackState.NotValid) break; @@ -113,7 +121,7 @@ namespace osu.Game.Rulesets.UI return true; } - private void updateClock() + private void updateClock(ref double proposedTime) { if (parentGameplayClock == null) setClock(); // LoadComplete may not be run yet, but we still want the clock. @@ -121,9 +129,6 @@ namespace osu.Game.Rulesets.UI // each update start with considering things in valid state. state = PlaybackState.Valid; - // our goal is to catch up to the time provided by the parent clock. - var proposedTime = parentGameplayClock.CurrentTime; - if (FrameStablePlayback) // if we require frame stability, the proposed time will be adjusted to move at most one known // frame interval in the current direction. From a59ea987b7811ee6e815d03e3af85e3d9bb27bae Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 29 Oct 2020 14:57:36 +0900 Subject: [PATCH 3/8] Make tests more resilient under headless execution --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 6485cbdad3..6c05049864 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -82,13 +82,21 @@ namespace osu.Game.Tests.Visual.Gameplay AddUntilStep("wait for frame starvation", () => replayHandler.NextFrame == null); checkPaused(true); + double? pausedTime = null; + + AddStep("store time", () => pausedTime = currentFrameStableTime); + sendFrames(); - checkPaused(false); AddUntilStep("wait for frame starvation", () => replayHandler.NextFrame == null); checkPaused(true); + + AddAssert("time advanced", () => currentFrameStableTime > pausedTime); } + private double currentFrameStableTime + => player.ChildrenOfType().First().FrameStableClock.CurrentTime; + [Test] public void TestPlayStartsWithNoFrames() { @@ -98,7 +106,7 @@ namespace osu.Game.Tests.Visual.Gameplay waitForPlayer(); checkPaused(true); - sendFrames(); + sendFrames(1000); // send enough frames to ensure play won't be paused checkPaused(false); } From 7dd3a748be4b0d7e7514d02f46b2a451a3bb3ae9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 29 Oct 2020 15:03:38 +0900 Subject: [PATCH 4/8] Add further test logic to ensure retry / restart flow is working correctly --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 6c05049864..2b1bf1810b 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -134,8 +134,16 @@ namespace osu.Game.Tests.Visual.Gameplay start(); sendFrames(); + waitForPlayer(); + + Player lastPlayer = null; + AddStep("store first player", () => lastPlayer = player); + start(); sendFrames(); + + waitForPlayer(); + AddAssert("player is different", () => lastPlayer != player); } [Test] From 6a31a313b6236521bb877dced8da5fb394e0dc40 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 29 Oct 2020 15:08:06 +0900 Subject: [PATCH 5/8] Fix stop watching test to check correct screen presence --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 2b1bf1810b..7c62cf47f4 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -127,7 +127,7 @@ namespace osu.Game.Tests.Visual.Gameplay } [Test] - public void TestHostStartsPlayingWhileAlreadyWatching() + public void TestHostRetriesWhileWatching() { loadSpectatingScreen(); @@ -171,9 +171,8 @@ namespace osu.Game.Tests.Visual.Gameplay sendFrames(); waitForPlayer(); - // should immediately exit and unbind from streaming client AddStep("stop spectating", () => (Stack.CurrentScreen as Player)?.Exit()); - AddUntilStep("spectating stopped", () => spectatorScreen.GetParentScreen() == null); + AddUntilStep("spectating stopped", () => spectatorScreen.GetChildScreen() == null); } [Test] From ce58bfdc4e845bc7141a7d0123445a3753c60860 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 29 Oct 2020 15:09:12 +0900 Subject: [PATCH 6/8] Add test covering host retry after returning to spectator screen --- .../Visual/Gameplay/TestSceneSpectator.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 7c62cf47f4..864e297eda 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -175,6 +175,23 @@ namespace osu.Game.Tests.Visual.Gameplay AddUntilStep("spectating stopped", () => spectatorScreen.GetChildScreen() == null); } + [Test] + public void TestStopWatchingThenHostRetries() + { + loadSpectatingScreen(); + + start(); + sendFrames(); + waitForPlayer(); + + AddStep("stop spectating", () => (Stack.CurrentScreen as Player)?.Exit()); + AddUntilStep("spectating stopped", () => spectatorScreen.GetChildScreen() == null); + + // host starts playing a new session + start(); + waitForPlayer(); + } + [Test] public void TestWatchingBeatmapThatDoesntExistLocally() { From fe409a55e63db279d4f45c5b91aeb935ac94d658 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 29 Oct 2020 15:10:11 +0900 Subject: [PATCH 7/8] Rename starvation test --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 864e297eda..8821067618 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -67,7 +67,7 @@ namespace osu.Game.Tests.Visual.Gameplay private Player player => Stack.CurrentScreen as Player; [Test] - public void TestBasicSpectatingFlow() + public void TestFrameStarvationAndResume() { loadSpectatingScreen(); From fa857514254b32a5b3b6ccc5143c1ad5078ad776 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 29 Oct 2020 15:10:42 +0900 Subject: [PATCH 8/8] Move helper functions to bottom of class --- .../Visual/Gameplay/TestSceneSpectator.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 8821067618..a4df450db9 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -61,11 +61,6 @@ namespace osu.Game.Tests.Visual.Gameplay finish(); } - private OsuFramedReplayInputHandler replayHandler => - (OsuFramedReplayInputHandler)Stack.ChildrenOfType().First().ReplayInputHandler; - - private Player player => Stack.CurrentScreen as Player; - [Test] public void TestFrameStarvationAndResume() { @@ -94,9 +89,6 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("time advanced", () => currentFrameStableTime > pausedTime); } - private double currentFrameStableTime - => player.ChildrenOfType().First().FrameStableClock.CurrentTime; - [Test] public void TestPlayStartsWithNoFrames() { @@ -203,6 +195,14 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("screen didn't change", () => Stack.CurrentScreen is Spectator); } + private OsuFramedReplayInputHandler replayHandler => + (OsuFramedReplayInputHandler)Stack.ChildrenOfType().First().ReplayInputHandler; + + private Player player => Stack.CurrentScreen as Player; + + private double currentFrameStableTime + => player.ChildrenOfType().First().FrameStableClock.CurrentTime; + private void waitForPlayer() => AddUntilStep("wait for player", () => Stack.CurrentScreen is Player); private void start(int? beatmapId = null) => AddStep("start play", () => testSpectatorStreamingClient.StartPlay(beatmapId ?? importedBeatmapId));