Merge pull request #11351 from bdach/client-playing-users

Change PlayingUsers population logic to match expectations

osu.Game.Tests/NonVisual/Multiplayer/StatefulMultiplayerClientTest.cs

@@ -0,0 +1,57 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System.Linq;
+using Humanizer;
+using NUnit.Framework;
+using osu.Framework.Testing;
+using osu.Game.Online.Multiplayer;
+using osu.Game.Tests.Visual.Multiplayer;
+using osu.Game.Users;
+
+namespace osu.Game.Tests.NonVisual.Multiplayer
+{
+    [HeadlessTest]
+    public class StatefulMultiplayerClientTest : MultiplayerTestScene
+    {
+        [Test]
+        public void TestPlayingUserTracking()
+        {
+            int id = 2000;
+
+            AddRepeatStep("add some users", () => Client.AddUser(new User { Id = id++ }), 5);
+            checkPlayingUserCount(0);
+
+            changeState(3, MultiplayerUserState.WaitingForLoad);
+            checkPlayingUserCount(3);
+
+            changeState(3, MultiplayerUserState.Playing);
+            checkPlayingUserCount(3);
+
+            changeState(3, MultiplayerUserState.Results);
+            checkPlayingUserCount(0);
+
+            changeState(6, MultiplayerUserState.WaitingForLoad);
+            checkPlayingUserCount(6);
+
+            AddStep("another user left", () => Client.RemoveUser(Client.Room?.Users.Last().User));
+            checkPlayingUserCount(5);
+
+            AddStep("leave room", () => Client.LeaveRoom());
+            checkPlayingUserCount(0);
+        }
+
+        private void checkPlayingUserCount(int expectedCount)
+            => AddAssert($"{"user".ToQuantity(expectedCount)} playing", () => Client.CurrentMatchPlayingUserIds.Count == expectedCount);
+
+        private void changeState(int userCount, MultiplayerUserState state)
+            => AddStep($"{"user".ToQuantity(userCount)} in {state}", () =>
+            {
+                for (int i = 0; i < userCount; ++i)
+                {
+                    var userId = Client.Room?.Users[i].UserID ?? throw new AssertionException("Room cannot be null!");
+                    Client.ChangeUserState(userId, state);
+                }
+            });
+    }
+}

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

@@ -62,8 +62,8 @@ namespace osu.Game.Tests.Visual.Multiplayer
 
                 streamingClient.Start(Beatmap.Value.BeatmapInfo.OnlineBeatmapID ?? 0);
 
-                Client.PlayingUsers.Clear();
+                Client.CurrentMatchPlayingUserIds.Clear();
-                Client.PlayingUsers.AddRange(streamingClient.PlayingUsers);
+                Client.CurrentMatchPlayingUserIds.AddRange(streamingClient.PlayingUsers);
 
                 Children = new Drawable[]
                 {
@@ -91,7 +91,7 @@ namespace osu.Game.Tests.Visual.Multiplayer
         [Test]
         public void TestUserQuit()
         {
-            AddRepeatStep("mark user quit", () => Client.PlayingUsers.RemoveAt(0), users);
+            AddRepeatStep("mark user quit", () => Client.CurrentMatchPlayingUserIds.RemoveAt(0), users);
         }
 
         public class TestMultiplayerStreaming : SpectatorStreamingClient

osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs

@@ -61,9 +61,9 @@ namespace osu.Game.Online.Multiplayer
         public MultiplayerRoom? Room { get; private set; }
 
         /// <summary>
-        /// The users currently in gameplay.
+        /// The users in the joined <see cref="Room"/> which are participating in the current gameplay loop.
         /// </summary>
-        public readonly BindableList<int> PlayingUsers = new BindableList<int>();
+        public readonly BindableList<int> CurrentMatchPlayingUserIds = new BindableList<int>();
 
         [Resolved]
         private UserLookupCache userLookupCache { get; set; } = null!;
@@ -133,6 +133,7 @@ namespace osu.Game.Online.Multiplayer
 
                 apiRoom = null;
                 Room = null;
+                CurrentMatchPlayingUserIds.Clear();
 
                 RoomUpdated?.Invoke();
             }, false);
@@ -253,7 +254,7 @@ namespace osu.Game.Online.Multiplayer
                     return;
 
                 Room.Users.Remove(user);
-                PlayingUsers.Remove(user.UserID);
+                CurrentMatchPlayingUserIds.Remove(user.UserID);
 
                 RoomUpdated?.Invoke();
             }, false);
@@ -302,8 +303,7 @@ namespace osu.Game.Online.Multiplayer
 
                 Room.Users.Single(u => u.UserID == userId).State = state;
 
-                if (state != MultiplayerUserState.Playing)
+                updateUserPlayingState(userId, state);
-                    PlayingUsers.Remove(userId);
 
                 RoomUpdated?.Invoke();
             }, false);
@@ -337,8 +337,6 @@ namespace osu.Game.Online.Multiplayer
                 if (Room == null)
                     return;
 
-                PlayingUsers.AddRange(Room.Users.Where(u => u.State == MultiplayerUserState.Playing).Select(u => u.UserID));
-
                 MatchStarted?.Invoke();
             }, false);
 
@@ -454,5 +452,24 @@ namespace osu.Game.Online.Multiplayer
             apiRoom.Playlist.Clear(); // Clearing should be unnecessary, but here for sanity.
             apiRoom.Playlist.Add(playlistItem);
         }
+
+        /// <summary>
+        /// For the provided user ID, update whether the user is included in <see cref="CurrentMatchPlayingUserIds"/>.
+        /// </summary>
+        /// <param name="userId">The user's ID.</param>
+        /// <param name="state">The new state of the user.</param>
+        private void updateUserPlayingState(int userId, MultiplayerUserState state)
+        {
+            bool wasPlaying = CurrentMatchPlayingUserIds.Contains(userId);
+            bool isPlaying = state >= MultiplayerUserState.WaitingForLoad && state <= MultiplayerUserState.FinishedPlay;
+
+            if (isPlaying == wasPlaying)
+                return;
+
+            if (isPlaying)
+                CurrentMatchPlayingUserIds.Add(userId);
+            else
+                CurrentMatchPlayingUserIds.Remove(userId);
+        }
     }
 }

osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs

@@ -200,7 +200,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer
         {
             Debug.Assert(client.Room != null);
 
-            int[] userIds = client.Room.Users.Where(u => u.State >= MultiplayerUserState.WaitingForLoad).Select(u => u.UserID).ToArray();
+            int[] userIds = client.CurrentMatchPlayingUserIds.ToArray();
 
             StartPlay(() => new MultiplayerPlayer(SelectedItem.Value, userIds));
         }

osu.Game/Screens/Play/HUD/MultiplayerGameplayLeaderboard.cs

@@ -84,11 +84,11 @@ namespace osu.Game.Screens.Play.HUD
             // BindableList handles binding in a really bad way (Clear then AddRange) so we need to do this manually..
             foreach (int userId in playingUsers)
             {
-                if (!multiplayerClient.PlayingUsers.Contains(userId))
+                if (!multiplayerClient.CurrentMatchPlayingUserIds.Contains(userId))
                     usersChanged(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, new[] { userId }));
             }
 
-            playingUsers.BindTo(multiplayerClient.PlayingUsers);
+            playingUsers.BindTo(multiplayerClient.CurrentMatchPlayingUserIds);
             playingUsers.BindCollectionChanged(usersChanged);
         }