fix: RemoveOwnership not locally notifying server that it gained owne…

…rship [MTT-6377] [MTT-6937][MTT-6936] (#2618) * fix This resolves a few ownership related issues specifically noticed when using an owner authoritative NetworkTransform and ownership is removed. This also assures NetworkVariables will be updated if ownership is removed just like they are when ownership is changed. This also fixes an issue where NetworkManager's ShutdownInProgress was not true upon the application quitting which was causing an exception to occur within NetworkVariableBase under specific conditions. * test Removing a warning expect check that is no longer needed. Adding additional checks to validate RemoveOwnership generates an OnOwnershipChanged notification.
Unity-Technologies · Jul 14, 2023 · 208d1c9 · 208d1c9
1 parent 9af9280
commit 208d1c9
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 58 deletions.
diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md
@@ -10,15 +10,14 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Added
 
-
 ### Fixed
 
 - Fixed issue where a `NetworkTransform` using full precision state updates was losing transform state updates when interpolation was enabled. (#2624)
 - Fixed issue where `NetworkObject.SpawnWithObservers` was not being honored for late joining clients. (#2623)
 - Fixed issue where invoking `NetworkManager.Shutdown` multiple times, depending upon the timing, could cause an exception. (#2622)
+- Fixed issue where removing ownership would not notify the server that it gained ownership. This also resolves the issue where an owner authoritative NetworkTransform would not properly initialize upon removing ownership from a remote client. (#2618)
 
-## Changed
-
+### Changed
 
 ## [1.5.1] - 2023-06-07
 

diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs
@@ -1035,6 +1035,8 @@ internal void ShutdownInternal()
         // Ensures that the NetworkManager is cleaned up before OnDestroy is run on NetworkObjects and NetworkBehaviours when quitting the application.
         private void OnApplicationQuit()
         {
+            // Make sure ShutdownInProgress returns true during this time
+            m_ShuttingDown = true;
             OnDestroy();
         }
 

diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs
@@ -113,12 +113,6 @@ internal void UpdateOwnershipTable(NetworkObject networkObject, ulong newOwner,
                     // Remove the previous owner's entry
                     OwnershipToObjectsTable[previousOwner].Remove(networkObject.NetworkObjectId);
 
-                    // Server or Host alway invokes the lost ownership notification locally
-                    if (NetworkManager.IsServer)
-                    {
-                        networkObject.InvokeBehaviourOnLostOwnership();
-                    }
-
                     // If we are removing the entry (i.e. despawning or client lost ownership)
                     if (isRemoving)
                     {
@@ -143,12 +137,6 @@ internal void UpdateOwnershipTable(NetworkObject networkObject, ulong newOwner,
             {
                 // Add the new ownership entry
                 OwnershipToObjectsTable[newOwner].Add(networkObject.NetworkObjectId, networkObject);
-
-                // Server or Host always invokes the gained ownership notification locally
-                if (NetworkManager.IsServer)
-                {
-                    networkObject.InvokeBehaviourOnGainedOwnership();
-                }
             }
             else if (isRemoving)
             {
@@ -227,43 +215,6 @@ public NetworkObject GetPlayerNetworkObject(ulong clientId)
             return null;
         }
 
-        internal void RemoveOwnership(NetworkObject networkObject)
-        {
-            if (!NetworkManager.IsServer)
-            {
-                throw new NotServerException("Only the server can change ownership");
-            }
-
-            if (!networkObject.IsSpawned)
-            {
-                throw new SpawnStateException("Object is not spawned");
-            }
-
-            // If we made it here then we are the server and if the server is determined to already be the owner
-            // then ignore the RemoveOwnership invocation.
-            if (networkObject.OwnerClientId == NetworkManager.ServerClientId)
-            {
-                return;
-            }
-
-            networkObject.OwnerClientId = NetworkManager.ServerClientId;
-
-            // Server removes the entry and takes over ownership before notifying
-            UpdateOwnershipTable(networkObject, NetworkManager.ServerClientId, true);
-
-            var message = new ChangeOwnershipMessage
-            {
-                NetworkObjectId = networkObject.NetworkObjectId,
-                OwnerClientId = networkObject.OwnerClientId
-            };
-            var size = NetworkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, NetworkManager.ConnectedClientsIds);
-
-            foreach (var client in NetworkManager.ConnectedClients)
-            {
-                NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject, size);
-            }
-        }
-
         /// <summary>
         /// Helper function to get a network client for a clientId from the NetworkManager.
         /// On the server this will check the <see cref="NetworkManager.ConnectedClients"/> list.
@@ -289,6 +240,11 @@ private bool TryGetNetworkClient(ulong clientId, out NetworkClient networkClient
             return false;
         }
 
+        internal void RemoveOwnership(NetworkObject networkObject)
+        {
+            ChangeOwnership(networkObject, NetworkManager.ServerClientId);
+        }
+
         internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
         {
             if (!NetworkManager.IsServer)
@@ -301,14 +257,21 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
                 throw new SpawnStateException("Object is not spawned");
             }
 
+            // Assign the new owner
             networkObject.OwnerClientId = clientId;
 
+            // Always notify locally on the server when ownership is lost
+            networkObject.InvokeBehaviourOnLostOwnership();
+
             networkObject.MarkVariablesDirty(true);
             NetworkManager.BehaviourUpdater.AddForUpdate(networkObject);
 
             // Server adds entries for all client ownership
             UpdateOwnershipTable(networkObject, networkObject.OwnerClientId);
 
+            // Always notify locally on the server when a new owner is assigned
+            networkObject.InvokeBehaviourOnGainedOwnership();
+
             var message = new ChangeOwnershipMessage
             {
                 NetworkObjectId = networkObject.NetworkObjectId,

diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkBehaviourGenericTests.cs
@@ -67,7 +67,6 @@ public IEnumerator ValidatedDisableddNetworkBehaviourWarning()
             // Set the child object to be inactive in the hierarchy
             childObject.SetActive(false);
 
-            LogAssert.Expect(LogType.Warning, $"{childObject.name} is disabled! Netcode for GameObjects does not support disabled NetworkBehaviours! The {childBehaviour.GetType().Name} component was skipped during ownership assignment!");
             LogAssert.Expect(LogType.Warning, $"{childObject.name} is disabled! Netcode for GameObjects does not support spawning disabled NetworkBehaviours! The {childBehaviour.GetType().Name} component was skipped during spawn!");
 
             parentNetworkObject.Spawn();

diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs
@@ -42,6 +42,12 @@ public class NetworkObjectOwnershipTests : NetcodeIntegrationTest
 
         public NetworkObjectOwnershipTests(HostOrServer hostOrServer) : base(hostOrServer) { }
 
+        public enum OwnershipChecks
+        {
+            Change,
+            Remove
+        }
+
         protected override void OnServerAndClientsCreated()
         {
             m_OwnershipPrefab = CreateNetworkObjectPrefab("OnwershipPrefab");
@@ -62,7 +68,7 @@ public void TestPlayerIsOwned()
         }
 
         [UnityTest]
-        public IEnumerator TestOwnershipCallbacks()
+        public IEnumerator TestOwnershipCallbacks([Values] OwnershipChecks ownershipChecks)
         {
             m_OwnershipObject = SpawnObject(m_OwnershipPrefab, m_ServerNetworkManager);
             m_OwnershipNetworkObject = m_OwnershipObject.GetComponent<NetworkObject>();
@@ -109,7 +115,17 @@ public IEnumerator TestOwnershipCallbacks()
             serverComponent.ResetFlags();
             clientComponent.ResetFlags();
 
-            serverObject.ChangeOwnership(NetworkManager.ServerClientId);
+            if (ownershipChecks == OwnershipChecks.Change)
+            {
+                // Validates that when ownership is changed back to the server it will get an OnGainedOwnership notification
+                serverObject.ChangeOwnership(NetworkManager.ServerClientId);
+            }
+            else
+            {
+                // Validates that when ownership is removed the server gets an OnGainedOwnership notification
+                serverObject.RemoveOwnership();
+            }
+
             yield return s_DefaultWaitForTick;
 
             Assert.That(serverComponent.OnGainedOwnershipFired);
@@ -125,7 +141,7 @@ public IEnumerator TestOwnershipCallbacks()
         /// Verifies that switching ownership between several clients works properly
         /// </summary>
         [UnityTest]
-        public IEnumerator TestOwnershipCallbacksSeveralClients()
+        public IEnumerator TestOwnershipCallbacksSeveralClients([Values] OwnershipChecks ownershipChecks)
         {
             // Build our message hook entries tables so we can determine if all clients received spawn or ownership messages
             var messageHookEntriesForSpawn = new List<MessageHookEntry>();
@@ -247,8 +263,17 @@ bool WaitForClientsToSpawnNetworkObject()
                 previousClientComponent = currentClientComponent;
             }
 
-            // Now change ownership back to the server
-            serverObject.ChangeOwnership(NetworkManager.ServerClientId);
+            if (ownershipChecks == OwnershipChecks.Change)
+            {
+                // Validates that when ownership is changed back to the server it will get an OnGainedOwnership notification
+                serverObject.ChangeOwnership(NetworkManager.ServerClientId);
+            }
+            else
+            {
+                // Validates that when ownership is removed the server gets an OnGainedOwnership notification
+                serverObject.RemoveOwnership();
+            }
+
             yield return WaitForConditionOrTimeOut(ownershipMessageHooks);
             Assert.False(s_GlobalTimeoutHelper.TimedOut, $"Timed out waiting for all clients to receive the {nameof(ChangeOwnershipMessage)} message (back to server).");