Skip to content

Commit b6c8608

Browse files
EmandMquantum-shubhamNoelStephensUnity
authored
fix: [UpPort] NetworkList changes pending during client synchronization are applied twice (#4216)
* fix: NetworkList changes pending during client synchronization are applied twice Override WriteFieldSynchronization in NetworkList<T> so a client synchronized while local changes are still pending receives the list as of the last sent delta, mirroring NetworkVariable<T>. The pending events are then applied exactly once with the next delta. Fixes #4179 * Update CHANGELOG * chore: retrigger CI * chore: retrigger CI * fix: keep NetworkObject queued when NetworkShow cannot flush throttled updates ForceSendIfDirtyOnNetworkShow removed the NetworkObject from the updater even when NetworkVariableUpdateTraits held back its dirty variables, so those changes were not sent until the variable was modified again. Re-queue the object when anything is still dirty after the forced flush, and add a regression test for showing an object while NetworkList updates are throttled. --------- Co-authored-by: Shubham Kumar <146586770+quantum-shubham@users.noreply.github.com> Co-authored-by: Noel Stephens <noel.stephens@unity3d.com>
1 parent c79b7eb commit b6c8608

4 files changed

Lines changed: 151 additions & 3 deletions

File tree

‎com.unity.netcode.gameobjects/CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
2727

2828
- Issue where a growable `FastBufferWriter` created with a size of 0 never returned from its first write or `TryBeginWrite` call that needed the buffer to grow. (#4224)
2929
- 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. (#4222)
30+
- 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)
31+
- 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. (#4216)
3032
- Issue where a client promoted to session owner could throw an exception when synchronizing a newly joining client. (#4214)
3133
- Issue where `NetworkManager.OnNetCodeDisconnect` was not invoked when a connection closed during a hybrid mode session, only when the session shut down. (#4209)
3234
- 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. (#4202)

‎com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviourUpdater.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,19 @@ internal void ForceSendIfDirtyOnNetworkShow(NetworkObject networkObject)
138138
// Remove it from the pending and queued dirty objects lists
139139
m_PendingDirtyNetworkObjects.Remove(networkObject);
140140
m_DirtyNetworkObjects.Remove(networkObject);
141+
142+
// Anything still dirty was held back by its update traits, keep it queued so a later tick sends it
143+
foreach (var behaviour in networkObject.ChildNetworkBehaviours.Values)
144+
{
145+
foreach (var networkVariable in behaviour.NetworkVariableFields)
146+
{
147+
if (networkVariable.IsDirty())
148+
{
149+
AddForUpdate(networkObject);
150+
return;
151+
}
152+
}
153+
}
141154
}
142155

143156
/// <summary>

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

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@ public class NetworkList<T> : NetworkVariableBase where T : unmanaged, IEquatabl
1717
private NativeList<T> m_List = new NativeList<T>(64, Allocator.Persistent);
1818
private NativeList<NetworkListEvent<T>> m_DirtyEvents = new NativeList<NetworkListEvent<T>>(64, Allocator.Persistent);
1919

20+
// The list as it was before the first local change that is still pending in m_DirtyEvents
21+
private NativeList<T> m_LastSentList = new NativeList<T>(64, Allocator.Persistent);
22+
private bool m_HasLastSentList;
23+
2024
/// <summary>
2125
/// Delegate type for list changed event
2226
/// </summary>
@@ -68,6 +72,7 @@ public override void ResetDirty()
6872
{
6973
m_DirtyEvents.Clear();
7074
}
75+
m_HasLastSentList = false;
7176
}
7277

7378
/// <inheritdoc cref="NetworkVariable{T}.IsDirty"/>
@@ -150,10 +155,42 @@ private T ReadValue(FastBufferReader reader)
150155
/// <inheritdoc cref="NetworkVariable{T}.WriteField"/>
151156
public override void WriteField(FastBufferWriter writer)
152157
{
153-
writer.WriteValueSafe((ushort)m_List.Length);
154-
for (int i = 0; i < m_List.Length; i++)
158+
WriteList(writer, m_List);
159+
}
160+
161+
internal override void WriteFieldSynchronization(FastBufferWriter writer)
162+
{
163+
// The pending events are sent to the synchronizing client with the next delta, so it must
164+
// receive the list they apply to. Otherwise it would apply them twice.
165+
if (m_HasLastSentList && m_DirtyEvents.Length > 0)
166+
{
167+
WriteList(writer, m_LastSentList);
168+
}
169+
else
170+
{
171+
base.WriteFieldSynchronization(writer);
172+
}
173+
}
174+
175+
private static void WriteList(FastBufferWriter writer, NativeList<T> list)
176+
{
177+
writer.WriteValueSafe((ushort)list.Length);
178+
for (int i = 0; i < list.Length; i++)
155179
{
156-
NetworkVariableSerialization<T>.Serializer.Write(writer, ref m_List.ElementAt(i));
180+
NetworkVariableSerialization<T>.Serializer.Write(writer, ref list.ElementAt(i));
181+
}
182+
}
183+
184+
/// <summary>
185+
/// Invoked before a local change. Keeps a copy of the list as of the last sent delta
186+
/// so <see cref="WriteFieldSynchronization"/> can send it while changes are pending.
187+
/// </summary>
188+
private void CaptureLastSentList()
189+
{
190+
if (m_DirtyEvents.Length == 0)
191+
{
192+
m_LastSentList.CopyFrom(m_List);
193+
m_HasLastSentList = true;
157194
}
158195
}
159196

@@ -440,6 +477,7 @@ public void Add(T item)
440477
return;
441478
}
442479

480+
CaptureLastSentList();
443481
m_List.Add(item);
444482

445483
var listEvent = new NetworkListEvent<T>()
@@ -467,6 +505,7 @@ public void Clear()
467505
return;
468506
}
469507

508+
CaptureLastSentList();
470509
m_List.Clear();
471510

472511
var listEvent = new NetworkListEvent<T>()
@@ -511,6 +550,7 @@ public bool Remove(T item)
511550
return false;
512551
}
513552

553+
CaptureLastSentList();
514554
m_List.RemoveAt(index);
515555
var listEvent = new NetworkListEvent<T>()
516556
{
@@ -554,6 +594,7 @@ public void Insert(int index, T item)
554594
return;
555595
}
556596

597+
CaptureLastSentList();
557598
if (index < m_List.Length)
558599
{
559600
m_List.InsertRangeWithBeginEnd(index, index + 1);
@@ -590,6 +631,7 @@ public void RemoveAt(int index)
590631
}
591632

592633
var value = m_List[index];
634+
CaptureLastSentList();
593635
m_List.RemoveAt(index);
594636

595637
var listEvent = new NetworkListEvent<T>()
@@ -632,6 +674,7 @@ public void Set(int index, T value, bool forceUpdate = false)
632674
return;
633675
}
634676

