fix: network objects to synchronize scene changes edge cases

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -13,6 +13,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 ### Fixed
 
 - Fixed exception being thrown when a `GameObject` with an associated `NetworkTransform` is disabled. (#3243)
+- Fixed issue where the scene migration synchronization table was not cleaned up if the `GameObject` of a `NetworkObject` is destroyed before it should have been. (#3230)
+- Fixed issue where the scene migration synchronization table was not cleaned up upon `NetworkManager` shutting down. (#3230)
 - Fixed `NetworkObject.DeferDespawn` to respect the `DestroyGameObject` parameter. (#3219)
 - Changed the `NetworkTimeSystem.Sync` method to use half RTT to calculate the desired local time offset as opposed to the full RTT. (#3212)
 - Fixed issue where a spawned `NetworkObject` that was registered with a prefab handler and owned by a client would invoke destroy more than once on the host-server side if the client disconnected while the `NetworkObject` was still spawned. (#3200)

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

@@ -401,7 +401,7 @@ public void NetworkUpdate(NetworkUpdateStage updateStage)
                         }
 
                         // Update any NetworkObject's registered to notify of scene migration changes.
-                        NetworkObject.UpdateNetworkObjectSceneChanges();
+                        SpawnManager.UpdateNetworkObjectSceneChanges();
 
                         // This should be invoked just prior to the MessageManager processes its outbound queue.
                         SceneManager.CheckForAndSendNetworkObjectSceneChanged();

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -319,6 +319,9 @@ private void CheckForInScenePlaced()
                     EditorUtility.SetDirty(this);
                 }
                 IsSceneObject = true;
+
+                // Default scene migration synchronization to false for in-scene placed NetworkObjects
+                SceneMigrationSynchronization = false;
             }
         }
 #endif // UNITY_EDITOR
@@ -1596,6 +1599,9 @@ private void OnDestroy()
             if (NetworkManager.IsListening && !isAuthority && IsSpawned &&
                 (IsSceneObject == null || (IsSceneObject.Value != true)))
             {
+                // If we destroyed a GameObject with a NetworkObject component on the non-authority side, handle cleaning up the SceneMigrationSynchronization.
+                NetworkManager.SpawnManager?.RemoveNetworkObjectFromSceneChangedUpdates(this);
+
                 // Clients should not despawn NetworkObjects while connected to a session, but we don't want to destroy the current call stack
                 // if this happens. Instead, we should just generate a network log error and exit early (as long as we are not shutting down).
                 if (!NetworkManager.ShutdownInProgress)
@@ -1617,6 +1623,9 @@ private void OnDestroy()
                 // Otherwise, clients can despawn NetworkObjects while shutting down and should not generate any messages when this happens
             }
 
+            // Always attempt to remove from scene changed updates
+            NetworkManager.SpawnManager?.RemoveNetworkObjectFromSceneChangedUpdates(this);
+
             if (NetworkManager.SpawnManager != null && NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject))
             {
                 if (this == networkObject)
@@ -2379,11 +2388,6 @@ internal void InvokeBehaviourNetworkSpawn()
         {
             NetworkManager.SpawnManager.UpdateOwnershipTable(this, OwnerClientId);
 
-            if (SceneMigrationSynchronization && NetworkManager.NetworkConfig.EnableSceneManagement)
-            {
-                AddNetworkObjectToSceneChangedUpdates(this);
-            }
-
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
                 if (ChildNetworkBehaviours[i].gameObject.activeInHierarchy)
@@ -2438,21 +2442,15 @@ internal void InternalInSceneNetworkObjectsSpawned()
             }
         }
 
-
-
         internal void InvokeBehaviourNetworkDespawn()
         {
             NetworkManager.SpawnManager.UpdateOwnershipTable(this, OwnerClientId, true);
+            NetworkManager.SpawnManager.RemoveNetworkObjectFromSceneChangedUpdates(this);
 
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
                 ChildNetworkBehaviours[i].InternalOnNetworkDespawn();
             }
-
-            if (SceneMigrationSynchronization && NetworkManager.NetworkConfig.EnableSceneManagement)
-            {
-                RemoveNetworkObjectFromSceneChangedUpdates(this);
-            }
         }
 
         private List<NetworkBehaviour> m_ChildNetworkBehaviours;
