fix: [Backport] rejected parent change restores the original pose

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -22,6 +22,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Issue where a rejected parent change, for example one made by a client without parenting authority, restored the `NetworkObject`'s parent but left it at the moved position when the change was made with `worldPositionStays` disabled. The original position, rotation and scale are now restored. (#4203)
 - Issue where moving a `NetworkObject` into another scene made the clients that did not observe it log "Trying to synchronize NetworkObjectId but it was not spawned". The scene migration is now only sent to the clients that observe the `NetworkObject`. (#4185)
 - Issue where a `NetworkObject` that was moved into another scene while hidden from a client spawned in that client's active scene when it was shown with `NetworkShow`, instead of the scene it is in on the server. (#4185)
 - 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)

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -2396,6 +2396,49 @@ internal void InvokeBehaviourOnNetworkObjectParentChanged(NetworkObject parentNe
         private Transform m_CachedParent; // What is our last set parent Transform reference?
         private bool m_CachedWorldPositionStays = true; // Used to preserve the world position stays parameter passed in TrySetParent
 
+        // The local transform's state before a parent change. These values are used if the parent change is reverted.
+        private Vector3 m_LocalPositionBeforeParentChange;
+        private Quaternion m_LocalRotationBeforeParentChange;
+        private Vector3 m_LocalScaleBeforeParentChange;
+        private bool m_RevertingParentChange;
+
+        private void OnBeforeTransformParentChanged()
+        {
+            // Skips the parent change RevertParentChange makes, so the values it restores are not overwritten.
+            if (m_RevertingParentChange)
+            {
+                return;
+            }
+            var currentTransform = transform;
+            m_LocalPositionBeforeParentChange = currentTransform.localPosition;
+            m_LocalRotationBeforeParentChange = currentTransform.localRotation;
+            m_LocalScaleBeforeParentChange = currentTransform.localScale;
+        }
+
+        /// <summary>
+        /// Restores the cached parent and the local transform prior to the parent change.
+        /// </summary>
+        /// <remarks>
+        /// Setting <see cref="Transform.parent"/> keeps the world position. That only restores the original pose when the change also kept the world position.
+        /// Restoring the local transform under the cached parent works for either worldPositionStays value.
+        /// </remarks>
+        private void RevertParentChange()
+        {
+            m_RevertingParentChange = true;
+            try
+            {
+                var currentTransform = transform;
+                currentTransform.SetParent(m_CachedParent, false);
+                currentTransform.localPosition = m_LocalPositionBeforeParentChange;
+                currentTransform.localRotation = m_LocalRotationBeforeParentChange;
+                currentTransform.localScale = m_LocalScaleBeforeParentChange;
+            }
+            finally
+            {
+                m_RevertingParentChange = false;
+            }
+        }
+
         /// <summary>
         /// With distributed authority, we need to have a way to determine if the parenting action is authorized.
         /// This is set when handling an incoming ParentSyncMessage and when running as a DAHost and a client has disconnected.
@@ -2553,7 +2596,7 @@ internal bool InternalTrySetParent(NetworkObject parent, bool worldPositionStays
         private void OnTransformParentChanged()
         {
             var networkManager = NetworkManager;
-            if (!AutoObjectParentSync || (networkManager != null && networkManager.ShutdownInProgress))
+            if (m_RevertingParentChange || !AutoObjectParentSync || (networkManager != null && networkManager.ShutdownInProgress))
             {
                 return;
             }
@@ -2565,7 +2608,7 @@ private void OnTransformParentChanged()
 
             if (networkManager == null || !networkManager.IsListening)
             {
-                transform.parent = m_CachedParent;
+                RevertParentChange();
                 // We want to log at any LogLevel, since we may not have a network manager may here.
                 NetworkLog.LogError($"[{name}] {nameof(networkManager)} is not listening, start a server or host before re-parenting.");
                 return;
@@ -2583,7 +2626,7 @@ private void OnTransformParentChanged()
                 }
                 else
                 {
-                    transform.parent = m_CachedParent;
+                    RevertParentChange();
                     if (networkManager.LogLevel <= LogLevel.Error)
                     {
                         NetworkLog.LogErrorServer($"[{name}] {nameof(NetworkObject)} can only be re-parented after being spawned!");
@@ -2598,7 +2641,7 @@ private void OnTransformParentChanged()
             // If we are spawned and don't have authority; reset the parent back to the cached parent and exit
             if (!isParentingAuthority)
             {
-                transform.parent = m_CachedParent;
+                RevertParentChange();
                 if (networkManager.LogLevel <= LogLevel.Error)
                 {
                     if (networkManager.DistributedAuthorityMode)
@@ -2619,7 +2662,7 @@ private void OnTransformParentChanged()
             {
                 if (!transform.parent.TryGetComponent(out NetworkObject parentObject))
                 {
-                    transform.parent = m_CachedParent;
+                    RevertParentChange();
                     AuthorityAppliedParenting = false;
                     if (networkManager.LogLevel <= LogLevel.Error)
                     {
@@ -2629,7 +2672,7 @@ private void OnTransformParentChanged()
                 }
                 if (!parentObject.IsSpawned)
                 {
-                    transform.parent = m_CachedParent;
+                    RevertParentChange();
                     AuthorityAppliedParenting = false;
                     if (networkManager.LogLevel <= LogLevel.Error)
                     {

com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectParentRevertTests.cs

@@ -0,0 +1,98 @@
+using System.Collections;
+using System.Text.RegularExpressions;
+using NUnit.Framework;
+using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine;
+using UnityEngine.TestTools;
+
+namespace Unity.Netcode.RuntimeTests
+{
+    /// <summary>
+    /// Validates that a rejected parent change restores the NetworkObject's parent and its original pose.
+    /// </summary>
+    [TestFixture(HostOrServer.Host)]
+    [TestFixture(HostOrServer.Server)]
+    [TestFixture(HostOrServer.DAHost)]
+    internal class NetworkObjectParentRevertTests : NetcodeIntegrationTest
+    {
+        // The parent is placed away from the child, so a parent change with worldPositionStays disabled moves the child.
+        private static readonly Vector3 k_ParentPosition = new Vector3(5f, 1.25f, 0f);
+        private static readonly Vector3 k_ChildPosition = new Vector3(0f, 1.25f, 3f);
+
+        protected override int NumberOfClients => 1;
+
+        private GameObject m_Prefab;
+
+        public NetworkObjectParentRevertTests(HostOrServer hostOrServer) : base(hostOrServer) { }
+
+        protected override void OnServerAndClientsCreated()
+        {
+            m_Prefab = CreateNetworkObjectPrefab("ParentRevertObject");
+            base.OnServerAndClientsCreated();
+        }
+
+        private void AssertPoseUnchanged(Transform child, Vector3 worldPosition, Vector3 localPosition)
+        {
+            Assert.IsNull(child.parent, $"{child.name}'s parent change should have been reverted!");
+            Assert.AreEqual(worldPosition, child.position, $"{child.name}'s world position changed!");
+            Assert.AreEqual(localPosition, child.localPosition, $"{child.name}'s local position changed!");
+        }
+
+        /// <summary>
+        /// Validates that a parent change made by a peer without parenting authority is reverted to the original pose.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator NonAuthorityParentChangeRestoresPose([Values] bool worldPositionStays)
+        {
+            var authority = GetAuthorityNetworkManager();
+            var parent = SpawnObject(m_Prefab, authority).GetComponent<NetworkObject>();
+            var child = SpawnObject(m_Prefab, authority).GetComponent<NetworkObject>();
+            yield return WaitForSpawnedOnAllOrTimeOut(parent);
+            AssertOnTimeout($"Timed out waiting for {parent.name} to spawn on all instances!");
+            yield return WaitForSpawnedOnAllOrTimeOut(child);
+            AssertOnTimeout($"Timed out waiting for {child.name} to spawn on all instances!");
+
+            var nonAuthority = GetNonAuthorityNetworkManager();
+            var localParent = nonAuthority.SpawnManager.SpawnedObjects[parent.NetworkObjectId].transform;
+            var localChild = nonAuthority.SpawnManager.SpawnedObjects[child.NetworkObjectId].transform;
+            localParent.position = k_ParentPosition;
+            localChild.position = k_ChildPosition;
+            var worldPosition = localChild.position;
+            var localPosition = localChild.localPosition;
+
+            LogAssert.Expect(LogType.Error, new Regex("re-parent"));
+            localChild.SetParent(localParent, worldPositionStays);
+            AssertPoseUnchanged(localChild, worldPosition, localPosition);
+        }
+
+        /// <summary>
+        /// Validates that a parent change under a GameObject without a <see cref="NetworkObject"/> is reverted to the original pose.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator NonNetworkObjectParentRestoresPose([Values] bool worldPositionStays)
+        {
+            var authority = GetAuthorityNetworkManager();
+            var child = SpawnObject(m_Prefab, authority).GetComponent<NetworkObject>();
+            yield return WaitForSpawnedOnAllOrTimeOut(child);
+            AssertOnTimeout($"Timed out waiting for {child.name} to spawn on all instances!");
+
+            var plainParent = new GameObject("PlainParent");
+            try
+            {
+                plainParent.transform.position = k_ParentPosition;
+                var childTransform = child.transform;
+                childTransform.position = k_ChildPosition;
+                var worldPosition = childTransform.position;
+                var localPosition = childTransform.localPosition;
+
+                LogAssert.Expect(LogType.Error, new Regex("Invalid parenting"));
+                childTransform.SetParent(plainParent.transform, worldPositionStays);
+                AssertPoseUnchanged(childTransform, worldPosition, localPosition);
+            }
+            finally
+            {
+                Object.Destroy(plainParent);
+            }
+        }
+    }
+}