677+
CaptureLastSentList();
635678
m_List[index] = value;
636679

637680
var listEvent = new NetworkListEvent<T>()
@@ -707,6 +750,11 @@ public override void Dispose()
707750
m_DirtyEvents.Dispose();
708751
}
709752

753+
if (m_LastSentList.IsCreated)
754+
{
755+
m_LastSentList.Dispose();
756+
}
757+
710758
base.Dispose();
711759
}
712760
}

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,91 @@ private bool OnVerifyData(StringBuilder errorLog)
9191
return true;
9292
}
9393

94+
/// <summary>
95+
/// Validates that a client synchronized while the list still has unsent changes
96+
/// does not apply those changes twice (snapshot + next delta).
97+
/// </summary>
98+
[UnityTest]
99+
public IEnumerator LateJoinWithPendingChanges()
100+
{
101+
var authority = GetAuthorityNetworkManager();
102+
var instantiatedObject = SpawnObject(m_ListObjectPrefab, authority).GetComponent<NetworkObject>();
103+
m_TestObjectId = instantiatedObject.NetworkObjectId;
104+
105+
yield return WaitForSpawnedOnAllOrTimeOut(instantiatedObject);
106+
AssertOnTimeout("[Setup] Failed to spawn list object");
107+
108+
var list = instantiatedObject.GetComponent<NetworkListTest>().TheList;
109+
m_ExpectedValues.Clear();
110+
for (int i = 0; i < 5; i++)
111+
{
112+
m_ExpectedValues.Add(i);
113+
list.Add(i);
114+
}
115+
116+
yield return WaitForConditionOrTimeOut(OnVerifyData);
117+
AssertOnTimeout("[Setup] Failed to add items to list");
118+
119+
// Hold back deltas so these changes are still pending when the new client synchronizes
120+
list.SetUpdateTraits(new NetworkVariableUpdateTraits { MinSecondsBetweenUpdates = 3600 });
121+
list.Add(100);
122+
m_ExpectedValues.Add(100);
123+
list.Insert(0, 101);
124+
m_ExpectedValues.Insert(0, 101);
125+
list.RemoveAt(2);
126+
m_ExpectedValues.RemoveAt(2);
127+
list[1] = 102;
128+
m_ExpectedValues[1] = 102;
129+
list.Remove(3);
130+
m_ExpectedValues.Remove(3);
131+
132+
yield return CreateAndStartNewClient();
133+
Assert.IsTrue(list.IsDirty(), "[Setup] Changes should still be pending after the new client synchronized");
134+
135+
list.SetUpdateTraits(default);
136+
yield return WaitForConditionOrTimeOut(OnVerifyData);
137+
AssertOnTimeout("Late joined client did not match the authority's list");
138+
}
139+
140+
/// <summary>
141+
/// Validates that showing an object while list updates are held back by update traits
142+
/// still delivers the pending changes once they can be sent.
143+
/// </summary>
144+
[UnityTest]
145+
public IEnumerator NetworkShowWithThrottledPendingChanges()
146+
{
147+
var authority = GetAuthorityNetworkManager();
148+
var nonAuthority = GetNonAuthorityNetworkManager();
149+
var instantiatedObject = SpawnObject(m_ListObjectPrefab, authority).GetComponent<NetworkObject>();
150+
m_TestObjectId = instantiatedObject.NetworkObjectId;
151+
152+
yield return WaitForSpawnedOnAllOrTimeOut(instantiatedObject);
153+
AssertOnTimeout("[Setup] Failed to spawn list object");
154+
155+
instantiatedObject.NetworkHide(nonAuthority.LocalClientId);
156+
yield return WaitForConditionOrTimeOut(() => !nonAuthority.SpawnManager.SpawnedObjects.ContainsKey(m_TestObjectId));
157+
AssertOnTimeout("[Setup] Failed to hide list object");
158+
159+
// Hold back deltas so these changes are still pending when the object is shown
160+
var list = instantiatedObject.GetComponent<NetworkListTest>().TheList;
161+
list.SetUpdateTraits(new NetworkVariableUpdateTraits { MinSecondsBetweenUpdates = 3600 });
162+
m_ExpectedValues.Clear();
163+
for (int i = 0; i < 5; i++)
164+
{
165+
m_ExpectedValues.Add(i);
166+
list.Add(i);
167+
}
168+
169+
instantiatedObject.NetworkShow(nonAuthority.LocalClientId);
170+
yield return WaitForConditionOrTimeOut(() => nonAuthority.SpawnManager.SpawnedObjects.ContainsKey(m_TestObjectId));
171+
AssertOnTimeout("[Setup] Failed to show list object");
172+
Assert.IsTrue(list.IsDirty(), "[Setup] Changes should still be pending after the object was shown");
173+
174+
list.SetUpdateTraits(default);
175+
yield return WaitForConditionOrTimeOut(OnVerifyData);
176+
AssertOnTimeout("Clients did not receive the changes that were pending when the object was shown");
177+
}
178+
94179
[UnityTest]
95180
public IEnumerator ValidateNetworkListSynchronization()
96181
{

0 commit comments

Comments
 (0)