From 4319b22db825fd39f99862642d42a6bc213821d7 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 10 Oct 2023 17:55:35 +0900 Subject: [PATCH 1/5] Add failing test coverage of player not pausing track correctly --- .../Multiplayer/TestSceneMultiplayerPlayer.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlayer.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlayer.cs index 45c5c67fff13..cbeff770c96d 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlayer.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlayer.cs @@ -11,6 +11,7 @@ using osu.Game.Online.Rooms; using osu.Game.Rulesets.Osu; using osu.Game.Screens.OnlinePlay.Multiplayer; +using osu.Game.Screens.Play; namespace osu.Game.Tests.Visual.Multiplayer { @@ -28,6 +29,11 @@ public override void SetUpSteps() Beatmap.Value = CreateWorkingBeatmap(new OsuRuleset().RulesetInfo); }); + AddStep("Start track playing", () => + { + Beatmap.Value.Track.Start(); + }); + AddStep("initialise gameplay", () => { Stack.Push(player = new MultiplayerPlayer(MultiplayerClient.ServerAPIRoom, new PlaylistItem(Beatmap.Value.BeatmapInfo) @@ -37,7 +43,14 @@ public override void SetUpSteps() }); AddUntilStep("wait for player to be current", () => player.IsCurrentScreen() && player.IsLoaded); + + AddAssert("gameplay clock is paused", () => player.ChildrenOfType().Single().IsPaused.Value); + AddAssert("gameplay clock is not running", () => !player.ChildrenOfType().Single().IsRunning); + AddStep("start gameplay", () => ((IMultiplayerClient)MultiplayerClient).GameplayStarted()); + + AddUntilStep("gameplay clock is not paused", () => !player.ChildrenOfType().Single().IsPaused.Value); + AddAssert("gameplay clock is running", () => player.ChildrenOfType().Single().IsRunning); } [Test] From eee7dc02da9611c00ac3d6bc16343507b11728a9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 10 Oct 2023 17:58:11 +0900 Subject: [PATCH 2/5] Reduce severity of clock running check to avoid production crashes --- osu.Game/Screens/Play/Player.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index ac9ce11cb536..91131b0aba27 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -1078,7 +1078,7 @@ public override void OnEntering(ScreenTransitionEvent e) protected virtual void StartGameplay() { if (GameplayClockContainer.IsRunning) - throw new InvalidOperationException($"{nameof(StartGameplay)} should not be called when the gameplay clock is already running"); + Logger.Error(new InvalidOperationException($"{nameof(StartGameplay)} should not be called when the gameplay clock is already running"), "Clock failure"); GameplayClockContainer.Reset(startClock: true); From 100c0cf5331ad7806424acfb7d1e3371c39553c3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 10 Oct 2023 17:58:37 +0900 Subject: [PATCH 3/5] Fix `MultiplayerPlayer` never calling `Reset` on `GameplayClockContainer` This call is responsible for ensuring the clock is in a stopped state. --- osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs index 7b448e4b5cba..e6d9dd4cd04f 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs @@ -148,6 +148,9 @@ protected override void StartGameplay() loadingDisplay.Show(); client.ChangeState(MultiplayerUserState.ReadyForGameplay); } + + // This will pause the clock, pending the gameplay started callback from the server. + GameplayClockContainer.Reset(); } private void failAndBail(string message = null) From f0bd97539379ad11f54dbdf354f0adca7565044a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 10 Oct 2023 18:16:12 +0900 Subject: [PATCH 4/5] Change `GameplayClockContainer.Reset` to directly call `GameplayClock.Stop` The reasoning is explained in the inline comment, but basically this was getting blocked by `isPaused` being in an initial `true` state (as it is on construction), while the source clock was still `IsRunning`. There's no real guarantee of sync between the source and the `isPaused` bindable right now. Maybe there should be in the future, but to restore sanity, let's ensure that a call to `Reset` can at least stop the track as we expect. --- osu.Game/Screens/Play/GameplayClockContainer.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Play/GameplayClockContainer.cs b/osu.Game/Screens/Play/GameplayClockContainer.cs index e6a38a994652..3c920babeeca 100644 --- a/osu.Game/Screens/Play/GameplayClockContainer.cs +++ b/osu.Game/Screens/Play/GameplayClockContainer.cs @@ -138,12 +138,15 @@ public void Stop() /// Resets this and the source to an initial state ready for gameplay. /// /// The time to seek to on resetting. If null, the existing will be used. - /// Whether to start the clock immediately, if not already started. + /// Whether to start the clock immediately. If false, the clock will remain stopped after this call. public void Reset(double? time = null, bool startClock = false) { bool wasPaused = isPaused.Value; - Stop(); + // The intention of the Reset method is to get things into a known sane state. + // As such, we intentionally stop the underlying clock directly here, bypassing Stop/StopGameplayClock. + // This is to avoid any kind of isPaused state checks and frequency ramping (as provided by MasterGameplayClockContainer). + GameplayClock.Stop(); if (time != null) StartTime = time.Value; From b9dadc52b7c571f4e77e4295b83df69384803ae9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 10 Oct 2023 22:54:19 +0900 Subject: [PATCH 5/5] Adjust comment to match current behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bartłomiej Dach --- osu.Game/Screens/Play/GameplayClockContainer.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/Play/GameplayClockContainer.cs b/osu.Game/Screens/Play/GameplayClockContainer.cs index 3c920babeeca..5a713fdae78a 100644 --- a/osu.Game/Screens/Play/GameplayClockContainer.cs +++ b/osu.Game/Screens/Play/GameplayClockContainer.cs @@ -138,7 +138,8 @@ public void Stop() /// Resets this and the source to an initial state ready for gameplay. /// /// The time to seek to on resetting. If null, the existing will be used. - /// Whether to start the clock immediately. If false, the clock will remain stopped after this call. + /// Whether to start the clock immediately. If false and the clock was already paused, the clock will remain paused after this call. + /// public void Reset(double? time = null, bool startClock = false) { bool wasPaused = isPaused.Value;