fix: Fix IsOwner/IsOwnedByServer being wrong on the server after calling RemoveOwnership [MTT-4259] (#2211)

* fix: Fix IsOwner/IsOwnedByServer being wrong on the server after calling RemoveOwnership

Author: ShadauxCat

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -36,6 +36,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed throwing an exception in `OnNetworkUpdate` causing other `OnNetworkUpdate` calls to not be executed. (#1739)
 - Fixed synchronization when Time.timeScale is set to 0. This changes timing update to use unscaled deltatime. Now network updates rate are independent from the local time scale. (#2171)
 - Fixed not sending all NetworkVariables to all clients when a client connects to a server. (#1987)
+- Fixed IsOwner/IsOwnedByServer being wrong on the server after calling RemoveOwnership (#2211)
 
 ## [1.0.2] - 2022-09-12
 

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

@@ -213,11 +213,11 @@ internal void RemoveOwnership(NetworkObject networkObject)
                 return;
             }
 
+            networkObject.OwnerClientId = NetworkManager.ServerClientId;
+
             // Server removes the entry and takes over ownership before notifying
             UpdateOwnershipTable(networkObject, NetworkManager.ServerClientId, true);
 
-            networkObject.OwnerClientId = NetworkManager.ServerClientId;
-
             var message = new ChangeOwnershipMessage
             {
                 NetworkObjectId = networkObject.NetworkObjectId,

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

@@ -1,3 +1,4 @@
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using NUnit.Framework;
@@ -86,10 +87,19 @@ internal void AssignMessageType<T>() where T : INetworkMessage
             Initialize();
         }
 
+        internal void AssignMessageType(Type type)
+        {
+            MessageType = type.Name;
+            m_MessageReceiptCheck = (message) =>
+            {
+                return message.GetType() == type;
+            };
+            Initialize();
+        }
+
         public MessageHookEntry(NetworkManager networkManager)
         {
             m_NetworkManager = networkManager;
         }
     }
 }
-

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

@@ -762,6 +762,39 @@ protected IEnumerator WaitForClientsConnectedOrTimeOut()
             yield return WaitForClientsConnectedOrTimeOut(m_ClientNetworkManagers);
         }
 
+        internal IEnumerator WaitForMessageReceived<T>(List<NetworkManager> wiatForReceivedBy) where T : INetworkMessage
+        {
+            // Build our message hook entries tables so we can determine if all clients received spawn or ownership messages
+            var messageHookEntriesForSpawn = new List<MessageHookEntry>();
+            foreach (var clientNetworkManager in wiatForReceivedBy)
+            {
+                var messageHook = new MessageHookEntry(clientNetworkManager);
+                messageHook.AssignMessageType<T>();
+                messageHookEntriesForSpawn.Add(messageHook);
+            }
+            // Used to determine if all clients received the CreateObjectMessage
+            var hooks = new MessageHooksConditional(messageHookEntriesForSpawn);
+            yield return WaitForConditionOrTimeOut(hooks);
+        }
+
+        internal IEnumerator WaitForMessagesReceived(List<Type> messagesInOrder, List<NetworkManager> wiatForReceivedBy)
+        {
+            // Build our message hook entries tables so we can determine if all clients received spawn or ownership messages
+            var messageHookEntriesForSpawn = new List<MessageHookEntry>();
+            foreach (var clientNetworkManager in wiatForReceivedBy)
+            {
+                foreach (var message in messagesInOrder)
+                {
+                    var messageHook = new MessageHookEntry(clientNetworkManager);
+                    messageHook.AssignMessageType(message);
+                    messageHookEntriesForSpawn.Add(messageHook);
+                }
+            }
+            // Used to determine if all clients received the CreateObjectMessage
+            var hooks = new MessageHooksConditional(messageHookEntriesForSpawn);
+            yield return WaitForConditionOrTimeOut(hooks);
+        }
+
         /// <summary>
         /// Creates a basic NetworkObject test prefab, assigns it to a new
         /// NetworkPrefab entry, and then adds it to the server and client(s)

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

@@ -9,13 +9,19 @@ namespace Unity.Netcode.RuntimeTests
 {
     public class NetworkObjectNetworkClientOwnedObjectsTests : NetcodeIntegrationTest
     {
+        private class DummyNetworkBehaviour : NetworkBehaviour
+        {
+
+        }
+
         protected override int NumberOfClients => 1;
         private NetworkPrefab m_NetworkPrefab;
         protected override void OnServerAndClientsCreated()
         {
             // create prefab
             var gameObject = new GameObject("ClientOwnedObject");
             var networkObject = gameObject.AddComponent<NetworkObject>();
+            gameObject.AddComponent<DummyNetworkBehaviour>();
             NetcodeIntegrationTestHelpers.MakeNetworkObjectTestPrefab(networkObject);
 
             m_NetworkPrefab = (new NetworkPrefab()
@@ -39,7 +45,7 @@ public IEnumerator ChangeOwnershipOwnedObjectsAddTest()
             serverObject.Spawn();
 
             // Provide enough time for the client to receive and process the spawned message.
-            yield return s_DefaultWaitForTick;
+            yield return WaitForMessageReceived<CreateObjectMessage>(m_ClientNetworkManagers.ToList());
 
             // The object is owned by server
             Assert.False(m_ServerNetworkManager.SpawnManager.GetClientOwnedObjects(m_ClientNetworkManagers[0].LocalClientId).Any(x => x.NetworkObjectId == serverObject.NetworkObjectId));
@@ -48,13 +54,71 @@ public IEnumerator ChangeOwnershipOwnedObjectsAddTest()
             serverObject.ChangeOwnership(m_ClientNetworkManagers[0].LocalClientId);
 
             // Provide enough time for the client to receive and process the change in ownership message.
-            yield return s_DefaultWaitForTick;
+            yield return WaitForMessageReceived<ChangeOwnershipMessage>(m_ClientNetworkManagers.ToList());
 
             // Ensure it's now added to the list
-            yield return WaitForConditionOrTimeOut(() => m_ClientNetworkManagers[0].SpawnManager.GetClientOwnedObjects(m_ClientNetworkManagers[0].LocalClientId).Any(x => x.NetworkObjectId == serverObject.NetworkObjectId));
-            Assert.False(s_GlobalTimeoutHelper.TimedOut, $"Timed out waiting for client to gain ownership!");
             Assert.True(m_ClientNetworkManagers[0].SpawnManager.GetClientOwnedObjects(m_ClientNetworkManagers[0].LocalClientId).Any(x => x.NetworkObjectId == serverObject.NetworkObjectId));
             Assert.True(m_ServerNetworkManager.SpawnManager.GetClientOwnedObjects(m_ClientNetworkManagers[0].LocalClientId).Any(x => x.NetworkObjectId == serverObject.NetworkObjectId));
         }
+
+        [UnityTest]
+        public IEnumerator WhenOwnershipIsChanged_OwnershipValuesUpdateCorrectly()
+        {
+            NetworkObject serverObject = Object.Instantiate(m_NetworkPrefab.Prefab).GetComponent<NetworkObject>();
+            serverObject.NetworkManagerOwner = m_ServerNetworkManager;
+            serverObject.Spawn();
+
+            // Provide enough time for the client to receive and process the spawned message.
+            yield return WaitForMessageReceived<CreateObjectMessage>(m_ClientNetworkManagers.ToList());
+
+            // The object is owned by server
+            Assert.False(m_ServerNetworkManager.SpawnManager.GetClientOwnedObjects(m_ClientNetworkManagers[0].LocalClientId).Any(x => x.NetworkObjectId == serverObject.NetworkObjectId));
+
+            // Change the ownership
+            serverObject.ChangeOwnership(m_ClientNetworkManagers[0].LocalClientId);
+
+            // Provide enough time for the client to receive and process the change in ownership message.
+            yield return WaitForMessageReceived<ChangeOwnershipMessage>(m_ClientNetworkManagers.ToList());
+
+            Assert.IsFalse(serverObject.IsOwner);
+            Assert.IsFalse(serverObject.IsOwnedByServer);
+            Assert.AreEqual(m_ClientNetworkManagers[0].LocalClientId, serverObject.OwnerClientId);
+
+            var serverBehaviour = serverObject.GetComponent<DummyNetworkBehaviour>();
+            Assert.IsFalse(serverBehaviour.IsOwner);
+            Assert.IsFalse(serverBehaviour.IsOwnedByServer);
+            Assert.AreEqual(m_ClientNetworkManagers[0].LocalClientId, serverBehaviour.OwnerClientId);
+
+            var clientObject = Object.FindObjectsOfType<NetworkObject>().Where((obj) => obj.NetworkManagerOwner == m_ClientNetworkManagers[0]).FirstOrDefault();
+
+            Assert.IsNotNull(clientObject);
+            Assert.IsTrue(clientObject.IsOwner);
+            Assert.IsFalse(clientObject.IsOwnedByServer);
+            Assert.AreEqual(m_ClientNetworkManagers[0].LocalClientId, clientObject.OwnerClientId);
+
+            var clientBehaviour = clientObject.GetComponent<DummyNetworkBehaviour>();
+            Assert.IsTrue(clientBehaviour.IsOwner);
+            Assert.IsFalse(clientBehaviour.IsOwnedByServer);
+            Assert.AreEqual(m_ClientNetworkManagers[0].LocalClientId, clientBehaviour.OwnerClientId);
+
+            serverObject.RemoveOwnership();
+
+            // Provide enough time for the client to receive and process the change in ownership message.
+            yield return WaitForMessageReceived<ChangeOwnershipMessage>(m_ClientNetworkManagers.ToList());
+
+            Assert.IsTrue(serverObject.IsOwner);
+            Assert.IsTrue(serverObject.IsOwnedByServer);
+            Assert.AreEqual(NetworkManager.ServerClientId, serverObject.OwnerClientId);
+            Assert.IsTrue(serverBehaviour.IsOwner);
+            Assert.IsTrue(serverBehaviour.IsOwnedByServer);
+            Assert.AreEqual(NetworkManager.ServerClientId, serverBehaviour.OwnerClientId);
+
+            Assert.IsFalse(clientObject.IsOwner);
+            Assert.IsTrue(clientObject.IsOwnedByServer);
+            Assert.AreEqual(NetworkManager.ServerClientId, clientObject.OwnerClientId);
+            Assert.IsFalse(clientBehaviour.IsOwner);
+            Assert.IsTrue(clientBehaviour.IsOwnedByServer);
+            Assert.AreEqual(NetworkManager.ServerClientId, clientBehaviour.OwnerClientId);
+        }
     }
 }