fix: NetworkObject.DeferDespawn not respecting DestroyGameObject parameter (#3219)

* fix: NetworkObject.DeferDespawn not respecting DestroyGameObject parameter

* test-fix

Checking to see if the failure was due to the network prefab being spawned when the host-server starts.

* test-fix

Assure all NetworkManagers are destroyed at the end of TearDown.

* Revert "test-fix"

This reverts commit 16659e8.

* test-fix-fix

Fixing the bug in my previous fix.

* Ensure DeferDespawn tick is always reset

* revert DestroyObject message change

* Update CHANGELOG.md

Adding PR number to changelog entry.

---------

Co-authored-by: NoelStephensUnity <[email protected]>

Author: EmandM

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -12,6 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- 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)
 - Fixed issue where `NetworkVariableBase` derived classes were not being re-initialized if the associated `NetworkObject` instance was not destroyed and re-spawned. (#3181)

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/DestroyObjectMessage.cs

@@ -150,7 +150,7 @@ public void Handle(ref NetworkContext context)
                     {
                         networkObject.DeferredDespawnTick = DeferredDespawnTick;
                         var hasCallback = networkObject.OnDeferredDespawnComplete != null;
-                        networkManager.SpawnManager.DeferDespawnNetworkObject(NetworkObjectId, DeferredDespawnTick, hasCallback);
+                        networkManager.SpawnManager.DeferDespawnNetworkObject(NetworkObjectId, DeferredDespawnTick, hasCallback, DestroyGameObject);
                         return;
                     }
                 }

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

@@ -1450,7 +1450,7 @@ internal void ServerSpawnSceneObjectsOnStartSweep()
             }
 
             // Since we are spawing in-scene placed NetworkObjects for already loaded scenes,
-            // we need to add any in-scene placed NetworkObject to our tracking table 
+            // we need to add any in-scene placed NetworkObject to our tracking table
             var clearFirst = true;
             foreach (var sceneLoaded in NetworkManager.SceneManager.ScenesLoaded)
             {
@@ -1588,6 +1588,7 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec
                     {
                         NetworkObjectId = networkObject.NetworkObjectId,
                         DeferredDespawnTick = networkObject.DeferredDespawnTick,
+                        // DANGO-TODO: Reconfigure Client-side object destruction on despawn
                         DestroyGameObject = networkObject.IsSceneObject != false ? destroyGameObject : true,
                         IsTargetedDestroy = false,
                         IsDistributedAuthority = distributedAuthority,
@@ -1601,6 +1602,7 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec
             }
 
             networkObject.IsSpawned = false;
+            networkObject.DeferredDespawnTick = 0;
 
             if (SpawnedObjects.Remove(networkObject.NetworkObjectId))
             {
@@ -1920,6 +1922,7 @@ internal struct DeferredDespawnObject
         {
             public int TickToDespawn;
             public bool HasDeferredDespawnCheck;
+            public bool DestroyGameObject;
             public ulong NetworkObjectId;
         }
 
@@ -1931,12 +1934,13 @@ internal struct DeferredDespawnObject
         /// <param name="networkObjectId">associated NetworkObject</param>
         /// <param name="tickToDespawn">when to despawn the NetworkObject</param>
         /// <param name="hasDeferredDespawnCheck">if true, user script is to be invoked to determine when to despawn</param>
-        internal void DeferDespawnNetworkObject(ulong networkObjectId, int tickToDespawn, bool hasDeferredDespawnCheck)
+        internal void DeferDespawnNetworkObject(ulong networkObjectId, int tickToDespawn, bool hasDeferredDespawnCheck, bool destroyGameObject)
         {
             var deferredDespawnObject = new DeferredDespawnObject()
             {
                 TickToDespawn = tickToDespawn,
                 HasDeferredDespawnCheck = hasDeferredDespawnCheck,
+                DestroyGameObject = destroyGameObject,
                 NetworkObjectId = networkObjectId,
             };
             DeferredDespawnObjects.Add(deferredDespawnObject);
@@ -2001,7 +2005,7 @@ internal void DeferredDespawnUpdate(NetworkTime serverTime)
                 }
                 var networkObject = SpawnedObjects[deferredObjectEntry.NetworkObjectId];
                 // Local instance despawns the instance
-                OnDespawnObject(networkObject, true);
+                OnDespawnObject(networkObject, deferredObjectEntry.DestroyGameObject);
                 DeferredDespawnObjects.Remove(deferredObjectEntry);
             }
         }

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs

@@ -1156,6 +1156,9 @@ protected void ShutdownAndCleanUp()
             // reset the m_ServerWaitForTick for the next test to initialize
             s_DefaultWaitForTick = new WaitForSecondsRealtime(1.0f / k_DefaultTickRate);
             VerboseDebug($"Exiting {nameof(ShutdownAndCleanUp)}");
+
+            // Assure any remaining NetworkManagers are destroyed
+            DestroyNetworkManagers();
         }
 
         protected IEnumerator CoroutineShutdownAndCleanUp()
@@ -1195,6 +1198,9 @@ protected IEnumerator CoroutineShutdownAndCleanUp()
             // reset the m_ServerWaitForTick for the next test to initialize
             s_DefaultWaitForTick = new WaitForSecondsRealtime(1.0f / k_DefaultTickRate);
             VerboseDebug($"Exiting {nameof(ShutdownAndCleanUp)}");
+
+            // Assure any remaining NetworkManagers are destroyed
+            DestroyNetworkManagers();
         }
 
         /// <summary>
@@ -1244,6 +1250,19 @@ public IEnumerator TearDown()
             VerboseDebug($"Exiting {nameof(TearDown)}");
             LogWaitForMessages();
             NetcodeLogAssert.Dispose();
+
+        }
+
+        /// <summary>
+        /// Destroys any remaining NetworkManager instances
+        /// </summary>
+        private void DestroyNetworkManagers()
+        {
+            var networkManagers = Object.FindObjectsByType<NetworkManager>(FindObjectsSortMode.None);
+            foreach (var networkManager in networkManagers)
+            {
+                Object.DestroyImmediate(networkManager.gameObject);
+            }
         }
 
         /// <summary>

com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/DeferredDespawningTests.cs

