fix: Parented in-scene objects under dynamically spawned objects are destroyed when the network session ends but original scene remains loaded. [MTT-7560] (#2737)

* fix

Fixes the issue with an in-scene placed NetworkObject being parented under a dynamically spawned NetworkObject and a sudden shutdown/exit occurs. This will remove the child in-scene placed NetworkObject from the parent during the NetworkManager shutdown process where it destroys all dynamically spawned NetworkObjects.'

Adjusting the parenting rules slightly to only deny parenting if the child or parent is not spawned.


* test

updating the test to reflect the updated error message when a client attempts to destroy a spawned NetworkObject while still connected to a session.
Updating the scene objects not destroyed test.

Adding `ChildSceneObjectsDoNotDestroyOnShutdown`

Adding ChildSceneObjectsDoNotDestroyOnShutdown test to validate this PR.
Updating ParentedNetworkTransformTest to provide enough time to let interpolation to finish.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -19,23 +19,26 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Added `SceneEventProgress.SceneManagementNotEnabled` return status to be returned when a `NetworkSceneManager` method is invoked and scene management is not enabled. (#2735)
 - Added `SceneEventProgress.ServerOnlyAction` return status to be returned when a `NetworkSceneManager` method is invoked by a client. (#2735)
 
-### Changed
-
-- `NetworkManager.ConnectedClientsIds` is now accessible on the client side and will contain the list of all clients in the session, including the host client if the server is operating in host mode (#2762)
-
 ### Fixed
 
-- Fixed a bug where having a class with Rpcs that inherits from a class without Rpcs that inherits from NetworkVariable would cause a compile error. (#2751)
-- Fixed issue where during client synchronization and scene loading, when client synchronization or the scene loading mode are set to `LoadSceneMode.Single`, a `CreateObjectMessage` could be received, processed, and the resultant spawned `NetworkObject` could be instantiated in the client's currently active scene that could, towards the end of the client synchronization or loading process, be unloaded and cause the newly created `NetworkObject` to be destroyed (and throw and exception). (#2735)
-- Fixed issue where `NetworkBehaviour.Synchronize` was not truncating the write buffer if nothing was serialized during `NetworkBehaviour.OnSynchronize` causing an additional 6 bytes to be written per `NetworkBehaviour` component instance. (#2749)
 - Fixed issue where a parented in-scene placed NetworkObject would be destroyed upon a client or server exiting a network session but not unloading the original scene in which the NetworkObject was placed. (#2737)
 - Fixed issue where during client synchronization and scene loading, when client synchronization or the scene loading mode are set to `LoadSceneMode.Single`, a `CreateObjectMessage` could be received, processed, and the resultant spawned `NetworkObject` could be instantiated in the client's currently active scene that could, towards the end of the client synchronization or loading process, be unloaded and cause the newly created `NetworkObject` to be destroyed (and throw and exception). (#2735)
 - Fixed issue where a `NetworkTransform` instance with interpolation enabled would result in wide visual motion gaps (stuttering) under above normal latency conditions and a 1-5% or higher packet are drop rate. (#2713)
 
 ### Changed
-- Changed `NetworkTransform` authoritative instance tick registration so a single `NetworkTransform` specific tick event update will update all authoritative instances to improve perofmance. (#2713)
 
+- `NetworkManager.ConnectedClientsIds` is now accessible on the client side and will contain the list of all clients in the session, including the host client if the server is operating in host mode (#2762)
 - Changed `NetworkSceneManager` to return a `SceneEventProgress` status and not throw exceptions for methods invoked when scene management is disabled and when a client attempts to access a `NetworkSceneManager` method by a client. (#2735)
+- Changed `NetworkTransform` authoritative instance tick registration so a single `NetworkTransform` specific tick event update will update all authoritative instances to improve perofmance. (#2713)
+
+## [1.7.1] - 2023-11-15
+
+### Added
+
+### Fixed
+
+- Fixed a bug where having a class with Rpcs that inherits from a class without Rpcs that inherits from NetworkVariable would cause a compile error. (#2751)
+- Fixed issue where `NetworkBehaviour.Synchronize` was not truncating the write buffer if nothing was serialized during `NetworkBehaviour.OnSynchronize` causing an additional 6 bytes to be written per `NetworkBehaviour` component instance. (#2749)
 
 ## [1.7.0] - 2023-10-11
 

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

@@ -703,7 +703,7 @@ private void OnDestroy()
                     // Since we still have a session connection, log locally and on the server to inform user of this issue.
                     if (NetworkManager.LogLevel <= LogLevel.Error)
                     {
-                        NetworkLog.LogErrorServer($"Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead.");
+                        NetworkLog.LogErrorServer($"[Invalid Destroy][{gameObject.name}][NetworkObjectId:{NetworkObjectId}] Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead.");
                     }
                     return;
                 }
@@ -969,20 +969,21 @@ public bool TrySetParent(NetworkObject parent, bool worldPositionStays = true)
                 return false;
             }
 
-            if (!NetworkManager.IsServer)
+            if (!NetworkManager.IsServer && !NetworkManager.ShutdownInProgress)
             {
                 return false;
             }
 
-            if (!IsSpawned)
+            // If the parent is not null fail only if either of the two is true:
+            // - This instance is spawned and the parent is not.
+            // - This instance is not spawned and the parent is.
+            // Basically, don't allow parenting when either the child or parent is not spawned.
+            // Caveat: if the parent is null then we can allow parenting whether the instance is or is not spawned.
+            if (parent != null && (IsSpawned ^ parent.IsSpawned))
             {
                 return false;
             }
 
-            if (parent != null && !parent.IsSpawned)
-            {
-                return false;
-            }
             m_CachedWorldPositionStays = worldPositionStays;
 
             if (parent == null)
@@ -1018,15 +1019,36 @@ private void OnTransformParentChanged()
 
             if (!NetworkManager.IsServer)
             {
-                transform.parent = m_CachedParent;
-                Debug.LogException(new NotServerException($"Only the server can reparent {nameof(NetworkObject)}s"));
+                // Log exception if we are a client and not shutting down.
+                if (!NetworkManager.ShutdownInProgress)
+                {
+                    transform.parent = m_CachedParent;
+                    Debug.LogException(new NotServerException($"Only the server can reparent {nameof(NetworkObject)}s"));
+                }
+                else // Otherwise, if we are removing a parent then go ahead and allow parenting to occur
+                if (transform.parent == null)
+                {
+                    m_LatestParent = null;
+                    m_CachedParent = null;
+                    InvokeBehaviourOnNetworkObjectParentChanged(null);
+                }
                 return;
             }
-
+            else // Otherwise, on the serer side if this instance is not spawned...
             if (!IsSpawned)
             {
-                transform.parent = m_CachedParent;
-                Debug.LogException(new SpawnStateException($"{nameof(NetworkObject)} can only be reparented after being spawned"));
+                // ,,,and we are removing the parent, then go ahead and allow parenting to occur
+                if (transform.parent == null)
+                {
+                    m_LatestParent = null;
+                    m_CachedParent = null;
+                    InvokeBehaviourOnNetworkObjectParentChanged(null);
+                }
+                else
+                {
+                    transform.parent = m_CachedParent;
+                    Debug.LogException(new SpawnStateException($"{nameof(NetworkObject)} can only be reparented after being spawned"));
+                }
                 return;
             }
             var removeParent = false;

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

@@ -728,17 +728,43 @@ internal void DespawnAndDestroyNetworkObjects()
                         // Leave destruction up to the handler
                         NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(networkObjects[i]);
                     }
-                    else if (networkObjects[i].IsSpawned)
+                    else
                     {
-                        // If it is an in-scene placed NetworkObject then just despawn
-                        // and let it be destroyed when the scene is unloaded. Otherwise, despawn and destroy it.
+                        // If it is an in-scene placed NetworkObject then just despawn and let it be destroyed when the scene
+                        // is unloaded. Otherwise, despawn and destroy it.
                         var shouldDestroy = !(networkObjects[i].IsSceneObject != null && networkObjects[i].IsSceneObject.Value);
 
-                        OnDespawnObject(networkObjects[i], shouldDestroy);
-                    }
-                    else if (networkObjects[i].IsSceneObject != null && !networkObjects[i].IsSceneObject.Value)
-                    {
-                        UnityEngine.Object.Destroy(networkObjects[i].gameObject);
+                        // If we are going to destroy this NetworkObject, check for any in-scene placed children that need to be removed
+                        if (shouldDestroy)
+                        {
+                            // Check to see if there are any in-scene placed children that are marked to be destroyed with the scene
+                            var childrenObjects = networkObjects[i].GetComponentsInChildren<NetworkObject>();
+                            foreach (var childObject in childrenObjects)
+                            {
+                                if (childObject == networkObjects[i])
+                                {
+                                    continue;
+                                }
+
+                                // If the child is an in-scene placed NetworkObject then remove the child from the parent (which was dynamically spawned)
+                                // and set its parent to root
+                                if (childObject.IsSceneObject != null && childObject.IsSceneObject.Value)
+                                {
+                                    childObject.TryRemoveParent(childObject.WorldPositionStays());
+                                }
+                            }
+                        }
+
+                        // If spawned, then despawn and potentially destroy. 
+                        if (networkObjects[i].IsSpawned)
+                        {
+                            OnDespawnObject(networkObjects[i], shouldDestroy);
+                        }
+                        else // Otherwise, if we are not spawned and we should destroy...then destroy.
+                        if (shouldDestroy)
+                        {
+                            UnityEngine.Object.Destroy(networkObjects[i].gameObject);
+                        }
                     }
                 }
             }

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

@@ -58,6 +58,9 @@ public enum ClientDestroyObject
             ShuttingDown,
             ActiveSession
         }
+
+        private string m_ClientPlayerName;
+        private ulong m_ClientNetworkObjectId;
         /// <summary>
         /// Validates the expected behavior when the client-side destroys a <see cref="NetworkObject"/>
         /// </summary>
@@ -78,7 +81,8 @@ public IEnumerator TestNetworkObjectClientDestroy([Values] ClientDestroyObject c
                 LogAssert.ignoreFailingMessages = true;
                 NetworkLog.NetworkManagerOverride = m_ClientNetworkManagers[0];
             }
-
+            m_ClientPlayerName = clientPlayer.gameObject.name;
+            m_ClientNetworkObjectId = clientPlayer.NetworkObjectId;
             Object.DestroyImmediate(clientPlayer.gameObject);
 
             // destroying a NetworkObject while a session is active is not allowed
@@ -91,12 +95,12 @@ public IEnumerator TestNetworkObjectClientDestroy([Values] ClientDestroyObject c
 
         private bool HaveLogsBeenReceived()
         {
-            if (!NetcodeLogAssert.HasLogBeenReceived(LogType.Error, "[Netcode] Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead."))
+            if (!NetcodeLogAssert.HasLogBeenReceived(LogType.Error, $"[Netcode] [Invalid Destroy][{m_ClientPlayerName}][NetworkObjectId:{m_ClientNetworkObjectId}] Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call Destroy or Despawn on the server/host instead."))
             {
                 return false;
             }
 
-            if (!NetcodeLogAssert.HasLogBeenReceived(LogType.Error, $"[Netcode-Server Sender={m_ClientNetworkManagers[0].LocalClientId}] Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead."))
+            if (!NetcodeLogAssert.HasLogBeenReceived(LogType.Error, $"[Netcode-Server Sender={m_ClientNetworkManagers[0].LocalClientId}] [Invalid Destroy][{m_ClientPlayerName}][NetworkObjectId:{m_ClientNetworkObjectId}] Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call Destroy or Despawn on the server/host instead."))
             {
                 return false;
             }

com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformTests.cs

@@ -602,7 +602,10 @@ public void ParentedNetworkTransformTest([Values] Precision precision, [Values]
             success = WaitForConditionOrTimeOutWithTimeTravel(AllChildObjectInstancesHaveChild);
             Assert.True(success, "Timed out waiting for all instances to have parented a child!");
 
+            // Provide two network ticks for interpolation to finalize
             TimeTravelToNextTick();
+            TimeTravelToNextTick();
+
             // This validates each child instance has preserved their local space values
             AllChildrenLocalTransformValuesMatch(false, ChildrenTransformCheckType.Connected_Clients);