fix: in-scene parenting WorldPositionStays transform synchronization without NetworkTransform [MTT-7856] (#2796)

* Test Fix

InSceneParentChildHandler was not adding the server side objects to check.

* fix

This fixes the issue with World Position Stays being set not working along this:
- The integration scene names not being added with complex nested children
- Parents and children were not being ordered as parent first then child

But now that this is fixed, the world position stays set to false no longer works (whack a mole).

Account for multiple children under any given parent for sorting.
Minor adjustment to how scale is synchronized with in-scene placed NetworkObjects.

* update

Improved SceneEventData parent sorting

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -13,13 +13,13 @@ Additional documentation and release notes are available at [Multiplayer Documen
 ### Fixed
 
 - Fixed issue where NetworkTransform could potentially attempt to "unregister" a named message prior to it being registered. (#2807)
+- Fixed issue where in-scene placed `NetworkObject`s with complex nested children `NetworkObject`s (more than one child in depth) would not synchronize properly if WorldPositionStays was set to true. (#2796)
 
 ### Changed
 
 - Changed `NetworkTransform` to now use `NetworkTransformMessage` as opposed to named messages for NetworkTransformState updates. (#2810)
 - Changed `CustomMessageManager` so it no longer attempts to register or "unregister" a null or empty string and will log an error if this condition occurs. (#2807)
 
-
 ## [1.8.0] - 2023-12-12
 
 ### Added

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

@@ -992,6 +992,11 @@ internal void SetCachedParent(Transform parentTransform)
             m_CachedParent = parentTransform;
         }
 
+        internal Transform GetCachedParent()
+        {
+            return m_CachedParent;
+        }
+
         internal ulong? GetNetworkParenting() => m_LatestParent;
 
         internal void SetNetworkParenting(ulong? latestParent, bool worldPositionStays)
@@ -1236,7 +1241,7 @@ private void OnTransformParentChanged()
         // we call CheckOrphanChildren() method and quickly iterate over OrphanChildren set and see if we can reparent/adopt one.
         internal static HashSet<NetworkObject> OrphanChildren = new HashSet<NetworkObject>();
 
-        internal bool ApplyNetworkParenting(bool removeParent = false, bool ignoreNotSpawned = false)
+        internal bool ApplyNetworkParenting(bool removeParent = false, bool ignoreNotSpawned = false, bool orphanedChildPass = false)
         {
             if (!AutoObjectParentSync)
             {
@@ -1324,9 +1329,17 @@ internal bool ApplyNetworkParenting(bool removeParent = false, bool ignoreNotSpa
             // If we made it here, then parent this instance under the parentObject
             var parentObject = NetworkManager.SpawnManager.SpawnedObjects[m_LatestParent.Value];
 
+            // If we are handling an orphaned child and its parent is orphaned too, then don't parent yet.
+            if (orphanedChildPass)
+            {
+                if (OrphanChildren.Contains(parentObject))
+                {
+                    return false;
+                }
+            }
+
             m_CachedParent = parentObject.transform;
             transform.SetParent(parentObject.transform, m_CachedWorldPositionStays);
-
             InvokeBehaviourOnNetworkObjectParentChanged(parentObject);
             return true;
         }
@@ -1336,7 +1349,7 @@ internal static void CheckOrphanChildren()
             var objectsToRemove = new List<NetworkObject>();
             foreach (var orphanObject in OrphanChildren)
             {
-                if (orphanObject.ApplyNetworkParenting())
+                if (orphanObject.ApplyNetworkParenting(orphanedChildPass: true))
                 {
                     objectsToRemove.Add(orphanObject);
                 }
@@ -1797,6 +1810,12 @@ internal SceneObject GetMessageSceneObject(ulong targetClientId)
                 var syncRotationPositionLocalSpaceRelative = obj.HasParent && !m_CachedWorldPositionStays;
                 var syncScaleLocalSpaceRelative = obj.HasParent && !m_CachedWorldPositionStays;
 
+                // Always synchronize in-scene placed object's scale using local space
+                if (obj.IsSceneObject)
+                {
+                    syncScaleLocalSpaceRelative = obj.HasParent;
+                }
+
                 // If auto object synchronization is turned off
                 if (!AutoObjectParentSync)
                 {
@@ -1808,7 +1827,6 @@ internal SceneObject GetMessageSceneObject(ulong targetClientId)
                     syncScaleLocalSpaceRelative = obj.HasParent;
                 }
 
-
                 obj.Transform = new SceneObject.TransformData
                 {
                     // If we are parented and we have the m_CachedWorldPositionStays disabled, then use local space

com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventData.cs

@@ -245,6 +245,73 @@ internal void InitializeForSynch()
             }
         }
 
+        /// <summary>
+        /// Used with SortParentedNetworkObjects to sort the children of the root parent NetworkObject
+        /// </summary>
+        /// <param name="first">object to be sorted</param>
+        /// <param name="second">object to be compared to for sorting the first object</param>
+        /// <returns></returns>
+        private int SortChildrenNetworkObjects(NetworkObject first, NetworkObject second)
+        {
+            var firstParent = first.GetCachedParent()?.GetComponent<NetworkObject>();
+            // If the second is the first's parent then move the first down
+            if (firstParent != null && firstParent == second)
+            {
+                return 1;
+            }
+
+            var secondParent = second.GetCachedParent()?.GetComponent<NetworkObject>();
+            // If the first is the second's parent then move the first up
+            if (secondParent != null && secondParent == first)
+            {
+                return -1;
+            }
+
+            // Otherwise, don't move the first at all
+            return 0;
+        }
+
+        /// <summary>
+        /// Sorts the synchronization order of the NetworkObjects to be serialized
+        /// by parents before children order
+        /// </summary>
+        private void SortParentedNetworkObjects()
+        {
+            var networkObjectList = m_NetworkObjectsSync.ToList();
+            foreach (var networkObject in networkObjectList)
+            {
+                // Find only the root parent NetworkObjects
+                if (networkObject.transform.childCount > 0 && networkObject.transform.parent == null)
+                {
+                    // Get all child NetworkObjects of the root
+                    var childNetworkObjects = networkObject.GetComponentsInChildren<NetworkObject>().ToList();
+
+                    childNetworkObjects.Sort(SortChildrenNetworkObjects);
+
+                    // Remove the root from the children list
+                    childNetworkObjects.Remove(networkObject);
+
+                    // Remove the root's children from the primary list
+                    foreach (var childObject in childNetworkObjects)
+                    {
+                        m_NetworkObjectsSync.Remove(childObject);
+                    }
+                    // Insert or Add the sorted children list
+                    var nextIndex = m_NetworkObjectsSync.IndexOf(networkObject) + 1;
+                    if (nextIndex == m_NetworkObjectsSync.Count)
+                    {
+                        m_NetworkObjectsSync.AddRange(childNetworkObjects);
+                    }
+                    else
+                    {
+                        m_NetworkObjectsSync.InsertRange(nextIndex, childNetworkObjects);
+                    }
+                }
+            }
+        }
+
+        internal static bool LogSerializationOrder = false;
+
         internal void AddSpawnedNetworkObjects()
         {
             m_NetworkObjectsSync.Clear();
@@ -256,22 +323,22 @@ internal void AddSpawnedNetworkObjects()
                 }
             }
 
-            // Sort by parents before children
-            m_NetworkObjectsSync.Sort(SortParentedNetworkObjects);
-
             // Sort by INetworkPrefabInstanceHandler implementation before the
             // NetworkObjects spawned by the implementation
             m_NetworkObjectsSync.Sort(SortNetworkObjects);
 
+            // The last thing we sort is parents before children
+            SortParentedNetworkObjects();
+
             // This is useful to know what NetworkObjects a client is going to be synchronized with
             // as well as the order in which they will be deserialized
-            if (m_NetworkManager.LogLevel == LogLevel.Developer)
+            if (LogSerializationOrder && m_NetworkManager.LogLevel == LogLevel.Developer)
             {
                 var messageBuilder = new System.Text.StringBuilder(0xFFFF);
-                messageBuilder.Append("[Server-Side Client-Synchronization] NetworkObject serialization order:");
+                messageBuilder.AppendLine("[Server-Side Client-Synchronization] NetworkObject serialization order:");
                 foreach (var networkObject in m_NetworkObjectsSync)
                 {
-                    messageBuilder.Append($"{networkObject.name}");
+                    messageBuilder.AppendLine($"{networkObject.name}");
                 }
                 NetworkLog.LogInfo(messageBuilder.ToString());
             }
@@ -362,32 +429,6 @@ private int SortNetworkObjects(NetworkObject first, NetworkObject second)
             return 0;
         }
 
-        /// <summary>
-        /// Sorts the synchronization order of the NetworkObjects to be serialized
-        /// by parents before children.
-        /// </summary>
-        /// <remarks>
-        /// This only handles late joining players. Spawning and nesting several children
-        /// dynamically is still handled by the orphaned child list when deserialized out of
-        /// hierarchical order (i.e. Spawn parent and child dynamically, parent message is
-        /// dropped and re-sent but child object is received and processed)
-        /// </remarks>
-        private int SortParentedNetworkObjects(NetworkObject first, NetworkObject second)
-        {
-            // If the first has a parent, move the first down
-            if (first.transform.parent != null)
-            {
-                return 1;
-            }
-            else // If the second has a parent and the first does not, then move the first up
-            if (second.transform.parent != null)
-            {
-                return -1;
-            }
-            return 0;
-        }
-
-
         /// <summary>
         /// Client and Server Side:
         /// Serializes data based on the SceneEvent type (<see cref="SceneEventType"/>)

com.unity.netcode.gameobjects/TestHelpers/Runtime/IntegrationTestSceneHandler.cs

@@ -165,13 +165,30 @@ private static void ProcessInSceneObjects(Scene scene, NetworkManager networkMan
 
             foreach (var sobj in inSceneNetworkObjects)
             {
-                if (sobj.NetworkManagerOwner != networkManager)
-                {
-                    sobj.NetworkManagerOwner = networkManager;
-                }
-                if (sobj.GetComponent<ObjectNameIdentifier>() == null && sobj.GetComponentInChildren<ObjectNameIdentifier>() == null)
+                ProcessInSceneObject(sobj, networkManager);
+            }
+        }
+
+        /// <summary>
+        /// Assures to apply an ObjectNameIdentifier to all children
+        /// </summary>
+        private static void ProcessInSceneObject(NetworkObject networkObject, NetworkManager networkManager)
+        {
+            if (networkObject.NetworkManagerOwner != networkManager)
+            {
+                networkObject.NetworkManagerOwner = networkManager;
+            }
+            if (networkObject.GetComponent<ObjectNameIdentifier>() == null)
+            {
+                networkObject.gameObject.AddComponent<ObjectNameIdentifier>();
+                var networkObjects = networkObject.gameObject.GetComponentsInChildren<NetworkObject>();
+                foreach (var child in networkObjects)
                 {
-                    sobj.gameObject.AddComponent<ObjectNameIdentifier>();
+                    if (child == networkObject)
+                    {
+                        continue;
+                    }
+                    ProcessInSceneObject(child, networkManager);
                 }
             }
         }

testproject/Assets/Tests/Manual/InSceneObjectParentingTests/InSceneParentChildHandler.cs

@@ -174,6 +174,7 @@ public override void OnNetworkSpawn()
                 {
                     ServerRootParent = this;
                 }
+                ServerRelativeInstances.Add(NetworkObjectId, this);
             }
             else
             {

testproject/Assets/Tests/Runtime/ObjectParenting/ParentingInSceneObjectsTests.cs

@@ -66,7 +66,7 @@ protected override IEnumerator OnTearDown()
 
         private void GeneratePositionDoesNotMatch(InSceneParentChildHandler serverHandler, InSceneParentChildHandler clientHandler)
         {
-            m_ErrorValidationLog.Append($"[Client-{clientHandler.NetworkManager.LocalClientId}] {nameof(NetworkObject)}-{clientHandler.NetworkObjectId}'s position {clientHandler.transform.position} does not equal the server-side position {serverHandler.transform.position}");
+            m_ErrorValidationLog.AppendLine($"[Client-{clientHandler.NetworkManager.LocalClientId}] {nameof(NetworkObject)}-{clientHandler.NetworkObjectId}'s position {clientHandler.transform.position} does not equal the server-side position {serverHandler.transform.position}");
         }
 
         private void GenerateRotationDoesNotMatch(InSceneParentChildHandler serverHandler, InSceneParentChildHandler clientHandler)
@@ -76,7 +76,7 @@ private void GenerateRotationDoesNotMatch(InSceneParentChildHandler serverHandle
 
         private void GenerateScaleDoesNotMatch(InSceneParentChildHandler serverHandler, InSceneParentChildHandler clientHandler)
         {
-            m_ErrorValidationLog.Append($"[Client-{clientHandler.NetworkManager.LocalClientId}] {nameof(NetworkObject)}-{clientHandler.NetworkObjectId}'s scale {clientHandler.transform.localScale} does not equal the server-side scale {serverHandler.transform.localScale}");
+            m_ErrorValidationLog.AppendLine($"[Client-{clientHandler.NetworkManager.LocalClientId}] {nameof(NetworkObject)}-{clientHandler.NetworkObjectId}'s scale {clientHandler.transform.localScale} does not equal the server-side scale {serverHandler.transform.localScale}");
         }
 
         private void GenerateParentIsNotCorrect(InSceneParentChildHandler handler, bool shouldHaveParent, bool isStillSpawnedCheck = false)
@@ -85,18 +85,41 @@ private void GenerateParentIsNotCorrect(InSceneParentChildHandler handler, bool
             var shouldNotBeSpawned = isStillSpawnedCheck ? " and is still spawned!" : string.Empty;
             if (!shouldHaveParent)
             {
-                m_ErrorValidationLog.Append($"[{serverOrClient}-{handler.NetworkManager.LocalClientId}] {nameof(NetworkObject)}-{handler.NetworkObjectId}'s still has the parent {handler.transform.parent.name} when it should be null{shouldNotBeSpawned}!");
+                m_ErrorValidationLog.AppendLine($"[{serverOrClient}-{handler.NetworkManager.LocalClientId}] {nameof(NetworkObject)}-{handler.NetworkObjectId}'s still has the parent {handler.transform.parent.name} when it should be null{shouldNotBeSpawned}!");
             }
             else
             {
-                m_ErrorValidationLog.Append($"[{serverOrClient}-{handler.NetworkManager.LocalClientId}] {nameof(NetworkObject)}-{handler.NetworkObjectId}'s does not have a parent when it should!");
+                m_ErrorValidationLog.AppendLine($"[{serverOrClient}-{handler.NetworkManager.LocalClientId}] {nameof(NetworkObject)}-{handler.NetworkObjectId}'s does not have a parent when it should!");
+            }
+        }
+
+        private void GenerateTransformInfo(InSceneParentChildHandler serverHandler, InSceneParentChildHandler clientHandler, ulong failingClient)
+        {
+            if (clientHandler.NetworkManager.LocalClientId != failingClient)
+            {
+                return;
+            }
+            var serverChild = serverHandler.transform;
+            var clientChild = clientHandler.transform;
+            m_ErrorValidationLog.AppendLine($"[server-child][{serverChild.name}] Pos: {serverChild.position} | Rot: {serverChild.eulerAngles} | Scale: {serverChild.localScale}");
+            m_ErrorValidationLog.AppendLine($"[client-child][{clientChild.name}] Pos: {clientChild.position} | Rot: {clientChild.eulerAngles} | Scale: {clientChild.localScale}");
+            var clientParent = clientHandler.transform.parent;
+            var serverParent = serverHandler.transform.parent;
+            while (serverParent && clientParent)
+            {
+                m_ErrorValidationLog.AppendLine($"[server-parent][{serverParent.name}] Pos: {serverParent.position} | Rot: {serverParent.eulerAngles} | Scale: {serverParent.localScale}");
+                m_ErrorValidationLog.AppendLine($"[client-parent][{clientParent.name}] Pos: {clientParent.position} | Rot: {clientParent.eulerAngles} | Scale: {clientParent.localScale}");
+                clientParent = clientParent.parent;
+                serverParent = serverParent.parent;
             }
         }
 
         private bool ValidateClientAgainstServerTransformValues()
         {
             // We reset this each time because we are only interested in the last time it checked and failed
             m_ErrorValidationLog.Clear();
+            var passed = true;
+            var failingClient = (ulong)0;
             foreach (var instance in InSceneParentChildHandler.ServerRelativeInstances)
             {
                 var serverInstanceTransform = instance.Value.transform;
@@ -108,25 +131,35 @@ private bool ValidateClientAgainstServerTransformValues()
                     if (!Approximately(serverInstanceTransform.position, clientInstanceTransform.position))
                     {
                         GeneratePositionDoesNotMatch(instance.Value, clientInstance);
-                        return false;
+                        passed = false;
+                        failingClient = clientInstance.NetworkManager.LocalClientId;
                     }
 
                     if (!Approximately(serverInstanceTransform.eulerAngles, clientInstanceTransform.eulerAngles))
                     {
-                        GeneratePositionDoesNotMatch(instance.Value, clientInstance);
-                        return false;
+                        GenerateRotationDoesNotMatch(instance.Value, clientInstance);
+                        passed = false;
+                        failingClient = clientInstance.NetworkManager.LocalClientId;
                     }
 
                     if (!Approximately(serverInstanceTransform.localScale, clientInstanceTransform.localScale))
                     {
-                        GeneratePositionDoesNotMatch(instance.Value, clientInstance);
-                        return false;
+                        GenerateScaleDoesNotMatch(instance.Value, clientInstance);
+                        passed = false;
+                        failingClient = clientInstance.NetworkManager.LocalClientId;
+                    }
+
+                    if (!passed && LogTransform)
+                    {
+                        GenerateTransformInfo(instance.Value, clientInstance, failingClient);
                     }
                 }
             }
-            return true;
+            return passed;
         }
 
+        internal bool LogTransform;
+
         private bool ValidateAllChildrenParentingStatus(bool checkForParent)
         {
             m_ErrorValidationLog.Clear();
@@ -215,7 +248,7 @@ public IEnumerator InSceneParentingTest([Values] ParentingSpace parentingSpace)
             // Make sure the late joining client synchronizes properly
             yield return CreateAndStartNewClient();
             yield return WaitForConditionOrTimeOut(ValidateClientAgainstServerTransformValues);
-            AssertOnTimeout($"Timed out waiting for the late joining client's transform values to match the server transform values!\n {m_ErrorValidationLog}");
+            AssertOnTimeout($"[Late Join 1] Timed out waiting for the late joining client's transform values to match the server transform values!\n {m_ErrorValidationLog}");
 
             // Remove the parents from all of the children
             InSceneParentChildHandler.ServerRootParent.DeparentAllChildren();