Merge pull request #12438 from smoogipoo/fix-initial-spectator-state-callback

Fix OnUserBeganPlaying not being invoked if already watching

osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs

@@ -288,7 +288,7 @@ namespace osu.Game.Tests.Visual.Gameplay
 
             public override void WatchUser(int userId)
             {
-                if (sentState)
+                if (!PlayingUsers.Contains(userId) && sentState)
                 {
                     // usually the server would do this.
                     sendState(beatmapId);

osu.Game/Online/Spectator/SpectatorStreamingClient.cs

@@ -47,6 +47,8 @@ namespace osu.Game.Online.Spectator
 
         private readonly BindableList<int> playingUsers = new BindableList<int>();
 
+        private readonly Dictionary<int, SpectatorState> playingUserStates = new Dictionary<int, SpectatorState>();
+
         [CanBeNull]
         private IBeatmap currentBeatmap;
 
@@ -69,7 +71,7 @@ namespace osu.Game.Online.Spectator
         public event Action<int, FrameDataBundle> OnNewFrames;
 
         /// <summary>
-        /// Called whenever a user starts a play session.
+        /// Called whenever a user starts a play session, or immediately if the user is being watched and currently in a play session.
         /// </summary>
         public event Action<int, SpectatorState> OnUserBeganPlaying;
 
@@ -123,7 +125,11 @@ namespace osu.Game.Online.Spectator
                     }
                     else
                     {
-                        playingUsers.Clear();
+                        lock (userLock)
+                        {
+                            playingUsers.Clear();
+                            playingUserStates.Clear();
+                        }
                     }
                 }, true);
             }
@@ -131,8 +137,13 @@ namespace osu.Game.Online.Spectator
 
         Task ISpectatorClient.UserBeganPlaying(int userId, SpectatorState state)
         {
-            if (!playingUsers.Contains(userId))
-                playingUsers.Add(userId);
+            lock (userLock)
+            {
+                if (!playingUsers.Contains(userId))
+                    playingUsers.Add(userId);
+
+                playingUserStates[userId] = state;
+            }
 
             OnUserBeganPlaying?.Invoke(userId, state);
 
@@ -141,7 +152,11 @@ namespace osu.Game.Online.Spectator
 
         Task ISpectatorClient.UserFinishedPlaying(int userId, SpectatorState state)
         {
-            playingUsers.Remove(userId);
+            lock (userLock)
+            {
+                playingUsers.Remove(userId);
+                playingUserStates.Remove(userId);
+            }
 
             OnUserFinishedPlaying?.Invoke(userId, state);
 
@@ -268,5 +283,37 @@ namespace osu.Game.Online.Spectator
 
             lastSendTime = Time.Current;
         }
+
+        /// <summary>
+        /// Attempts to retrieve the <see cref="SpectatorState"/> for a currently-playing user.
+        /// </summary>
+        /// <param name="userId">The user.</param>
+        /// <param name="state">The current <see cref="SpectatorState"/> for the user, if they're playing. <c>null</c> if the user is not playing.</param>
+        /// <returns><c>true</c> if successful (the user is playing), <c>false</c> otherwise.</returns>
+        public bool TryGetPlayingUserState(int userId, out SpectatorState state)
+        {
+            lock (userLock)
+                return playingUserStates.TryGetValue(userId, out state);
+        }
+
+        /// <summary>
+        /// Bind an action to <see cref="OnUserBeganPlaying"/> with the option of running the bound action once immediately.
+        /// </summary>
+        /// <param name="callback">The action to perform when a user begins playing.</param>
+        /// <param name="runOnceImmediately">Whether the action provided in <paramref name="callback"/> should be run once immediately for all users currently playing.</param>
+        public void BindUserBeganPlaying(Action<int, SpectatorState> callback, bool runOnceImmediately = false)
+        {
+            // The lock is taken before the event is subscribed to to prevent doubling of events.
+            lock (userLock)
+            {
+                OnUserBeganPlaying += callback;
+
+                if (!runOnceImmediately)
+                    return;
+
+                foreach (var (userId, state) in playingUserStates)
+                    callback(userId, state);
+            }
+        }
     }
 }

osu.Game/Screens/Spectate/SpectatorScreen.cs

