diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 7247324fe1..2b5a367421 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -22,6 +22,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed an issue where mixed authority nested `NetworkTransform` instances could stop child/nested instances from updating because an authoritative `NetworkTransform` (root or otherwise) would remove the `NetworkObject` from the update group, preventing non-authoritative instances from updating their state on the authority side. (#4169) ### Security diff --git a/com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs b/com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs index 46b7ef24e5..9ac503a926 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs @@ -3715,6 +3715,38 @@ private void ResetInterpolatedStateToCurrentAuthoritativeState() m_ScaleInterpolator.ResetTo(transform.parent, transform.localScale, serverTime); } + /// + /// Determines if this has any instances that are non-authority and are updated during the same update stage. + /// + /// + /// See to better understand how the parameter is used to determine which update stage to check for non-authority instances. + /// + /// true to check the instances updated during the standard update and false to check the instances updated during the fixed update. + /// true if a non-authority NetworkTransform exists on this NetworkObject and false if there are none. + private bool HasNonAuthorityNetworkTransform(bool forUpdate) + { + var networkTransforms = NetworkObject.NetworkTransforms; + for (int i = 0; i < networkTransforms.Count; i++) + { + var networkTransform = networkTransforms[i]; +#if COM_UNITY_MODULES_PHYSICS || COM_UNITY_MODULES_PHYSICS2D + // If the update stages don't match, then skip this instance. + // Reference: + // forUpdate is true for the standard update and false for the fixed update. + // m_UseRigidbodyForMotion is false for the standard update and true for the fixed update. + if (forUpdate == networkTransform.m_UseRigidbodyForMotion) + { + continue; + } +#endif + if (!(networkTransform.IsServerAuthoritative() ? networkTransform.IsServer : networkTransform.IsOwner)) + { + return true; + } + } + return false; + } + /// /// The internal initialization method to allow for internal API adjustments /// @@ -3777,8 +3809,12 @@ private void InternalInitialization(bool isOwnershipChange = false) if (CanCommitToTransform) { - // Make sure authority doesn't get added to updates (no need to do this on the authority side) - m_CachedNetworkManager.NetworkTransformRegistration(NetworkObject, forUpdate, false); + // If there are no non-authority NetworkTransform instances on this NetworkObject using this update, then remove this instance from the NetworkManager's update list. + // Otherwise, we need to keep it registered for updates so the non-authority instances will process their received state updates and apply them to the transform. + if (!HasNonAuthorityNetworkTransform(forUpdate)) + { + m_CachedNetworkManager.NetworkTransformRegistration(NetworkObject, forUpdate, false); + } if (UseHalfFloatPrecision) { m_HalfPositionState = new NetworkDeltaPosition(currentPosition, m_CachedNetworkManager.ServerTime.Tick, math.bool3(SyncPositionX, SyncPositionY, SyncPositionZ)); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedAuthorityTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedAuthorityTests.cs index 760e916bda..c8d9d144bf 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedAuthorityTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedAuthorityTests.cs @@ -1,5 +1,6 @@ using System.Collections; using System.Text; +using NUnit.Framework; using Unity.Netcode.Components; using Unity.Netcode.TestHelpers.Runtime; using UnityEngine; @@ -7,6 +8,8 @@ namespace Unity.Netcode.RuntimeTests { + [TestFixture(HostOrServer.Host)] + [TestFixture(HostOrServer.Server)] internal class NetworkTransformMixedAuthorityTests : IntegrationTestWithApproximation { private const float k_MotionMagnitude = 5.5f; @@ -14,65 +17,79 @@ internal class NetworkTransformMixedAuthorityTests : IntegrationTestWithApproxim protected override int NumberOfClients => 2; - private StringBuilder m_ErrorMsg = new StringBuilder(); - - protected override void OnCreatePlayerPrefab() + /// + /// The root's authority mode for each case. The nested child uses the inverse. + /// + private static readonly NetworkTransform.AuthorityModes[] k_RootAuthorityModes = { - m_PlayerPrefab.AddComponent(); + NetworkTransform.AuthorityModes.Server, + NetworkTransform.AuthorityModes.Owner, + }; + + private GameObject[] m_MixedAuthorityPrefabs; - var childGameObject = new GameObject(); - childGameObject.transform.parent = m_PlayerPrefab.transform; - var childNetworkTransform = childGameObject.AddComponent(); - childNetworkTransform.AuthorityMode = NetworkTransform.AuthorityModes.Owner; - childNetworkTransform.InLocalSpace = true; + private StringBuilder m_ErrorMsg = new StringBuilder(); - base.OnCreatePlayerPrefab(); + public NetworkTransformMixedAuthorityTests(HostOrServer hostOrServer) : base(hostOrServer) + { } - private void MovePlayers() + protected override void OnServerAndClientsCreated() { - foreach (var networkManager in m_NetworkManagers) + m_MixedAuthorityPrefabs = new GameObject[k_RootAuthorityModes.Length]; + for (int i = 0; i < k_RootAuthorityModes.Length; i++) { - var direction = GetRandomVector3(-1.0f, 1.0f); - var playerObject = networkManager.LocalClient.PlayerObject; - var playerObjectId = networkManager.LocalClient.PlayerObject.NetworkObjectId; - // Server authoritative - var serverPlayerClone = m_ServerNetworkManager.SpawnManager.SpawnedObjects[playerObjectId]; - serverPlayerClone.transform.position += direction * k_MotionMagnitude; - // Owner authoritative - var childTransform = networkManager.LocalClient.PlayerObject.transform.GetChild(0); - childTransform.localPosition += direction * k_MotionMagnitude; + var rootAuthorityMode = k_RootAuthorityModes[i]; + var prefab = CreateNetworkObjectPrefab($"MixedAuthority-{rootAuthorityMode}Root"); + prefab.AddComponent().AuthorityMode = rootAuthorityMode; + + var childGameObject = new GameObject(); + childGameObject.transform.parent = prefab.transform; + var childNetworkTransform = childGameObject.AddComponent(); + childNetworkTransform.AuthorityMode = InverseOf(rootAuthorityMode); + childNetworkTransform.InLocalSpace = true; + + m_MixedAuthorityPrefabs[i] = prefab; } + + base.OnServerAndClientsCreated(); + } + + private static NetworkTransform.AuthorityModes InverseOf(NetworkTransform.AuthorityModes authorityMode) + { + return authorityMode == NetworkTransform.AuthorityModes.Server ? NetworkTransform.AuthorityModes.Owner : NetworkTransform.AuthorityModes.Server; + } + + /// + /// Returns the instance with authority over a set to the given authority mode. + /// + private NetworkObject GetAuthorityInstance(NetworkObject instance, NetworkManager owner, NetworkTransform.AuthorityModes authorityMode) + { + return GetManagersInstance(authorityMode == NetworkTransform.AuthorityModes.Server ? m_ServerNetworkManager : owner, instance); } - private bool AllInstancePositionsMatch() + private bool AllInstancePositionsMatch(NetworkObject instance, NetworkManager owner, NetworkTransform.AuthorityModes rootAuthorityMode) { m_ErrorMsg.Clear(); + var authorityRootPosition = GetAuthorityInstance(instance, owner, rootAuthorityMode).transform.position; + var authorityChildPosition = GetAuthorityInstance(instance, owner, InverseOf(rootAuthorityMode)).transform.GetChild(0).localPosition; + + // The authority instances are compared too. An instance with authority over one nested + // NetworkTransform is still non-authority for the other. foreach (var networkManager in m_NetworkManagers) { - var playerObject = networkManager.LocalClient.PlayerObject; - var playerObjectId = networkManager.LocalClient.PlayerObject.NetworkObjectId; - var serverRootPosition = m_ServerNetworkManager.SpawnManager.SpawnedObjects[playerObjectId].transform.position; - var ownerChildPosition = networkManager.LocalClient.PlayerObject.transform.GetChild(0).localPosition; - foreach (var client in m_NetworkManagers) - { - if (client == networkManager) - { - continue; - } - var playerClone = client.SpawnManager.SpawnedObjects[playerObjectId]; - var cloneRootPosition = playerClone.transform.position; - var cloneChildPosition = playerClone.transform.GetChild(0).localPosition; + var clone = GetManagersInstance(networkManager, instance); + var cloneRootPosition = clone.transform.position; + var cloneChildPosition = clone.transform.GetChild(0).localPosition; - if (!Approximately(serverRootPosition, cloneRootPosition)) - { - m_ErrorMsg.AppendLine($"[{playerObject.name}][{playerClone.name}] Root mismatch ({GetVector3Values(serverRootPosition)})({GetVector3Values(cloneRootPosition)})!"); - } + if (!Approximately(authorityRootPosition, cloneRootPosition)) + { + m_ErrorMsg.AppendLine($"[{rootAuthorityMode}Root][{GetDisplayName(networkManager)}] Root mismatch ({GetVector3Values(authorityRootPosition)})({GetVector3Values(cloneRootPosition)})!"); + } - if (!Approximately(ownerChildPosition, cloneChildPosition)) - { - m_ErrorMsg.AppendLine($"[{playerObject.name}][{playerClone.name}] Child mismatch ({GetVector3Values(ownerChildPosition)})({GetVector3Values(cloneChildPosition)})!"); - } + if (!Approximately(authorityChildPosition, cloneChildPosition)) + { + m_ErrorMsg.AppendLine($"[{rootAuthorityMode}Root][{GetDisplayName(networkManager)}] Child mismatch ({GetVector3Values(authorityChildPosition)})({GetVector3Values(cloneChildPosition)})!"); } } return m_ErrorMsg.Length == 0; @@ -80,18 +97,45 @@ private bool AllInstancePositionsMatch() /// /// Client-Server Only - /// Validates that mixed authority is working properly - /// Root -- Server Authoritative - /// |--Child -- Owner Authoritative + /// Validates that mixed authority is working properly for both arrangements: + /// Root -- Server or Owner authoritative + /// |--Child -- The inverse of the root's authority mode /// [UnityTest] public IEnumerator MixedAuthorityTest() { - for (int i = 0; i < k_Iterations; i++) + // A client owns the instance so the owner authoritative half is never also the server. + var owner = m_ClientNetworkManagers[0]; + for (int i = 0; i < k_RootAuthorityModes.Length; i++) { - MovePlayers(); - yield return WaitForConditionOrTimeOut(AllInstancePositionsMatch); - AssertOnTimeout($"Transforms failed to synchronize!"); + var rootAuthorityMode = k_RootAuthorityModes[i]; + var instance = SpawnObject(m_MixedAuthorityPrefabs[i], owner).GetComponent(); + yield return WaitForSpawnedOnAllOrTimeOut(instance); + AssertOnTimeout($"[{rootAuthorityMode}Root] Failed to spawn {instance.name} on all clients!"); + + // An instance stays registered for updates while any of its nested NetworkTransform components is non-authority. + foreach (var networkManager in m_NetworkManagers) + { + var clone = GetManagersInstance(networkManager, instance); + var hasNonAuthority = false; + foreach (var networkTransform in clone.NetworkTransforms) + { + hasNonAuthority |= !networkTransform.CanCommitToTransform; + } + Assert.AreEqual(hasNonAuthority, networkManager.NetworkTransformUpdate.ContainsKey(instance.NetworkObjectId), $"[{rootAuthorityMode}Root][{GetDisplayName(networkManager)}] Unexpected update registration!"); + } + + for (int iteration = 0; iteration < k_Iterations; iteration++) + { + var direction = GetRandomVector3(-1.0f, 1.0f); + GetAuthorityInstance(instance, owner, rootAuthorityMode).transform.position += direction * k_MotionMagnitude; + GetAuthorityInstance(instance, owner, InverseOf(rootAuthorityMode)).transform.GetChild(0).localPosition += direction * k_MotionMagnitude; + + yield return WaitForConditionOrTimeOut(() => AllInstancePositionsMatch(instance, owner, rootAuthorityMode)); + AssertOnTimeout($"[{rootAuthorityMode}Root] Transforms failed to synchronize!\n{m_ErrorMsg}"); + } + + instance.Despawn(); } } } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedMotionModelTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedMotionModelTests.cs new file mode 100644 index 0000000000..25ec9894eb --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedMotionModelTests.cs @@ -0,0 +1,67 @@ +#if COM_UNITY_MODULES_PHYSICS +using System.Collections; +using NUnit.Framework; +using Unity.Netcode.Components; +using Unity.Netcode.TestHelpers.Runtime; +using UnityEngine; +using UnityEngine.TestTools; + +namespace Unity.Netcode.RuntimeTests +{ + internal class NetworkTransformMixedMotionModelTests : NetcodeIntegrationTest + { + protected override int NumberOfClients => 2; + + private GameObject m_MixedMotionModelPrefab; + + protected override void OnServerAndClientsCreated() + { + m_MixedMotionModelPrefab = CreateNetworkObjectPrefab("MixedMotionModel"); + + // The root is owner authoritative and driven by the rigidbody, which places it in the fixed update registration + var rootNetworkTransform = m_MixedMotionModelPrefab.AddComponent(); + rootNetworkTransform.AuthorityMode = NetworkTransform.AuthorityModes.Owner; + var rigidbody = m_MixedMotionModelPrefab.AddComponent(); + rigidbody.useGravity = false; + rigidbody.detectCollisions = false; + m_MixedMotionModelPrefab.AddComponent().UseRigidBodyForMotion = true; + + // The nested child is server authoritative and driven by the transform, which places it in the update registration + var childGameObject = new GameObject(); + childGameObject.transform.parent = m_MixedMotionModelPrefab.transform; + var childNetworkTransform = childGameObject.AddComponent(); + childNetworkTransform.AuthorityMode = NetworkTransform.AuthorityModes.Server; + childNetworkTransform.InLocalSpace = true; + + base.OnServerAndClientsCreated(); + } + + /// + /// A NetworkObject that mixes both the authority motion model and the rigidbody motion model has each nested + /// NetworkTransform registered under a different update. Gaining authority over the instance in one update + /// should not leave it registered for the other. + /// + [UnityTest] + public IEnumerator UpdateRegistrationFollowsMotionModel() + { + var instance = SpawnObject(m_MixedMotionModelPrefab, m_ServerNetworkManager).GetComponent(); + yield return WaitForSpawnedOnAllOrTimeOut(instance); + AssertOnTimeout($"Failed to spawn {instance.name} on all clients!"); + + var newOwner = m_ClientNetworkManagers[0]; + + // Establish the baseline before ownership is transferred, otherwise the check below would still pass if this instance was never registered for the fixed update to begin with. + Assert.True(newOwner.NetworkTransformFixedUpdate.ContainsKey(instance.NetworkObjectId), $"Client-{newOwner.LocalClientId} should initially be registered for the fixed update!"); + + instance.ChangeOwnership(newOwner.LocalClientId); + yield return WaitForConditionOrTimeOut(() => newOwner.SpawnManager.SpawnedObjects[instance.NetworkObjectId].OwnerClientId == newOwner.LocalClientId); + AssertOnTimeout($"Client-{newOwner.LocalClientId} never gained ownership of {instance.name}!"); + + // The new owner is the authority for the rigidbody driven root, so nothing on this instance needs the fixed + // update any longer. The server authoritative child still needs the standard update. + Assert.False(newOwner.NetworkTransformFixedUpdate.ContainsKey(instance.NetworkObjectId), $"Client-{newOwner.LocalClientId} is still registered for the fixed update!"); + Assert.True(newOwner.NetworkTransformUpdate.ContainsKey(instance.NetworkObjectId), $"Client-{newOwner.LocalClientId} is not registered for the update!"); + } + } +} +#endif diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedMotionModelTests.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedMotionModelTests.cs.meta new file mode 100644 index 0000000000..8b1683109d --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformMixedMotionModelTests.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: b12b5cdba06a3814fbe6cf7c4df188f7 \ No newline at end of file diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs index 5b2f5a986b..fe1185462b 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs @@ -322,6 +322,46 @@ protected NetworkManager GetNonAuthorityNetworkManager(int nonAuthorityIndex) return null; } + /// + /// Gets the name of the given to use in assertion messages: + /// - Dedicated server: "Server" + /// - Host: "Host" + /// - Client: "Client-" followed by its + /// + /// The to get the name of + /// The name of the + protected string GetDisplayName(NetworkManager networkManager) + { + if (networkManager.IsServer) + { + return networkManager.IsHost ? "Host" : "Server"; + } + return $"Client-{networkManager.LocalClientId}"; + } + + /// + /// Gets the relative instance of the with the given identifier. + /// + /// The to get the relative instance from + /// The identifier of the wanted + /// The relative instance + protected NetworkObject GetManagersInstance(NetworkManager networkManager, ulong networkObjectId) + { + Assert.True(networkManager.SpawnManager.SpawnedObjects.ContainsKey(networkObjectId), $"{GetDisplayName(networkManager)} has no spawned {nameof(NetworkObject)} with an identifier of {networkObjectId}!"); + return networkManager.SpawnManager.SpawnedObjects[networkObjectId]; + } + + /// + /// Gets the relative instance of the given . + /// + /// The to get the relative instance from + /// Any instance of the wanted + /// The relative instance + protected NetworkObject GetManagersInstance(NetworkManager networkManager, NetworkObject networkObject) + { + return GetManagersInstance(networkManager, networkObject.NetworkObjectId); + } + /// /// Contains each client relative set of player NetworkObject instances /// [Client Relative set of player instances][The player instance ClientId][The player instance's NetworkObject]