Merge pull request #16713 from frenzibyte/fix-spectator-gcc-state-2

Fix multiplayer spectator occasionally rewinding time unexpectedly

osu.Game.Tests/Visual/Multiplayer/TestSceneMultiSpectatorScreen.cs

@@ -347,19 +347,44 @@ namespace osu.Game.Tests.Visual.Multiplayer
             AddAssert($"{PLAYER_1_ID} score quit still set", () => getLeaderboardScore(PLAYER_1_ID).HasQuit.Value);
         }
 
-        private void loadSpectateScreen(bool waitForPlayerLoad = true)
+        /// <summary>
+        /// Tests spectating with a gameplay start time set to a negative value.
+        /// Simulating beatmaps with high <see cref="BeatmapInfo.AudioLeadIn"/> or negative time storyboard elements.
+        /// </summary>
+        [Test]
+        public void TestNegativeGameplayStartTime()
         {
-            AddStep("load screen", () =>
+            start(PLAYER_1_ID);
+
+            loadSpectateScreen(false, -500);
+
+            // to ensure negative gameplay start time does not affect spectator, send frames exactly after StartGameplay().
+            // (similar to real spectating sessions in which the first frames get sent between StartGameplay() and player load complete)
+            AddStep("send frames at gameplay start", () => getInstance(PLAYER_1_ID).OnGameplayStarted += () => SpectatorClient.SendFrames(PLAYER_1_ID, 100));
+
+            AddUntilStep("wait for player load", () => spectatorScreen.AllPlayersLoaded);
+
+            AddWaitStep("wait for progression", 3);
+
+            assertNotCatchingUp(PLAYER_1_ID);
+            assertRunning(PLAYER_1_ID);
+        }
+
+        private void loadSpectateScreen(bool waitForPlayerLoad = true, double? gameplayStartTime = null)
+        {
+            AddStep(!gameplayStartTime.HasValue ? "load screen" : $"load screen (start = {gameplayStartTime}ms)", () =>
             {
                 Beatmap.Value = beatmapManager.GetWorkingBeatmap(importedBeatmap);
                 Ruleset.Value = importedBeatmap.Ruleset;
 
-                LoadScreen(spectatorScreen = new MultiSpectatorScreen(playingUsers.ToArray()));
+                LoadScreen(spectatorScreen = new TestMultiSpectatorScreen(playingUsers.ToArray(), gameplayStartTime));
             });
 
             AddUntilStep("wait for screen load", () => spectatorScreen.LoadState == LoadState.Loaded && (!waitForPlayerLoad || spectatorScreen.AllPlayersLoaded));
         }
 
+        private void start(int userId, int? beatmapId = null) => start(new[] { userId }, beatmapId);
+
         private void start(int[] userIds, int? beatmapId = null)
         {
             AddStep("start play", () =>
@@ -419,6 +444,12 @@ namespace osu.Game.Tests.Visual.Multiplayer
         private void assertMuted(int userId, bool muted)
             => AddAssert($"{userId} {(muted ? "is" : "is not")} muted", () => getInstance(userId).Mute == muted);
 
+        private void assertRunning(int userId)
+            => AddAssert($"{userId} clock running", () => getInstance(userId).GameplayClock.IsRunning);
+
+        private void assertNotCatchingUp(int userId)
+            => AddAssert($"{userId} in sync", () => !getInstance(userId).GameplayClock.IsCatchingUp);
+
         private void waitForCatchup(int userId)
             => AddUntilStep($"{userId} not catching up", () => !getInstance(userId).GameplayClock.IsCatchingUp);
 
@@ -429,5 +460,19 @@ namespace osu.Game.Tests.Visual.Multiplayer
         private GameplayLeaderboardScore getLeaderboardScore(int userId) => spectatorScreen.ChildrenOfType<GameplayLeaderboardScore>().Single(s => s.User?.Id == userId);
 
         private int[] getPlayerIds(int count) => Enumerable.Range(PLAYER_1_ID, count).ToArray();
+
+        private class TestMultiSpectatorScreen : MultiSpectatorScreen
+        {
+            private readonly double? gameplayStartTime;
+
+            public TestMultiSpectatorScreen(MultiplayerRoomUser[] users, double? gameplayStartTime = null)
+                : base(users)
+            {
+                this.gameplayStartTime = gameplayStartTime;
+            }
+
+            protected override MasterGameplayClockContainer CreateMasterGameplayClockContainer(WorkingBeatmap beatmap)
+                => new MasterGameplayClockContainer(beatmap, gameplayStartTime ?? 0, gameplayStartTime.HasValue);
+        }
     }
 }

osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorPlayer.cs

@@ -55,6 +55,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
             public SpectatorGameplayClockContainer([NotNull] IClock sourceClock)
                 : base(sourceClock)
             {
+                // the container should initially be in a stopped state until the catch-up clock is started by the sync manager.
+                Stop();
             }
 
             protected override void Update()

osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs

@@ -9,6 +9,7 @@ using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
+using osu.Game.Beatmaps;
 using osu.Game.Graphics;
 using osu.Game.Online.Multiplayer;
 using osu.Game.Online.Spectator;
@@ -68,7 +69,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
             Container leaderboardContainer;
             Container scoreDisplayContainer;
 
-            masterClockContainer = new MasterGameplayClockContainer(Beatmap.Value, 0);
+            masterClockContainer = CreateMasterGameplayClockContainer(Beatmap.Value);
 
             InternalChildren = new[]
             {
@@ -235,5 +236,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
 
             return base.OnBackButton();
         }
+
+        protected virtual MasterGameplayClockContainer CreateMasterGameplayClockContainer(WorkingBeatmap beatmap) => new MasterGameplayClockContainer(beatmap, 0);
     }
 }

osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/PlayerArea.cs

@@ -24,6 +24,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
     /// </summary>
     public class PlayerArea : CompositeDrawable
     {
+        /// <summary>
+        /// Raised after <see cref="Player.StartGameplay"/> is called on <see cref="Player"/>.
+        /// </summary>
+        public event Action OnGameplayStarted;
+
         /// <summary>
         /// Whether a <see cref="Player"/> is loaded in the area.
         /// </summary>
@@ -93,7 +98,13 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
                 }
             };
 
-            stack.Push(new MultiSpectatorPlayerLoader(Score, () => new MultiSpectatorPlayer(Score, GameplayClock)));
+            stack.Push(new MultiSpectatorPlayerLoader(Score, () =>
+            {
+                var player = new MultiSpectatorPlayer(Score, GameplayClock);
+                player.OnGameplayStarted += () => OnGameplayStarted?.Invoke();
+                return player;
+            }));
+
             loadingLayer.Hide();
         }
 

osu.Game/Screens/Play/GameplayClockContainer.cs

@@ -101,7 +101,7 @@ namespace osu.Game.Screens.Play
         /// <summary>
         /// Stops gameplay.
         /// </summary>
-        public virtual void Stop() => IsPaused.Value = true;
+        public void Stop() => IsPaused.Value = true;
 
         /// <summary>
         /// Resets this <see cref="GameplayClockContainer"/> and the source to an initial state ready for gameplay.

osu.Game/Screens/Play/Player.cs

@@ -45,6 +45,11 @@ namespace osu.Game.Screens.Play
         /// </summary>
         public const double RESULTS_DISPLAY_DELAY = 1000.0;
 
+        /// <summary>
+        /// Raised after <see cref="StartGameplay"/> is called.
+        /// </summary>
+        public event Action OnGameplayStarted;
+
         public override bool AllowBackButton => false; // handled by HoldForMenuButton
 
         protected override UserActivity InitialActivity => new UserActivity.InSoloGame(Beatmap.Value.BeatmapInfo, Ruleset.Value);
@@ -958,7 +963,9 @@ namespace osu.Game.Screens.Play
             updateGameplayState();
 
             GameplayClockContainer.FadeInFromZero(750, Easing.OutQuint);
+
             StartGameplay();
+            OnGameplayStarted?.Invoke();
         }
 
         /// <summary>