fix: collections not detecting deltas backport to v1.x.x (#3005)

* fix

Fixing some of the collections issues.
Adding a unified permissions error.
Replacing throw exceptions with logging errors and returning.

* tesat

adjustments for replacing throw exception and added internal unified permissions write error.

* fix

Adjustments made due to other issues with direct assignment.

* fix

back port typo.

* style

removing using directive.

* 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

Adding missed change log entries.

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -8,10 +8,21 @@ Additional documentation and release notes are available at [Multiplayer Documen
 [Unreleased]
 ### 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. (#3005)
+
 ### Fixed
 
+- Fixed issue using collections within `NetworkVariable` where the collection would not detect changes to items or nested items. (#3005)
+- Fixed issue where `List`, `Dictionary`, and `HashSet` collections would not uniquely duplicate nested collections. (#3005)
+- Fixed Issue where a state with dual triggers, inbound and outbound, could cause a false layer to layer state transition message to be sent to non-authority `NetworkAnimator` instances and cause a warning message to be logged. (#2999)
+- Fixed issue where `FixedStringSerializer<T>` was using `NetworkVariableSerialization<byte>.AreEqual` to determine if two bytes were equal causes an exception to be thrown due to no byte serializer having been defined. (#2992)
+
 ### Changed
 
+- Changed permissions exception thrown in `NetworkList` to exiting early with a logged error that is now a unified permissions message within `NetworkVariableBase`. (#3005)
+- Changed permissions exception thrown in `NetworkVariable.Value` to exiting early with a logged error that is now a unified permissions message within `NetworkVariableBase`. (#3005)
+
+
 ## [1.10.0] - 2024-07-22
 
 ### Added

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

@@ -369,7 +369,8 @@ namespace Unity.Netcode
             // check write permissions
             if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return;
             }
 
             m_List.Add(item);
@@ -390,7 +391,8 @@ namespace Unity.Netcode
             // check write permissions
             if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return;
             }
 
             m_List.Clear();
@@ -416,7 +418,8 @@ namespace Unity.Netcode
             // check write permissions
             if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return false;
             }
 
             int index = m_List.IndexOf(item);
@@ -451,7 +454,8 @@ namespace Unity.Netcode
             // check write permissions
             if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return;
             }
 
             if (index < m_List.Length)
@@ -480,7 +484,8 @@ namespace Unity.Netcode
             // check write permissions
             if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
             {
-                throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
+                LogWritePermissionError();
+                return;
             }
 
             var value = m_List[index];
@@ -505,7 +510,8 @@ namespace Unity.Netcode
                 // check write permissions
                 if (!CanClientWrite(m_NetworkBehaviour.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

@@ -79,27 +79,59 @@ namespace Unity.Netcode
         /// <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_NetworkBehaviour && !CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
+                // Compare the Value being applied to the current value
+                if (!NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref value))
                 {
-                    throw new InvalidOperationException("Client is not allowed to write to this NetworkVariable");
+                    T previousValue = m_InternalValue;
+                    m_InternalValue = value;
+                    SetDirty(true);
+                    m_IsDisposed = false;
+                    OnValueChanged?.Invoke(previousValue, m_InternalValue);
                 }
-
-                Set(value);
-                m_IsDisposed = false;
             }
         }
 
+        /// <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();
+
+            // 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()
         {
             return ref m_InternalValue;
@@ -184,9 +216,8 @@ namespace Unity.Netcode
         private protected void Set(T value)
         {
             SetDirty(true);
-            T previousValue = m_InternalValue;
             m_InternalValue = value;
-            OnValueChanged?.Invoke(previousValue, m_InternalValue);
+            OnValueChanged?.Invoke(m_PreviousValue, m_InternalValue);
         }
 
         /// <summary>
@@ -205,20 +236,22 @@ namespace Unity.Netcode
         /// <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

@@ -32,21 +32,47 @@ namespace Unity.Netcode
         /// Maintains a link to the associated NetworkBehaviour
         /// </summary>
         private protected NetworkBehaviour m_NetworkBehaviour;
+        private NetworkManager m_InternalNetworkManager;
 
         public NetworkBehaviour GetBehaviour()
         {
             return m_NetworkBehaviour;
         }
 
+        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
+            {
+                if (m_InternalNetworkManager == null && m_NetworkBehaviour && m_NetworkBehaviour.NetworkObject?.NetworkManager)
+                {
+                    m_InternalNetworkManager = m_NetworkBehaviour.NetworkObject?.NetworkManager;
+                }
+                return m_InternalNetworkManager;
+            }
+        }
+
         /// <summary>
         /// Initializes the NetworkVariable
         /// </summary>
         /// <param name="networkBehaviour">The NetworkBehaviour the NetworkVariable belongs to</param>
         public void Initialize(NetworkBehaviour networkBehaviour)
         {
+            m_InternalNetworkManager = null;
             m_NetworkBehaviour = networkBehaviour;
-            if (m_NetworkBehaviour.NetworkManager)
+            if (m_NetworkBehaviour && m_NetworkBehaviour.NetworkObject?.NetworkManager)
             {
+                m_InternalNetworkManager = m_NetworkBehaviour.NetworkObject?.NetworkManager;
+
                 if (m_NetworkBehaviour.NetworkManager.NetworkTimeSystem != null)
                 {
                     UpdateLastSentTime();

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

@@ -349,7 +349,10 @@ namespace Unity.Netcode
             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);
             }
         }
     }
@@ -421,6 +424,9 @@ namespace Unity.Netcode
             duplicatedValue.Clear();
             foreach (var item in value)
             {
+                // Handles nested HashSets
+                T subValue = default;
+                NetworkVariableSerialization<T>.Duplicate(item, ref subValue);
                 duplicatedValue.Add(item);
             }
         }
@@ -497,7 +503,12 @@ namespace Unity.Netcode
             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);
             }
         }
     }

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableCollectionsTests.cs.meta

