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

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -25,6 +25,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Issue where a `FastBufferReader` created with an offset and the default length of -1 read past the end of its source. The `ArraySegment<byte>` overload that takes an offset now applies it from the start of the segment. (#4221)
 - Issue where a client synchronized while a `NetworkList<T>` still had unsent changes applied those changes twice, resulting in duplicate entries and index-based events landing on the wrong elements on that client. (#4215)
 - Issue where showing a `NetworkObject` with `NetworkShow` while its `NetworkVariable` updates were held back by `NetworkVariableUpdateTraits` stopped those pending changes from being sent until the variable was modified again. (#4215)
+- 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 @@
         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 @@
         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 @@
 
             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 @@
                 }
                 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 @@
             // 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 @@
             {
                 if (!transform.parent.TryGetComponent(out NetworkObject parentObject))
                 {
-                    transform.parent = m_CachedParent;
+                    RevertParentChange();
                     AuthorityAppliedParenting = false;
                     if (networkManager.LogLevel <= LogLevel.Error)
                     {
@@ -2629,7 +2672,7 @@
                 }
                 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);
+            }
+        }
+    }
+}