update - 1:1 Review changes

NoelStephensUnity · NoelStephensUnity · commit a89bfbbcae9f · 2025-05-12T17:25:37.000-05:00
Modifications after doing 1:1 with Emma.
diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs
@@ -275,6 +275,8 @@ public void Handle(ref NetworkContext context)
 
             foreach (var clientId in ConnectedClientIds)
             {
+                // DANGO-TODO: Revisit the entire connection sequence and determine why we would need to check both cases as we shouldn't have to =or= we could
+                // try removing this after the Rust server connection sequence stuff is resolved. (Might be only needed if scene management is disabled)
                 // If there is any disconnect between the connection sequence of Ids vs ConnectedClients, then add the client.
                 if (!networkManager.ConnectionManager.ConnectedClientIds.Contains(clientId) || !networkManager.ConnectionManager.ConnectedClients.ContainsKey(clientId))
                 {
diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs
@@ -194,18 +194,7 @@ protected NetworkManager GetAuthorityNetworkManager()
                         return client;
                     }
                 }
-
-                // If we have not found a session owner and we are not
-                // auto-starting the session owner, then return the first 
-                // client NetworkManager.
-                if (!ShouldAutoStartSessionOwner())
-                {
-                    return m_NetworkManagers[0];
-                }
-                else
-                {
-                    Assert.Fail("No DA session owner found!");
-                }
+                Assert.Fail("No DA session owner found!");
             }
 
             return m_ServerNetworkManager;
