fix: NetworkShow client synchronization duplicate players and NetworkShow with ChangeOwnership fixes [Backport]

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -15,6 +15,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where the initial client synchronization pre-serialization process was not excluding spawned `NetworkObjects` that already had pending visibility for the client being synchronized. (#3493)
+- 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. (#3493)
 - 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. (#3465)
 - Fixed issue where when a client changes ownership via RPC the `NetworkBehaviour.OnOwnershipChanged` can result in identical previous and current owner identifiers. (#3434)
 

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

@@ -35,6 +35,15 @@ public void Handle(ref NetworkContext context)
         {
             var networkManager = (NetworkManager)context.SystemOwner;
             var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId];
+
+            // Sanity check that we are not sending duplicated or unnecessary change ownership messages
+            if (networkObject.OwnerClientId == OwnerClientId)
+            {
+                // Log error and then ignore the message
+                NetworkLog.LogError($"[Receiver: Client-{networkManager.LocalClientId}][Sender: Client-{context.SenderId}] Detected unnecessary ownership changed message for {networkObject.name} (NID:{NetworkObjectId}).");
+                return;
+            }
+
             var originalOwner = networkObject.OwnerClientId;
 
             networkObject.OwnerClientId = OwnerClientId;

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

@@ -317,6 +317,12 @@ internal void AddSpawnedNetworkObjects()
             m_NetworkObjectsSync.Clear();
             foreach (var sobj in m_NetworkManager.SpawnManager.SpawnedObjectsList)
             {
+                var spawnedObject = sobj;
+                // Don't synchronize objects that have pending visibility as that will be sent as a CreateObjectMessage towards the end of the current frame
+                if (TargetClientId != NetworkManager.ServerClientId && m_NetworkManager.SpawnManager.IsObjectVisibilityPending(TargetClientId, ref spawnedObject))
+                {
+                    continue;
+                }
                 if (sobj.Observers.Contains(TargetClientId))
                 {
                     m_NetworkObjectsSync.Add(sobj);

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

@@ -321,6 +321,10 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
 
             foreach (var client in NetworkManager.ConnectedClients)
             {
+                if (IsObjectVisibilityPending(client.Key, ref networkObject))
+                {
+                    continue;
+                }
                 if (networkObject.IsNetworkVisibleTo(client.Value.ClientId))
                 {
                     var size = NetworkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, client.Value.ClientId);
@@ -343,6 +347,23 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
             m_LastChangeInOwnership[networkObject.NetworkObjectId] = Time.realtimeSinceStartup + (tickFrequency * k_MaximumTickOwnershipChangeMultiplier);
         }
 
+        /// <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 (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/NetworkObject/PlayerSpawnObjectVisibilityTests.cs

@@ -0,0 +1,91 @@
+using System.Collections;
+using System.Text.RegularExpressions;
+using NUnit.Framework;
+using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine;
+using UnityEngine.TestTools;
+
+
+namespace Unity.Netcode.RuntimeTests
+{
+    [TestFixture(HostOrServer.Server)]
+    [TestFixture(HostOrServer.Host)]
+    internal class PlayerSpawnObjectVisibilityTests : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 0;
+
+        public enum PlayerSpawnStages
+        {
+            OnNetworkSpawn,
+            OnNetworkPostSpawn,
+        }
+
+        public PlayerSpawnObjectVisibilityTests(HostOrServer hostOrServer) : base(hostOrServer) { }
+
+        public class PlayerVisibilityTestComponent : NetworkBehaviour
+        {
+            public PlayerSpawnStages Stage;
+
+            private void Awake()
+            {
+                var networkObject = GetComponent<NetworkObject>();
+                // Assure the player prefab will not spawn with observers.
+                // This assures that when the server/host spawns the connecting client's
+                // player prefab, the spawn object will initially not be spawnd on the client side.
+                networkObject.SpawnWithObservers = false;
+            }
+
+            public override void OnNetworkSpawn()
+            {
+                ShowToClient(PlayerSpawnStages.OnNetworkSpawn);
+                base.OnNetworkSpawn();
+            }
+
+            protected override void OnNetworkPostSpawn()
+            {
+                ShowToClient(PlayerSpawnStages.OnNetworkPostSpawn);
+                base.OnNetworkPostSpawn();
+            }
+
+            private void ShowToClient(PlayerSpawnStages currentStage)
+            {
+                if (!IsServer || Stage != currentStage)
+                {
+                    return;
+                }
+                NetworkObject.NetworkShow(OwnerClientId);
+            }
+        }
+
+        protected override void OnCreatePlayerPrefab()
+        {
+            m_PlayerPrefab.AddComponent<PlayerVisibilityTestComponent>();
+            base.OnCreatePlayerPrefab();
+        }
+
+        /// <summary>
+        /// Tests the scenario where under a client-server network topology if a player prefab
+        /// is spawned by the server with no observers but the player prefab itself has server
+        /// side script that will network show the spawned object to the owning client.
+        ///
+        /// Because NetworkShow will defer the CreateObjectMessage until the late update, the
+        /// server/host needs to filter out including anything within the synchronization
+        /// message that already has pending visibility.
+        /// </summary>
+        /// <param name="spawnStage">Spawn stages to test</param>
+        /// <returns>IEnumerator</returns>
+        [UnityTest]
+        public IEnumerator NetworkShowOnSpawnTest([Values] PlayerSpawnStages spawnStage)
+        {
+            m_PlayerPrefab.GetComponent<PlayerVisibilityTestComponent>().Stage = spawnStage;
+
+            yield return CreateAndStartNewClient();
+
+            yield return new WaitForSeconds(0.25f);
+
+            NetcodeLogAssert.LogWasNotReceived(LogType.Warning, new Regex("but it is already in the spawned list!"));
+            var client = m_ClientNetworkManagers[0];
+            Assert.True(client.LocalClient.PlayerObject != null, $"Client-{client.LocalClientId} does not have a player object!");
+        }
+    }
+}

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

@@ -601,5 +601,89 @@ 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_ClientNetworkManagers)
+            {
+                if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(m_OwnershipNetworkObject.NetworkObjectId))
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        private bool ObjectHiddenOnNonAuthorityClients()
+        {
+            foreach (var networkManager in m_ClientNetworkManagers)
+            {
+                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;
+        }
+
+        /// <summary>
+        /// Validates when invoking NetworkObject.NetworkShow and NetworkObject.ChangeOwnership
+        /// back-to-back it will not attempt to send a change ownership message since the visibility
+        /// message (CreateObjectMessage) is deferred until the end of the frame.
+        /// </summary>
+        /// <returns>IEnumerator</returns>
+        [UnityTest]
+        public IEnumerator NetworkShowAndChangeOwnership()
+        {
+            var authority = m_ServerNetworkManager;
+
+            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_ClientNetworkManagers)
+            {
+                m_OwnershipNetworkObject.NetworkHide(client.LocalClientId);
+            }
+
+            yield return WaitForConditionOrTimeOut(ObjectHiddenOnNonAuthorityClients);
+            AssertOnTimeout("Timed out waiting for all clients to hide the ownership object!");
+
+            m_NewOwner = m_ClientNetworkManagers[0];
+            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}!");
+        }
     }
 }