@@ -9,15 +9,25 @@
 
 namespace Unity.Netcode.RuntimeTests
 {
+    [TestFixture(SetDestroyGameObject.DestroyGameObject)]
+    [TestFixture(SetDestroyGameObject.DontDestroyGameObject)]
     internal class DeferredDespawningTests : IntegrationTestWithApproximation
     {
         private const int k_DaisyChainedCount = 5;
         protected override int NumberOfClients => 2;
         private List<GameObject> m_DaisyChainedDespawnObjects = new List<GameObject>();
         private List<ulong> m_HasReachedEnd = new List<ulong>();
 
-        public DeferredDespawningTests() : base(HostOrServer.DAHost)
+        public enum SetDestroyGameObject
         {
+            DestroyGameObject,
+            DontDestroyGameObject,
+        }
+        private bool m_DestroyGameObject;
+
+        public DeferredDespawningTests(SetDestroyGameObject destroyGameObject) : base(HostOrServer.DAHost)
+        {
+            m_DestroyGameObject = destroyGameObject == SetDestroyGameObject.DestroyGameObject;
         }
 
         protected override void OnServerAndClientsCreated()
@@ -45,12 +55,28 @@ protected override void OnServerAndClientsCreated()
         [UnityTest]
         public IEnumerator DeferredDespawning()
         {
+            // Setup for test
+            DeferredDespawnDaisyChained.DestroyGameObject = m_DestroyGameObject;
             DeferredDespawnDaisyChained.EnableVerbose = m_EnableVerboseDebug;
+            DeferredDespawnDaisyChained.ClientRelativeInstances = new Dictionary<ulong, Dictionary<ulong, DeferredDespawnDaisyChained>>();
+
+            // Spawn the initial object
             var rootInstance = SpawnObject(m_DaisyChainedDespawnObjects[0], m_ServerNetworkManager);
             DeferredDespawnDaisyChained.ReachedLastChainInstance = ReachedLastChainObject;
+
+            // Wait for the chain of objects to spawn and despawn
             var timeoutHelper = new TimeoutHelper(300);
             yield return WaitForConditionOrTimeOut(HaveAllClientsReachedEndOfChain, timeoutHelper);
             AssertOnTimeout($"Timed out waiting for all children to reach the end of their chained deferred despawns!", timeoutHelper);
+
+            if (m_DestroyGameObject)
+            {
+                Assert.IsTrue(rootInstance == null); // Assert.IsNull doesn't work here
+            }
+            else
+            {
+                Assert.IsTrue(rootInstance != null); // Assert.IsNotNull doesn't work here
+            }
         }
 
         private bool HaveAllClientsReachedEndOfChain()
@@ -88,9 +114,10 @@ private void ReachedLastChainObject(ulong clientId)
     internal class DeferredDespawnDaisyChained : NetworkBehaviour
     {
         public static bool EnableVerbose;
+        public static bool DestroyGameObject;
         public static Action<ulong> ReachedLastChainInstance;
         private const int k_StartingDeferTick = 4;
-        public static Dictionary<ulong, Dictionary<ulong, DeferredDespawnDaisyChained>> ClientRelativeInstances = new Dictionary<ulong, Dictionary<ulong, DeferredDespawnDaisyChained>>();
+        public static Dictionary<ulong, Dictionary<ulong, DeferredDespawnDaisyChained>> ClientRelativeInstances;
         public bool IsRoot;
         public GameObject PrefabToSpawnWhenDespawned;
         public bool WasContactedByPeviousChainMember { get; private set; }
@@ -182,7 +209,7 @@ private void InvokeDespawn()
             {
                 FailTest($"[{nameof(InvokeDespawn)}] Client is not the authority but this was invoked (integration test logic issue)!");
             }
-            NetworkObject.DeferDespawn(DeferDespawnTick);
+            NetworkObject.DeferDespawn(DeferDespawnTick, DestroyGameObject);
         }
 
         public override void OnDeferringDespawn(int despawnTick)
@@ -241,7 +268,7 @@ private void Update()
                     continue;
                 }
 
-                // This should happen shortly afte the instances spawns (based on the deferred despawn count)
+                // This should happen shortly after the instances spawn (based on the deferred despawn count)
                 if (!IsRoot && !ClientRelativeInstances[clientId][NetworkObjectId].WasContactedByPeviousChainMember)
                 {
                     // exit early if the non-authority instance has not been contacted yet

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

@@ -53,6 +53,12 @@ protected override bool CanStartServerAndClients()
             return m_CanStartServerAndClients;
         }
 
+        public enum DespawnMode
+        {
+            Despawn,
+            DeferDespawn,
+        }
+
         /// <summary>
         /// This verifies that in-scene placed NetworkObjects will be properly
         /// synchronized if:
@@ -61,8 +67,13 @@ protected override bool CanStartServerAndClients()
         /// NetworkObject as a NetworkPrefab
         /// </summary>
         [UnityTest]
-        public IEnumerator InSceneNetworkObjectSynchAndSpawn()
+        public IEnumerator InSceneNetworkObjectSynchAndSpawn([Values] DespawnMode despawnMode)
         {
+            if (!m_DistributedAuthority && despawnMode == DespawnMode.DeferDespawn)
+            {
+                Assert.Ignore($"Test ignored as DeferDespawn is only valid with Distributed Authority mode.");
+            }
+
             NetworkObjectTestComponent.VerboseDebug = true;
             // 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
@@ -99,7 +110,16 @@ public IEnumerator InSceneNetworkObjectSynchAndSpawn()
 
             // Despawn the in-scene placed NetworkObject
             Debug.Log("Despawning In-Scene placed NetworkObject");
-            serverObject.Despawn(false);
+
+            if (despawnMode == DespawnMode.Despawn)
+            {
+                serverObject.Despawn(false);
+            }
+            else
+            {
+                serverObject.DeferDespawn(1, false);
+            }
+
             yield return WaitForConditionOrTimeOut(() => NetworkObjectTestComponent.SpawnedInstances.Count == 0);
             AssertOnTimeout($"Timed out waiting for all in-scene instances to be despawned!  Current spawned count: {NetworkObjectTestComponent.SpawnedInstances.Count()}");