fix: owner changing ownership causing identical previous and current ids when using a distributed authority network topology (#3347)

This fixes an issue with the synchronization of NetworkVariables when
changing ownership where it was possible to have the previous owner be
equal to the current owner when using a distributed authority network
topology and the owner handled changing the ownership.

This includes an additional check for ditry states of any collections
base NetworkVariables.

<!-- Add short version of the JIRA ticket to the PR title (e.g. "feat:
new shiny feature [MTT-123]") -->

fix: #3343 
close: #3343 

## Changelog

- Fixed issue where an owner that changes ownership, when using a
distributed authority network topology, could yield identical previous
and current owner identifiers. This could also cause `NetworkTransform`
to fail to change ownership which would leave the previous owner still
subscribed to network tick events.

## Testing and Documentation

- Includes integration test
`NetworkObjectOwnershipTests.TestAuthorityChangingOwnership`.
- No documentation changes or additions were necessary.

<!-- Uncomment and mark items off with a * if this PR deprecates any
API:
### Deprecated API
- [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter
yyyy-mm-dd)` entry.
- [ ] An [api updater] was added.
- [ ] Deprecation of the API is explained in the CHANGELOG.
- [ ] The users can understand why this API was removed and what they
should use instead.
-->

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -20,6 +20,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 - Fixed issue when using a distributed authority network topology and many clients attempt to connect simultaneously the session owner could max-out the maximum in-flight reliable messages allowed, start dropping packets, and some of the connecting clients would fail to fully synchronize. (#3350)
 - Fixed issue when using a distributed authority network topology and scene management was disabled clients would not be able to spawn any new network prefab instances until synchronization was complete. (#3350)
+- Fixed issue where an owner that changes ownership, when using a distributed authority network topology, could yield identical previous and current owner identifiers. This could also cause `NetworkTransform` to fail to change ownership which would leave the previous owner still subscribed to network tick events. (#3347)
 - Fixed issue where the `MaximumInterpolationTime` could not be modified from within the inspector view or runtime. (#3337)
 - Fixed `ChangeOwnership` changing ownership to clients that are not observers. This also happened with automated object distribution. (#3323)
 - Fixed issue where `AnticipatedNetworkVariable` previous value returned by `AnticipatedNetworkVariable.OnAuthoritativeValueChanged` is updated correctly on the non-authoritative side. (#3306)

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

@@ -1185,14 +1185,23 @@ internal void MarkVariablesDirty(bool dirty)
             }
         }
 
-        internal void MarkOwnerReadVariablesDirty()
+        /// <summary>
+        /// For owner read permissions, when changing ownership we need to do a full synchronization
+        /// of all NetworkVariables that are owner read permission based since the owner is the only
+        /// instance that knows what the most current values are.
+        /// </summary>
+        internal void MarkOwnerReadDirtyAndCheckOwnerWriteIsDirty()
         {
             for (int j = 0; j < NetworkVariableFields.Count; j++)
             {
                 if (NetworkVariableFields[j].ReadPerm == NetworkVariableReadPermission.Owner)
                 {
                     NetworkVariableFields[j].SetDirty(true);
                 }
+                if (NetworkVariableFields[j].WritePerm == NetworkVariableWritePermission.Owner)
+                {
+                    NetworkVariableFields[j].OnCheckIsDirtyState();
+                }
             }
         }
 

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

@@ -108,8 +108,6 @@ internal void NetworkBehaviourUpdate(bool forceSend = false)
                 foreach (var dirtyobj in m_DirtyNetworkObjects)
                 {
                     dirtyobj.PostNetworkVariableWrite(forceSend);
-                    // Once done processing, we set the previous owner id to the current owner id
-                    dirtyobj.PreviousOwnerId = dirtyobj.OwnerClientId;
                 }
                 m_DirtyNetworkObjects.Clear();
             }

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

@@ -2567,12 +2567,39 @@ internal void MarkVariablesDirty(bool dirty)
             }
         }
 
-        internal void MarkOwnerReadVariablesDirty()
+        /// <summary>
+        /// Used when changing ownership, this will mark any owner read permission base NetworkVariables as dirty
+        /// and will check if any owner write permission NetworkVariables are dirty (primarily for collections) so
+        /// the new owner will get a full state update prior to changing ownership.
+        /// </summary>
+        /// <remarks>
+        /// We have to pass in the original owner and previous owner to "reset" back to the current state of this
+        /// NetworkObject in order to preserve the same ownership change flow. By the time this is invoked, the
+        /// new and previous owner ids have already been set.
+        /// </remarks>
+        /// <param name="originalOwnerId">the owner prior to beginning the change in ownership change.</param>
+        /// <param name="originalPreviousOwnerId">the previous owner prior to beginning the change in ownership change.</param>
+        internal void SynchronizeOwnerNetworkVariables(ulong originalOwnerId, ulong originalPreviousOwnerId)
         {
+            var currentOwnerId = OwnerClientId;
+            OwnerClientId = originalOwnerId;
+            PreviousOwnerId = originalPreviousOwnerId;
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
-                ChildNetworkBehaviours[i].MarkOwnerReadVariablesDirty();
+                ChildNetworkBehaviours[i].MarkOwnerReadDirtyAndCheckOwnerWriteIsDirty();
             }
+
+            // Now set the new owner and previous owner identifiers back to their original new values
+            // before we run the NetworkBehaviourUpdate. For owner read only permissions this order of
+            // operations is **particularly important** as we need to first (above) mark things as dirty
+            // from the context of the original owner and then second (below) we need to send the messages
+            // which requires the new owner to be set for owner read permission NetworkVariables.
+            OwnerClientId = currentOwnerId;
+            PreviousOwnerId = originalOwnerId;
+
+            // Force send a state update for all owner read NetworkVariables  and any currently dirty
+            // owner write NetworkVariables.
+            NetworkManager.BehaviourUpdater.NetworkBehaviourUpdate(true);
         }
 
         // NGO currently guarantees that the client will receive spawn data for all objects in one network tick.

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

@@ -381,11 +381,8 @@ private void HandleOwnershipChange(ref NetworkContext context)
 
             if (originalOwner == networkManager.LocalClientId && !networkManager.DistributedAuthorityMode)
             {
-                // Mark any owner read variables as dirty
-                networkObject.MarkOwnerReadVariablesDirty();
-                // Immediately queue any pending deltas and order the message before the
-                // change in ownership message.
-                networkManager.BehaviourUpdater.NetworkBehaviourUpdate(true);
+                // Fully synchronize NetworkVariables with either read or write ownership permissions. 
+                networkObject.SynchronizeOwnerNetworkVariables(originalOwner, networkObject.PreviousOwnerId);
             }
 
             // Always invoke ownership change notifications

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs

@@ -166,6 +166,13 @@ public bool CheckDirtyState(bool forceCheck = false)
             return isDirty;
         }
 
+        /// <inheritdoc/>
+        internal override void OnCheckIsDirtyState()
+        {
+            CheckDirtyState();
+            base.OnCheckIsDirtyState();
+        }
+
         internal ref T RefValue()
         {
             return ref m_InternalValue;

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs

@@ -373,6 +373,15 @@ internal ulong OwnerClientId()
             return m_NetworkBehaviour.NetworkObject.OwnerClientId;
         }
 
+        /// <summary>
+        /// Primarily to check for collections dirty states when doing
+        /// a fully owner read/write NetworkVariable update.
+        /// </summary>
+        internal virtual void OnCheckIsDirtyState()
+        {
+
+        }
+
         /// <summary>
         /// Writes the dirty changes, that is, the changes since the variable was last dirty, to the writer
         /// </summary>

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

@@ -432,6 +432,15 @@ internal void RemoveOwnership(NetworkObject networkObject)
 
         internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool isAuthorized, bool isRequestApproval = false)
         {
+            if (clientId == networkObject.OwnerClientId)
+            {
+                if (NetworkManager.LogLevel <= LogLevel.Developer)
+                {
+                    Debug.LogWarning($"[{nameof(NetworkSpawnManager)}][{nameof(ChangeOwnership)}] Attempting to change ownership to Client-{clientId} when the owner is already {networkObject.OwnerClientId}! (Ignoring)");
+                }
+                return;
+            }
+
             // For client-server:
             // If ownership changes faster than the latency between the client-server and there are NetworkVariables being updated during ownership changes,
             // then notify the user they could potentially lose state updates if developer logging is enabled.
@@ -530,6 +539,10 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool
                 return;
             }
 
+            // Save the previous owner to work around a bit of a nasty issue with assuring NetworkVariables are serialized when ownership changes
+            var originalPreviousOwnerId = networkObject.PreviousOwnerId;
+            var originalOwner = networkObject.OwnerClientId;
+
             // Used to distinguish whether a new owner should receive any currently dirty NetworkVariable updates
             networkObject.PreviousOwnerId = networkObject.OwnerClientId;
 
@@ -545,13 +558,10 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool
             // Always notify locally on the server when a new owner is assigned
             networkObject.InvokeBehaviourOnGainedOwnership();
 
-            if (networkObject.PreviousOwnerId == NetworkManager.LocalClientId)
+            // If we are the original owner, then we want to synchronize owner read & write NetworkVariables.
+            if (originalOwner == NetworkManager.LocalClientId)
             {
-                // Mark any owner read variables as dirty
-                networkObject.MarkOwnerReadVariablesDirty();
-                // Immediately queue any pending deltas and order the message before the
-                // change in ownership message.
-                NetworkManager.BehaviourUpdater.NetworkBehaviourUpdate(true);
+                networkObject.SynchronizeOwnerNetworkVariables(originalOwner, originalPreviousOwnerId);
             }
 
             var size = 0;

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

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using NUnit.Framework;
+using Unity.Netcode.Components;
 using Unity.Netcode.TestHelpers.Runtime;
 using UnityEngine;
 using UnityEngine.TestTools;
@@ -25,6 +26,12 @@ public override void OnGainedOwnership()
             OnGainedOwnershipFired = true;
         }
 
+        protected override void OnOwnershipChanged(ulong previous, ulong current)
+        {
+            Assert.True(previous != current, $"[{nameof(OnOwnershipChanged)}][Invalid Parameters] Invoked and the previous ({previous}) equals the current ({current})!");
+            base.OnOwnershipChanged(previous, current);
+        }
+
         public override void OnNetworkSpawn()
         {
             if (!SpawnedInstances.ContainsKey(NetworkManager.LocalClientId))
@@ -39,6 +46,12 @@ public void ResetFlags()
             OnLostOwnershipFired = false;
             OnGainedOwnershipFired = false;
         }
+
+        [Rpc(SendTo.Authority)]
+        public void ChangeOwnershipRpc(RpcParams rpcParams = default)
+        {
+            NetworkObject.ChangeOwnership(rpcParams.Receive.SenderClientId);
+        }
     }
 
 
@@ -71,6 +84,7 @@ protected override void OnServerAndClientsCreated()
         {
             m_OwnershipPrefab = CreateNetworkObjectPrefab("OnwershipPrefab");
             m_OwnershipPrefab.AddComponent<NetworkObjectOwnershipComponent>();
+            m_OwnershipPrefab.AddComponent<NetworkTransform>();
             if (m_DistributedAuthority)
             {
                 m_OwnershipPrefab.GetComponent<NetworkObject>().SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable);
@@ -465,7 +479,87 @@ public IEnumerator TestOwnedObjectCounts()
 
             yield return WaitForConditionOrTimeOut(ServerHasCorrectClientOwnedObjectCount);
             AssertOnTimeout($"Server does not have the correct count for all clients spawned {k_NumberOfSpawnedObjects} {nameof(NetworkObject)}s!");
+        }
 
+        /// <summary>
+        /// Validates that when changing ownership NetworkTransform does not enter into a bad state
+        /// because the previous and current owner identifiers are the same. For client-server this
+        /// ends up always being the server, but for distributed authority the authority changes when
+        /// ownership changes.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator TestAuthorityChangingOwnership()
+        {
+            var authorityManager = (NetworkManager)null;
+            var allNetworkManagers = m_ClientNetworkManagers.ToList();
+            allNetworkManagers.Add(m_ServerNetworkManager);
+
+            if (m_DistributedAuthority)
+            {
+                var authorityId = Random.Range(1, NumberOfClients) - 1;
+                authorityManager = m_ClientNetworkManagers[authorityId];
+                m_OwnershipObject = SpawnObject(m_OwnershipPrefab, authorityManager);
+                m_OwnershipNetworkObject = m_OwnershipObject.GetComponent<NetworkObject>();
+            }
+            else
+            {
+                authorityManager = m_ServerNetworkManager;
+                m_OwnershipObject = SpawnObject(m_OwnershipPrefab, m_ServerNetworkManager);
+                m_OwnershipNetworkObject = m_OwnershipObject.GetComponent<NetworkObject>();
+            }
+            var ownershipNetworkObjectId = m_OwnershipNetworkObject.NetworkObjectId;
+            bool WaitForClientsToSpawnNetworkObject()
+            {
+                foreach (var clientNetworkManager in m_ClientNetworkManagers)
+                {
+                    if (!clientNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(ownershipNetworkObjectId))
+                    {
+                        return false;
+                    }
+                }
+                return true;
+            }
+
+            yield return WaitForConditionOrTimeOut(WaitForClientsToSpawnNetworkObject);
+            AssertOnTimeout($"Timed out waiting for all clients to spawn the {m_OwnershipNetworkObject.name} {nameof(NetworkObject)} instance!");
+
+            var currentTargetOwner = (ulong)0;
+            bool WaitForAllInstancesToChangeOwnership()
+            {
+                foreach (var clientNetworkManager in m_ClientNetworkManagers)
+                {
+                    if (!clientNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(ownershipNetworkObjectId))
+                    {
+                        return false;
+                    }
+                    if (clientNetworkManager.SpawnManager.SpawnedObjects[ownershipNetworkObjectId].OwnerClientId != currentTargetOwner)
+                    {
+                        return false;
+                    }
+                }
+                return true;
+            }
+
+            // Change ownership a few times and as long as the previous and current owners are not the same when
+            // OnOwnershipChanged is invoked then the test passed.
+            foreach (var networkManager in allNetworkManagers)
+            {
+                if (networkManager == authorityManager)
+                {
+                    continue;
+                }
+                var clonedObject = networkManager.SpawnManager.SpawnedObjects[ownershipNetworkObjectId];
+
+                if (clonedObject.OwnerClientId == networkManager.LocalClientId)
+                {
+                    continue;
+                }
+
+                var testComponent = clonedObject.GetComponent<NetworkObjectOwnershipComponent>();
+                testComponent.ChangeOwnershipRpc();
+                yield return WaitForAllInstancesToChangeOwnership();
+                AssertOnTimeout($"Timed out waiting for all instances to change ownership to Client-{networkManager.LocalClientId}!");
+            }
         }
     }
 }