fix: network objects to synchronize scene changes edge cases (#3230)

* fix

Clean up scene migration synchronization table entry when GameObject is destroyed when it should not have been.

* fix

Clean up scene migration synchronization table when NetworkManager shuts down.

* fix

adding check for any in-scene placed NetworkObjects.
removing whenever destroyed (if entry still exists).

* test

Minor test to validate the NetworkObject is no longer within the NetworkObject.NetworkObjectsToSynchronizeSceneChanges list upon destroying it prior to it being despawned.

* update

Adding change log entries.

* fix

Removing static NetworkObjectsToSynchronizeSceneChanges and CleanUpDisposedObjects and migrating them to normal properties within the NetworkSpawnManager.

* test

Adding a validation at the end of the scene load-unload test to assure the NetworkObjectsToSynchronizeSceneChanges has no remaining in-scene placed NetworkObjects when all scenes are unloaded.
Removing a verbose setting from InScenePlacedNetworkObjectTests.

* fix

Fixing issue with logic for scene migration synchronization.

* update

switching from a list to a stack

* test

Had to adjust for the changes from static to NetworkManager relative (for DA mode).

Author: NoelStephensUnity

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)
 - Fixed issue where a `NetworkObject` with nested `NetworkTransform` components of varying authority modes was not being taken into consideration and would break both the initial `NetworkTransform` synchronization and fail to properly handle synchronized state updates of the nested `NetworkTransform` components. (#3209)
 - Fixed issue with distributing parented children that have the distributable and/or transferrable permissions set and have the same owner as the root parent, that has the distributable permission set, were not being distributed to the same client upon the owning client disconnecting when using a distributed authority network topology. (#3203)

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)
@@ -2392,11 +2401,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)
@@ -2451,21 +2455,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;
@@ -3276,31 +3274,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;
@@ -3323,20 +3296,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>();
+
+        // Pre-allocating to avoid the initial constructor hit
+        internal Stack<ulong> CleanUpDisposedObjects = new Stack<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 unsafe void UpdateNetworkObjectSceneChanges()
+        {
+            foreach (var entry in NetworkObjectsToSynchronizeSceneChanges)
+            {
+                // If it fails the first update then don't add for updates
+                if (!entry.Value.UpdateForSceneChanges())
+                {
+                    CleanUpDisposedObjects.Push(entry.Key);
+                }
+            }
+
+            // Clean up any NetworkObjects that no longer exist (destroyed before they should be or the like)
+            while (CleanUpDisposedObjects.Count > 0)
+            {
+                NetworkObjectsToSynchronizeSceneChanges.Remove(CleanUpDisposedObjects.Pop());
+            }
+        }
+
         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,14 @@ 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!");
             }
+            if (m_DistributedAuthority)
+            {
+                Assert.IsFalse(m_ClientNetworkManagers[1].SpawnManager.NetworkObjectsToSynchronizeSceneChanges.ContainsKey(m_ClientNetworkObjectId), $"Player object {m_ClientNetworkObjectId} still exists within {nameof(NetworkSpawnManager.NetworkObjectsToSynchronizeSceneChanges)}!");
+            }
+            else
+            {
+                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;