@@ -0,0 +1,11 @@
+fileFormatVersion: 2
+guid: 939ac41f36685f84e94a4b66ebbb6d8c
+MonoImporter:
+  externalObjects: {}
+  serializedVersion: 2
+  defaultReferences: []
+  executionOrder: 0
+  icon: {instanceID: 0}
+  userData: 
+  assetBundleName: 
+  assetBundleVariant: 

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

@@ -254,7 +254,8 @@ namespace Unity.Netcode.RuntimeTests
             var oldValue = testCompClient.ServerWritable_Position.Value;
             var newValue = oldValue + new Vector3(Random.Range(0, 100.0f), Random.Range(0, 100.0f), Random.Range(0, 100.0f));
 
-            Assert.That(() => testCompClient.ServerWritable_Position.Value = newValue, Throws.TypeOf<InvalidOperationException>());
+            LogAssert.Expect(LogType.Error, testCompClient.ServerWritable_Position.GetWritePermissionError());
+            testCompClient.ServerWritable_Position.Value = newValue;
             yield return WaitForPositionsAreEqual(testCompServer.ServerWritable_Position, oldValue);
 
             yield return WaitForServerWritableAreEqualOnAll();
@@ -283,7 +284,8 @@ namespace Unity.Netcode.RuntimeTests
             var oldValue = testCompServer.OwnerWritable_Position.Value;
             var newValue = oldValue + new Vector3(Random.Range(0, 100.0f), Random.Range(0, 100.0f), Random.Range(0, 100.0f));
 
-            Assert.That(() => testCompServer.OwnerWritable_Position.Value = newValue, Throws.TypeOf<InvalidOperationException>());
+            LogAssert.Expect(LogType.Error, testCompServer.OwnerWritable_Position.GetWritePermissionError());
+            testCompServer.OwnerWritable_Position.Value = newValue;
             yield return WaitForPositionsAreEqual(testCompServer.OwnerWritable_Position, oldValue);
 
             yield return WaitForOwnerWritableAreEqualOnAll();
@@ -589,8 +591,8 @@ namespace Unity.Netcode.RuntimeTests
         {
             InitializeServerAndClients(useHost);
 
-            // client must not be allowed to write to a server auth variable
-            Assert.Throws<InvalidOperationException>(() => m_Player1OnClient1.TheScalar.Value = k_TestVal1);
+            LogAssert.Expect(LogType.Error, m_Player1OnClient1.TheScalar.GetWritePermissionError());
+            m_Player1OnClient1.TheScalar.Value = k_TestVal1;
         }
 
         /// <summary>