@@ -255,7 +244,7 @@ private bool GetServiceEnvironmentVariable()
         {
             if (!m_UseCmbServiceEnv && m_UseCmbServiceEnvString == null)
             {
-                m_UseCmbServiceEnvString = Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "false";
+                m_UseCmbServiceEnvString = NetcodeIntegrationTestHelpers.GetCMBServiceEnvironentVariable();
                 if (bool.TryParse(m_UseCmbServiceEnvString.ToLower(), out bool isTrue))
                 {
                     m_UseCmbServiceEnv = isTrue;
@@ -276,11 +265,7 @@ private bool GetServiceEnvironmentVariable()
         /// <returns>true if a DAHost test should run against a hosted CMB service instance; otherwise false</returns>
         protected virtual bool UseCMBService()
         {
-#if USE_CMB_SERVICE
-            return true;
-#else
             return m_UseCmbService;
-#endif
         }
 
         protected virtual NetworkTopologyTypes OnGetNetworkTopologyType()
@@ -983,13 +968,8 @@ private void ClientNetworkManagerPostStart(NetworkManager networkManager)
                 m_PlayerNetworkObjects.Add(networkManager.LocalClientId, new Dictionary<ulong, NetworkObject>());
             }
 
-#if UNITY_2023_1_OR_NEWER
             // Get all player instances for the current client NetworkManager instance
             var clientPlayerClones = Object.FindObjectsByType<NetworkObject>(FindObjectsSortMode.None).Where((c) => c.IsPlayerObject && c.OwnerClientId == networkManager.LocalClientId).ToList();
-#else
-            // Get all player instances for the current client NetworkManager instance
-            var clientPlayerClones = Object.FindObjectsOfType<NetworkObject>().Where((c) => c.IsPlayerObject && c.OwnerClientId == networkManager.LocalClientId).ToList();
-#endif
             // Add this player instance to each client player entry
             foreach (var playerNetworkObject in clientPlayerClones)
             {
@@ -1004,13 +984,8 @@ private void ClientNetworkManagerPostStart(NetworkManager networkManager)
                     m_PlayerNetworkObjects[playerNetworkObject.NetworkManager.LocalClientId].Add(networkManager.LocalClientId, playerNetworkObject);
                 }
             }
-#if UNITY_2023_1_OR_NEWER
             // For late joining clients, add the remaining (if any) cloned versions of each client's player
             clientPlayerClones = Object.FindObjectsByType<NetworkObject>(FindObjectsSortMode.None).Where((c) => c.IsPlayerObject && c.NetworkManager == networkManager).ToList();
-#else
-            // For late joining clients, add the remaining (if any) cloned versions of each client's player
-            clientPlayerClones = Object.FindObjectsOfType<NetworkObject>().Where((c) => c.IsPlayerObject && c.NetworkManager == networkManager).ToList();
-#endif
             foreach (var playerNetworkObject in clientPlayerClones)
             {
                 if (!m_PlayerNetworkObjects[networkManager.LocalClientId].ContainsKey(playerNetworkObject.OwnerClientId))
@@ -1058,6 +1033,10 @@ protected virtual bool ShouldCheckForSpawnedPlayers()
         /// <summary>
         /// Starts the session owner and awaits for it to connect before starting the remaining clients.
         /// </summary>
+        /// <remarks>
+        /// DANGO-TODO: Renove this when the Rust server connection sequence is fixed and we don't have to pre-start
+        /// the session owner.
+        /// </remarks>
         private IEnumerator StartSessionOwner()
         {
             VerboseDebug("Starting session owner...");
@@ -1068,14 +1047,6 @@ private IEnumerator StartSessionOwner()
             VerboseDebug("Session owner connected and approved.");
         }
 
-        /// <summary>
-        /// Determines whether the session owner will be auto-started prior to any other client
-        /// </summary>
-        internal virtual bool ShouldAutoStartSessionOwner()
-        {
-            return true;
-        }
-
         /// <summary>
         /// This starts the server and clients as long as <see cref="CanStartServerAndClients"/>
         /// returns true.
@@ -1086,7 +1057,9 @@ protected IEnumerator StartServerAndClients()
             {
                 VerboseDebug($"Entering {nameof(StartServerAndClients)}");
 
-                if (m_UseCmbService && ShouldAutoStartSessionOwner())
+                // DANGO-TODO: Renove this when the Rust server connection sequence is fixed and we don't have to pre-start
+                // the session owner.
+                if (m_UseCmbService)
                 {
                     VerboseDebug("Using a distributed authority CMB Server for connection.");
                     yield return StartSessionOwner();
@@ -2018,23 +1991,8 @@ public NetcodeIntegrationTest(HostOrServer hostOrServer)
             InitializeTestConfiguration(m_NetworkTopologyType, hostOrServer);
         }
 
-        /// <summary>
-        /// Override this virtual method to execute script that runs before the base class constructor has finished executing.<br />
-        /// </summary>
-        /// <remarks>
-        /// ***NOTE***
-        /// When this method is invoked there will have been no properties set (i.e. nothing is configured). <br />
-        /// Primarily this is to set things like environemnt variables or other more external configurations that could
-        /// determine how the <see cref="NetcodeIntegrationTest"/> is configured.
-        /// </remarks>
-        protected virtual void OnPreInitializeConfiguration()
-        {
-        }
-
         private void InitializeTestConfiguration(NetworkTopologyTypes networkTopologyType, HostOrServer? hostOrServer)
         {
-            OnPreInitializeConfiguration();
-
             NetworkMessageManager.EnableMessageOrderConsoleLog = false;
 
             // Set m_NetworkTopologyType first because m_DistributedAuthority is calculated from it.
diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs
@@ -172,13 +172,26 @@ public static void RegisterHandlers(NetworkManager networkManager, bool serverSi
             }
         }
 
+        /// <summary>
+        /// Gets the CMB_SERVICE environemnt variable or returns "false" if it does not exist
+        /// </summary>
+        /// <returns>string</returns>
+        internal static string GetCMBServiceEnvironentVariable()
+        {
+#if USE_CMB_SERVICE
+            return "true";
+#else
+            return Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "false";
+#endif
+        }
+
         /// <summary>
         /// Use for non <see cref="NetcodeIntegrationTest"/> derived integration tests to automatically ignore the
         /// test if running against a CMB server.
         /// </summary>
         internal static void IgnoreIfServiceEnviromentVariableSet()
         {
-            if (bool.TryParse(Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "false", out bool isTrue) ? isTrue : false)
+            if (bool.TryParse(GetCMBServiceEnvironentVariable(), out bool isTrue) ? isTrue : false)
             {
                 Assert.Ignore("[CMB-Server Test Run] Skipping non-distributed authority test.");
             }
@@ -327,7 +340,7 @@ internal static NetworkManager CreateNewClient(int identifier, bool mockTranspor
         public static bool CreateNewClients(int clientCount, out NetworkManager[] clients, bool useMockTransport = false, bool useCmbService = false)
         {
             clients = new NetworkManager[clientCount];
-            // Pre-identify NetworkManager identifiers based on network topology type
+            // Pre-identify NetworkManager identifiers based on network topology type (Rust server starts at client identifier 1 and considers itself 0)
             var startCount = useCmbService ? 1 : 0;
             for (int i = 0; i < clientCount; i++)
             {
@@ -549,6 +562,8 @@ public static bool Start(bool host, NetworkManager server, NetworkManager[] clie
 
             foreach (var client in clients)
             {
+                // DANGO-TODO: Renove this entire check when the Rust server connection sequence is fixed and we don't have to pre-start
+                // the session owner.
                 if (client.IsConnectedClient)
                 {
                     // Skip starting the session owner
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/DistributedAuthorityCodecTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/DistributedAuthorityCodecTests.cs
@@ -30,7 +30,7 @@ namespace Unity.Netcode.RuntimeTests
     /// </remarks>
     internal class DistributedAuthorityCodecTests : NetcodeIntegrationTest
     {
-        protected override int NumberOfClients => 1;
+        protected override int NumberOfClients => 0;
 
         // Use the CMB Service for all tests
         protected override bool UseCMBService() => true;
@@ -68,14 +68,6 @@ public void TestAuthorityRpc(byte[] _)
             }
         }
 
-        /// <summary>
-        /// Don't auto start the session owner for codec tests
-        /// </summary>
-        internal override bool ShouldAutoStartSessionOwner()
-        {
-            return false;
-        }
-
         protected override void OnOneTimeSetup()
         {
             // Prevents the tests from running if no CMB Service is detected
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourUpdaterTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourUpdaterTests.cs
@@ -168,7 +168,7 @@ public NetworkBehaviourUpdaterTests(HostOrServer hostOrServer, int numberOfClien
                 SecondType = second
             };
             // Adjust the client count if connecting to the service.
-            m_ClientCount = UseCMBService() ? numberOfClients + 1 : numberOfClients;
+            m_ClientCount = numberOfClients;
         }
 
         protected override IEnumerator OnSetup()
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectSpawnManyObjectsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectSpawnManyObjectsTests.cs
@@ -68,6 +68,8 @@ public IEnumerator WhenManyObjectsAreSpawnedAtOnce_AllAreReceived()
 
             AssertOnTimeout($"Timed out waiting for the client to spawn {k_SpawnedObjects} objects! Time to spawn: {timeSpawned} | Time to timeout: {timeStarted - Time.realtimeSinceStartup}", timeoutHelper);
 
+            // Provide one full tick for all messages to finish being processed.
+            // DANGO-TODO: Determine if this is only when testing against Rust server (i.e. messages still pending and clients shutting down before they are dequeued)
             yield return s_DefaultWaitForTick;
         }
     }
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVarBufferCopyTest.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVarBufferCopyTest.cs
@@ -101,15 +101,10 @@ public override void OnNetworkSpawn()
                 base.OnNetworkSpawn();
             }
         }
-        protected override int NumberOfClients => m_ClientCount;
-
-        private const int k_ClientCount = 1;
-        private int m_ClientCount = k_ClientCount;
+        protected override int NumberOfClients => 1;
 
         public NetworkVarBufferCopyTest(HostOrServer hostOrServer) : base(hostOrServer)
         {
-            // Adjust the client count if connecting to the CMB service.
-            m_ClientCount = UseCMBService() ? k_ClientCount + 1 : k_ClientCount;
         }
 
         private static List<DummyNetBehaviour> s_ClientDummyNetBehavioursSpawned = new List<DummyNetBehaviour>();
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Physics/NetworkRigidbodyTest.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Physics/NetworkRigidbodyTest.cs
@@ -339,10 +339,8 @@ protected void Log(string msg)
     [TestFixture(HostOrServer.DAHost, ContactEventTypes.WithInfo)]
     internal class RigidbodyContactEventManagerTests : IntegrationTestWithApproximation
     {
-        protected override int NumberOfClients => m_ClientCount;
+        protected override int NumberOfClients => 1;
 
-        private const int k_ClientCount = 1;
-        private int m_ClientCount = k_ClientCount;
         private GameObject m_RigidbodyContactEventManager;
 
         public enum ContactEventTypes
@@ -356,8 +354,6 @@ public enum ContactEventTypes
 
         public RigidbodyContactEventManagerTests(HostOrServer hostOrServer, ContactEventTypes contactEventType) : base(hostOrServer)
         {
-            // Adjust the client count if connecting to the CMB service.
-            m_ClientCount = UseCMBService() ? k_ClientCount + 1 : k_ClientCount;
             m_ContactEventType = contactEventType;
         }
 

Original file line number	Diff line number	Diff line change
`@@ -275,6 +275,8 @@ public void Handle(ref NetworkContext context)`
`275`	`275`
`276`	`276`	`foreach (var clientId in ConnectedClientIds)`
`277`	`277`	`{`
	`278`	`+ // DANGO-TODO: Revisit the entire connection sequence and determine why we would need to check both cases as we shouldn't have to =or= we could`
	`279`	`+ // try removing this after the Rust server connection sequence stuff is resolved. (Might be only needed if scene management is disabled)`
`278`	`280`	`// If there is any disconnect between the connection sequence of Ids vs ConnectedClients, then add the client.`
`279`	`281`	`if (!networkManager.ConnectionManager.ConnectedClientIds.Contains(clientId) \|\| !networkManager.ConnectionManager.ConnectedClients.ContainsKey(clientId))`
`280`	`282`	`{`
Original file line number	Diff line number	Diff line change
`@@ -172,13 +172,26 @@ public static void RegisterHandlers(NetworkManager networkManager, bool serverSi`
`172`	`172`	`}`
`173`	`173`	`}`
`174`	`174`
	`175`	`+ /// <summary>`
	`176`	`+ /// Gets the CMB_SERVICE environemnt variable or returns "false" if it does not exist`
	`177`	`+ /// </summary>`
	`178`	`+ /// <returns>string</returns>`
	`179`	`+ internal static string GetCMBServiceEnvironentVariable()`
	`180`	`+ {`
	`181`	`+#if USE_CMB_SERVICE`
	`182`	`+ return "true";`
	`183`	`+#else`
	`184`	`+ return Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "false";`
	`185`	`+#endif`
	`186`	`+ }`
	`187`	`+`
`175`	`188`	`/// <summary>`
`176`	`189`	`/// Use for non <see cref="NetcodeIntegrationTest"/> derived integration tests to automatically ignore the`
`177`	`190`	`/// test if running against a CMB server.`
`178`	`191`	`/// </summary>`
`179`	`192`	`internal static void IgnoreIfServiceEnviromentVariableSet()`
`180`	`193`	`{`
`181`		`- if (bool.TryParse(Environment.GetEnvironmentVariable("USE_CMB_SERVICE") ?? "false", out bool isTrue) ? isTrue : false)`
	`194`	`+ if (bool.TryParse(GetCMBServiceEnvironentVariable(), out bool isTrue) ? isTrue : false)`
`182`	`195`	`{`
`183`	`196`	`Assert.Ignore("[CMB-Server Test Run] Skipping non-distributed authority test.");`
`184`	`197`	`}`
`@@ -327,7 +340,7 @@ internal static NetworkManager CreateNewClient(int identifier, bool mockTranspor`
`327`	`340`	`public static bool CreateNewClients(int clientCount, out NetworkManager[] clients, bool useMockTransport = false, bool useCmbService = false)`
`328`	`341`	`{`
`329`	`342`	`clients = new NetworkManager[clientCount];`
`330`		`- // Pre-identify NetworkManager identifiers based on network topology type`
	`343`	`+ // Pre-identify NetworkManager identifiers based on network topology type (Rust server starts at client identifier 1 and considers itself 0)`
`331`	`344`	`var startCount = useCmbService ? 1 : 0;`
`332`	`345`	`for (int i = 0; i < clientCount; i++)`
`333`	`346`	`{`
`@@ -549,6 +562,8 @@ public static bool Start(bool host, NetworkManager server, NetworkManager[] clie`
`549`	`562`
`550`	`563`	`foreach (var client in clients)`
`551`	`564`	`{`
	`565`	`+ // DANGO-TODO: Renove this entire check when the Rust server connection sequence is fixed and we don't have to pre-start`
	`566`	`+ // the session owner.`
`552`	`567`	`if (client.IsConnectedClient)`
`553`	`568`	`{`
`554`	`569`	`// Skip starting the session owner`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ namespace Unity.Netcode.RuntimeTests`
`30`	`30`	`/// </remarks>`
`31`	`31`	`internal class DistributedAuthorityCodecTests : NetcodeIntegrationTest`
`32`	`32`	`{`
`33`		`- protected override int NumberOfClients => 1;`
	`33`	`+ protected override int NumberOfClients => 0;`
`34`	`34`
`35`	`35`	`// Use the CMB Service for all tests`
`36`	`36`	`protected override bool UseCMBService() => true;`
`@@ -68,14 +68,6 @@ public void TestAuthorityRpc(byte[] _)`
`68`	`68`	`}`
`69`	`69`	`}`
`70`	`70`
`71`		`- /// <summary>`
`72`		`- /// Don't auto start the session owner for codec tests`
`73`		`- /// </summary>`
`74`		`- internal override bool ShouldAutoStartSessionOwner()`
`75`		`- {`
`76`		`- return false;`
`77`		`- }`
`78`		`-`
`79`	`71`	`protected override void OnOneTimeSetup()`
`80`	`72`	`{`
`81`	`73`	`// Prevents the tests from running if no CMB Service is detected`
Original file line number	Diff line number	Diff line change
`@@ -168,7 +168,7 @@ public NetworkBehaviourUpdaterTests(HostOrServer hostOrServer, int numberOfClien`
`168`	`168`	`SecondType = second`
`169`	`169`	`};`
`170`	`170`	`// Adjust the client count if connecting to the service.`
`171`		`- m_ClientCount = UseCMBService() ? numberOfClients + 1 : numberOfClients;`
	`171`	`+ m_ClientCount = numberOfClients;`
`172`	`172`	`}`
`173`	`173`
`174`	`174`	`protected override IEnumerator OnSetup()`
Original file line number	Diff line number	Diff line change
`@@ -68,6 +68,8 @@ public IEnumerator WhenManyObjectsAreSpawnedAtOnce_AllAreReceived()`
`68`	`68`
`69`	`69`	`AssertOnTimeout($"Timed out waiting for the client to spawn {k_SpawnedObjects} objects! Time to spawn: {timeSpawned} \| Time to timeout: {timeStarted - Time.realtimeSinceStartup}", timeoutHelper);`
`70`	`70`
	`71`	`+ // Provide one full tick for all messages to finish being processed.`
	`72`	`+ // DANGO-TODO: Determine if this is only when testing against Rust server (i.e. messages still pending and clients shutting down before they are dequeued)`
`71`	`73`	`yield return s_DefaultWaitForTick;`
`72`	`74`	`}`
`73`	`75`	`}`
Original file line number	Diff line number	Diff line change
`@@ -101,15 +101,10 @@ public override void OnNetworkSpawn()`
`101`	`101`	`base.OnNetworkSpawn();`
`102`	`102`	`}`
`103`	`103`	`}`
`104`		`- protected override int NumberOfClients => m_ClientCount;`
`105`		`-`
`106`		`- private const int k_ClientCount = 1;`
`107`		`- private int m_ClientCount = k_ClientCount;`
	`104`	`+ protected override int NumberOfClients => 1;`
`108`	`105`
`109`	`106`	`public NetworkVarBufferCopyTest(HostOrServer hostOrServer) : base(hostOrServer)`
`110`	`107`	`{`
`111`		`- // Adjust the client count if connecting to the CMB service.`
`112`		`- m_ClientCount = UseCMBService() ? k_ClientCount + 1 : k_ClientCount;`
`113`	`108`	`}`
`114`	`109`
`115`	`110`	`private static List<DummyNetBehaviour> s_ClientDummyNetBehavioursSpawned = new List<DummyNetBehaviour>();`
Original file line number	Diff line number	Diff line change
`@@ -339,10 +339,8 @@ protected void Log(string msg)`
`339`	`339`	`[TestFixture(HostOrServer.DAHost, ContactEventTypes.WithInfo)]`
`340`	`340`	`internal class RigidbodyContactEventManagerTests : IntegrationTestWithApproximation`
`341`	`341`	`{`
`342`		`- protected override int NumberOfClients => m_ClientCount;`
	`342`	`+ protected override int NumberOfClients => 1;`
`343`	`343`
`344`		`- private const int k_ClientCount = 1;`
`345`		`- private int m_ClientCount = k_ClientCount;`
`346`	`344`	`private GameObject m_RigidbodyContactEventManager;`
`347`	`345`
`348`	`346`	`public enum ContactEventTypes`
`@@ -356,8 +354,6 @@ public enum ContactEventTypes`
`356`	`354`
`357`	`355`	`public RigidbodyContactEventManagerTests(HostOrServer hostOrServer, ContactEventTypes contactEventType) : base(hostOrServer)`
`358`	`356`	`{`
`359`		`- // Adjust the client count if connecting to the CMB service.`
`360`		`- m_ClientCount = UseCMBService() ? k_ClientCount + 1 : k_ClientCount;`
`361`	`357`	`m_ContactEventType = contactEventType;`
`362`	`358`	`}`
`363`	`359`