fix: making NetworkServer.players a readonly collection

This will stop other scripts from adding or removing items from the set. We can also remove the set and just use the values from the connection dictionary.

BREAKING CHANGE: NetworkServer.Players is now a IReadOnlyCollection<INetworkPlayer>

Assets/Mirage/Runtime/NetworkServer.cs

@@ -124,10 +124,9 @@ public class NetworkServer : MonoBehaviour, INetworkServer
         /// <summary>
         /// A list of local connections on the server.
         /// </summary>
-        public readonly HashSet<INetworkPlayer> Players = new HashSet<INetworkPlayer>();
-        readonly Dictionary<IConnection, INetworkPlayer> connections = new Dictionary<IConnection, INetworkPlayer>();
+        public IReadOnlyCollection<INetworkPlayer> Players => connections.Values;
 
-        IReadOnlyCollection<INetworkPlayer> INetworkServer.Players => Players;
+        readonly Dictionary<IConnection, INetworkPlayer> connections = new Dictionary<IConnection, INetworkPlayer>();
 
         /// <summary>
         /// <para>Checks if the server has been started.</para>
@@ -159,7 +158,6 @@ public void Stop()
             }
 
             // just clear list, connections will be disconnected when peer is closed
-            Players.Clear();
             connections.Clear();
             LocalPlayer = null;
 
@@ -348,9 +346,6 @@ public void AddConnection(INetworkPlayer player)
         {
             if (!Players.Contains(player))
             {
-                // connection cannot be null here or conn.connectionId
-                // would throw NRE
-                Players.Add(player);
                 connections.Add(player.Connection, player);
             }
         }
@@ -361,7 +356,6 @@ public void AddConnection(INetworkPlayer player)
         /// <param name="connectionId">The id of the connection to remove.</param>
         public void RemoveConnection(INetworkPlayer player)
         {
-            Players.Remove(player);
             connections.Remove(player.Connection);
         }
 

Assets/Mirage/Samples~/ChangeScene/Scripts/SceneSwitcherHud.cs

@@ -38,7 +38,7 @@ public void Room2ButtonHandler()
 
         public void AdditiveButtonHandler()
         {
-            HashSet<INetworkPlayer> players = sceneManager.Server.Players;
+            IReadOnlyCollection<INetworkPlayer> players = sceneManager.Server.Players;
 
             if (additiveLoaded)
             {

Assets/Tests/Common/TestExtensions.cs

@@ -0,0 +1,30 @@
+using System.Collections.Generic;
+using System.Reflection;
+using Mirage.SocketLayer;
+using NSubstitute;
+
+namespace Mirage.Tests
+{
+    public static class NetworkIdentityTestExtensions
+    {
+        public static void SetSceneId(this NetworkIdentity identity, int id, int hash = 0)
+        {
+            FieldInfo fieldInfo = typeof(NetworkIdentity).GetField("_sceneId", BindingFlags.Instance | BindingFlags.NonPublic);
+            fieldInfo.SetValue(identity, (ulong)((((long)hash) << 32) | (long)id));
+        }
+    }
+
+    public static class NetworkServerTestExtensions
+    {
+        public static void AddTestPlayer(this NetworkServer server, INetworkPlayer player)
+        {
+            FieldInfo info = typeof(NetworkServer).GetField("connections", BindingFlags.Instance | BindingFlags.NonPublic);
+            var connections = (Dictionary<IConnection, INetworkPlayer>)info.GetValue(server);
+
+            IConnection connectiion = Substitute.For<IConnection>();
+            player.Connection.Returns(connectiion);
+            connections.Add(connectiion, player);
+        }
+    }
+}
+

Assets/Tests/Editor/NetworkIdentityCallbackTests.cs

@@ -792,8 +792,8 @@ public void RebuildObserversAddsReadyServerConnectionsIfNotImplemented()
             notReadyConnection.SceneIsReady.Returns(false);
 
             // add some server connections
-            server.Players.Add(readyConnection);
-            server.Players.Add(notReadyConnection);
+            server.AddTestPlayer(readyConnection);
+            server.AddTestPlayer(notReadyConnection);
 
             // rebuild observers should add all ready server connections
             // because no component implements OnRebuildObservers

Assets/Tests/Runtime/Host/PlayerSceneReadyHostTests.cs

@@ -21,12 +21,12 @@ public void SetAllClientsNotReadyTest()
             // add first ready client
             (_, NetworkPlayer first) = PipedConnections(ClientMessageHandler, ServerMessageHandler);
             first.SceneIsReady = true;
-            server.Players.Add(first);
+            server.AddTestPlayer(first);
 
             // add second ready client
             (_, NetworkPlayer second) = PipedConnections(ClientMessageHandler, ServerMessageHandler);
             second.SceneIsReady = true;
-            server.Players.Add(second);
+            server.AddTestPlayer(second);
 
             // set all not ready
             sceneManager.SetAllClientsNotReady(null);

Assets/Tests/Runtime/NetworkIdentityCallbackTests.cs

@@ -54,8 +54,8 @@ public void AddAllReadyServerConnectionsToObservers()
             player2.SceneIsReady.Returns(false);
 
             // add some server connections
-            server.Players.Add(player1);
-            server.Players.Add(player2);
+            server.AddTestPlayer(player1);
+            server.AddTestPlayer(player2);
 
             // add a host connection
             server.AddLocalConnection(client, Substitute.For<SocketLayer.IConnection>());