@@ -3263,31 +3261,6 @@ internal void SceneChangedUpdate(Scene scene, bool notify = false)
             }
         }
 
-        internal static Dictionary<ulong, NetworkObject> NetworkObjectsToSynchronizeSceneChanges = new Dictionary<ulong, NetworkObject>();
-
-        internal static void AddNetworkObjectToSceneChangedUpdates(NetworkObject networkObject)
-        {
-            if (!NetworkObjectsToSynchronizeSceneChanges.ContainsKey(networkObject.NetworkObjectId))
-            {
-                NetworkObjectsToSynchronizeSceneChanges.Add(networkObject.NetworkObjectId, networkObject);
-            }
-
-            networkObject.UpdateForSceneChanges();
-        }
-
-        internal static void RemoveNetworkObjectFromSceneChangedUpdates(NetworkObject networkObject)
-        {
-            NetworkObjectsToSynchronizeSceneChanges.Remove(networkObject.NetworkObjectId);
-        }
-
-        internal static void UpdateNetworkObjectSceneChanges()
-        {
-            foreach (var entry in NetworkObjectsToSynchronizeSceneChanges)
-            {
-                entry.Value.UpdateForSceneChanges();
-            }
-        }
-
         private void Awake()
         {
             m_ChildNetworkBehaviours = null;
@@ -3310,20 +3283,25 @@ private void Awake()
         /// to add this same functionality to in-scene placed NetworkObjects until we have a way to generate
         /// per-NetworkObject-instance unique GlobalObjectIdHash values for in-scene placed NetworkObjects.
         /// </remarks>
-        internal void UpdateForSceneChanges()
+        internal bool UpdateForSceneChanges()
         {
             // Early exit if SceneMigrationSynchronization is disabled, there is no NetworkManager assigned,
             // the NetworkManager is shutting down, the NetworkObject is not spawned, it is an in-scene placed
             // NetworkObject, or the GameObject's current scene handle is the same as the SceneOriginHandle
             if (!SceneMigrationSynchronization || !IsSpawned || NetworkManager == null || NetworkManager.ShutdownInProgress ||
-                !NetworkManager.NetworkConfig.EnableSceneManagement || IsSceneObject != false || gameObject.scene.handle == SceneOriginHandle)
+                !NetworkManager.NetworkConfig.EnableSceneManagement || IsSceneObject != false || !gameObject)
             {
-                return;
+                // Stop checking for a scene migration
+                return false;
+            }
+            else if (gameObject.scene.handle != SceneOriginHandle)
+            {
+                // If the scene handle has changed, then update and send notification
+                SceneChangedUpdate(gameObject.scene, true);
             }
 
-            // Otherwise, this has to be a dynamically spawned NetworkObject that has been
-            // migrated to a new scene.
-            SceneChangedUpdate(gameObject.scene, true);
+            // Return true (continue checking for scene migration)
+            return true;
         }
 
         /// <summary>

com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

@@ -153,17 +153,6 @@ private void RemovePlayerObject(NetworkObject playerObject, bool destroyingObjec
             {
                 NetworkManager.ConnectionManager.ConnectedClients[playerObject.OwnerClientId].PlayerObject = null;
             }
-
-            // If we want to keep the observers, then exit early
-            //if (keepObservers)
-            //{
-            //    return;
-            //}
-
-            //foreach (var player in m_PlayerObjects)
-            //{
-            //    player.Observers.Remove(playerObject.OwnerClientId);
-            //}
         }
 
         internal void MarkObjectForShowingTo(NetworkObject networkObject, ulong clientId)
