fix: Nested NetworkBehaviours don't de-register or Invoke OnNetworkDespawn if destroyed while the parent NetworkObject remains spawned [NCCBUG-137] (#2091)

* fix

NCCBUG-137

This resolves the issue where destroying a GameObject with one or more NetworkBehaviour components that is a child or any child generation of the NetworkObject/GameObject that theNetworkBehaviour(s) are assigned to would still try to access the destroyed NetworkBehaviour since it was still an entry within the NetworkObject.ChildNetworkBehaviours list.  It also resolves the issue where under this scenario the NetworkBehaviours would not have their OnNetworkDespawn method invoked.

* update

NCCBUG-137

updating the changelog

* fix

We need to check IsSpawned when logging a warning about a NetworkBehaviour not having an assigned NetworkObject (this is only valid if NetworkBehaviour.IsSpawned == true)

* fix

Needed to adjust to account for the condition when it is valid is when you have a NetworkBehaviour that is spawned but there is no assigned NetworkObject.

* test

NCCBUG-137
This is the integration validation test for this PR fix.

* update

Adding PR number to the changelog entry

* test

adding check for OnNetworkDespawn being invoked when a child NetworkBehaviour is destroyed. but the parent NetworkObject persists and remains spawned.

* style

MTT-4260
was created for this technical debt.
removing reminder to create a ticket.

* fix

fixing merge issue.

* fix

Removing the invocation of NetworkBehaviour's OnNetworkDespawn when a NetworkBehaviour is destroyed while a NetworkObject is still spawned.

* style

removing unused namespace.

* Update com.unity.netcode.gameobjects/CHANGELOG.md

Co-authored-by: Jesse Olmer <[email protected]>

