Merge pull request ppy#31844 from bdach/fix-mp-players-appearing-as-spectators

Fix spectator list showing other users in multiplayer room even if they're not spectating

Author: peppy

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

@@ -8,11 +8,14 @@
 using osu.Framework.Utils;
 using osu.Game.Beatmaps;
 using osu.Game.Graphics;
+using osu.Game.Online.Multiplayer;
 using osu.Game.Online.Spectator;
 using osu.Game.Rulesets.Osu;
 using osu.Game.Rulesets.Osu.Scoring;
 using osu.Game.Screens.Play;
 using osu.Game.Screens.Play.HUD;
+using osu.Game.Tests.Visual.Multiplayer;
+using osu.Game.Tests.Visual.OnlinePlay;
 using osu.Game.Tests.Visual.Spectator;
 
 namespace osu.Game.Tests.Visual.Gameplay
@@ -28,20 +31,23 @@ public void TestBasics()
             SpectatorList list = null!;
             Bindable<LocalUserPlayingState> playingState = new Bindable<LocalUserPlayingState>();
             GameplayState gameplayState = new GameplayState(new Beatmap(), new OsuRuleset(), healthProcessor: new OsuHealthProcessor(0), localUserPlayingState: playingState);
-            TestSpectatorClient client = new TestSpectatorClient();
+            TestSpectatorClient spectatorClient = new TestSpectatorClient();
+            TestMultiplayerClient multiplayerClient = new TestMultiplayerClient(new TestMultiplayerRoomManager(new TestRoomRequestsHandler()));
 
             AddStep("create spectator list", () =>
             {
                 Children = new Drawable[]
                 {
-                    client,
+                    spectatorClient,
+                    multiplayerClient,
                     new DependencyProvidingContainer
                     {
                         RelativeSizeAxes = Axes.Both,
                         CachedDependencies =
                         [
                             (typeof(GameplayState), gameplayState),
-                            (typeof(SpectatorClient), client)
+                            (typeof(SpectatorClient), spectatorClient),
+                            (typeof(MultiplayerClient), multiplayerClient),
                         ],
                         Child = list = new SpectatorList
                         {
@@ -57,7 +63,7 @@ public void TestBasics()
             AddRepeatStep("add a user", () =>
             {
                 int id = Interlocked.Increment(ref counter);
-                ((ISpectatorClient)client).UserStartedWatching([
+                ((ISpectatorClient)spectatorClient).UserStartedWatching([
                     new SpectatorUser
                     {
                         OnlineID = id,
@@ -66,7 +72,8 @@ public void TestBasics()
                 ]);
             }, 10);
 
-            AddRepeatStep("remove random user", () => ((ISpectatorClient)client).UserEndedWatching(client.WatchingUsers[RNG.Next(client.WatchingUsers.Count)].OnlineID), 5);
+            AddRepeatStep("remove random user", () => ((ISpectatorClient)spectatorClient).UserEndedWatching(
+                spectatorClient.WatchingUsers[RNG.Next(spectatorClient.WatchingUsers.Count)].OnlineID), 5);
 
             AddStep("change font to venera", () => list.Font.Value = Typeface.Venera);
             AddStep("change font to torus", () => list.Font.Value = Typeface.Torus);

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

@@ -2,7 +2,9 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Collections.Generic;
 using System.Collections.Specialized;
+using System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Extensions.Color4Extensions;
@@ -17,6 +19,7 @@
 using osu.Game.Online.Chat;
 using osu.Game.Localisation.HUD;
 using osu.Game.Localisation.SkinComponents;
+using osu.Game.Online.Multiplayer;
 using osu.Game.Online.Spectator;
 using osu.Game.Skinning;
 using osuTK;
@@ -28,17 +31,17 @@ public partial class SpectatorList : CompositeDrawable, ISerialisableDrawable
     {
         private const int max_spectators_displayed = 10;
 
-        public BindableList<SpectatorUser> Spectators { get; } = new BindableList<SpectatorUser>();
-        public Bindable<LocalUserPlayingState> UserPlayingState { get; } = new Bindable<LocalUserPlayingState>();
-
         [SettingSource(typeof(SkinnableComponentStrings), nameof(SkinnableComponentStrings.Font), nameof(SkinnableComponentStrings.FontDescription))]
         public Bindable<Typeface> Font { get; } = new Bindable<Typeface>(Typeface.Torus);
 
         [SettingSource(typeof(SkinnableComponentStrings), nameof(SkinnableComponentStrings.TextColour), nameof(SkinnableComponentStrings.TextColourDescription))]
         public BindableColour4 HeaderColour { get; } = new BindableColour4(Colour4.White);
 
-        protected OsuSpriteText Header { get; private set; } = null!;
+        private BindableList<SpectatorUser> watchingUsers { get; } = new BindableList<SpectatorUser>();
+        private Bindable<LocalUserPlayingState> userPlayingState { get; } = new Bindable<LocalUserPlayingState>();
+        private int displayedSpectatorCount;
 
+        private OsuSpriteText header = null!;
         private FillFlowContainer mainFlow = null!;
         private FillFlowContainer<SpectatorListEntry> spectatorsFlow = null!;
         private DrawablePool<SpectatorListEntry> pool = null!;
@@ -49,6 +52,9 @@ public partial class SpectatorList : CompositeDrawable, ISerialisableDrawable
         [Resolved]
         private GameplayState gameplayState { get; set; } = null!;
 
+        [Resolved]
+        private MultiplayerClient multiplayerClient { get; set; } = null!;
+
         [BackgroundDependencyLoader]
         private void load(OsuColour colours)
         {
@@ -63,7 +69,7 @@ private void load(OsuColour colours)
                     Direction = FillDirection.Vertical,
                     Children = new Drawable[]
                     {
-                        Header = new OsuSpriteText
+                        header = new OsuSpriteText
                         {
                             Colour = colours.Blue0,
                             Font = OsuFont.GetFont(size: 12, weight: FontWeight.Bold),
@@ -78,18 +84,18 @@ private void load(OsuColour colours)
                 pool = new DrawablePool<SpectatorListEntry>(max_spectators_displayed),
             };
 
-            HeaderColour.Value = Header.Colour;
+            HeaderColour.Value = header.Colour;
         }
 
         protected override void LoadComplete()
         {
             base.LoadComplete();
 
-            ((IBindableList<SpectatorUser>)Spectators).BindTo(client.WatchingUsers);
-            ((IBindable<LocalUserPlayingState>)UserPlayingState).BindTo(gameplayState.PlayingState);
+            ((IBindableList<SpectatorUser>)watchingUsers).BindTo(client.WatchingUsers);
+            ((IBindable<LocalUserPlayingState>)userPlayingState).BindTo(gameplayState.PlayingState);
 
-            Spectators.BindCollectionChanged(onSpectatorsChanged, true);
-            UserPlayingState.BindValueChanged(_ => updateVisibility());
+            watchingUsers.BindCollectionChanged(onSpectatorsChanged, true);
+            userPlayingState.BindValueChanged(_ => updateVisibility());
 
             Font.BindValueChanged(_ => updateAppearance());
             HeaderColour.BindValueChanged(_ => updateAppearance(), true);
@@ -100,6 +106,20 @@ protected override void LoadComplete()
 
         private void onSpectatorsChanged(object? sender, NotifyCollectionChangedEventArgs e)
         {
+            // the multiplayer gameplay leaderboard relies on calling `SpectatorClient.WatchUser()` to get updates on users' total scores.
+            // this has an unfortunate side effect of other players showing up in `SpectatorClient.WatchingUsers`.
+            //
+            // we do not generally wish to display other players in the room as spectators due to that implementation detail,
+            // therefore this code is intended to filter out those players on the client side.
+            //
+            // note that the way that this is done is rather specific to the multiplayer use case and therefore carries a lot of assumptions
+            // (e.g. that the `MultiplayerRoomUser`s have the correct `State` at the point wherein they issue the `WatchUser()` calls).
+            // the more proper way to do this (which is by subscribing to `WatchingUsers` and `RoomUpdated`, and doing a proper diff to a third list on any change of either)
+            // is a lot more difficult to write correctly, given that we also rely on `BindableList`'s collection changed event arguments to properly animate this component.
+            var excludedUserIds = new HashSet<int>();
+            if (multiplayerClient.Room != null)
+                excludedUserIds.UnionWith(multiplayerClient.Room.Users.Where(u => u.State != MultiplayerUserState.Spectating).Select(u => u.UserID));
+
             switch (e.Action)
             {
                 case NotifyCollectionChangedAction.Add:
@@ -109,6 +129,9 @@ private void onSpectatorsChanged(object? sender, NotifyCollectionChangedEventArg
                         var spectator = (SpectatorUser)e.NewItems![i]!;
                         int index = Math.Max(e.NewStartingIndex, 0) + i;
 
+                        if (excludedUserIds.Contains(spectator.OnlineID))
+                            continue;
+
                         if (index >= max_spectators_displayed)
                             break;
 
@@ -125,10 +148,10 @@ private void onSpectatorsChanged(object? sender, NotifyCollectionChangedEventArg
                     for (int i = 0; i < spectatorsFlow.Count; i++)
                         spectatorsFlow.SetLayoutPosition(spectatorsFlow[i], i);
 
-                    if (Spectators.Count >= max_spectators_displayed && spectatorsFlow.Count < max_spectators_displayed)
+                    if (watchingUsers.Count >= max_spectators_displayed && spectatorsFlow.Count < max_spectators_displayed)
                     {
                         for (int i = spectatorsFlow.Count; i < max_spectators_displayed; i++)
-                            addNewSpectatorToList(i, Spectators[i]);
+                            addNewSpectatorToList(i, watchingUsers[i]);
                     }
 
                     break;
@@ -144,7 +167,8 @@ private void onSpectatorsChanged(object? sender, NotifyCollectionChangedEventArg
                     throw new NotSupportedException();
             }
 
-            Header.Text = SpectatorListStrings.SpectatorCount(Spectators.Count).ToUpper();
+            displayedSpectatorCount = watchingUsers.Count(s => !excludedUserIds.Contains(s.OnlineID));
+            header.Text = SpectatorListStrings.SpectatorCount(displayedSpectatorCount).ToUpper();
             updateVisibility();
 
             for (int i = 0; i < spectatorsFlow.Count; i++)
@@ -160,7 +184,7 @@ private void addNewSpectatorToList(int i, SpectatorUser spectator)
             var entry = pool.Get(entry =>
             {
                 entry.Current.Value = spectator;
-                entry.UserPlayingState = UserPlayingState;
+                entry.UserPlayingState = userPlayingState;
             });
 
             spectatorsFlow.Insert(i, entry);
@@ -169,15 +193,15 @@ private void addNewSpectatorToList(int i, SpectatorUser spectator)
         private void updateVisibility()
         {
             // We don't want to show spectators when we are watching a replay.
-            mainFlow.FadeTo(Spectators.Count > 0 && UserPlayingState.Value != LocalUserPlayingState.NotPlaying ? 1 : 0, 250, Easing.OutQuint);
+            mainFlow.FadeTo(displayedSpectatorCount > 0 && userPlayingState.Value != LocalUserPlayingState.NotPlaying ? 1 : 0, 250, Easing.OutQuint);
         }
 
         private void updateAppearance()
         {
-            Header.Font = OsuFont.GetFont(Font.Value, 12, FontWeight.Bold);
-            Header.Colour = HeaderColour.Value;
+            header.Font = OsuFont.GetFont(Font.Value, 12, FontWeight.Bold);
+            header.Colour = HeaderColour.Value;
 
-            Width = Header.DrawWidth;
+            Width = header.DrawWidth;
         }
 
         private partial class SpectatorListEntry : PoolableDrawable