fix: DestroyWithScene causes errors on non-authority side

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -14,6 +14,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed distributed authority related issue where enabling the `NetworkObject.DestroyWithScene` would cause errors when a destroying non-authority instances due to loading (single mode) or unloading scene events. (#3500)
 
 ### Changed
 

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

@@ -1200,6 +1200,7 @@ public void SetSceneObjectStatus(bool isSceneObject = false)
         /// Gets whether or not the object should be automatically removed when the scene is unloaded.
         /// </summary>
         public bool DestroyWithScene { get; set; }
+        internal bool DestroyPendingSceneEvent;
 
         /// <summary>
         /// When set to true and the active scene is changed, this will automatically migrate the <see cref="NetworkObject"/>
@@ -1720,10 +1721,11 @@ private void OnDestroy()
                 return;
             }
 
-            // Authority is the server (client-server) and the owner or DAHost (distributed authority) when destroying a NetworkObject
-            var isAuthority = HasAuthority || NetworkManager.DAHost;
+            // An authorized destroy is when done by the authority instance or done due to a scene event and the NetworkObject
+            // was marked as destroy pending scene event (which means the destroy with scene property was set).
+            var isAuthorityDestroy = HasAuthority || NetworkManager.DAHost || DestroyPendingSceneEvent;
 
