fix: [UpPort] NetworkList changes pending during client synchronization are applied twice

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -23,6 +23,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- 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. (#4216)
 - Issue where a client's clock kept alternating between running slightly fast and slightly slow on every frame, even after it had synchronized with the server. Interpolated objects juddered (a periodic motion artifact caused by mismatched frame rates): they moved in uneven steps instead of smoothly. Barely noticeable at the default `NetworkTimeSystem.AdjustmentRatio`, and clearly visible at higher values.
 - Issue where the IL post-processor could not resolve core library types, such as `Guid` and `List<T>`, in assemblies compiled against .NET reference assemblies, for example when targeting .NET 10. Those split the core library across `System.Runtime`, `System.Collections` and other assemblies.
 - Fixed issue where a prefab added to a `NetworkPrefabsList` while a session was running was registered more than once and logged a duplicate `GlobalObjectIdHash` error. (#4184)

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs

@@ -17,6 +17,10 @@ public class NetworkList<T> : NetworkVariableBase where T : unmanaged, IEquatabl
         private NativeList<T> m_List = new NativeList<T>(64, Allocator.Persistent);
         private NativeList<NetworkListEvent<T>> m_DirtyEvents = new NativeList<NetworkListEvent<T>>(64, Allocator.Persistent);
 
+        // The list as it was before the first local change that is still pending in m_DirtyEvents
+        private NativeList<T> m_LastSentList = new NativeList<T>(64, Allocator.Persistent);
+        private bool m_HasLastSentList;
+
         /// <summary>
         /// Delegate type for list changed event
         /// </summary>
@@ -68,6 +72,7 @@ public override void ResetDirty()
             {
                 m_DirtyEvents.Clear();
             }
+            m_HasLastSentList = false;
         }
 
         /// <inheritdoc cref="NetworkVariable{T}.IsDirty"/>
@@ -150,10 +155,42 @@ private T ReadValue(FastBufferReader reader)
         /// <inheritdoc cref="NetworkVariable{T}.WriteField"/>
         public override void WriteField(FastBufferWriter writer)
         {
-            writer.WriteValueSafe((ushort)m_List.Length);
-            for (int i = 0; i < m_List.Length; i++)
+            WriteList(writer, m_List);
+        }
+
+        internal override void WriteFieldSynchronization(FastBufferWriter writer)
+        {
+            // The pending events are sent to the synchronizing client with the next delta, so it must
+            // receive the list they apply to. Otherwise it would apply them twice.
+            if (m_HasLastSentList && m_DirtyEvents.Length > 0)
+            {
+                WriteList(writer, m_LastSentList);
+            }
+            else
+            {
+                base.WriteFieldSynchronization(writer);
+            }
+        }
+
+        private static void WriteList(FastBufferWriter writer, NativeList<T> list)
+        {
+            writer.WriteValueSafe((ushort)list.Length);
+            for (int i = 0; i < list.Length; i++)
             {
-                NetworkVariableSerialization<T>.Serializer.Write(writer, ref m_List.ElementAt(i));
+                NetworkVariableSerialization<T>.Serializer.Write(writer, ref list.ElementAt(i));
+            }
+        }
+
+        /// <summary>
+        /// Invoked before a local change. Keeps a copy of the list as of the last sent delta
+        /// so <see cref="WriteFieldSynchronization"/> can send it while changes are pending.
+        /// </summary>
+        private void CaptureLastSentList()
+        {
+            if (m_DirtyEvents.Length == 0)
+            {
+                m_LastSentList.CopyFrom(m_List);
+                m_HasLastSentList = true;
             }
         }
 
@@ -440,6 +477,7 @@ public void Add(T item)
                 return;
             }
 
+            CaptureLastSentList();
             m_List.Add(item);
 
             var listEvent = new NetworkListEvent<T>()
@@ -467,6 +505,7 @@ public void Clear()
                 return;
             }
 
+            CaptureLastSentList();
             m_List.Clear();
 
             var listEvent = new NetworkListEvent<T>()
@@ -511,6 +550,7 @@ public bool Remove(T item)
                 return false;
             }
 
+            CaptureLastSentList();
             m_List.RemoveAt(index);
             var listEvent = new NetworkListEvent<T>()
             {
@@ -554,6 +594,7 @@ public void Insert(int index, T item)
                 return;
             }
 
+            CaptureLastSentList();
             if (index < m_List.Length)
             {
                 m_List.InsertRangeWithBeginEnd(index, index + 1);
@@ -590,6 +631,7 @@ public void RemoveAt(int index)
             }
 
             var value = m_List[index];
+            CaptureLastSentList();
             m_List.RemoveAt(index);
 
             var listEvent = new NetworkListEvent<T>()
@@ -632,6 +674,7 @@ public void Set(int index, T value, bool forceUpdate = false)
                 return;
             }
 
+            CaptureLastSentList();
             m_List[index] = value;
 
             var listEvent = new NetworkListEvent<T>()
@@ -707,6 +750,11 @@ public override void Dispose()
                 m_DirtyEvents.Dispose();
             }
 
+            if (m_LastSentList.IsCreated)
+            {
+                m_LastSentList.Dispose();
+            }
+
             base.Dispose();
         }
     }

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkListTests.cs

@@ -91,6 +91,52 @@ private bool OnVerifyData(StringBuilder errorLog)
             return true;
         }
 
+        /// <summary>
+        /// Validates that a client synchronized while the list still has unsent changes
+        /// does not apply those changes twice (snapshot + next delta).
+        /// </summary>
+        [UnityTest]
+        public IEnumerator LateJoinWithPendingChanges()
+        {
+            var authority = GetAuthorityNetworkManager();
+            var instantiatedObject = SpawnObject(m_ListObjectPrefab, authority).GetComponent<NetworkObject>();
+            m_TestObjectId = instantiatedObject.NetworkObjectId;
+
+            yield return WaitForSpawnedOnAllOrTimeOut(instantiatedObject);
+            AssertOnTimeout("[Setup] Failed to spawn list object");
+
+            var list = instantiatedObject.GetComponent<NetworkListTest>().TheList;
+            m_ExpectedValues.Clear();
+            for (int i = 0; i < 5; i++)
+            {
+                m_ExpectedValues.Add(i);
+                list.Add(i);
+            }
+
+            yield return WaitForConditionOrTimeOut(OnVerifyData);
+            AssertOnTimeout("[Setup] Failed to add items to list");
+
+            // Hold back deltas so these changes are still pending when the new client synchronizes
+            list.SetUpdateTraits(new NetworkVariableUpdateTraits { MinSecondsBetweenUpdates = 3600 });
+            list.Add(100);
+            m_ExpectedValues.Add(100);
+            list.Insert(0, 101);
+            m_ExpectedValues.Insert(0, 101);
+            list.RemoveAt(2);
+            m_ExpectedValues.RemoveAt(2);
+            list[1] = 102;
+            m_ExpectedValues[1] = 102;
+            list.Remove(3);
+            m_ExpectedValues.Remove(3);
+
+            yield return CreateAndStartNewClient();
+            Assert.IsTrue(list.IsDirty(), "[Setup] Changes should still be pending after the new client synchronized");
+
+            list.SetUpdateTraits(default);
+            yield return WaitForConditionOrTimeOut(OnVerifyData);
+            AssertOnTimeout("Late joined client did not match the authority's list");
+        }
+
         [UnityTest]
         public IEnumerator ValidateNetworkListSynchronization()
         {