fix: NetworkVariables with NetworkVariableUpdateTraits can cause other NetworkVariables to drop changes (#3465)

This PR resolves an issue where a `NetworkBehaviour` with multiple
`NetworkVariables` could drop changes if one of the `NetworkVariables`
has unique `NetworkVariableUpdateTraits` set, is dirty, but is not ready
to send.

Based on user @khyperia's submission #3462.

## Changelog

- Fixed: issue where `NetworkVariable`s on a `NetworkBehaviour` could
fail to synchronize changes if one has `NetworkVariableUpdateTraits` set
and is dirty but is not ready to send.

## Testing and Documentation

- Includes integration test
`NetworkVariableTraitsTests.WhenNonTraitsIsDirtyButTraitsIsNotReadyToSend`.
- No documentation changes or additions were necessary.

<!-- Uncomment and mark items off with a * if this PR deprecates any
API:
### Deprecated API
- [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter
yyyy-mm-dd)` entry.
- [ ] An [api updater] was added.
- [ ] Deprecation of the API is explained in the CHANGELOG.
- [ ] The users can understand why this API was removed and what they
should use instead.
-->

## Backport

This required and up-port to v2.x (#3466)

---------

Co-authored-by: khyperia <[email protected]>

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -14,6 +14,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where `NetworkVariable`s on a `NetworkBehaviour` could fail to synchronize changes if one has `NetworkVariableUpdateTraits` set and is dirty but is not ready to send. (#3465)
 - Fixed issue where when a client changes ownership via RPC the `NetworkBehaviour.OnOwnershipChanged` can result in identical previous and current owner identifiers. (#3434)
 
 ### Changed

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -1002,8 +1002,8 @@ internal void NetworkVariableUpdate(ulong targetClientId, int behaviourIndex, bo
                         if (networkVariable.CanSend())
                         {
                             shouldSend = true;
+                            break;
                         }
-                        break;
                     }
                 }
 

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

@@ -8,6 +8,7 @@ namespace Unity.Netcode.RuntimeTests
     public class NetworkVariableTraitsComponent : NetworkBehaviour
     {
         public NetworkVariable<float> TheVariable = new NetworkVariable<float>();
+        internal NetworkVariable<float> AnotherVariable = new NetworkVariable<float>();
     }
 
     public class NetworkVariableTraitsTests : NetcodeIntegrationTest
@@ -52,9 +53,10 @@ public void WhenNewValueIsLessThanThreshold_VariableIsNotSerialized()
 
             TimeTravel(2, 120);
 
-            Assert.AreEqual(0.05f, serverComponent.TheVariable.Value); ;
-            Assert.AreEqual(0, testComponent.TheVariable.Value); ;
+            Assert.AreEqual(0.05f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0, testComponent.TheVariable.Value);
         }
+
         [Test]
         public void WhenNewValueIsGreaterThanThreshold_VariableIsSerialized()
         {
@@ -66,8 +68,8 @@ public void WhenNewValueIsGreaterThanThreshold_VariableIsSerialized()
 
             TimeTravel(2, 120);
 
-            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value); ;
-            Assert.AreEqual(0.15f, testComponent.TheVariable.Value); ;
+            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0.15f, testComponent.TheVariable.Value);
         }
 
         [Test]
@@ -83,13 +85,13 @@ public void WhenNewValueIsLessThanThresholdButMaxTimeHasPassed_VariableIsSeriali
 
             TimeTravel(1 / 60f * 119, 119);
 
-            Assert.AreEqual(0.05f, serverComponent.TheVariable.Value); ;
-            Assert.AreEqual(0, testComponent.TheVariable.Value); ;
+            Assert.AreEqual(0.05f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0, testComponent.TheVariable.Value);
 
             TimeTravel(1 / 60f * 4, 4);
 
-            Assert.AreEqual(0.05f, serverComponent.TheVariable.Value); ;
-            Assert.AreEqual(0.05f, testComponent.TheVariable.Value); ;
+            Assert.AreEqual(0.05f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0.05f, testComponent.TheVariable.Value);
         }
 
         [Test]
@@ -105,13 +107,13 @@ public void WhenNewValueIsGreaterThanThresholdButMinTimeHasNotPassed_VariableIsN
 
             TimeTravel(1 / 60f * 119, 119);
 
-            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value); ;
-            Assert.AreEqual(0, testComponent.TheVariable.Value); ;
+            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0, testComponent.TheVariable.Value);
 
             TimeTravel(1 / 60f * 4, 4);
 
-            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value); ;
-            Assert.AreEqual(0.15f, testComponent.TheVariable.Value); ;
+            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0.15f, testComponent.TheVariable.Value);
         }
 
         [Test]
@@ -126,13 +128,48 @@ public void WhenNoThresholdIsSetButMinTimeHasNotPassed_VariableIsNotSerialized()
 
             TimeTravel(1 / 60f * 119, 119);
 
-            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value); ;
-            Assert.AreEqual(0, testComponent.TheVariable.Value); ;
+            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0, testComponent.TheVariable.Value);
+
+            TimeTravel(1 / 60f * 4, 4);
+
+            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0.15f, testComponent.TheVariable.Value);
+        }
 
+        /// <summary>
+        /// Integration test to validate that a <see cref="NetworkVariable{T}"/> with <see cref="NetworkVariableUpdateTraits"/>
+        /// does not cause other <see cref="NetworkVariable{T}"/>s to miss an update when they are dirty but the one with
+        /// traits is not ready to send an update.
+        /// </summary>
+        [Test]
+        public void WhenNonTraitsIsDirtyButTraitsIsNotReadyToSend()
+        {
+            var serverComponent = GetServerComponent();
+            var testComponent = GetTestComponent();
+            serverComponent.TheVariable.SetUpdateTraits(new NetworkVariableUpdateTraits { MinSecondsBetweenUpdates = 2 });
+            serverComponent.TheVariable.LastUpdateSent = m_ServerNetworkManager.NetworkTimeSystem.LocalTime;
+
+            serverComponent.TheVariable.Value = 0.15f;
+            // Set the non-traits NetworkVariable value
+            serverComponent.AnotherVariable.Value = 1.0f;
+
+            TimeTravel(1 / 60f * 119, 119);
+            // We don't expect the traits NetworkVariable to update
+            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0, testComponent.TheVariable.Value);
+            // We should expect the non-traits NetworkVariable to update
+            Assert.AreEqual(1.0f, serverComponent.AnotherVariable.Value);
+            Assert.AreEqual(1.0f, testComponent.AnotherVariable.Value);
+
+            serverComponent.AnotherVariable.Value = 1.5f;
             TimeTravel(1 / 60f * 4, 4);
 
-            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value); ;
-            Assert.AreEqual(0.15f, testComponent.TheVariable.Value); ;
+            // We should expect both NetworkVariables to update
+            Assert.AreEqual(0.15f, serverComponent.TheVariable.Value);
+            Assert.AreEqual(0.15f, testComponent.TheVariable.Value);
+            Assert.AreEqual(1.5f, serverComponent.AnotherVariable.Value);
+            Assert.AreEqual(1.5f, testComponent.AnotherVariable.Value);
         }
     }
 }