Skip to content

Commit 133c125

Browse files
NoelStephensUnityNoelStephensUnity
authored
fix: collections not detecting deltas (#3004)
* fix First pass in fixing the issues with managed collections. * test Adjustments to handle the removal of throwing exceptions. * style removing whitespace * fix Detecting a delta by direct assignment of an already assigned list will not work with NetworkVariable and would require a pretty hefty rework on how we handle detecting a delta in a collection without impacting non-collection types. * fix we don't need to duplicate anymore since we are not detecting whether any items in a collection are different when directly assigning the same collection. * chore cleaning up some of the NetworkVariable associated tests so they are contained in a subfolder and so each test is contained in its own CS file. * fix Mark that we have a previous value for disposing when destroyed. * fix This fixes an issue with duplicating nested lists, hashsets, and dictionaries. * test This is the first set of tests for Lists, including nested, that test using base types and INetworkSerializable. Still have HashSet and Dictionary to cover. * test Added Dictionary and HashSet to the tests. * style Removing whitespaces * update adding changelog entries * update setting the right PR associated with these updates
1 parent d2bc5d4 commit 133c125

29 files changed

+3894
-837
lines changed

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,20 @@ Additional documentation and release notes are available at [Multiplayer Documen
1010

1111
### Added
1212

13+
- Added `NetworkVariable.CheckDirtyState` that is to be used in tandem with collections in order to detect whether the collection or an item within the collection has changed. (#3004)
14+
1315
### Fixed
1416

17+
- Fixed issue using collections within `NetworkVariable` where the collection would not detect changes to items or nested items. (#3004)
18+
- Fixed issue where `List`, `Dictionary`, and `HashSet` collections would not uniquely duplicate nested collections. (#3004)
1519
- Fixed issue where `NotAuthorityTarget` would include the service observer in the list of targets to send the RPC to as opposed to excluding the service observer as it should. (#3000)
1620
- Fixed issue where `ProxyRpcTargetGroup` could attempt to send a message if there were no targets to send to. (#3000)
1721

1822
### Changed
1923

24+
- Changed permissions exception thrown in `NetworkList` to exiting early with a logged error that is now a unified permissions message within `NetworkVariableBase`. (#3004)
25+
- Changed permissions exception thrown in `NetworkVariable.Value` to exiting early with a logged error that is now a unified permissions message within `NetworkVariableBase`. (#3004)
26+
2027

2128
## [2.0.0-pre.3] - 2024-07-23
2229

‎com.unity.netcode.gameobjects/Runtime/Messaging/Messages/NetworkVariableDeltaMessage.cs‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ internal struct NetworkVariableDeltaMessage : INetworkMessage
2525

2626
private const string k_Name = "NetworkVariableDeltaMessage";
2727

28-
// DANGO-TODO: Made some modifications here that overlap/won't play nice with EnsureNetworkVariableLenghtSafety.
29-
// Worth either merging or more cleanly separating these codepaths.
3028
public void Serialize(FastBufferWriter writer, int targetVersion)
3129
{
3230
if (!writer.TryBeginWrite(FastBufferWriter.GetWriteSize(NetworkObjectId) + FastBufferWriter.GetWriteSize(NetworkBehaviourIndex)))
@@ -126,10 +124,6 @@ public void Serialize(FastBufferWriter writer, int targetVersion)
126124
}
127125
else
128126
{
129-
// DANGO-TODO:
130-
// Complex types with custom type serialization (either registered custom types or INetworkSerializable implementations) will be problematic
131-
// Non-complex types always provide a full state update per delta
132-
// DANGO-TODO: Add NetworkListEvent<T>.EventType awareness to the cloud-state server
133127
if (networkManager.DistributedAuthorityMode)
134128
{
135129
var size_marker = writer.Position;
@@ -167,8 +161,6 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
167161
return true;
168162
}
169163

170-
// DANGO-TODO: Made some modifications here that overlap/won't play nice with EnsureNetworkVariableLenghtSafety.
171-
// Worth either merging or more cleanly separating these codepaths.
172164
public void Handle(ref NetworkContext context)
173165
{
174166
var networkManager = (NetworkManager)context.SystemOwner;

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

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,8 @@ public void Add(T item)
393393
// check write permissions
394394
if (!CanClientWrite(m_NetworkManager.LocalClientId))
395395
{
396-
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
396+
LogWritePermissionError();
397+
return;
397398
}
398399

399400
m_List.Add(item);
@@ -414,7 +415,8 @@ public void Clear()
414415
// check write permissions
415416
if (!CanClientWrite(m_NetworkManager.LocalClientId))
416417
{
417-
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
418+
LogWritePermissionError();
419+
return;
418420
}
419421

420422
m_List.Clear();
@@ -440,7 +442,8 @@ public bool Remove(T item)
440442
// check write permissions
441443
if (!CanClientWrite(m_NetworkManager.LocalClientId))
442444
{
443-
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
445+
LogWritePermissionError();
446+
return false;
444447
}
445448

446449
int index = m_List.IndexOf(item);
@@ -475,7 +478,8 @@ public void Insert(int index, T item)
475478
// check write permissions
476479
if (!CanClientWrite(m_NetworkManager.LocalClientId))
477480
{
478-
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
481+
LogWritePermissionError();
482+
return;
479483
}
480484

481485
if (index < m_List.Length)
@@ -520,6 +524,8 @@ public void RemoveAt(int index)
520524
HandleAddListEvent(listEvent);
521525
}
522526

527+
528+
523529
/// <inheritdoc />
524530
public T this[int index]
525531
{
@@ -529,7 +535,8 @@ public T this[int index]
529535
// check write permissions
530536
if (!CanClientWrite(m_NetworkManager.LocalClientId))
531537
{
532-
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
538+
LogWritePermissionError();
539+
return;
533540
}
534541

535542
var previousValue = m_List[index];

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

Lines changed: 44 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -95,25 +95,57 @@ public void Reset(T value = default)
9595
/// <summary>
9696
/// The value of the NetworkVariable container
9797
/// </summary>
98+
/// <remarks>
99+
/// When assigning collections to <see cref="Value"/>, unless it is a completely new collection this will not
100+
/// detect any deltas with most managed collection classes since assignment of one collection value to another
101+
/// is actually just a reference to the collection itself. <br />
102+
/// To detect deltas in a collection, you should invoke <see cref="CheckDirtyState"/> after making modifications to the collection.
103+
/// </remarks>
98104
public virtual T Value
99105
{
100106
get => m_InternalValue;
101107
set
102108
{
103-
// Compare bitwise
104-
if (NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref value))
109+
if (m_NetworkManager && !CanClientWrite(m_NetworkManager.LocalClientId))
105110
{
111+
LogWritePermissionError();
106112
return;
107113
}
108114

109-
if (m_NetworkManager && !CanClientWrite(m_NetworkManager.LocalClientId))
115+
// Compare the Value being applied to the current value
116+
if (!NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref value))
110117
{
111-
throw new InvalidOperationException($"[Client-{m_NetworkManager.LocalClientId}][{m_NetworkBehaviour.name}][{Name}] Write permissions ({WritePerm}) for this client instance is not allowed!");
118+
T previousValue = m_InternalValue;
119+
m_InternalValue = value;
120+
SetDirty(true);
121+
m_IsDisposed = false;
122+
OnValueChanged?.Invoke(previousValue, m_InternalValue);
112123
}
124+
}
125+
}
126+
127+
/// <summary>
128+
/// Invoke this method to check if a collection's items are dirty.
129+
/// The default behavior is to exit early if the <see cref="NetworkVariable{T}"/> is already dirty.
130+
/// </summary>
131+
/// <param name="forceCheck"> when true, this check will force a full item collection check even if the NetworkVariable is already dirty</param>
132+
/// <remarks>
133+
/// This is to be used as a way to check if a <see cref="NetworkVariable{T}"/> containing a managed collection has any changees to the collection items.<br />
134+
/// If you invoked this when a collection is dirty, it will not trigger the <see cref="OnValueChanged"/> unless you set <param name="forceCheck"/> to true. <br />
135+
/// </remarks>
136+
public bool CheckDirtyState(bool forceCheck = false)
137+
{
138+
var isDirty = base.IsDirty();
113139

114-
Set(value);
140+
// Compare the previous with the current if not dirty or forcing a check.
141+
if ((!isDirty || forceCheck) && !NetworkVariableSerialization<T>.AreEqual(ref m_PreviousValue, ref m_InternalValue))
142+
{
143+
SetDirty(true);
144+
OnValueChanged?.Invoke(m_PreviousValue, m_InternalValue);
115145
m_IsDisposed = false;
146+
isDirty = true;
116147
}
148+
return isDirty;
117149
}
118150

119151
internal ref T RefValue()
@@ -194,19 +226,6 @@ public override void ResetDirty()
194226
base.ResetDirty();
195227
}
196228

197-
/// <summary>
198-
/// Sets the <see cref="Value"/>, marks the <see cref="NetworkVariable{T}"/> dirty, and invokes the <see cref="OnValueChanged"/> callback
199-
/// if there are subscribers to that event.
200-
/// </summary>
201-
/// <param name="value">the new value of type `T` to be set/></param>
202-
private protected void Set(T value)
203-
{
204-
SetDirty(true);
205-
T previousValue = m_InternalValue;
206-
m_InternalValue = value;
207-
OnValueChanged?.Invoke(previousValue, m_InternalValue);
208-
}
209-
210229
/// <summary>
211230
/// Writes the variable to the writer
212231
/// </summary>
@@ -223,20 +242,22 @@ public override void WriteDelta(FastBufferWriter writer)
223242
/// <param name="keepDirtyDelta">Whether or not the container should keep the dirty delta, or mark the delta as consumed</param>
224243
public override void ReadDelta(FastBufferReader reader, bool keepDirtyDelta)
225244
{
245+
// In order to get managed collections to properly have a previous and current value, we have to
246+
// duplicate the collection at this point before making any modifications to the current.
247+
m_HasPreviousValue = true;
248+
NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_PreviousValue);
249+
NetworkVariableSerialization<T>.ReadDelta(reader, ref m_InternalValue);
250+
226251
// todo:
227252
// keepDirtyDelta marks a variable received as dirty and causes the server to send the value to clients
228253
// In a prefect world, whether a variable was A) modified locally or B) received and needs retransmit
229254
// would be stored in different fields
230-
231-
T previousValue = m_InternalValue;
232-
NetworkVariableSerialization<T>.ReadDelta(reader, ref m_InternalValue);
233-
234255
if (keepDirtyDelta)
235256
{
236257
SetDirty(true);
237258
}
238259

239-
OnValueChanged?.Invoke(previousValue, m_InternalValue);
260+
OnValueChanged?.Invoke(m_PreviousValue, m_InternalValue);
240261
}
241262

242263
/// <inheritdoc />

‎com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,16 @@ public abstract class NetworkVariableBase : IDisposable
3737

3838
internal virtual NetworkVariableType Type => NetworkVariableType.Unknown;
3939

40+
internal string GetWritePermissionError()
41+
{
42+
return $"|Client-{m_NetworkManager.LocalClientId}|{m_NetworkBehaviour.name}|{Name}| Write permissions ({WritePerm}) for this client instance is not allowed!";
43+
}
44+
45+
internal void LogWritePermissionError()
46+
{
47+
Debug.LogError(GetWritePermissionError());
48+
}
49+
4050
private protected NetworkManager m_NetworkManager
4151
{
4252
get

‎com.unity.netcode.gameobjects/Runtime/NetworkVariable/Serialization/TypedSerializerImplementations.cs‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -458,7 +458,10 @@ public void Duplicate(in List<T> value, ref List<T> duplicatedValue)
458458
duplicatedValue.Clear();
459459
foreach (var item in value)
460460
{
461-
duplicatedValue.Add(item);
461+
// This handles the nested list scenario List<List<T>>
462+
T subValue = default;
463+
NetworkVariableSerialization<T>.Duplicate(item, ref subValue);
464+
duplicatedValue.Add(subValue);
462465
}
463466
}
464467
}
@@ -548,6 +551,9 @@ public void Duplicate(in HashSet<T> value, ref HashSet<T> duplicatedValue)
548551
duplicatedValue.Clear();
549552
foreach (var item in value)
550553
{
554+
// Handles nested HashSets
555+
T subValue = default;
556+
NetworkVariableSerialization<T>.Duplicate(item, ref subValue);
551557
duplicatedValue.Add(item);
552558
}
553559
}
@@ -641,7 +647,12 @@ public void Duplicate(in Dictionary<TKey, TVal> value, ref Dictionary<TKey, TVal
641647
duplicatedValue.Clear();
642648
foreach (var item in value)
643649
{
644-
duplicatedValue.Add(item.Key, item.Value);
650+
// Handles nested dictionaries
651+
TKey subKey = default;
652+
TVal subValue = default;
653+
NetworkVariableSerialization<TKey>.Duplicate(item.Key, ref subKey);
654+
NetworkVariableSerialization<TVal>.Duplicate(item.Value, ref subValue);
655+
duplicatedValue.Add(subKey, subValue);
645656
}
646657
}
647658
}

‎com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable.meta‎

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVarBufferCopyTest.cs renamed to com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVarBufferCopyTest.cs

File renamed without changes.

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVarBufferCopyTest.cs.meta renamed to com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVarBufferCopyTest.cs.meta

File renamed without changes.

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableAnticipationTests.cs renamed to com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableAnticipationTests.cs

File renamed without changes.

0 commit comments

Comments
 (0)