fix: NetworksSenemanager order of operations and scene migration preprocess (#2532)

* fix

Change the order of operations where clients match the server's currently active scene and then spawn and synchronize NetworkObjects locally on the server.

Preprocess the scenes containing NetworkObjects to send scene migration notifications for in the event the NetworkObjects were despawned in the same frame.

* fix

Change the logic to detect if setting the synchronization mode is occurring when clients are already connected so that it is checking against whether it is a host vs server.

* Style

fixing white space issues.

* test

Added test to verify that late joining clients synchronize properly when a NetworkObject is migrated into a new scene and then despawn and destroyed during a late joining client's initial synchronization.

* update

Updating changelog

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -15,6 +15,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed issue where some temporary debug console logging was left in a merged PR. (#2562)
 - Fixed the "Generate Default Network Prefabs List" setting not loading correctly and always reverting to being checked. (#2545)
 - Fixed missing value on `NetworkListEvent` for `EventType.RemoveAt` events.  (#2542,#2543)
+- Fixed issue where a server would include scene migrated and then despawned NetworkObjects to a client that was being synchronized. (#2532)
 - Fixed the inspector throwing exceptions when attempting to render `NetworkVariable`s of enum types. (#2529)
 - Making a `NetworkVariable` with an `INetworkSerializable` type that doesn't meet the `new()` constraint will now create a compile-time error instead of an editor crash (#2528)
 - Fixed Multiplayer Tools package installation docs page link on the NetworkManager popup. (#2526)
@@ -26,10 +27,10 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ## Changed
 
+- Connecting clients being synchronized now switch to the server's active scene before spawning and synchronizing NetworkObjects. (#2532)
 - Updated `UnityTransport` dependency on `com.unity.transport` to 1.3.4. (#2533)
 - Improved performance of NetworkBehaviour initialization by replacing reflection when initializing NetworkVariables with compile-time code generation, which should help reduce hitching during additive scene loads. (#2522)
 
-
 ## [1.4.0] - 2023-04-10
 
 ### Added

com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs

@@ -337,7 +337,7 @@ public void SetClientSynchronizationMode(ref NetworkManager networkManager, Load
                 return;
             }
             else // Warn users if they are changing this after there are clients already connected and synchronized
-            if (networkManager.ConnectedClientsIds.Count > (networkManager.IsServer ? 0 : 1) && sceneManager.ClientSynchronizationMode != mode)
+            if (networkManager.ConnectedClientsIds.Count > (networkManager.IsHost ? 1 : 0) && sceneManager.ClientSynchronizationMode != mode)
             {
                 if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
                 {

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

@@ -2026,8 +2026,6 @@ private void HandleClientSceneEvent(uint sceneEventId)
                         {
                             // Include anything in the DDOL scene
                             PopulateScenePlacedObjects(DontDestroyOnLoadScene, false);
-                            // Synchronize the NetworkObjects for this scene
-                            sceneEventData.SynchronizeSceneNetworkObjects(NetworkManager);
 
                             // If needed, set the currently active scene
                             if (HashToBuildIndex.ContainsKey(sceneEventData.ActiveSceneHash))
@@ -2039,7 +2037,10 @@ private void HandleClientSceneEvent(uint sceneEventId)
                                 }
                             }
 
-                            // If needed, migrate dynamically spawned NetworkObjects to the same scene as on the server side
+                            // Spawn and Synchronize all NetworkObjects
+                            sceneEventData.SynchronizeSceneNetworkObjects(NetworkManager);
+
+                            // If needed, migrate dynamically spawned NetworkObjects to the same scene as they are on the server
                             SynchronizeNetworkObjectScene();
 
                             sceneEventData.SceneEventType = SceneEventType.SynchronizeComplete;
@@ -2468,6 +2469,9 @@ internal void MigrateNetworkObjectsIntoScenes()
             ObjectsMigratedIntoNewScene.Clear();
         }
 
+
+        private List<int> m_ScenesToRemoveFromObjectMigration = new List<int>();
+
         /// <summary>
         /// Should be invoked during PostLateUpdate just prior to the NetworkMessageManager processes its outbound message queue.
         /// </summary>
@@ -2479,6 +2483,40 @@ internal void CheckForAndSendNetworkObjectSceneChanged()
                 return;
             }
 
+            // Double check that the NetworkObjects to migrate still exist
+            m_ScenesToRemoveFromObjectMigration.Clear();
+            foreach (var sceneEntry in ObjectsMigratedIntoNewScene)
+            {
+                for (int i = sceneEntry.Value.Count - 1; i >= 0; i--)
+                {
+                    // Remove NetworkObjects that are no longer spawned
+                    if (!sceneEntry.Value[i].IsSpawned)
+                    {
+                        sceneEntry.Value.RemoveAt(i);
+                    }
+                }
+                // If the scene entry no longer has any NetworkObjects to migrate
+                // then add it to the list of scenes to be removed from the table
+                // of scenes containing NetworkObjects to migrate.
+                if (sceneEntry.Value.Count == 0)
+                {
+                    m_ScenesToRemoveFromObjectMigration.Add(sceneEntry.Key);
+                }
+            }
+
+            // Remove sceneHandle entries that no longer have any NetworkObjects remaining
+            foreach (var sceneHandle in m_ScenesToRemoveFromObjectMigration)
+            {
+                ObjectsMigratedIntoNewScene.Remove(sceneHandle);
+            }
+
+            // If there is nothing to send a migration notification for then exit
+            if (ObjectsMigratedIntoNewScene.Count == 0)
+            {
+                return;
+            }
+
+            // Some NetworkObjects still exist, send the message
             var sceneEvent = BeginSceneEvent();
             sceneEvent.SceneEventType = SceneEventType.ObjectSceneChanged;
             SendSceneEventData(sceneEvent.SceneEventId, NetworkManager.ConnectedClientsIds.Where(c => c != NetworkManager.ServerClientId).ToArray());

testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkObjectSceneMigrationTests.cs

@@ -207,21 +207,51 @@ public IEnumerator MigrateIntoNewSceneTest()
 
             // Register for the server-side client synchronization so we can send an object scene migration event at the same time
             // the new client begins to synchronize
-            m_ServerNetworkManager.SceneManager.OnSynchronize += SceneManager_OnSynchronize;
+            m_ServerNetworkManager.SceneManager.OnSynchronize += MigrateObjects_OnSynchronize;
 
             // Verify that a late joining client synchronizes properly even while new scene migrations occur
             // during its synchronization
             yield return CreateAndStartNewClient();
             yield return WaitForConditionOrTimeOut(VerifySpawnedObjectsMigrated);
 
             AssertOnTimeout($"[Late Joined Client] Timed out waiting for all clients to migrate all NetworkObjects into the appropriate scenes!");
+
+            // Verify that a late joining client synchronizes properly even if we migrate
+            // during its synchronization and despawn some of the NetworkObjects migrated.
+            m_ServerNetworkManager.SceneManager.OnSynchronize += MigrateAndDespawnObjects_OnSynchronize;
+            yield return CreateAndStartNewClient();
+            yield return WaitForConditionOrTimeOut(VerifySpawnedObjectsMigrated);
+
+            AssertOnTimeout($"[Late Joined Client] Timed out waiting for all clients to migrate all NetworkObjects into the appropriate scenes!");
+        }
+
+        /// <summary>
+        /// Part of NetworkObject scene migration tests to verify that a NetworkObject
+        /// migrated to a scene and then despawned will be handled properly for clients
+        /// in the middle of synchronization.
+        /// </summary>
+        private void MigrateAndDespawnObjects_OnSynchronize(ulong clientId)
+        {
+            var objectCount = 0;
+            // Migrate the NetworkObjects into different scenes than they originally were migrated into
+            for (int i = m_ServerSpawnedPrefabInstances.Count - 1; i >= 0; i--)
+            {
+                var scene = m_ScenesLoaded[i % m_ScenesLoaded.Count];
+                SceneManager.MoveGameObjectToScene(m_ServerSpawnedPrefabInstances[i].gameObject, scene);
+                // De-spawn every-other object
+                if (i % 2 == 0)
+                {
+                    m_ServerSpawnedPrefabInstances[objectCount + i].Despawn();
+                    m_ServerSpawnedPrefabInstances.RemoveAt(i);
+                }
+            }
         }
 
         /// <summary>
         /// Migrate objects into other scenes when a client begins synchronization
         /// </summary>
         /// <param name="clientId"></param>
-        private void SceneManager_OnSynchronize(ulong clientId)
+        private void MigrateObjects_OnSynchronize(ulong clientId)
         {
             var objectCount = k_MaxObjectsToSpawn - 1;
 
@@ -233,6 +263,9 @@ private void SceneManager_OnSynchronize(ulong clientId)
                 SceneManager.MoveGameObjectToScene(m_ServerSpawnedPrefabInstances[objectCount - 2].gameObject, scene);
                 objectCount -= 3;
             }
+
+            // Unsubscribe to this event for this part of the test
+            m_ServerNetworkManager.SceneManager.OnSynchronize -= MigrateObjects_OnSynchronize;
         }
 
         /// <summary>