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

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -10,13 +10,20 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Added
 
+- 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)
+
 ### Fixed
 
+- Fixed issue using collections within `NetworkVariable` where the collection would not detect changes to items or nested items. (#3004)
+- Fixed issue where `List`, `Dictionary`, and `HashSet` collections would not uniquely duplicate nested collections. (#3004)
 - 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)
 - Fixed issue where `ProxyRpcTargetGroup` could attempt to send a message if there were no targets to send to. (#3000)
 
 ### Changed
 
+- Changed permissions exception thrown in `NetworkList` to exiting early with a logged error that is now a unified permissions message within `NetworkVariableBase`. (#3004)
+- Changed permissions exception thrown in `NetworkVariable.Value` to exiting early with a logged error that is now a unified permissions message within `NetworkVariableBase`. (#3004)
+
 
 ## [2.0.0-pre.3] - 2024-07-23
 

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

@@ -25,8 +25,6 @@ internal struct NetworkVariableDeltaMessage : INetworkMessage
 
         private const string k_Name = "NetworkVariableDeltaMessage";
 
-        // DANGO-TODO: Made some modifications here that overlap/won't play nice with EnsureNetworkVariableLenghtSafety.
-        // Worth either merging or more cleanly separating these codepaths.
         public void Serialize(FastBufferWriter writer, int targetVersion)
         {
             if (!writer.TryBeginWrite(FastBufferWriter.GetWriteSize(NetworkObjectId) + FastBufferWriter.GetWriteSize(NetworkBehaviourIndex)))
@@ -126,10 +124,6 @@ public void Serialize(FastBufferWriter writer, int targetVersion)
                     }
                     else
                     {
-                        // DANGO-TODO:
-                        // Complex types with custom type serialization (either registered custom types or INetworkSerializable implementations) will be problematic
-                        // Non-complex types always provide a full state update per delta
-                        // DANGO-TODO: Add NetworkListEvent<T>.EventType awareness to the cloud-state server
                         if (networkManager.DistributedAuthorityMode)
                         {
                             var size_marker = writer.Position;
@@ -167,8 +161,6 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
             return true;
         }
 
-        // DANGO-TODO: Made some modifications here that overlap/won't play nice with EnsureNetworkVariableLenghtSafety.
-        // Worth either merging or more cleanly separating these codepaths.
         public void Handle(ref NetworkContext context)
         {
             var networkManager = (NetworkManager)context.SystemOwner;

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

@@ -393,7 +393,8 @@ public void Add(T item)
             // check write permissions
             if (!CanClientWrite(m_NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return;
             }
 
             m_List.Add(item);
@@ -414,7 +415,8 @@ public void Clear()
             // check write permissions
             if (!CanClientWrite(m_NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return;
             }
 
             m_List.Clear();
@@ -440,7 +442,8 @@ public bool Remove(T item)
             // check write permissions
             if (!CanClientWrite(m_NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return false;
             }
 
             int index = m_List.IndexOf(item);
@@ -475,7 +478,8 @@ public void Insert(int index, T item)
             // check write permissions
             if (!CanClientWrite(m_NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return;
             }
 
             if (index < m_List.Length)
@@ -520,6 +524,8 @@ public void RemoveAt(int index)
             HandleAddListEvent(listEvent);
         }
 
+
+
         /// <inheritdoc />
         public T this[int index]
         {
@@ -529,7 +535,8 @@ public T this[int index]
                 // check write permissions
                 if (!CanClientWrite(m_NetworkManager.LocalClientId))
                 {
-                    throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                    LogWritePermissionError();
+                    return;
                 }
 
                 var previousValue = m_List[index];

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

@@ -95,25 +95,57 @@ public void Reset(T value = default)
         /// <summary>
         /// The value of the NetworkVariable container
         /// </summary>
+        /// <remarks>
+        /// When assigning collections to <see cref="Value"/>, unless it is a completely new collection this will not
+        /// detect any deltas with most managed collection classes since assignment of one collection value to another
+        /// is actually just a reference to the collection itself. <br />
+        /// To detect deltas in a collection, you should invoke <see cref="CheckDirtyState"/> after making modifications to the collection.
+        /// </remarks>
         public virtual T Value
         {
             get => m_InternalValue;
             set
             {
-                // Compare bitwise
-                if (NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref value))
+                if (m_NetworkManager && !CanClientWrite(m_NetworkManager.LocalClientId))
                 {
+                    LogWritePermissionError();
                     return;
                 }
 
-                if (m_NetworkManager && !CanClientWrite(m_NetworkManager.LocalClientId))
+                // Compare the Value being applied to the current value
+                if (!NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref value))
                 {
-                    throw new InvalidOperationException($"[Client-{m_NetworkManager.LocalClientId}][{m_NetworkBehaviour.name}][{Name}] Write permissions ({WritePerm}) for this client instance is not allowed!");
+                    T previousValue = m_InternalValue;
+                    m_InternalValue = value;
+                    SetDirty(true);
+                    m_IsDisposed = false;
+                    OnValueChanged?.Invoke(previousValue, m_InternalValue);
                 }
+            }
+        }
+
+        /// <summary>
+        /// Invoke this method to check if a collection's items are dirty.
+        /// The default behavior is to exit early if the <see cref="NetworkVariable{T}"/> is already dirty.
+        /// </summary>
+        /// <param name="forceCheck"> when true, this check will force a full item collection check even if the NetworkVariable is already dirty</param>
+        /// <remarks>
+        /// 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 />
+        /// 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 />
+        /// </remarks>
+        public bool CheckDirtyState(bool forceCheck = false)
+        {
+            var isDirty = base.IsDirty();
 
-                Set(value);
+            // Compare the previous with the current if not dirty or forcing a check.
+            if ((!isDirty || forceCheck) && !NetworkVariableSerialization<T>.AreEqual(ref m_PreviousValue, ref m_InternalValue))
+            {
+                SetDirty(true);
+                OnValueChanged?.Invoke(m_PreviousValue, m_InternalValue);
                 m_IsDisposed = false;
+                isDirty = true;
             }
+            return isDirty;
         }
 
         internal ref T RefValue()
@@ -194,19 +226,6 @@ public override void ResetDirty()
             base.ResetDirty();
         }
 
-        /// <summary>
-        /// Sets the <see cref="Value"/>, marks the <see cref="NetworkVariable{T}"/> dirty, and invokes the <see cref="OnValueChanged"/> callback
-        /// if there are subscribers to that event.
-        /// </summary>
-        /// <param name="value">the new value of type `T` to be set/></param>
-        private protected void Set(T value)
-        {
-            SetDirty(true);
-            T previousValue = m_InternalValue;
-            m_InternalValue = value;
-            OnValueChanged?.Invoke(previousValue, m_InternalValue);
-        }
-
         /// <summary>
         /// Writes the variable to the writer
         /// </summary>
@@ -223,20 +242,22 @@ public override void WriteDelta(FastBufferWriter writer)
         /// <param name="keepDirtyDelta">Whether or not the container should keep the dirty delta, or mark the delta as consumed</param>
         public override void ReadDelta(FastBufferReader reader, bool keepDirtyDelta)
         {
+            // In order to get managed collections to properly have a previous and current value, we have to
+            // duplicate the collection at this point before making any modifications to the current.
+            m_HasPreviousValue = true;
+            NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_PreviousValue);
+            NetworkVariableSerialization<T>.ReadDelta(reader, ref m_InternalValue);
+
             // todo:
             // keepDirtyDelta marks a variable received as dirty and causes the server to send the value to clients
             // In a prefect world, whether a variable was A) modified locally or B) received and needs retransmit
             // would be stored in different fields
-
-            T previousValue = m_InternalValue;
-            NetworkVariableSerialization<T>.ReadDelta(reader, ref m_InternalValue);
-
             if (keepDirtyDelta)
             {
                 SetDirty(true);
             }
 
-            OnValueChanged?.Invoke(previousValue, m_InternalValue);
+            OnValueChanged?.Invoke(m_PreviousValue, m_InternalValue);
         }
 
         /// <inheritdoc />

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

@@ -37,6 +37,16 @@ public abstract class NetworkVariableBase : IDisposable
 
         internal virtual NetworkVariableType Type => NetworkVariableType.Unknown;
 
+        internal string GetWritePermissionError()
+        {
+            return $"|Client-{m_NetworkManager.LocalClientId}|{m_NetworkBehaviour.name}|{Name}| Write permissions ({WritePerm}) for this client instance is not allowed!";
+        }
+
+        internal void LogWritePermissionError()
+        {
+            Debug.LogError(GetWritePermissionError());
+        }
+
         private protected NetworkManager m_NetworkManager
         {
             get

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

@@ -458,7 +458,10 @@ public void Duplicate(in List<T> value, ref List<T> duplicatedValue)
             duplicatedValue.Clear();
             foreach (var item in value)
             {
-                duplicatedValue.Add(item);
+                // This handles the nested list scenario List<List<T>>
+                T subValue = default;
+                NetworkVariableSerialization<T>.Duplicate(item, ref subValue);
+                duplicatedValue.Add(subValue);
             }
         }
     }
@@ -548,6 +551,9 @@ public void Duplicate(in HashSet<T> value, ref HashSet<T> duplicatedValue)
             duplicatedValue.Clear();
             foreach (var item in value)
             {
+                // Handles nested HashSets
+                T subValue = default;
+                NetworkVariableSerialization<T>.Duplicate(item, ref subValue);
                 duplicatedValue.Add(item);
             }
         }
@@ -641,7 +647,12 @@ public void Duplicate(in Dictionary<TKey, TVal> value, ref Dictionary<TKey, TVal
             duplicatedValue.Clear();
             foreach (var item in value)
             {
-                duplicatedValue.Add(item.Key, item.Value);
+                // Handles nested dictionaries
+                TKey subKey = default;
+                TVal subValue = default;
+                NetworkVariableSerialization<TKey>.Duplicate(item.Key, ref subKey);
+                NetworkVariableSerialization<TVal>.Duplicate(item.Value, ref subValue);
+                duplicatedValue.Add(subKey, subValue);
             }
         }
     }