fix: [Up-port] mixed authority nested NetworkTransform stops updating non-authority instances

Up-port of #4169.

The NetworkManager update registration is per-NetworkObject while the
authority motion model is per-NetworkTransform, so the first authority
instance to initialize removed the entire NetworkObject from the update
group and its non-authority siblings stopped being updated.

Gate the removal on whether any NetworkTransform on the NetworkObject is
still non-authority for that same update.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -18,6 +18,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Issue with mixed authority nested `NetworkTransform` instances can stop child/nested `NetworkTransform` instances from updating due to a parent (root or otherwise) `NetworkTransform` that is the authority instance will remove the `NetworkObject` completely from the non-authority update group causing non-authority instances to never update their state (interpolating or not) on the authority side. (#4169)
+
 ### Security
 
 ### Obsolete

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

@@ -3736,6 +3736,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>
@@ -3807,8 +3839,12 @@ internal virtual 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,77 +1,93 @@
 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, NetworkTransform.AuthorityModes.Server)]
+    [TestFixture(HostOrServer.Host, NetworkTransform.AuthorityModes.Owner)]
+    [TestFixture(HostOrServer.Server, NetworkTransform.AuthorityModes.Server)]
+    [TestFixture(HostOrServer.Server, NetworkTransform.AuthorityModes.Owner)]
     internal class NetworkTransformMixedAuthorityTests : IntegrationTestWithApproximation
     {
         private const float k_MotionMagnitude = 5.5f;
         private const int k_Iterations = 4;
 
         protected override int NumberOfClients => 2;
 
+        private readonly NetworkTransform.AuthorityModes m_RootAuthorityMode;
+        private readonly NetworkTransform.AuthorityModes m_ChildAuthorityMode;
+
         private StringBuilder m_ErrorMsg = new StringBuilder();
 
+        public NetworkTransformMixedAuthorityTests(HostOrServer hostOrServer, NetworkTransform.AuthorityModes rootAuthorityMode) : base(hostOrServer)
+        {
+            m_RootAuthorityMode = rootAuthorityMode;
+            m_ChildAuthorityMode = rootAuthorityMode == NetworkTransform.AuthorityModes.Server ? NetworkTransform.AuthorityModes.Owner : NetworkTransform.AuthorityModes.Server;
+        }
+
         protected override void OnCreatePlayerPrefab()
         {
-            m_PlayerPrefab.AddComponent<NetworkTransform>();
+            m_PlayerPrefab.AddComponent<NetworkTransform>().AuthorityMode = m_RootAuthorityMode;
 
             var childGameObject = new GameObject();
             childGameObject.transform.parent = m_PlayerPrefab.transform;
             var childNetworkTransform = childGameObject.AddComponent<NetworkTransform>();
-            childNetworkTransform.AuthorityMode = NetworkTransform.AuthorityModes.Owner;
+            childNetworkTransform.AuthorityMode = m_ChildAuthorityMode;
             childNetworkTransform.InLocalSpace = true;
 
             base.OnCreatePlayerPrefab();
         }
 
+        /// <summary>
+        /// Returns the instance of <paramref name="player"/>'s player object that has authority over a
+        /// <see cref="NetworkTransform"/> using the <paramref name="authorityMode"/> authority mode.
+        /// </summary>
+        private NetworkObject GetAuthorityInstance(NetworkManager player, NetworkTransform.AuthorityModes authorityMode)
+        {
+            var authority = authorityMode == NetworkTransform.AuthorityModes.Server ? m_ServerNetworkManager : player;
+            return authority.SpawnManager.SpawnedObjects[player.LocalClient.PlayerObject.NetworkObjectId];
+        }
+
         private void MovePlayers()
         {
-            foreach (var networkManager in m_NetworkManagers)
+            foreach (var networkManager in m_ClientNetworkManagers)
             {
                 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;
+                GetAuthorityInstance(networkManager, m_RootAuthorityMode).transform.position += direction * k_MotionMagnitude;
+                GetAuthorityInstance(networkManager, m_ChildAuthorityMode).transform.GetChild(0).localPosition += direction * k_MotionMagnitude;
             }
         }
 
         private bool AllInstancePositionsMatch()
         {
             m_ErrorMsg.Clear();
-            foreach (var networkManager in m_NetworkManagers)
+            foreach (var networkManager in m_ClientNetworkManagers)
             {
-                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;
+                var authorityRootPosition = GetAuthorityInstance(networkManager, m_RootAuthorityMode).transform.position;
+                var authorityChildPosition = GetAuthorityInstance(networkManager, m_ChildAuthorityMode).transform.GetChild(0).localPosition;
+
+                // The authority instances are compared too, as an instance with authority over one nested
+                // NetworkTransform is still non-authority for the other.
                 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;
 
-                    if (!Approximately(serverRootPosition, cloneRootPosition))
+                    if (!Approximately(authorityRootPosition, cloneRootPosition))
                     {
-                        m_ErrorMsg.AppendLine($"[{playerObject.name}][{playerClone.name}] Root mismatch ({GetVector3Values(serverRootPosition)})({GetVector3Values(cloneRootPosition)})!");
+                        m_ErrorMsg.AppendLine($"[Client-{client.LocalClientId}][{playerClone.name}] Root mismatch ({GetVector3Values(authorityRootPosition)})({GetVector3Values(cloneRootPosition)})!");
                     }
 
-                    if (!Approximately(ownerChildPosition, cloneChildPosition))
+                    if (!Approximately(authorityChildPosition, cloneChildPosition))
                     {
-                        m_ErrorMsg.AppendLine($"[{playerObject.name}][{playerClone.name}] Child mismatch ({GetVector3Values(ownerChildPosition)})({GetVector3Values(cloneChildPosition)})!");
+                        m_ErrorMsg.AppendLine($"[Client-{client.LocalClientId}][{playerClone.name}] Child mismatch ({GetVector3Values(authorityChildPosition)})({GetVector3Values(cloneChildPosition)})!");
                     }
                 }
             }
@@ -81,8 +97,8 @@ private bool AllInstancePositionsMatch()
         /// <summary>
         /// Client-Server Only
         /// Validates that mixed authority is working properly
-        /// Root -- Server Authoritative
-        /// |--Child -- Owner Authoritative
+        /// Root -- Server or Owner authoritative
+        /// |--Child -- The inverse of the root's authority mode
         /// </summary>
         [UnityTest]
         public IEnumerator MixedAuthorityTest()
@@ -91,7 +107,31 @@ public IEnumerator MixedAuthorityTest()
             {
                 MovePlayers();
                 yield return WaitForConditionOrTimeOut(AllInstancePositionsMatch);
-                AssertOnTimeout($"Transforms failed to synchronize!");
+                AssertOnTimeout($"Transforms failed to synchronize!\n{m_ErrorMsg}");
+            }
+        }
+
+        /// <summary>
+        /// The update registration is per-NetworkObject while the authority motion model is per-NetworkTransform,
+        /// so an instance stays registered for as long as any one of its nested NetworkTransform components is
+        /// non-authority.
+        /// </summary>
+        [Test]
+        public void MixedAuthorityUpdateRegistration()
+        {
+            foreach (var networkManager in m_ClientNetworkManagers)
+            {
+                var playerObjectId = networkManager.LocalClient.PlayerObject.NetworkObjectId;
+                foreach (var client in m_NetworkManagers)
+                {
+                    var playerClone = client.SpawnManager.SpawnedObjects[playerObjectId];
+                    var hasNonAuthority = false;
+                    foreach (var networkTransform in playerClone.NetworkTransforms)
+                    {
+                        hasNonAuthority |= !networkTransform.CanCommitToTransform;
+                    }
+                    Assert.AreEqual(hasNonAuthority, client.NetworkTransformUpdate.ContainsKey(playerObjectId), $"[Client-{client.LocalClientId}][{playerClone.name}] Unexpected update registration!");
+                }
             }
         }
     }

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

@@ -0,0 +1,63 @@
+#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];
+            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