Co-authored-by: Jesse Olmer <[email protected]>

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -16,6 +16,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 - Fixed issue where a client owner of a `NetworkVariable` with both owner read and write permissions would not update the server side when changed. (#2097)
 - Fixed issue when attempting to spawn a parent `GameObject`, with `NetworkObject` component attached, that has one or more child `GameObject`s, that are inactive in the hierarchy, with `NetworkBehaviour` components it will no longer attempt to spawn the associated `NetworkBehaviour`(s) or invoke ownership changed notifications but will log a warning message. (#2096)
+- Fixed an issue where destroying a NetworkBehaviour would not deregister it from the parent NetworkObject, leading to exceptions when the parent was later destroyed. (#2091)
 - Fixed issue where `NetworkObject.NetworkHide` was despawning and destroying, as opposed to only despawning, in-scene placed `NetworkObject`s. (#2086)
 - Fixed issue where `NetworkAnimator` would not synchronize a looping animation for late joining clients if it was at the very end of its loop. (#2076)
 - Fixed issue where `NetworkAnimator` was not removing its subscription from `OnClientConnectedCallback` when despawned during the shutdown sequence. (#2074)

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

@@ -331,7 +331,8 @@ public NetworkObject NetworkObject
                 // in Update and/or in FixedUpdate could still be checking NetworkBehaviour.NetworkObject directly (i.e. does it exist?)
                 // or NetworkBehaviour.IsSpawned (i.e. to early exit if not spawned) which, in turn, could generate several Warning messages
                 // per spawned NetworkObject.  Checking for ShutdownInProgress prevents these unnecessary LogWarning messages.
-                if (m_NetworkObject == null && (NetworkManager.Singleton == null || !NetworkManager.Singleton.ShutdownInProgress))
+                // We must check IsSpawned, otherwise a warning will be logged under certain valid conditions (see OnDestroy)
+                if (IsSpawned && m_NetworkObject == null && (NetworkManager.Singleton == null || !NetworkManager.Singleton.ShutdownInProgress))
                 {
                     if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
                     {
@@ -759,6 +760,14 @@ protected NetworkObject GetNetworkObject(ulong networkId)
         /// </summary>
         public virtual void OnDestroy()
         {
+            if (NetworkObject != null && NetworkObject.IsSpawned && IsSpawned)
+            {
+                // If the associated NetworkObject is still spawned then this
+                // NetworkBehaviour will be removed from the NetworkObject's
+                // ChildNetworkBehaviours list.
+                NetworkObject.OnNetworkBehaviourDestroyed(this);
+            }
+
             // this seems odd to do here, but in fact especially in tests we can find ourselves
             //  here without having called InitializedVariables, which causes problems if any
             //  of those variables use native containers (e.g. NetworkList) as they won't be
@@ -770,6 +779,7 @@ public virtual void OnDestroy()
                 InitializeVariables();
             }
 
+
             for (int i = 0; i < NetworkVariableFields.Count; i++)
             {
                 NetworkVariableFields[i].Dispose();

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

@@ -1219,5 +1219,21 @@ internal uint HostCheckForGlobalObjectIdHashOverride()
 
             return GlobalObjectIdHash;
         }
+
+        /// <summary>
+        /// Removes a NetworkBehaviour from the ChildNetworkBehaviours list when destroyed
+        /// while the NetworkObject is still spawned.
+        /// </summary>
+        internal void OnNetworkBehaviourDestroyed(NetworkBehaviour networkBehaviour)
+        {
+            if (networkBehaviour.IsSpawned && IsSpawned)
+            {
+                if (NetworkManager.LogLevel == LogLevel.Developer)
+                {
+                    NetworkLog.LogWarning($"{nameof(NetworkBehaviour)}-{networkBehaviour.name} is being destroyed while {nameof(NetworkObject)}-{name} is still spawned! (could break state synchronization)");
+                }
+                ChildNetworkBehaviours.Remove(networkBehaviour);
+            }
+        }
     }
 }

com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs

@@ -22,6 +22,13 @@ protected override bool CanStartServerAndClients()
 
         public class SimpleNetworkBehaviour : NetworkBehaviour
         {
+            public bool OnNetworkDespawnCalled;
+
+            public override void OnNetworkDespawn()
+            {
+                OnNetworkDespawnCalled = true;
+                base.OnNetworkDespawn();
+            }
         }
 
         protected override IEnumerator OnSetup()
@@ -83,6 +90,9 @@ public IEnumerator ValidateNoSpam()
             // set the log level to developer
             m_ServerNetworkManager.LogLevel = LogLevel.Developer;
 
+            // The only valid condition for this would be if the NetworkBehaviour is spawned.
+            simpleNetworkBehaviour.IsSpawned = true;
+
             // Verify the warning gets logged under normal conditions
             var isNull = simpleNetworkBehaviour.NetworkObject == null;
             LogAssert.Expect(LogType.Warning, $"[Netcode] Could not get {nameof(NetworkObject)} for the {nameof(NetworkBehaviour)}. Are you missing a {nameof(NetworkObject)} component?");
@@ -98,5 +108,44 @@ public IEnumerator ValidateNoSpam()
             networkObjectToTest.Despawn();
             Object.Destroy(networkObjectToTest);
         }
+
+        /// <summary>
+        /// This validates the fix for when a child GameObject with a NetworkBehaviour
+        /// is deleted while the parent GameObject with a NetworkObject is spawned and
+        /// is not deleted until a later time would cause an exception due to the
+        /// NetworkBehaviour not being removed from the NetworkObject.ChildNetworkBehaviours
+        /// list.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator ValidateDeleteChildNetworkBehaviour()
+        {
+            m_AllowServerToStart = true;
+
+            yield return s_DefaultWaitForTick;
+
+            // Now just start the Host
+            yield return StartServerAndClients();
+
+            var parentObject = new GameObject();
+            var childObject = new GameObject();
+            childObject.transform.parent = parentObject.transform;
+            var parentNetworkObject = parentObject.AddComponent<NetworkObject>();
+            childObject.AddComponent<SimpleNetworkBehaviour>();
+
+            parentNetworkObject.Spawn();
+            yield return s_DefaultWaitForTick;
+
+            // Destroy the child object with child NetworkBehaviour
+            Object.Destroy(childObject);
+
+            yield return s_DefaultWaitForTick;
+
+            // Assure no log messages are logged when they should not be logged
+            LogAssert.NoUnexpectedReceived();
+
+            // Destroy the parent object which should not cause any exceptions
+            // (validating the fix)
+            Object.Destroy(parentObject);
+        }
     }
 }