-            if (NetworkManager.IsListening && !isAuthority && IsSpawned &&
+            if (NetworkManager.IsListening && !isAuthorityDestroy && 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.

com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs

@@ -293,13 +293,20 @@ public void MoveObjectsFromSceneToDontDestroyOnLoad(ref NetworkManager networkMa
             // Create a local copy of the spawned objects list since the spawn manager will adjust the list as objects
             // are despawned.
             var localSpawnedObjectsHashSet = new HashSet<NetworkObject>(networkManager.SpawnManager.SpawnedObjectsList);
+            var distributedAuthority = networkManager.DistributedAuthorityMode;
             foreach (var networkObject in localSpawnedObjectsHashSet)
             {
                 if (networkObject == null || (networkObject != null && networkObject.gameObject.scene.handle != scene.handle))
                 {
                     continue;
                 }
 
+                // Check to determine if we need to allow destroying a non-authority instance
+                if (distributedAuthority && networkObject.DestroyWithScene && !networkObject.HasAuthority)
+                {
+                    networkObject.DestroyPendingSceneEvent = true;
+                }
+
                 // Only NetworkObjects marked to not be destroyed with the scene and are not already in the DDOL are preserved
                 if (!networkObject.DestroyWithScene && networkObject.gameObject.scene != networkManager.SceneManager.DontDestroyOnLoadScene)
                 {
@@ -309,7 +316,7 @@ public void MoveObjectsFromSceneToDontDestroyOnLoad(ref NetworkManager networkMa
                         UnityEngine.Object.DontDestroyOnLoad(networkObject.gameObject);
                     }
                 }
-                else if (networkManager.IsServer)
+                else if (networkObject.HasAuthority)
                 {
                     networkObject.Despawn();
                 }

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

@@ -1062,7 +1062,8 @@ internal NetworkObject GetSceneRelativeInSceneNetworkObject(uint globalObjectIdH
         /// <param name="targetClientIds">array of client identifiers to receive the scene event message</param>
         private void SendSceneEventData(uint sceneEventId, ulong[] targetClientIds)
         {
-            if (targetClientIds.Length == 0 && !NetworkManager.DistributedAuthorityMode)
+            var distributedAuthority = NetworkManager.DistributedAuthorityMode;
+            if (targetClientIds.Length == 0 && !distributedAuthority)
             {
                 // This would be the Host/Server with no clients connected
                 // Silently return as there is nothing to be done
@@ -1072,7 +1073,7 @@ private void SendSceneEventData(uint sceneEventId, ulong[] targetClientIds)
             sceneEvent.SenderClientId = NetworkManager.LocalClientId;
 
             // Send related message to the CMB service
-            if (NetworkManager.DistributedAuthorityMode && NetworkManager.CMBServiceConnection && HasSceneAuthority())
+            if (distributedAuthority && NetworkManager.CMBServiceConnection && HasSceneAuthority())
             {
                 sceneEvent.TargetClientId = NetworkManager.ServerClientId;
                 var message = new SceneEventMessage
@@ -1092,7 +1093,7 @@ private void SendSceneEventData(uint sceneEventId, ulong[] targetClientIds)
                 {
                     EventData = sceneEvent,
                 };
-                var sendTarget = NetworkManager.CMBServiceConnection ? NetworkManager.ServerClientId : clientId;
+                var sendTarget = distributedAuthority && !NetworkManager.DAHost ? NetworkManager.ServerClientId : clientId;
                 var size = NetworkManager.ConnectionManager.SendMessage(ref message, k_DeliveryType, sendTarget);
                 NetworkManager.NetworkMetrics.TrackSceneEventSent(clientId, (uint)sceneEvent.SceneEventType, SceneNameFromHash(sceneEvent.SceneHash), size);
             }
@@ -2679,13 +2680,20 @@ internal void MoveObjectsToDontDestroyOnLoad()
             // Create a local copy of the spawned objects list since the spawn manager will adjust the list as objects
             // are despawned.
             var localSpawnedObjectsHashSet = new HashSet<NetworkObject>(NetworkManager.SpawnManager.SpawnedObjectsList);
+            var distributedAuthority = NetworkManager.DistributedAuthorityMode;
             foreach (var networkObject in localSpawnedObjectsHashSet)
             {
                 if (networkObject == null || (networkObject != null && networkObject.gameObject.scene == DontDestroyOnLoadScene))
                 {
                     continue;
                 }
 
+                // Check to determine if we need to allow destroying a non-authority instance
+                if (distributedAuthority && networkObject.DestroyWithScene && !networkObject.HasAuthority)
+                {
+                    networkObject.DestroyPendingSceneEvent = true;
+                }
+
                 // Only NetworkObjects marked to not be destroyed with the scene
                 if (!networkObject.DestroyWithScene)
                 {

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

@@ -1,4 +1,5 @@
 using System.Collections;
+using System.Collections.Generic;
 using NUnit.Framework;
 using Unity.Netcode.TestHelpers.Runtime;
 using UnityEngine;
@@ -21,56 +22,91 @@ internal class NetworkObjectDestroyTests : NetcodeIntegrationTest
     {
         protected override int NumberOfClients => 2;
 
-        // TODO: [CmbServiceTests] Adapt to run with the service
-        protected override bool UseCMBService()
+        public class DestroyTestComponent : NetworkBehaviour
         {
-            return false;
+            public static List<string> ObjectsDestroyed = new List<string>();
+
+            public override void OnDestroy()
+            {
+                ObjectsDestroyed.Add(gameObject.name);
+                base.OnDestroy();
+            }
         }
 
         public NetworkObjectDestroyTests(NetworkTopologyTypes networkTopologyType) : base(networkTopologyType) { }
 
+        protected override IEnumerator OnSetup()
+        {
+            // Re-apply the default for each test
+            LogAssert.ignoreFailingMessages = false;
+            DestroyTestComponent.ObjectsDestroyed.Clear();
+            return base.OnSetup();
+        }
+
         protected override void OnCreatePlayerPrefab()
         {
+            m_PlayerPrefab.AddComponent<DestroyTestComponent>();
             var playerNetworkObject = m_PlayerPrefab.GetComponent<NetworkObject>();
             playerNetworkObject.SceneMigrationSynchronization = true;
             base.OnCreatePlayerPrefab();
         }
 
+        private NetworkManager GetAuthorityOfNetworkObject(ulong networkObjectId)
+        {
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(networkObjectId))
+                {
+                    continue;
+                }
+
+                if (networkManager.SpawnManager.SpawnedObjects[networkObjectId].HasAuthority)
+                {
+                    return networkManager;
+                }
+            }
+            return null;
+        }
+
+        private bool NetworkObjectDoesNotExist(ulong networkObjectId)
+        {
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                if (networkManager.SpawnManager.SpawnedObjects.ContainsKey(networkObjectId))
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
         /// <summary>
-        /// Tests that a server can destroy a NetworkObject and that it gets despawned correctly.
+        /// Tests that the authority NetworkManager instance of a NetworkObject is allowed to destroy it.
         /// </summary>
-        /// <returns></returns>
+        /// <returns>IEnumerator</returns>
         [UnityTest]
         public IEnumerator TestNetworkObjectAuthorityDestroy()
         {
-            // This is the *SERVER VERSION* of the *CLIENT PLAYER*
-            var serverClientPlayerResult = new NetcodeIntegrationTestHelpers.ResultWrapper<NetworkObject>();
-            yield return NetcodeIntegrationTestHelpers.GetNetworkObjectByRepresentation(x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId, m_ServerNetworkManager, serverClientPlayerResult);
 
-            // This is the *CLIENT VERSION* of the *CLIENT PLAYER*
-            var clientClientPlayerResult = new NetcodeIntegrationTestHelpers.ResultWrapper<NetworkObject>();
-            yield return NetcodeIntegrationTestHelpers.GetNetworkObjectByRepresentation(x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId, m_ClientNetworkManagers[0], clientClientPlayerResult);
+            var ownerNetworkManager = m_ClientNetworkManagers[1];
+            var clientId = ownerNetworkManager.LocalClientId;
+            var localClientPlayer = ownerNetworkManager.LocalClient.PlayerObject;
+            var localNetworkObjectId = localClientPlayer.NetworkObjectId;
 
-            Assert.IsNotNull(serverClientPlayerResult.Result.gameObject);
-            Assert.IsNotNull(clientClientPlayerResult.Result.gameObject);
+            var authorityNetworkManager = GetAuthorityOfNetworkObject(localClientPlayer.NetworkObjectId);
+            Assert.True(authorityNetworkManager != null, $"Could not find the authority of {localClientPlayer}!");
 
-            var targetNetworkManager = m_ClientNetworkManagers[0];
-            if (m_DistributedAuthority)
-            {
-                targetNetworkManager = m_ClientNetworkManagers[1];
-                // destroy the authoritative player (distributed authority)
-                Object.Destroy(clientClientPlayerResult.Result.gameObject);
-            }
-            else
-            {
-                // destroy the authoritative player (client-server)
-                Object.Destroy(serverClientPlayerResult.Result.gameObject);
-            }
+            var authorityPlayerClone = authorityNetworkManager.ConnectedClients[clientId].PlayerObject;
+
+            // Have the authority NetworkManager destroy the player instance
+            Object.Destroy(authorityPlayerClone.gameObject);
+
+            var messageListener = m_DistributedAuthority ? m_ClientNetworkManagers[0] : m_ClientNetworkManagers[1];
 
-            yield return NetcodeIntegrationTestHelpers.WaitForMessageOfTypeHandled<DestroyObjectMessage>(targetNetworkManager);
+            yield return NetcodeIntegrationTestHelpers.WaitForMessageOfTypeHandled<DestroyObjectMessage>(messageListener);
 
-            Assert.IsTrue(serverClientPlayerResult.Result == null); // Assert.IsNull doesn't work here
-            Assert.IsTrue(clientClientPlayerResult.Result == null);
+            yield return WaitForConditionOrTimeOut(() => NetworkObjectDoesNotExist(localNetworkObjectId));
+            AssertOnTimeout($"Not all network managers despawned and destroyed player instance NetworkObjectId: {localNetworkObjectId}");
 
             // validate that any unspawned networkobject can be destroyed
             var go = new GameObject();
@@ -97,62 +133,47 @@ public enum ClientDestroyObject
         public IEnumerator TestNetworkObjectClientDestroy([Values] ClientDestroyObject clientDestroyObject)
         {
             var isShuttingDown = clientDestroyObject == ClientDestroyObject.ShuttingDown;
-            var clientPlayer = m_ClientNetworkManagers[0].LocalClient.PlayerObject;
-            var clientId = clientPlayer.OwnerClientId;
 
-            //destroying a NetworkObject while shutting down is allowed
+            var localNetworkManager = m_ClientNetworkManagers[1];
+            var clientId = localNetworkManager.LocalClientId;
+            var localClientPlayer = localNetworkManager.LocalClient.PlayerObject;
+
+            var nonAuthorityClient = m_ClientNetworkManagers[0];
+            var clientPlayerClone = nonAuthorityClient.ConnectedClients[clientId].PlayerObject;
+
             if (isShuttingDown)
             {
-                if (m_DistributedAuthority)
-                {
-                    // Shutdown the 2nd client
-                    m_ClientNetworkManagers[1].Shutdown();
-                }
-                else
-                {
-                    // Shutdown the
-                    m_ClientNetworkManagers[0].Shutdown();
-                }
+                // The non-authority client is allowed to destroy any spawned object it does not
+                // have authority over when it shuts down.
+                nonAuthorityClient.Shutdown();
             }
             else
             {
+                // The non-authority client is =NOT= allowed to destroy any spawned object it does not
+                // have authority over during runtime.
                 LogAssert.ignoreFailingMessages = true;
-                NetworkLog.NetworkManagerOverride = m_ClientNetworkManagers[0];
+                NetworkLog.NetworkManagerOverride = nonAuthorityClient;
+                Object.Destroy(clientPlayerClone.gameObject);
             }
 
-            m_ClientPlayerName = clientPlayer.gameObject.name;
-            m_ClientNetworkObjectId = clientPlayer.NetworkObjectId;
-            if (m_DistributedAuthority)
-            {
-                m_ClientPlayerName = m_PlayerNetworkObjects[m_ClientNetworkManagers[1].LocalClientId][m_ClientNetworkManagers[0].LocalClientId].gameObject.name;
-                m_ClientNetworkObjectId = m_PlayerNetworkObjects[m_ClientNetworkManagers[1].LocalClientId][m_ClientNetworkManagers[0].LocalClientId].NetworkObjectId;
-
-                if (!isShuttingDown)
-                {
-                    NetworkLog.NetworkManagerOverride = m_ClientNetworkManagers[1];
-                }
-                // the 2nd client attempts to destroy the 1st client's player object (if shutting down then "ok" if not then not "ok")
-                Object.DestroyImmediate(m_PlayerNetworkObjects[m_ClientNetworkManagers[1].LocalClientId][m_ClientNetworkManagers[0].LocalClientId].gameObject);
-            }
-            else
-            {
-                // the 1st client attempts to destroy its own player object (if shutting down then "ok" if not then not "ok")
-                Object.DestroyImmediate(m_ClientNetworkManagers[0].LocalClient.PlayerObject.gameObject);
-            }
+            m_ClientPlayerName = clientPlayerClone.gameObject.name;
+            m_ClientNetworkObjectId = clientPlayerClone.NetworkObjectId;
 
             // destroying a NetworkObject while a session is active is not allowed
             if (!isShuttingDown)
             {
                 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)}!");
+                bool NonAuthorityClientDestroyed()
+                {
+                    return DestroyTestComponent.ObjectsDestroyed.Contains(m_ClientPlayerName);
+                }
+
+                yield return WaitForConditionOrTimeOut(NonAuthorityClientDestroyed);
+                AssertOnTimeout($"Timed out waiting for player object {m_ClientNetworkObjectId} to no longer exist within {nameof(NetworkSpawnManager.NetworkObjectsToSynchronizeSceneChanges)}!");
             }
         }
 
@@ -183,8 +204,14 @@ private bool HaveLogsBeenReceived()
         protected override IEnumerator OnTearDown()
         {
             NetworkLog.NetworkManagerOverride = null;
-            LogAssert.ignoreFailingMessages = false;
             return base.OnTearDown();
         }
+
+        protected override void OnOneTimeTearDown()
+        {
+            // Re-apply the default as the last exiting action
+            LogAssert.ignoreFailingMessages = false;
+            base.OnOneTimeTearDown();
+        }
     }
 }