Merge pull request #19938 from frenzibyte/fix-multi-spectator-results-screen

Fix multi-spectator potentially getting stuck for passed players

osu.Game/Online/Spectator/SpectatorClient.cs

@@ -65,11 +65,12 @@ namespace osu.Game.Online.Spectator
         public virtual event Action<int, SpectatorState>? OnUserFinishedPlaying;
 
         /// <summary>
-        /// All users currently being watched.
+        /// A dictionary containing all users currently being watched, with the number of watching components for each user.
         /// </summary>
-        private readonly List<int> watchedUsers = new List<int>();
+        private readonly Dictionary<int, int> watchedUsersRefCounts = new Dictionary<int, int>();
 
         private readonly BindableDictionary<int, SpectatorState> watchedUserStates = new BindableDictionary<int, SpectatorState>();
+
         private readonly BindableList<int> playingUsers = new BindableList<int>();
         private readonly SpectatorState currentState = new SpectatorState();
 
@@ -94,12 +95,15 @@ namespace osu.Game.Online.Spectator
                 if (connected.NewValue)
                 {
                     // get all the users that were previously being watched
-                    int[] users = watchedUsers.ToArray();
+                    var users = new Dictionary<int, int>(watchedUsersRefCounts);
-                    watchedUsers.Clear();
+                    watchedUsersRefCounts.Clear();
 
                     // resubscribe to watched users.
-                    foreach (int userId in users)
+                    foreach ((int user, int watchers) in users)
-                        WatchUser(userId);
+                    {
+                        for (int i = 0; i < watchers; i++)
+                            WatchUser(user);
+                    }
 
                     // re-send state in case it wasn't received
                     if (IsPlaying)
@@ -121,7 +125,7 @@ namespace osu.Game.Online.Spectator
                 if (!playingUsers.Contains(userId))
                     playingUsers.Add(userId);
 
-                if (watchedUsers.Contains(userId))
+                if (watchedUsersRefCounts.ContainsKey(userId))
                     watchedUserStates[userId] = state;
 
                 OnUserBeganPlaying?.Invoke(userId, state);
@@ -136,7 +140,7 @@ namespace osu.Game.Online.Spectator
             {
                 playingUsers.Remove(userId);
 
-                if (watchedUsers.Contains(userId))
+                if (watchedUsersRefCounts.ContainsKey(userId))
                     watchedUserStates[userId] = state;
 
                 OnUserFinishedPlaying?.Invoke(userId, state);
@@ -232,11 +236,13 @@ namespace osu.Game.Online.Spectator
         {
             Debug.Assert(ThreadSafety.IsUpdateThread);
 
-            if (watchedUsers.Contains(userId))
+            if (watchedUsersRefCounts.ContainsKey(userId))
+            {
+                watchedUsersRefCounts[userId]++;
                 return;
+            }
 
-            watchedUsers.Add(userId);
+            watchedUsersRefCounts.Add(userId, 1);
-
             WatchUserInternal(userId);
         }
 
@@ -246,7 +252,13 @@ namespace osu.Game.Online.Spectator
             // Todo: This should not be a thing, but requires framework changes.
             Schedule(() =>
             {
-                watchedUsers.Remove(userId);
+                if (watchedUsersRefCounts.TryGetValue(userId, out int watchers) && watchers > 1)
+                {
+                    watchedUsersRefCounts[userId]--;
+                    return;
+                }
+
+                watchedUsersRefCounts.Remove(userId);
                 watchedUserStates.Remove(userId);
                 StopWatchingUserInternal(userId);
             });

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

@@ -197,19 +197,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
         protected override void StartGameplay(int userId, SpectatorGameplayState spectatorGameplayState)
             => instances.Single(i => i.UserId == userId).LoadScore(spectatorGameplayState.Score);
 
-        protected override void EndGameplay(int userId, SpectatorState state)
+        protected override void QuitGameplay(int userId)
         {
-            // Allowed passed/failed users to complete their remaining replay frames.
-            // The failed state isn't really possible in multiplayer (yet?) but is added here just for safety in case it starts being used.
-            if (state.State == SpectatedUserState.Passed || state.State == SpectatedUserState.Failed)
-                return;
-
-            // we could also potentially receive EndGameplay with "Playing" state, at which point we can only early-return and hope it's a passing player.
-            // todo: this shouldn't exist, but it's here as a hotfix for an issue with multi-spectator screen not proceeding to results screen.
-            // see: https://github.com/ppy/osu/issues/19593
-            if (state.State == SpectatedUserState.Playing)
-                return;
-
             RemoveUser(userId);
 
             var instance = instances.Single(i => i.UserId == userId);

osu.Game/Screens/Play/SoloSpectator.cs

@@ -182,7 +182,7 @@ namespace osu.Game.Screens.Play
             scheduleStart(spectatorGameplayState);
         }
 
-        protected override void EndGameplay(int userId, SpectatorState state)
+        protected override void QuitGameplay(int userId)
         {
             scheduledStart?.Cancel();
             immediateSpectatorGameplayState = null;

osu.Game/Screens/Spectate/SpectatorScreen.cs

@@ -115,14 +115,9 @@ namespace osu.Game.Screens.Spectate
             {
                 case NotifyDictionaryChangedAction.Add:
                 case NotifyDictionaryChangedAction.Replace:
-                    foreach ((int userId, var state) in e.NewItems.AsNonNull())
+                    foreach ((int userId, SpectatorState state) in e.NewItems.AsNonNull())
                         onUserStateChanged(userId, state);
                     break;
-
-                case NotifyDictionaryChangedAction.Remove:
-                    foreach ((int userId, SpectatorState state) in e.OldItems.AsNonNull())
-                        onUserStateRemoved(userId, state);
-                    break;
             }
         }
 
@@ -136,33 +131,21 @@ namespace osu.Game.Screens.Spectate
 
             switch (newState.State)
             {
-                case SpectatedUserState.Passed:
-                    // Make sure that gameplay completes to the end.
-                    if (gameplayStates.TryGetValue(userId, out var gameplayState))
-                        gameplayState.Score.Replay.HasReceivedAllFrames = true;
-                    break;
-
                 case SpectatedUserState.Playing:
                     Schedule(() => OnNewPlayingUserState(userId, newState));
                     startGameplay(userId);
                     break;
+
+                case SpectatedUserState.Passed:
+                    markReceivedAllFrames(userId);
+                    break;
+
+                case SpectatedUserState.Quit:
+                    quitGameplay(userId);
+                    break;
             }
         }
 
-        private void onUserStateRemoved(int userId, SpectatorState state)
-        {
-            if (!userMap.ContainsKey(userId))
-                return;
-
-            if (!gameplayStates.TryGetValue(userId, out var gameplayState))
-                return;
-
-            gameplayState.Score.Replay.HasReceivedAllFrames = true;
-
-            gameplayStates.Remove(userId);
-            Schedule(() => EndGameplay(userId, state));
-        }
-
         private void startGameplay(int userId)
         {
             Debug.Assert(userMap.ContainsKey(userId));
@@ -196,6 +179,29 @@ namespace osu.Game.Screens.Spectate
             Schedule(() => StartGameplay(userId, gameplayState));
         }
 
+        /// <summary>
+        /// Marks an existing gameplay session as received all frames.
+        /// </summary>
+        private void markReceivedAllFrames(int userId)
+        {
+            if (gameplayStates.TryGetValue(userId, out var gameplayState))
+                gameplayState.Score.Replay.HasReceivedAllFrames = true;
+        }
+
+        private void quitGameplay(int userId)
+        {
+            if (!userMap.ContainsKey(userId))
+                return;
+
+            if (!gameplayStates.ContainsKey(userId))
+                return;
+
+            markReceivedAllFrames(userId);
+
+            gameplayStates.Remove(userId);
+            Schedule(() => QuitGameplay(userId));
+        }
+
         /// <summary>
         /// Invoked when a spectated user's state has changed to a new state indicating the player is currently playing.
         /// </summary>
@@ -211,11 +217,10 @@ namespace osu.Game.Screens.Spectate
         protected abstract void StartGameplay(int userId, [NotNull] SpectatorGameplayState spectatorGameplayState);
 
         /// <summary>
-        /// Ends gameplay for a user.
+        /// Quits gameplay for a user.
         /// </summary>
-        /// <param name="userId">The user to end gameplay for.</param>
+        /// <param name="userId">The user to quit gameplay for.</param>
-        /// <param name="state">The final user state.</param>
+        protected abstract void QuitGameplay(int userId);
-        protected abstract void EndGameplay(int userId, SpectatorState state);
 
         /// <summary>
         /// Stops spectating a user.
@@ -223,10 +228,10 @@ namespace osu.Game.Screens.Spectate
         /// <param name="userId">The user to stop spectating.</param>
         protected void RemoveUser(int userId)
         {
-            if (!userStates.TryGetValue(userId, out var state))
+            if (!userStates.ContainsKey(userId))
                 return;
 
-            onUserStateRemoved(userId, state);
+            quitGameplay(userId);
 
             users.Remove(userId);
             userMap.Remove(userId);