feat: Rework the DestroyObject path on the non-authority client (#3291)

* feat: Rework the DestroyObject path on the non-authority client

* Changelog.md

* update

Adding warning if the NetworkObject does not exist since we should not be receiving a message to destroy a NetworkObject if it is either already destroyed =or= the local client is destroying objects when it should not be.

* update

Changing Debug to NetworkLog

---------

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 DestroyObject flow on non-authority game clients. (#3291)
 - 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)

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

@@ -1,4 +1,5 @@
 using System.Linq;
+using System.Runtime.CompilerServices;
 
 namespace Unity.Netcode
 {
@@ -81,7 +82,7 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
 
             reader.ReadValueSafe(out DestroyGameObject);
 
-            if (!networkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject))
+            if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(NetworkObjectId))
             {
                 // Client-Server mode we always defer where in distributed authority mode we only defer if it is not a targeted destroy
                 if (!networkManager.DistributedAuthorityMode || (networkManager.DistributedAuthorityMode && !IsTargetedDestroy))
@@ -95,80 +96,90 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
         public void Handle(ref NetworkContext context)
         {
             var networkManager = (NetworkManager)context.SystemOwner;
+            networkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject);
 
-            var networkObject = (NetworkObject)null;
-            if (!networkManager.DistributedAuthorityMode)
+            // The DAHost needs to forward despawn messages to the other clients
+            if (networkManager.DAHost)
             {
-                // If this NetworkObject does not exist on this instance then exit early
-                if (!networkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out networkObject))
-                {
-                    return;
-                }
-            }
-            else
-            {
-                networkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out networkObject);
-                if (!networkManager.DAHost && networkObject == null)
+                HandleDAHostForwardMessage(context.SenderId, ref networkManager, networkObject);
+
+                // DAHost adds the object to the queue only if it is not a targeted destroy, or it is and the target is the DAHost client.
+                if (networkObject && DeferredDespawnTick > 0 && (!IsTargetedDestroy || (IsTargetedDestroy && TargetClientId == 0)))
                 {
-                    // If this NetworkObject does not exist on this instance then exit early
+                    HandleDeferredDespawn(ref networkManager, ref networkObject);
                     return;
                 }
             }
-            // DANGO-TODO: This is just a quick way to foward despawn messages to the remaining clients
-            if (networkManager.DistributedAuthorityMode && networkManager.DAHost)
+
+            // If this NetworkObject does not exist on this instance then exit early
+            if (!networkObject)
             {
-                var message = new DestroyObjectMessage
-                {
-                    NetworkObjectId = NetworkObjectId,
-                    DestroyGameObject = DestroyGameObject,
-                    IsDistributedAuthority = true,
-                    IsTargetedDestroy = IsTargetedDestroy,
-                    TargetClientId = TargetClientId, // Just always populate this value whether we write it or not
-                    DeferredDespawnTick = DeferredDespawnTick,
-                };
-                var ownerClientId = networkObject == null ? context.SenderId : networkObject.OwnerClientId;
-                var clientIds = networkObject == null ? networkManager.ConnectedClientsIds.ToList() : networkObject.Observers.ToList();
-
-                foreach (var clientId in clientIds)
+                if (networkManager.LogLevel <= LogLevel.Developer)
                 {
-                    if (clientId == networkManager.LocalClientId || clientId == ownerClientId)
-                    {
-                        continue;
-                    }
-                    networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, clientId);
+                    NetworkLog.LogWarning($"[{nameof(DestroyObjectMessage)}] Received destroy object message for NetworkObjectId ({NetworkObjectId}) on Client-{networkManager.LocalClientId}, but that {nameof(NetworkObject)} does not exist!");
                 }
+                return;
             }
 
