Merge pull request #12885 from smoogipoo/fix-spectator-playing-state-5

Fix crash when re-spectating a multiplayer room

osu.Android.props

@@ -52,6 +52,6 @@
   </ItemGroup>
   <ItemGroup>
     <PackageReference Include="ppy.osu.Game.Resources" Version="2021.422.0" />
-    <PackageReference Include="ppy.osu.Framework.Android" Version="2021.513.0" />
+    <PackageReference Include="ppy.osu.Framework.Android" Version="2021.521.0" />
   </ItemGroup>
 </Project>

osu.Game/Online/Spectator/SpectatorClient.cs

@@ -40,10 +40,10 @@ namespace osu.Game.Online.Spectator
         private readonly List<int> watchingUsers = new List<int>();
 
         public IBindableList<int> PlayingUsers => playingUsers;
-
         private readonly BindableList<int> playingUsers = new BindableList<int>();
 
-        private readonly Dictionary<int, SpectatorState> playingUserStates = new Dictionary<int, SpectatorState>();
+        public IBindableDictionary<int, SpectatorState> PlayingUserStates => playingUserStates;
+        private readonly BindableDictionary<int, SpectatorState> playingUserStates = new BindableDictionary<int, SpectatorState>();
 
         private IBeatmap? currentBeatmap;
 
@@ -200,6 +200,7 @@ namespace osu.Game.Online.Spectator
             Schedule(() =>
             {
                 watchingUsers.Remove(userId);
+                playingUserStates.Remove(userId);
                 StopWatchingUserInternal(userId);
             });
         }
@@ -256,33 +257,5 @@ 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)
-        {
-            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.
-            OnUserBeganPlaying += callback;
-
-            if (!runOnceImmediately)
-                return;
-
-            foreach (var (userId, state) in playingUserStates)
-                callback(userId, state);
-        }
     }
 }

osu.Game/Screens/Spectate/SpectatorScreen.cs

@@ -9,6 +9,7 @@ using System.Threading.Tasks;
 using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
+using osu.Framework.Extensions.ObjectExtensions;
 using osu.Game.Beatmaps;
 using osu.Game.Database;
 using osu.Game.Online.Spectator;
@@ -42,6 +43,8 @@ namespace osu.Game.Screens.Spectate
         [Resolved]
         private UserLookupCache userLookupCache { get; set; }
 
+        private readonly IBindableDictionary<int, SpectatorState> playingUserStates = new BindableDictionary<int, SpectatorState>();
+
         private readonly Dictionary<int, User> userMap = new Dictionary<int, User>();
         private readonly Dictionary<int, GameplayState> gameplayStates = new Dictionary<int, GameplayState>();
 
@@ -65,8 +68,9 @@ namespace osu.Game.Screens.Spectate
                 foreach (var u in users.Result)
                     userMap[u.Id] = u;
 
-                spectatorClient.BindUserBeganPlaying(userBeganPlaying, true);
+                playingUserStates.BindTo(spectatorClient.PlayingUserStates);
-                spectatorClient.OnUserFinishedPlaying += userFinishedPlaying;
+                playingUserStates.BindCollectionChanged(onPlayingUserStatesChanged, true);
+
                 spectatorClient.OnNewFrames += userSentFrames;
 
                 managerUpdated = beatmaps.ItemUpdated.GetBoundCopy();
@@ -102,7 +106,7 @@ namespace osu.Game.Screens.Spectate
 
             foreach (var (userId, _) in userMap)
             {
-                if (!spectatorClient.TryGetPlayingUserState(userId, out var userState))
+                if (!playingUserStates.TryGetValue(userId, out var userState))
                     continue;
 
                 if (beatmapSet.Beatmaps.Any(b => b.OnlineBeatmapID == userState.BeatmapID))
@@ -110,7 +114,31 @@ namespace osu.Game.Screens.Spectate
             }
         }
 
