diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 01a351dce1..79d8e69c83 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/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) diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs index 9b980d2430..32f162c50f 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs +++ b/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; } diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index ba9db8329d..3b5bb2e6e0 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/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,6 +579,7 @@ 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 @@ -585,6 +587,11 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool { 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); } @@ -592,10 +599,11 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId, bool { 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 } } + /// + /// 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 (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) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Metrics/OwnershipChangeMetricsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Metrics/OwnershipChangeMetricsTests.cs index cc08dcd5b7..3f04820155 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/Metrics/OwnershipChangeMetricsTests.cs +++ b/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); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs index bd8d0d3fbe..9c2e0cabaa 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs +++ b/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(); + + 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}!"); + } } } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformTests.cs index 02b02e8fd3..43e03491a2 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformTests.cs +++ b/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; /// /// Constructor /// @@ -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"); }