@@ -1161,6 +1150,8 @@ private void SpawnNetworkObjectLocallyCommon(NetworkObject networkObject, ulong
             networkObject.ApplyNetworkParenting();
             NetworkObject.CheckOrphanChildren();
 
+            AddNetworkObjectToSceneChangedUpdates(networkObject);
+
             networkObject.InvokeBehaviourNetworkSpawn();
 
             NetworkManager.DeferredMessageManager.ProcessTriggers(IDeferredNetworkMessageManager.TriggerType.OnSpawn, networkId);
@@ -1196,6 +1187,50 @@ private void SpawnNetworkObjectLocallyCommon(NetworkObject networkObject, ulong
             }
         }
 
+
+        internal Dictionary<ulong, NetworkObject> NetworkObjectsToSynchronizeSceneChanges = new Dictionary<ulong, NetworkObject>();
+        internal List<ulong> CleanUpDisposedObjects = new List<ulong>();
+
+        internal void AddNetworkObjectToSceneChangedUpdates(NetworkObject networkObject)
+        {
+            if ((networkObject.SceneMigrationSynchronization && NetworkManager.NetworkConfig.EnableSceneManagement) &&
+                !NetworkObjectsToSynchronizeSceneChanges.ContainsKey(networkObject.NetworkObjectId))
+            {
+                if (networkObject.UpdateForSceneChanges())
+                {
+                    NetworkObjectsToSynchronizeSceneChanges.Add(networkObject.NetworkObjectId, networkObject);
+                }
+            }
+        }
+
+        internal void RemoveNetworkObjectFromSceneChangedUpdates(NetworkObject networkObject)
+        {
+            if ((networkObject.SceneMigrationSynchronization && NetworkManager.NetworkConfig.EnableSceneManagement) &&
+                NetworkObjectsToSynchronizeSceneChanges.ContainsKey(networkObject.NetworkObjectId))
+            {
+                NetworkObjectsToSynchronizeSceneChanges.Remove(networkObject.NetworkObjectId);
+            }
+        }
+
+        internal void UpdateNetworkObjectSceneChanges()
+        {
+            foreach (var entry in NetworkObjectsToSynchronizeSceneChanges)
+            {
+                // If it fails the first update then don't add for updates
+                if (!entry.Value.UpdateForSceneChanges())
+                {
+                    CleanUpDisposedObjects.Add(entry.Key);
+                }
+            }
+
+            // Clean up any NetworkObjects that no longer exist (destroyed before they should be or the like)
+            foreach (var networkObjectId in CleanUpDisposedObjects)
+            {
+                NetworkObjectsToSynchronizeSceneChanges.Remove(networkObjectId);
+            }
+            CleanUpDisposedObjects.Clear();
+        }
+
         internal void SendSpawnCallForObject(ulong clientId, NetworkObject networkObject)
         {
             // If we are a host and sending to the host's client id, then we can skip sending ourselves the spawn message.
@@ -1729,6 +1764,17 @@ internal NetworkSpawnManager(NetworkManager networkManager)
             NetworkManager = networkManager;
         }
 
+        ~NetworkSpawnManager()
+        {
+            Shutdown();
+        }
+
+        internal void Shutdown()
+        {
+            NetworkObjectsToSynchronizeSceneChanges.Clear();
+            CleanUpDisposedObjects.Clear();
+        }
+
         /// <summary>
         /// DANGO-TODO: Until we have the CMB Server end-to-end with all features verified working via integration tests,
         /// I am keeping this debug toggle available. (NSS)

com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectDestroyTests.cs

@@ -23,6 +23,13 @@ internal class NetworkObjectDestroyTests : NetcodeIntegrationTest
 
         public NetworkObjectDestroyTests(NetworkTopologyTypes networkTopologyType) : base(networkTopologyType) { }
 
+        protected override void OnCreatePlayerPrefab()
+        {
+            var playerNetworkObject = m_PlayerPrefab.GetComponent<NetworkObject>();
+            playerNetworkObject.SceneMigrationSynchronization = true;
+            base.OnCreatePlayerPrefab();
+        }
+
         /// <summary>
         /// Tests that a server can destroy a NetworkObject and that it gets despawned correctly.
         /// </summary>
