fix: mixed authority nested NetworkTransform stops updating non-authority nested NetworkTransforms (#4169)

* fix

Fixing issue where a `NetworkObject` with nested `NetworkTransform` components using different `NetworkTransform.AuthorityMode` settings stop updating non-authority instances, because the first authority instance removes the entire `NetworkObject` from the update registration.

* test

The tests that validate this fix.

* update

Adding changelog entry

* style

Updating comments for clarity

* fix

Addressing the u-pr bot's findings.

* test

Adding a test to validate the, now fixed, gap u-pr bot found.

* update

Applying u-pr's suggestion to this test.

* test: collapse the mixed authority fixtures and add GetManagersInstance

The root authority mode moved from a TestFixture argument into an array
walked inside the test, which takes the fixture count from 4 to 2 and the
case count from 8 to 2. Only HostOrServer still needs a session per value.

That required moving the nested NetworkTransform components off the player
prefab and onto two prefabs spawned and despawned per case, so the owner is
now an explicit client rather than every player in turn.

NetcodeIntegrationTest gains GetManagersInstance for resolving a NetworkObject
relative to a NetworkManager, which asserts rather than throwing
KeyNotFoundException when the instance is missing.

* Update com.unity.netcode.gameobjects/CHANGELOG.md

Co-authored-by: u-pr[bot] <205906871+u-pr[bot]@users.noreply.github.com>

* update

---------

Co-authored-by: u-pr[bot] <205906871+u-pr[bot]@users.noreply.github.com>

Author: NoelStephensUnity

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
 

com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs

@@ -3715,6 +3715,38 @@ private void ResetInterpolatedStateToCurrentAuthoritativeState()
             m_ScaleInterpolator.ResetTo(transform.parent, transform.localScale, serverTime);
         }
 
+        /// <summary>
+        /// Determines if this <see cref="NetworkObject"/> has any <see cref="NetworkTransform"/> instances that are non-authority and are updated during the same update stage.
+        /// </summary>
+        /// <remarks>
+        /// See <see cref="InternalInitialization"/> to better understand how the <paramref name="forUpdate"/> parameter is used to determine which update stage to check for non-authority <see cref="NetworkTransform"/> instances.
+        /// </remarks>
+        /// <param name="forUpdate">true to check the instances updated during the standard update and false to check the instances updated during the fixed update.</param>
+        /// <returns>true if a non-authority NetworkTransform exists on this NetworkObject and false if there are none.</returns>
+        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;
+        }
+
         /// <summary>
         /// The internal initialization method to allow for internal API adjustments
         /// </summary>
@@ -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));

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

@@ -1,97 +1,141 @@
 using System.Collections;
 using System.Text;
+using NUnit.Framework;
 using Unity.Netcode.Components;
 using Unity.Netcode.TestHelpers.Runtime;
 using UnityEngine;
 using UnityEngine.TestTools;
 
 namespace Unity.Netcode.RuntimeTests
 {
+    [TestFixture(HostOrServer.Host)]
+    [TestFixture(HostOrServer.Server)]
     internal class NetworkTransformMixedAuthorityTests : IntegrationTestWithApproximation
     {
         private const float k_MotionMagnitude = 5.5f;
         private const int k_Iterations = 4;
 
         protected override int NumberOfClients => 2;
 
-        private StringBuilder m_ErrorMsg = new StringBuilder();
-
-        protected override void OnCreatePlayerPrefab()
+        /// <summary>
+        /// The root's authority mode for each case. The nested child uses the inverse.
+        /// </summary>
+        private static readonly NetworkTransform.AuthorityModes[] k_RootAuthorityModes =
         {
-            m_PlayerPrefab.AddComponent<NetworkTransform>();
+            NetworkTransform.AuthorityModes.Server,
+            NetworkTransform.AuthorityModes.Owner,
+        };
+
+        private GameObject[] m_MixedAuthorityPrefabs;
 
-            var childGameObject = new GameObject();
-            childGameObject.transform.parent = m_PlayerPrefab.transform;
-            var childNetworkTransform = childGameObject.AddComponent<NetworkTransform>();
-            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<NetworkTransform>().AuthorityMode = rootAuthorityMode;
+
+                var childGameObject = new GameObject();
+                childGameObject.transform.parent = prefab.transform;
+                var childNetworkTransform = childGameObject.AddComponent<NetworkTransform>();
+                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;
+        }
+
+        /// <summary>
+        /// Returns the instance with authority over a <see cref="NetworkTransform"/> set to the given authority mode.
+        /// </summary>
+        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;
         }
 
         /// <summary>
         /// 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
         /// </summary>
         [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<NetworkObject>();
+                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();
             }
         }
     }

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<NetworkTransform>();
+            rootNetworkTransform.AuthorityMode = NetworkTransform.AuthorityModes.Owner;
+            var rigidbody = m_MixedMotionModelPrefab.AddComponent<Rigidbody>();
+            rigidbody.useGravity = false;
+            rigidbody.detectCollisions = false;
+            m_MixedMotionModelPrefab.AddComponent<NetworkRigidbody>().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<NetworkTransform>();
+            childNetworkTransform.AuthorityMode = NetworkTransform.AuthorityModes.Server;
+            childNetworkTransform.InLocalSpace = true;
+
+            base.OnServerAndClientsCreated();
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator UpdateRegistrationFollowsMotionModel()
+        {
+            var instance = SpawnObject(m_MixedMotionModelPrefab, m_ServerNetworkManager).GetComponent<NetworkObject>();
+            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