diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 3d8cc3cc01..7ad0374887 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/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) diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs index e4a5b35a52..eb59afc618 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs +++ b/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; diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventData.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventData.cs index b1f2864a52..e3d33b7a0a 100644 --- a/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventData.cs +++ b/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); diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index e6dd8879bd..e503346722 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/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); } + /// + /// Will determine if a client has been granted visibility for a NetworkObject but + /// the has yet to be generated for it. Under this case, + /// the client might not need to be sent a message (i.e. + /// the client to check + /// the to check if it is pending show + [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) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/PlayerSpawnObjectVisibilityTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/PlayerSpawnObjectVisibilityTests.cs new file mode 100644 index 0000000000..4b1f603965 --- /dev/null +++ b/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(); + // 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(); + base.OnCreatePlayerPrefab(); + } + + /// + /// 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. + /// + /// Spawn stages to test + /// IEnumerator + [UnityTest] + public IEnumerator NetworkShowOnSpawnTest([Values] PlayerSpawnStages spawnStage) + { + m_PlayerPrefab.GetComponent().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!"); + } + } +} diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/PlayerSpawnObjectVisibilityTests.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/PlayerSpawnObjectVisibilityTests.cs.meta new file mode 100644 index 0000000000..9cbbd88668 --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/PlayerSpawnObjectVisibilityTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 635d9e057e7179446906eccfd7fc9a90 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs index a68a0412dd..80fd18b2db 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs +++ b/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; + } + + /// + /// 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. + /// + /// IEnumerator + [UnityTest] + public IEnumerator NetworkShowAndChangeOwnership() + { + var authority = m_ServerNetworkManager; + + m_OwnershipObject = SpawnObject(m_PrefabToSpawn, authority); + m_OwnershipNetworkObject = m_OwnershipObject.GetComponent(); + + 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}!"); + } } }