@@ -44,7 +44,6 @@ namespace osu.Game.Screens.Spectate
         private readonly object stateLock = new object();
 
         private readonly Dictionary<int, User> userMap = new Dictionary<int, User>();
-        private readonly Dictionary<int, SpectatorState> spectatorStates = new Dictionary<int, SpectatorState>();
         private readonly Dictionary<int, GameplayState> gameplayStates = new Dictionary<int, GameplayState>();
 
         private IBindable<WeakReference<BeatmapSetInfo>> managerUpdated;
@@ -62,26 +61,42 @@ namespace osu.Game.Screens.Spectate
         {
             base.LoadComplete();
 
-            spectatorClient.OnUserBeganPlaying += userBeganPlaying;
-            spectatorClient.OnUserFinishedPlaying += userFinishedPlaying;
-            spectatorClient.OnNewFrames += userSentFrames;
-
-            foreach (var id in userIds)
+            populateAllUsers().ContinueWith(_ => Schedule(() =>
             {
-                userLookupCache.GetUserAsync(id).ContinueWith(u => Schedule(() =>
+                spectatorClient.BindUserBeganPlaying(userBeganPlaying, true);
+                spectatorClient.OnUserFinishedPlaying += userFinishedPlaying;
+                spectatorClient.OnNewFrames += userSentFrames;
+
+                managerUpdated = beatmaps.ItemUpdated.GetBoundCopy();
+                managerUpdated.BindValueChanged(beatmapUpdated);
+
+                lock (stateLock)
                 {
-                    if (u.Result == null)
+                    foreach (var (id, _) in userMap)
+                        spectatorClient.WatchUser(id);
+                }
+            }));
+        }
+
+        private Task populateAllUsers()
+        {
+            var userLookupTasks = new Task[userIds.Length];
+
+            for (int i = 0; i < userIds.Length; i++)
+            {
+                var userId = userIds[i];
+
+                userLookupTasks[i] = userLookupCache.GetUserAsync(userId).ContinueWith(task =>
+                {
+                    if (!task.IsCompletedSuccessfully)
                         return;
 
                     lock (stateLock)
-                        userMap[id] = u.Result;
-
-                    spectatorClient.WatchUser(id);
-                }), TaskContinuationOptions.OnlyOnRanToCompletion);
+                        userMap[userId] = task.Result;
+                });
             }
 
-            managerUpdated = beatmaps.ItemUpdated.GetBoundCopy();
-            managerUpdated.BindValueChanged(beatmapUpdated);
+            return Task.WhenAll(userLookupTasks);
         }
 
         private void beatmapUpdated(ValueChangedEvent<WeakReference<BeatmapSetInfo>> e)
@@ -91,9 +106,12 @@ namespace osu.Game.Screens.Spectate
 
             lock (stateLock)
             {
-                foreach (var (userId, state) in spectatorStates)
+                foreach (var (userId, _) in userMap)
                 {
-                    if (beatmapSet.Beatmaps.Any(b => b.OnlineBeatmapID == state.BeatmapID))
+                    if (!spectatorClient.TryGetPlayingUserState(userId, out var userState))
+                        continue;
+
+                    if (beatmapSet.Beatmaps.Any(b => b.OnlineBeatmapID == userState.BeatmapID))
                         updateGameplayState(userId);
                 }
             }
@@ -109,7 +127,10 @@ namespace osu.Game.Screens.Spectate
                 if (!userMap.ContainsKey(userId))
                     return;
 
-                spectatorStates[userId] = state;
+                // The user may have stopped playing.
+                if (!spectatorClient.TryGetPlayingUserState(userId, out _))
+                    return;
+
                 Schedule(() => OnUserStateChanged(userId, state));
 
                 updateGameplayState(userId);
@@ -122,7 +143,10 @@ namespace osu.Game.Screens.Spectate
             {
                 Debug.Assert(userMap.ContainsKey(userId));
 
-                var spectatorState = spectatorStates[userId];
+                // The user may have stopped playing.
+                if (!spectatorClient.TryGetPlayingUserState(userId, out var spectatorState))
+                    return;
+
                 var user = userMap[userId];
 
                 var resolvedRuleset = rulesets.AvailableRulesets.FirstOrDefault(r => r.ID == spectatorState.RulesetID)?.CreateInstance();