com.unity.netcode.gameobjects/Tests/Runtime/OwnerPermissionTests.cs

@@ -1,4 +1,3 @@
-using System;
 using System.Collections;
 using System.Collections.Generic;
 using Unity.Netcode.TestHelpers.Runtime;
@@ -130,72 +129,44 @@ namespace Unity.Netcode.RuntimeTests
                 for (var clientWriting = 0; clientWriting < 3; clientWriting++)
                 {
                     // ==== Server-writable NetworkVariable ====
-                    var gotException = false;
-                    Debug.Log($"Writing to server-write variable on object {objectIndex} on client {clientWriting}");
+                    VerboseDebug($"Writing to server-write variable on object {objectIndex} on client {clientWriting}");
 
-                    try
+                    nextValueToWrite++;
+                    if (clientWriting != serverIndex)
                     {
-                        nextValueToWrite++;
-                        OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkVariableServer.Value = nextValueToWrite;
+                        LogAssert.Expect(LogType.Error, OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkVariableServer.GetWritePermissionError());
                     }
-                    catch (Exception)
-                    {
-                        gotException = true;
-                    }
-
-                    // Verify server-owned netvar can only be written by server
-                    Debug.Assert(gotException == (clientWriting != serverIndex));
+                    OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkVariableServer.Value = nextValueToWrite;
 
                     // ==== Owner-writable NetworkVariable ====
-                    gotException = false;
-                    Debug.Log($"Writing to owner-write variable on object {objectIndex} on client {clientWriting}");
+                    VerboseDebug($"Writing to owner-write variable on object {objectIndex} on client {clientWriting}");
 
-                    try
+                    nextValueToWrite++;
+                    if (clientWriting != objectIndex)
                     {
-                        nextValueToWrite++;
-                        OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkVariableOwner.Value = nextValueToWrite;
+                        LogAssert.Expect(LogType.Error, OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkVariableOwner.GetWritePermissionError());
                     }
-                    catch (Exception)
-                    {
-                        gotException = true;
-                    }
-
-                    // Verify client-owned netvar can only be written by owner
-                    Debug.Assert(gotException == (clientWriting != objectIndex));
+                    OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkVariableOwner.Value = nextValueToWrite;
 
                     // ==== Server-writable NetworkList ====
-                    gotException = false;
-                    Debug.Log($"Writing to server-write list on object {objectIndex} on client {clientWriting}");
+                    VerboseDebug($"Writing to [Add] server-write NetworkList on object {objectIndex} on client {clientWriting}");
 
-                    try
+                    nextValueToWrite++;
+                    if (clientWriting != serverIndex)
                     {
-                        nextValueToWrite++;
-                        OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkListServer.Add(nextValueToWrite);
+                        LogAssert.Expect(LogType.Error, OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkListServer.GetWritePermissionError());
                     }
-                    catch (Exception)
-                    {
-                        gotException = true;
-                    }
-
-                    // Verify server-owned networkList can only be written by server
-                    Debug.Assert(gotException == (clientWriting != serverIndex));
+                    OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkListServer.Add(nextValueToWrite);
 
                     // ==== Owner-writable NetworkList ====
-                    gotException = false;
-                    Debug.Log($"Writing to owner-write list on object {objectIndex} on client {clientWriting}");
+                    VerboseDebug($"Writing to [Add] owner-write NetworkList on object {objectIndex} on client {clientWriting}");
 
-                    try
+                    nextValueToWrite++;
+                    if (clientWriting != objectIndex)
                     {
-                        nextValueToWrite++;
-                        OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkListOwner.Add(nextValueToWrite);
+                        LogAssert.Expect(LogType.Error, OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkListOwner.GetWritePermissionError());
                     }
-                    catch (Exception)
-                    {
-                        gotException = true;
-                    }
-
-                    // Verify client-owned networkList can only be written by owner
-                    Debug.Assert(gotException == (clientWriting != objectIndex));
+                    OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkListOwner.Add(nextValueToWrite);
 
                     yield return WaitForTicks(m_ServerNetworkManager, 5);
                     yield return WaitForTicks(m_ClientNetworkManagers[0], 5);