-            // If we are deferring the despawn, then add it to the deferred despawn queue
             if (networkManager.DistributedAuthorityMode)
             {
-                if (DeferredDespawnTick > 0)
+                // If we are deferring the despawn, then add it to the deferred despawn queue
+                // If DAHost has reached this point, it is not valid to add to the queue
+                if (DeferredDespawnTick > 0 && !networkManager.DAHost)
                 {
-                    // Clients always add it to the queue while DAHost will only add it to the queue if it is not a targeted destroy or it is and the target is the
-                    // DAHost client.
-                    if (!networkManager.DAHost || (networkManager.DAHost && (!IsTargetedDestroy || (IsTargetedDestroy && TargetClientId == 0))))
-                    {
-                        networkObject.DeferredDespawnTick = DeferredDespawnTick;
-                        var hasCallback = networkObject.OnDeferredDespawnComplete != null;
-                        networkManager.SpawnManager.DeferDespawnNetworkObject(NetworkObjectId, DeferredDespawnTick, hasCallback, DestroyGameObject);
-                        return;
-                    }
+                    HandleDeferredDespawn(ref networkManager, ref networkObject);
+                    return;
                 }
 
                 // If this is targeted and we are not the target, then just update our local observers for this object
-                if (IsTargetedDestroy && TargetClientId != networkManager.LocalClientId && networkObject != null)
+                if (IsTargetedDestroy && TargetClientId != networkManager.LocalClientId)
                 {
                     networkObject.Observers.Remove(TargetClientId);
                     return;
                 }
             }
 
-            if (networkObject != null)
+            // Otherwise just despawn the NetworkObject right now
+            networkManager.SpawnManager.OnDespawnNonAuthorityObject(networkObject);
+            networkManager.NetworkMetrics.TrackObjectDestroyReceived(context.SenderId, networkObject, context.MessageSize);
+        }
+
+        /// <summary>
+        /// Handles forwarding the <see cref="DestroyObjectMessage"/> when acting as the DA Host
+        /// </summary>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private void HandleDAHostForwardMessage(ulong senderId, ref NetworkManager networkManager, NetworkObject networkObject)
+        {
+            var message = new DestroyObjectMessage
             {
-                // Otherwise just despawn the NetworkObject right now
-                networkManager.SpawnManager.OnDespawnObject(networkObject, DestroyGameObject);
-                networkManager.NetworkMetrics.TrackObjectDestroyReceived(context.SenderId, networkObject, context.MessageSize);
+                NetworkObjectId = NetworkObjectId,
+                DestroyGameObject = DestroyGameObject,
+                IsDistributedAuthority = true,
+                IsTargetedDestroy = IsTargetedDestroy,
+                TargetClientId = TargetClientId, // Just always populate this value whether we write it or not
+                DeferredDespawnTick = DeferredDespawnTick,
+            };
+            var ownerClientId = networkObject == null ? senderId : networkObject.OwnerClientId;
+            var clientIds = networkObject == null ? networkManager.ConnectedClientsIds.ToList() : networkObject.Observers.ToList();
+
+            foreach (var clientId in clientIds)
+            {
+                if (clientId != networkManager.LocalClientId && clientId != ownerClientId)
+                {
+                    networkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, clientId);
+                }
             }
         }
+
+        /// <summary>
+        /// Handles adding to the deferred despawn queue when the <see cref="DestroyObjectMessage"/> indicates a deferred despawn
+        /// </summary>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private void HandleDeferredDespawn(ref NetworkManager networkManager, ref NetworkObject networkObject)
+        {
+            networkObject.DeferredDespawnTick = DeferredDespawnTick;
+            var hasCallback = networkObject.OnDeferredDespawnComplete != null;
+            networkManager.SpawnManager.DeferDespawnNetworkObject(NetworkObjectId, DeferredDespawnTick, hasCallback);
+        }
     }
 }

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

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Text;
 using UnityEngine;
@@ -1470,7 +1471,6 @@ internal void ServerSpawnSceneObjectsOnStartSweep()
 #else
             var networkObjects = UnityEngine.Object.FindObjectsOfType<NetworkObject>();
 #endif
-            var isConnectedCMBService = NetworkManager.CMBServiceConnection;
             var networkObjectsToSpawn = new List<NetworkObject>();
             for (int i = 0; i < networkObjects.Length; i++)
             {
@@ -1510,15 +1510,30 @@ internal void ServerSpawnSceneObjectsOnStartSweep()
             networkObjectsToSpawn.Clear();
         }
 