@@ -133,6 +140,7 @@ public IEnumerator TestNetworkObjectClientDestroy([Values] ClientDestroyObject c
                 yield return WaitForConditionOrTimeOut(HaveLogsBeenReceived);
                 AssertOnTimeout($"Not all expected logs were received when destroying a {nameof(NetworkObject)} on the client side during an active session!");
             }
+            Assert.IsFalse(m_ClientNetworkManagers[0].SpawnManager.NetworkObjectsToSynchronizeSceneChanges.ContainsKey(m_ClientNetworkObjectId), $"Player object {m_ClientNetworkObjectId} still exists within {nameof(NetworkSpawnManager.NetworkObjectsToSynchronizeSceneChanges)}!");
         }
 
         private bool HaveLogsBeenReceived()

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

@@ -74,7 +74,7 @@ public IEnumerator InSceneNetworkObjectSynchAndSpawn([Values] DespawnMode despaw
                 Assert.Ignore($"Test ignored as DeferDespawn is only valid with Distributed Authority mode.");
             }
 
-            NetworkObjectTestComponent.VerboseDebug = true;
+            NetworkObjectTestComponent.VerboseDebug = false;
             // Because despawning a client will cause it to shutdown and clean everything in the
             // scene hierarchy, we have to prevent one of the clients from spawning initially before
             // we test synchronizing late joining clients with despawned in-scene placed NetworkObjects.

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

@@ -2,6 +2,7 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Linq;
+using System.Text;
 using NUnit.Framework;
 using Unity.Netcode;
 using Unity.Netcode.TestHelpers.Runtime;
@@ -309,6 +310,8 @@ private bool DataPoolVerifySceneClient(int sceneIndex, string sceneName, LoadSce
             return true;
         }
 
+
+        private StringBuilder m_ErrorMsg = new StringBuilder();
         /// <summary>
         /// Small to heavy scene loading scenario to test the dynamically generated SceneEventData objects under a load.
         /// Will load from 1 to 32 scenes in both single and additive ClientSynchronizationMode
@@ -358,11 +361,30 @@ public IEnumerator SceneEventDataPoolSceneLoadingTest([Values(1, 2, 4, 6)] int n
             }
 
             yield return UnloadAllScenes(true);
+            Assert.IsTrue(CheckNetworkObjectsToSynchronizeSceneChanges(m_ServerNetworkManager), $"{nameof(NetworkSpawnManager.NetworkObjectsToSynchronizeSceneChanges)} validation check failure!\n {m_ErrorMsg}");
+            m_ServerNetworkManager.SceneManager.OnSceneEvent -= ServerSceneManager_OnSceneEvent;
+            // Validate that the NetworkObjectsToSynchronizeSceneChanges does not persist entries when scenes are unloaded.
             foreach (var client in m_ClientNetworkManagers)
             {
+                Assert.IsTrue(CheckNetworkObjectsToSynchronizeSceneChanges(client), $"{nameof(NetworkSpawnManager.NetworkObjectsToSynchronizeSceneChanges)} validation check failure!\n {m_ErrorMsg}");
                 client.SceneManager.OnUnloadComplete -= SceneManager_OnUnloadComplete;
             }
-            m_ServerNetworkManager.SceneManager.OnSceneEvent -= ServerSceneManager_OnSceneEvent;
+        }
+
+        private bool CheckNetworkObjectsToSynchronizeSceneChanges(NetworkManager networkManager)
+        {
+            m_ErrorMsg.Clear();
+            if (networkManager.SpawnManager.NetworkObjectsToSynchronizeSceneChanges.Count > 0)
+            {
+                foreach (var entry in networkManager.SpawnManager.NetworkObjectsToSynchronizeSceneChanges)
+                {
+                    if (entry.Value.IsSceneObject.HasValue && entry.Value.IsSceneObject.Value)
+                    {
+                        m_ErrorMsg.AppendLine($"{entry.Value.name} still exists within {nameof(NetworkSpawnManager.NetworkObjectsToSynchronizeSceneChanges)}!");
+                    }
+                }
+            }
+            return m_ErrorMsg.Length == 0;
         }
 
         private string m_SceneBeingUnloaded;