fix: NetworkShow followed by ChangeOwnership generates ownership message error on target client (#3468)

## Summary
[MTTB-1337](https://jira.unity3d.com/browse/MTTB-1337)

This addresses the
[Forum-Support-1646996](https://discussions.unity.com/t/does-networkshow-assign-ownership-internally-in-unity-netcode/1646996/10)
issue where it was determined if you invoke `NetworkObject.NetworkShow`
and then immediately invoke `NetworkObject.ChangeOwnership` (or
somewhere within the same callstack for that frame) the target client
will get an error regarding an unnecessary `ChangeOwnershipMessage`.

Because `NetworkObject.NetworkShow` invocations are actually queued and
handled at the end of the netcode frame, the `ChangeOwnershipMessage`
would proceed the `CreateObjectMesage`(_NetworkShow_). Since the
`CreateObjectMesage` is created at the end of the frame for each queued
`NetworkObject.NetworkShow` invocation, when created it would use the
new owner assigned earlier and ownership would be applied upon
showing/spawning the `NetworkObject` on the target client side. Upon the
`NetworkObject` becoming "visible"/spawned, the deferred
`ChangeOwnershipMessage` (because it was processed before the
`NetworkObjct` was created) would be handled on the target client side
and would detect the newly spawned `NetworkObject` was already owned by
the client and log an error message.

### CMB Service Update: (Jira ticket required)

This PR includes a modification to the `ChangeOwnershipMessage`
generated when a change in ownership occurs during a CMB Service hosted
distributed authority session. These modifications should be compatible
with current and previous versions of the CMB Service.

#### The NGO-SDK Change:
- `ChangeOwnershipMessage.ClientIds` is populated with a list of all
clients that should receive the message.
- `ChangeOwnershipMessage.ClientIdCount` provides the number of clients
(size of the array).

#### CMB Service Update: 
- Use the above properties when forwarding the `ChangeOwnershipMessage`
for a full ownership change.

## Changelog

- Fixed: Issue where invoking `NetworkObject.NetworkShow` and
`NetworkObject.ChangeOwnership` consecutively within the same call stack
location could result in an unnecessary change in ownership error
message generated on the target client side.

## Testing and Documentation

- Includes integration test
`NetworkObjectOwnershipTests.NetworkShowAndChangeOwnership`.
- 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.
-->

## Backport
No backport is required.
This issue is specific to NGO v2.x.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -14,6 +14,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where invoking `NetworkObject.NetworkShow` and `NetworkObject.ChangeOwnership` consecutively within the same call stack location could result in an unnecessary change in ownership error message generated on the target client side. (#3468)
 - Fixed issue where `NetworkVariable`s on a `NetworkBehaviour` could fail to synchronize changes if one has `NetworkVariableUpdateTraits` set and is dirty but is not ready to send. (#3466)
 - Fixed inconsistencies in the `OnSceneEvent` callback. (#3458)
 - Fixed issues with the `NetworkBehaviour` and `NetworkVariable` length safety checks. (#3405)

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

@@ -333,7 +333,7 @@ private void HandleOwnershipChange(ref NetworkContext context)
             if (networkObject.OwnerClientId == OwnerClientId)
             {
                 // Log error and then ignore the message
-                UnityEngine.Debug.LogError($"Client-{context.SenderId} ({RequestClientId}) sent unnecessary ownership changed message for {NetworkObjectId}.");
+                NetworkLog.LogError($"[Receiver: Client-{networkManager.LocalClientId}][Sender: Client-{context.SenderId}][RID: {RequestClientId}] Detected unnecessary ownership changed message for {networkObject.name} (NID:{NetworkObjectId}).");
                 return;
             }
 

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

@@ -565,6 +565,7 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool
             }
 
             var size = 0;
+
             if (NetworkManager.DistributedAuthorityMode)
             {
                 var message = new ChangeOwnershipMessage
@@ -578,24 +579,31 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool
                     OwnershipFlags = (ushort)networkObject.Ownership,
                 };
                 // If we are connected to the CMB service or not the DAHost (i.e. pure DA-Clients only)
+
                 if (NetworkManager.CMBServiceConnection || !NetworkManager.DAHost)
                 {
                     // Always update the network properties in distributed authority mode for the client gaining ownership
                     for (int i = 0; i < networkObject.ChildNetworkBehaviours.Count; i++)
                     {
                         networkObject.ChildNetworkBehaviours[i].UpdateNetworkProperties();
                     }
+
+                    // Populate valid target client identifiers that should receive this change in ownership message.
+                    message.ClientIds = NetworkManager.ConnectedClientsIds.Where((c) => !IsObjectVisibilityPending(c, ref networkObject) && networkObject.IsNetworkVisibleTo(c)).ToArray();
+                    message.ClientIdCount = message.ClientIds.Length;
+
                     size = NetworkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, NetworkManager.ServerClientId);
                     NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(NetworkManager.LocalClientId, networkObject, size);
                 }
                 else // We are the DAHost so broadcast the ownership change
                 {
                     foreach (var client in NetworkManager.ConnectedClients)
                     {
-                        if (client.Value.ClientId == NetworkManager.ServerClientId)
+                        if (client.Value.ClientId == NetworkManager.ServerClientId || IsObjectVisibilityPending(client.Key, ref networkObject))
                         {
                             continue;
                         }
+
                         if (networkObject.IsNetworkVisibleTo(client.Value.ClientId))
                         {
                             size = NetworkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, client.Value.ClientId);
@@ -613,8 +621,17 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool
                 };
                 foreach (var client in NetworkManager.ConnectedClients)
                 {
+                    if (client.Value.ClientId == NetworkManager.ServerClientId || IsObjectVisibilityPending(client.Key, ref networkObject))
+                    {
+                        continue;
+                    }
                     if (networkObject.IsNetworkVisibleTo(client.Value.ClientId))
                     {
+                        if (client.Key != client.Value.ClientId)
+                        {
+                            NetworkLog.LogError($"[Client-{client.Key}] Client key ({client.Key}) does not match the {nameof(NetworkClient)} client Id {client.Value.ClientId}! Client-{client.Key} will not receive ownership changed message!");
+                            continue;
+                        }
                         size = NetworkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, client.Value.ClientId);
                         NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject, size);
                     }
@@ -639,6 +656,27 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool
             }
         }
 
+        /// <summary>
+        /// Will determine if a client has been granted visibility for a NetworkObject but
+        /// the <see cref="CreateObjectMessage"/> has yet to be generated for it. Under this case,
+        /// the client might not need to be sent a message (i.e. <see cref="ChangeOwnershipMessage")
+        /// </summary>
+        /// <param name="clientId">the client to check</param>
+        /// <param name="networkObject">the <see cref="NetworkObject"/> to check if it is pending show</param>
+        [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
+        internal bool IsObjectVisibilityPending(ulong clientId, ref NetworkObject networkObject)
+        {
+            if (NetworkManager.DistributedAuthorityMode && ClientsToShowObject.ContainsKey(networkObject))
+            {
+                return ClientsToShowObject[networkObject].Contains(clientId);
+            }
+            else if (ObjectsToShowToClient.ContainsKey(clientId))
+            {
+                return ObjectsToShowToClient[clientId].Contains(networkObject);
+            }
+            return false;
+        }
+
         internal bool HasPrefab(NetworkObject.SceneObject sceneObject)
         {
             if (!NetworkManager.NetworkConfig.EnableSceneManagement || !sceneObject.IsSceneObject)

com.unity.netcode.gameobjects/Tests/Runtime/Metrics/OwnershipChangeMetricsTests.cs

@@ -73,13 +73,6 @@ public IEnumerator TrackOwnershipChangeSentMetric()
 
             var ownershipChangeSent = metricValues.First();
             Assert.AreEqual(networkObject.NetworkObjectId, ownershipChangeSent.NetworkId.NetworkId);
-            Assert.AreEqual(Server.LocalClientId, ownershipChangeSent.Connection.Id);
-            Assert.AreEqual(0, ownershipChangeSent.BytesCount);
-
-            // The first metric is to the server(self), so its size is now correctly reported as 0.
-            // Let's check the last one instead, to have a valid value
-            ownershipChangeSent = metricValues.Last();
-            Assert.AreEqual(networkObject.NetworkObjectId, ownershipChangeSent.NetworkId.NetworkId);
             Assert.AreEqual(Client.LocalClientId, ownershipChangeSent.Connection.Id);
 
             var serializedLength = GetWriteSizeForOwnerChange(networkObject, 1);

com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs

@@ -753,5 +753,88 @@ public IEnumerator NetworkShowHideAroundListModify()
                 Compare(ShowHideObject.ObjectsPerClientId[0].MyList, ShowHideObject.ObjectsPerClientId[2].MyList);
             }
         }
+
+        private GameObject m_OwnershipObject;
+        private NetworkObject m_OwnershipNetworkObject;
+        private NetworkManager m_NewOwner;
+        private ulong m_ObjectId;
+
+
+        private bool AllObjectsSpawnedOnClients()
+        {
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(m_OwnershipNetworkObject.NetworkObjectId))
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        private bool ObjectHiddenOnNonAuthorityClients()
+        {
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                if (networkManager.LocalClientId == m_OwnershipNetworkObject.OwnerClientId)
+                {
+                    continue;
+                }
+                if (networkManager.SpawnManager.SpawnedObjects.ContainsKey(m_OwnershipNetworkObject.NetworkObjectId))
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        private bool OwnershipHasChanged()
+        {
+            if (!m_NewOwner.SpawnManager.SpawnedObjects.ContainsKey(m_ObjectId))
+            {
+                return false;
+            }
+            return m_NewOwner.SpawnManager.SpawnedObjects[m_ObjectId].OwnerClientId == m_NewOwner.LocalClientId;
+        }
+
+
+        [UnityTest]
+        public IEnumerator NetworkShowAndChangeOwnership()
+        {
+            var authority = GetAuthorityNetworkManager();
+
+            m_OwnershipObject = SpawnObject(m_PrefabToSpawn, authority);
+            m_OwnershipNetworkObject = m_OwnershipObject.GetComponent<NetworkObject>();
+
+            yield return WaitForConditionOrTimeOut(AllObjectsSpawnedOnClients);
+            AssertOnTimeout("Timed out waiting for all clients to spawn the ownership object!");
+
+            VerboseDebug($"Hiding object {m_OwnershipNetworkObject.NetworkObjectId} on all clients");
+            foreach (var client in m_NetworkManagers)
+            {
+                if (client == authority)
+                {
+                    continue;
+                }
+                m_OwnershipNetworkObject.NetworkHide(client.LocalClientId);
+            }
+
+            yield return WaitForConditionOrTimeOut(ObjectHiddenOnNonAuthorityClients);
+            AssertOnTimeout("Timed out waiting for all clients to hide the ownership object!");
+
+            m_NewOwner = GetNonAuthorityNetworkManager();
+            Assert.AreNotEqual(m_OwnershipNetworkObject.OwnerClientId, m_NewOwner.LocalClientId, $"Client-{m_NewOwner.LocalClientId} should not have ownership of object {m_OwnershipNetworkObject.NetworkObjectId}!");
+            Assert.False(m_NewOwner.SpawnManager.SpawnedObjects.ContainsKey(m_OwnershipNetworkObject.NetworkObjectId), $"Client-{m_NewOwner.LocalClientId} should not have object {m_OwnershipNetworkObject.NetworkObjectId} spawned!");
+
+            // Run NetworkShow and ChangeOwnership directly after one-another
+            VerboseDebug($"Calling {nameof(NetworkObject.NetworkShow)} on object {m_OwnershipNetworkObject.NetworkObjectId} for client {m_NewOwner.LocalClientId}");
+            m_OwnershipNetworkObject.NetworkShow(m_NewOwner.LocalClientId);
+            VerboseDebug($"Calling {nameof(NetworkObject.ChangeOwnership)} on object {m_OwnershipNetworkObject.NetworkObjectId} for client {m_NewOwner.LocalClientId}");
+            m_OwnershipNetworkObject.ChangeOwnership(m_NewOwner.LocalClientId);
+            m_ObjectId = m_OwnershipNetworkObject.NetworkObjectId;
+            yield return WaitForConditionOrTimeOut(OwnershipHasChanged);
+            AssertOnTimeout($"Timed out waiting for clients-{m_NewOwner.LocalClientId} to gain ownership of object {m_OwnershipNetworkObject.NetworkObjectId}!");
+            VerboseDebug($"Client {m_NewOwner.LocalClientId} now owns object {m_OwnershipNetworkObject.NetworkObjectId}!");
+        }
     }
 }

com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformTests.cs

@@ -80,6 +80,8 @@ namespace Unity.Netcode.RuntimeTests
     internal class NetworkTransformTests : NetworkTransformBase
     {
         protected const int k_TickRate = 60;
+
+        protected const int k_DefaultTimeTravelFrames = 1000;
         /// <summary>
         /// Constructor
         /// </summary>
@@ -749,11 +751,11 @@ public void TestAuthoritativeTransformChangeOneAtATime([Values] TransformSpace t
             if (overideState == OverrideState.CommitToTransform)
             {
                 // Wait for the deltas to be pushed
-                success = WaitForConditionOrTimeOutWithTimeTravel(() => m_AuthoritativeTransform.StatePushed, 600);
+                success = WaitForConditionOrTimeOutWithTimeTravel(() => m_AuthoritativeTransform.StatePushed, k_DefaultTimeTravelFrames);
                 Assert.True(success, $"[Position] Timed out waiting for state to be pushed ({m_AuthoritativeTransform.StatePushed})!");
             }
 
-            success = WaitForConditionOrTimeOutWithTimeTravel(() => PositionsMatch(), 600);
+            success = WaitForConditionOrTimeOutWithTimeTravel(() => PositionsMatch(), k_DefaultTimeTravelFrames);
             Assert.True(success, $"Timed out waiting for positions to match {m_AuthoritativeTransform.transform.position} | {m_NonAuthoritativeTransform.transform.position}");
 
             // test rotation
@@ -784,12 +786,12 @@ public void TestAuthoritativeTransformChangeOneAtATime([Values] TransformSpace t
             if (overideState == OverrideState.CommitToTransform)
             {
                 // Wait for the deltas to be pushed
-                success = WaitForConditionOrTimeOutWithTimeTravel(() => m_AuthoritativeTransform.StatePushed, 600);
+                success = WaitForConditionOrTimeOutWithTimeTravel(() => m_AuthoritativeTransform.StatePushed, k_DefaultTimeTravelFrames);
                 Assert.True(success, $"[Rotation] Timed out waiting for state to be pushed ({m_AuthoritativeTransform.StatePushed})!");
             }
 
             // Make sure the values match
-            success = WaitForConditionOrTimeOutWithTimeTravel(() => RotationsMatch(), 600);
+            success = WaitForConditionOrTimeOutWithTimeTravel(() => RotationsMatch(), k_DefaultTimeTravelFrames);
             Assert.True(success, $"Timed out waiting for rotations to match");
 
             m_AuthoritativeTransform.StatePushed = false;
@@ -818,12 +820,12 @@ public void TestAuthoritativeTransformChangeOneAtATime([Values] TransformSpace t
             if (overideState == OverrideState.CommitToTransform)
             {
                 // Wait for the deltas to be pushed
-                success = WaitForConditionOrTimeOutWithTimeTravel(() => m_AuthoritativeTransform.StatePushed, 600);
+                success = WaitForConditionOrTimeOutWithTimeTravel(() => m_AuthoritativeTransform.StatePushed, k_DefaultTimeTravelFrames);
                 Assert.True(success, $"[Rotation] Timed out waiting for state to be pushed ({m_AuthoritativeTransform.StatePushed})!");
             }
 
             // Make sure the scale values match
-            success = WaitForConditionOrTimeOutWithTimeTravel(() => ScaleValuesMatch(), 600);
+            success = WaitForConditionOrTimeOutWithTimeTravel(() => ScaleValuesMatch(), k_DefaultTimeTravelFrames);
             Assert.True(success, $"Timed out waiting for scale values to match");
         }