Merge branch 'develop' into automated-project-builders-backport

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -17,6 +17,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed ensuring OnValueChanged callback is still triggered on the authority when a collection changes and then reverts to the previous value in the same frame. (#3544)
 - Fixed `NullReferenceException` on `NetworkList` when used without a NetworkManager in scene. (#3502)
 - Fixed inconsistencies in the `OnSceneEvent` callback. (#3487)
 - Fixed issue where `NetworkClient` could persist some settings if re-using the same `NetworkManager` instance. (#3494)

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

@@ -57,7 +57,7 @@ public override void OnInitialize()
             base.OnInitialize();
 
             m_HasPreviousValue = true;
-            NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_InternalOriginalValue);
+            NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_LastInternalValue);
             NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_PreviousValue);
         }
 
@@ -73,7 +73,7 @@ public NetworkVariable(T value = default,
             : base(readPerm, writePerm)
         {
             m_InternalValue = value;
-            m_InternalOriginalValue = default;
+            m_LastInternalValue = default;
             // Since we start with IsDirty = true, this doesn't need to be duplicated
             // right away. It won't get read until after ResetDirty() is called, and
             // the duplicate will be made there. Avoiding calling
@@ -92,25 +92,45 @@ public void Reset(T value = default)
             if (m_NetworkBehaviour == null || m_NetworkBehaviour != null && !m_NetworkBehaviour.NetworkObject.IsSpawned)
             {
                 m_InternalValue = value;
-                NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_InternalOriginalValue);
+                NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_LastInternalValue);
                 m_PreviousValue = default;
             }
         }
 
         /// <summary>
-        /// The internal value of the NetworkVariable
+        /// The current internal value of the NetworkVariable.
         /// </summary>
+        /// <remarks>
+        /// When using collections, this InternalValue can be updated directly without going through the <see cref="NetworkVariable{T}.Value"/> setter.
+        /// </remarks>
         [SerializeField]
         private protected T m_InternalValue;
 
-        // The introduction of standard .NET collections caused an issue with permissions since there is no way to detect changes in the
-        // collection without doing a full comparison. While this approach does consume more memory per collection instance, it is the
-        // lowest risk approach to resolving the issue where a client with no write permissions could make changes to a collection locally
-        // which can cause a myriad of issues.
-        private protected T m_InternalOriginalValue;
+        /// <summary>
+        /// The last valid/authorized value of the network variable.
+        /// </summary>
+        /// <remarks>
+        /// The introduction of standard .NET collections caused an issue with permissions since there is no way to detect changes in the
+        /// collection without doing a full comparison. While this approach does consume more memory per collection instance, it is the
+        /// lowest risk approach to resolving the issue where a client with no write permissions could make changes to a collection locally
+        /// which can cause a myriad of issues.
+        /// </remarks>
+        private protected T m_LastInternalValue;
 
+        /// <summary>
+        /// The most recent value that was synchronized over the network.
+        /// Synchronized over the network at the end of the frame in which the <see cref="NetworkVariable{T}"/> was marked dirty.
+        /// </summary>
+        /// <remarks>
+        /// Only contains the value synchronized over the network at the end of the last frame.
+        /// All in-between changes on the authority are tracked by <see cref="m_LastInternalValue"/>.
+        /// </remarks>
         private protected T m_PreviousValue;
 
+        /// <summary>
+        /// Whether this network variable has had changes synchronized over the network.
+        /// Indicates whether <see cref="m_PreviousValue"/> is populated and valid.
+        /// </summary>
         private bool m_HasPreviousValue;
         private bool m_IsDisposed;
 
@@ -139,7 +159,7 @@ public virtual T Value
                 {
                     T previousValue = m_InternalValue;
                     m_InternalValue = value;
-                    NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_InternalOriginalValue);
+                    NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_LastInternalValue);
                     SetDirty(true);
                     m_IsDisposed = false;
                     OnValueChanged?.Invoke(previousValue, m_InternalValue);
@@ -165,20 +185,21 @@ public bool CheckDirtyState(bool forceCheck = false)
             if (CannotWrite)
             {
                 // If modifications are detected, then revert back to the last known current value
-                if (!NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref m_InternalOriginalValue))
+                if (!NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref m_LastInternalValue))
                 {
-                    NetworkVariableSerialization<T>.Duplicate(m_InternalOriginalValue, ref m_InternalValue);
+                    NetworkVariableSerialization<T>.Duplicate(m_LastInternalValue, ref m_InternalValue);
                 }
                 return false;
             }
 