+        /// <summary>
+        /// Called when destroying an object after receiving a <see cref="DestroyObjectMessage"/>.
+        /// Processes logic for how to destroy objects on the non-authority client.
+        /// </summary>
+        internal void OnDespawnNonAuthorityObject([NotNull] NetworkObject networkObject)
+        {
+            if (networkObject.HasAuthority)
+            {
+                NetworkLog.LogError($"OnDespawnNonAuthorityObject called on object {networkObject.NetworkObjectId} when is current client {NetworkManager.LocalClientId} has authority on this object.");
+            }
+
+            // On the non-authority, never destroy the game object when InScenePlaced, otherwise always destroy on non-authority side
+            OnDespawnObject(networkObject, networkObject.IsSceneObject == false);
+        }
+
         internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObject, bool modeDestroy = false)
         {
-            if (NetworkManager == null)
+            if (!NetworkManager)
             {
                 return;
             }
 
             // We have to do this check first as subsequent checks assume we can access NetworkObjectId.
-            if (networkObject == null)
+            if (!networkObject)
             {
                 Debug.LogWarning($"Trying to destroy network object but it is null");
                 return;
@@ -1632,8 +1647,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,
+                        DestroyGameObject = destroyGameObject,
                         IsTargetedDestroy = false,
                         IsDistributedAuthority = distributedAuthority,
                     };
@@ -2006,7 +2020,6 @@ internal struct DeferredDespawnObject
         {
             public int TickToDespawn;
             public bool HasDeferredDespawnCheck;
-            public bool DestroyGameObject;
             public ulong NetworkObjectId;
         }
 
@@ -2018,13 +2031,12 @@ 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, bool destroyGameObject)
+        internal void DeferDespawnNetworkObject(ulong networkObjectId, int tickToDespawn, bool hasDeferredDespawnCheck)
         {
             var deferredDespawnObject = new DeferredDespawnObject()
             {
                 TickToDespawn = tickToDespawn,
                 HasDeferredDespawnCheck = hasDeferredDespawnCheck,
-                DestroyGameObject = destroyGameObject,
                 NetworkObjectId = networkObjectId,
             };
             DeferredDespawnObjects.Add(deferredDespawnObject);
@@ -2041,15 +2053,21 @@ internal void DeferredDespawnUpdate(NetworkTime serverTime)
                 return;
             }
             var currentTick = serverTime.Tick;
-            var deferredCallbackCount = DeferredDespawnObjects.Count();
-            for (int i = 0; i < deferredCallbackCount; i++)
+            var deferredDespawnCount = DeferredDespawnObjects.Count;
+            // Loop forward and to process user callbacks and update despawn ticks
+            for (int i = 0; i < deferredDespawnCount; i++)
             {
                 var deferredObjectEntry = DeferredDespawnObjects[i];
                 if (!deferredObjectEntry.HasDeferredDespawnCheck)
                 {
                     continue;
                 }
-                var networkObject = SpawnedObjects[deferredObjectEntry.NetworkObjectId];
+
+                if (!SpawnedObjects.TryGetValue(deferredObjectEntry.NetworkObjectId, out var networkObject))
+                {
+                    continue;
+                }
+
                 // Double check to make sure user did not remove the callback
                 if (networkObject.OnDeferredDespawnComplete != null)
                 {
@@ -2074,23 +2092,21 @@ internal void DeferredDespawnUpdate(NetworkTime serverTime)
             }
 
             // Parse backwards so we can remove objects as we parse through them
-            for (int i = DeferredDespawnObjects.Count - 1; i >= 0; i--)
+            for (int i = deferredDespawnCount - 1; i >= 0; i--)
             {
                 var deferredObjectEntry = DeferredDespawnObjects[i];
                 if (deferredObjectEntry.TickToDespawn >= currentTick)
                 {
                     continue;
                 }
 
-                if (!SpawnedObjects.ContainsKey(deferredObjectEntry.NetworkObjectId))
+                if (SpawnedObjects.TryGetValue(deferredObjectEntry.NetworkObjectId, out var networkObject))
                 {
-                    DeferredDespawnObjects.Remove(deferredObjectEntry);
-                    continue;
+                    // Local instance despawns the instance
+                    OnDespawnNonAuthorityObject(networkObject);
                 }
-                var networkObject = SpawnedObjects[deferredObjectEntry.NetworkObjectId];
-                // Local instance despawns the instance
-                OnDespawnObject(networkObject, deferredObjectEntry.DestroyGameObject);
-                DeferredDespawnObjects.Remove(deferredObjectEntry);
+
+                DeferredDespawnObjects.RemoveAt(i);
             }
         }