-        private void userBeganPlaying(int userId, SpectatorState state)
+        private void onPlayingUserStatesChanged(object sender, NotifyDictionaryChangedEventArgs<int, SpectatorState> e)
+        {
+            switch (e.Action)
+            {
+                case NotifyDictionaryChangedAction.Add:
+                    foreach (var (userId, state) in e.NewItems.AsNonNull())
+                        onUserStateAdded(userId, state);
+                    break;
+
+                case NotifyDictionaryChangedAction.Remove:
+                    foreach (var (userId, _) in e.OldItems.AsNonNull())
+                        onUserStateRemoved(userId);
+                    break;
+
+                case NotifyDictionaryChangedAction.Replace:
+                    foreach (var (userId, _) in e.OldItems.AsNonNull())
+                        onUserStateRemoved(userId);
+
+                    foreach (var (userId, state) in e.NewItems.AsNonNull())
+                        onUserStateAdded(userId, state);
+                    break;
+            }
+        }
+
+        private void onUserStateAdded(int userId, SpectatorState state)
         {
             if (state.RulesetID == null || state.BeatmapID == null)
                 return;
@@ -118,24 +146,30 @@ namespace osu.Game.Screens.Spectate
             if (!userMap.ContainsKey(userId))
                 return;
 
-            // The user may have stopped playing.
+            Schedule(() => OnUserStateChanged(userId, state));
-            if (!spectatorClient.TryGetPlayingUserState(userId, out _))
+            updateGameplayState(userId);
+        }
+
+        private void onUserStateRemoved(int userId)
+        {
+            if (!userMap.ContainsKey(userId))
                 return;
 
-            Schedule(() => OnUserStateChanged(userId, state));
+            if (!gameplayStates.TryGetValue(userId, out var gameplayState))
+                return;
 
-            updateGameplayState(userId);
+            gameplayState.Score.Replay.HasReceivedAllFrames = true;
+
+            gameplayStates.Remove(userId);
+            Schedule(() => EndGameplay(userId));
         }
 
         private void updateGameplayState(int userId)
         {
             Debug.Assert(userMap.ContainsKey(userId));
 
-            // The user may have stopped playing.
-            if (!spectatorClient.TryGetPlayingUserState(userId, out var spectatorState))
-                return;
-
             var user = userMap[userId];
+            var spectatorState = playingUserStates[userId];
 
             var resolvedRuleset = rulesets.AvailableRulesets.FirstOrDefault(r => r.ID == spectatorState.RulesetID)?.CreateInstance();
             if (resolvedRuleset == null)
@@ -186,20 +220,6 @@ namespace osu.Game.Screens.Spectate
             }
         }
 
-        private void userFinishedPlaying(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));
-        }
-
         /// <summary>
         /// Invoked when a spectated user's state has changed.
         /// </summary>
@@ -226,7 +246,7 @@ namespace osu.Game.Screens.Spectate
         /// <param name="userId">The user to stop spectating.</param>
         protected void RemoveUser(int userId)
         {
-            userFinishedPlaying(userId, null);
+            onUserStateRemoved(userId);
 
             userIds.Remove(userId);
             userMap.Remove(userId);
@@ -240,8 +260,6 @@ namespace osu.Game.Screens.Spectate
 
             if (spectatorClient != null)
             {
-                spectatorClient.OnUserBeganPlaying -= userBeganPlaying;
-                spectatorClient.OnUserFinishedPlaying -= userFinishedPlaying;
                 spectatorClient.OnNewFrames -= userSentFrames;
 
                 foreach (var (userId, _) in userMap)

osu.Game/osu.Game.csproj

@@ -29,7 +29,7 @@
     <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="5.0.0" />
     <PackageReference Include="Microsoft.NETCore.Targets" Version="3.1.0" />
     <PackageReference Include="Newtonsoft.Json" Version="13.0.1" />
-    <PackageReference Include="ppy.osu.Framework" Version="2021.513.0" />
+    <PackageReference Include="ppy.osu.Framework" Version="2021.521.0" />
     <PackageReference Include="ppy.osu.Game.Resources" Version="2021.422.0" />
     <PackageReference Include="Sentry" Version="3.3.4" />
     <PackageReference Include="SharpCompress" Version="0.28.2" />

osu.iOS.props

@@ -70,7 +70,7 @@
     <Reference Include="System.Net.Http" />
   </ItemGroup>
   <ItemGroup Label="Package References">
-    <PackageReference Include="ppy.osu.Framework.iOS" Version="2021.513.0" />
+    <PackageReference Include="ppy.osu.Framework.iOS" Version="2021.521.0" />
     <PackageReference Include="ppy.osu.Game.Resources" Version="2021.422.0" />
   </ItemGroup>
   <!-- See https://github.com/dotnet/runtime/issues/35988 (can be removed after Xamarin uses net5.0 / net6.0) -->
@@ -93,7 +93,7 @@
     <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="2.2.6" />
     <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite.Core" Version="2.2.6" />
     <PackageReference Include="Newtonsoft.Json" Version="12.0.3" />
-    <PackageReference Include="ppy.osu.Framework" Version="2021.513.0" />
+    <PackageReference Include="ppy.osu.Framework" Version="2021.521.0" />
     <PackageReference Include="SharpCompress" Version="0.28.2" />
     <PackageReference Include="NUnit" Version="3.13.2" />
     <PackageReference Include="SharpRaven" Version="2.4.0" />