-            // 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))
+            // Compare the last internal value with the current value if not dirty or forcing a check.
+            if ((!isDirty || forceCheck) && !NetworkVariableSerialization<T>.AreEqual(ref m_LastInternalValue, ref m_InternalValue))
             {
                 SetDirty(true);
-                OnValueChanged?.Invoke(m_PreviousValue, m_InternalValue);
+                OnValueChanged?.Invoke(m_LastInternalValue, m_InternalValue);
                 m_IsDisposed = false;
                 isDirty = true;
+                NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_LastInternalValue);
             }
             return isDirty;
         }
@@ -211,11 +232,11 @@ public override void Dispose()
 
             m_InternalValue = default;
 
-            if (m_InternalOriginalValue is IDisposable internalOriginalValueDisposable)
+            if (m_LastInternalValue is IDisposable internalOriginalValueDisposable)
             {
                 internalOriginalValueDisposable.Dispose();
             }
-            m_InternalOriginalValue = default;
+            m_LastInternalValue = default;
 
             if (m_HasPreviousValue && m_PreviousValue is IDisposable previousValueDisposable)
             {
@@ -242,9 +263,9 @@ public override bool IsDirty()
         {
             // If the client does not have write permissions but the internal value is determined to be locally modified and we are applying updates, then we should revert
             // to the original collection value prior to applying updates (primarily for collections).
-            if (!NetworkUpdaterCheck && CannotWrite && !NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref m_InternalOriginalValue))
+            if (!NetworkUpdaterCheck && CannotWrite && !NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref m_LastInternalValue))
             {
-                NetworkVariableSerialization<T>.Duplicate(m_InternalOriginalValue, ref m_InternalValue);
+                NetworkVariableSerialization<T>.Duplicate(m_LastInternalValue, ref m_InternalValue);
                 return true;
             }
 
@@ -283,7 +304,7 @@ public override void ResetDirty()
                 m_HasPreviousValue = true;
                 NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_PreviousValue);
                 // Once updated, assure the original current value is updated for future comparison purposes
-                NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_InternalOriginalValue);
+                NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_LastInternalValue);
             }
             base.ResetDirty();
         }
@@ -318,9 +339,9 @@ public override void ReadDelta(FastBufferReader reader, bool keepDirtyDelta)
         {
             // If the client does not have write permissions but the internal value is determined to be locally modified and we are applying updates, then we should revert
             // to the original collection value prior to applying updates (primarily for collections).
-            if (CannotWrite && !NetworkVariableSerialization<T>.AreEqual(ref m_InternalOriginalValue, ref m_InternalValue))
+            if (CannotWrite && !NetworkVariableSerialization<T>.AreEqual(ref m_LastInternalValue, ref m_InternalValue))
             {
-                NetworkVariableSerialization<T>.Duplicate(m_InternalOriginalValue, ref m_InternalValue);
+                NetworkVariableSerialization<T>.Duplicate(m_LastInternalValue, ref m_InternalValue);
             }
 
             NetworkVariableSerialization<T>.ReadDelta(reader, ref m_InternalValue);
@@ -351,17 +372,17 @@ internal override void PostDeltaRead()
             m_HasPreviousValue = true;
             NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_PreviousValue);
             // Once updated, assure the original current value is updated for future comparison purposes
-            NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_InternalOriginalValue);
+            NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_LastInternalValue);
         }
 
         /// <inheritdoc />
         public override void ReadField(FastBufferReader reader)
         {
             // If the client does not have write permissions but the internal value is determined to be locally modified and we are applying updates, then we should revert
             // to the original collection value prior to applying updates (primarily for collections).
-            if (CannotWrite && !NetworkVariableSerialization<T>.AreEqual(ref m_InternalOriginalValue, ref m_InternalValue))
+            if (CannotWrite && !NetworkVariableSerialization<T>.AreEqual(ref m_LastInternalValue, ref m_InternalValue))
             {
-                NetworkVariableSerialization<T>.Duplicate(m_InternalOriginalValue, ref m_InternalValue);
+                NetworkVariableSerialization<T>.Duplicate(m_LastInternalValue, ref m_InternalValue);
             }
 
             NetworkVariableSerialization<T>.Read(reader, ref m_InternalValue);
@@ -373,7 +394,7 @@ public override void ReadField(FastBufferReader reader)
             NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_PreviousValue);
 
             // Once updated, assure the original current value is updated for future comparison purposes
-            NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_InternalOriginalValue);
+            NetworkVariableSerialization<T>.Duplicate(m_InternalValue, ref m_LastInternalValue);
         }
 
         /// <inheritdoc />