fix: Fixed Owner-written NetworkVariable infinitely write themselves (#2128)

* fix: Fixed Owner-written NetworkVariable infinitely write themselves (#2109)
Fixed by doing the pre-write as a separate step

* nit PR review :-)

* Adding test

Author: jeffreyrainy

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -16,6 +16,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed Owner-written NetworkVariable infinitely write themselves (#2109)
+- Fixed NetworkList issue that showed when inserting at the very end of a NetworkList (#2099)
 - Fixed issue where a client owner of a `NetworkVariable` with both owner read and write permissions would not update the server side when changed. (#2097)
 - Fixed issue when attempting to spawn a parent `GameObject`, with `NetworkObject` component attached, that has one or more child `GameObject`s, that are inactive in the hierarchy, with `NetworkBehaviour` components it will no longer attempt to spawn the associated `NetworkBehaviour`(s) or invoke ownership changed notifications but will log a warning message. (#2096)
 - Fixed an issue where destroying a NetworkBehaviour would not deregister it from the parent NetworkObject, leading to exceptions when the parent was later destroyed. (#2091)
@@ -24,9 +26,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed issue where `NetworkAnimator` would not synchronize a looping animation for late joining clients if it was at the very end of its loop. (#2076)
 - Fixed issue where `NetworkAnimator` was not removing its subscription from `OnClientConnectedCallback` when despawned during the shutdown sequence. (#2074)
 - Fixed IsServer and IsClient being set to false before object despawn during the shutdown sequence. (#2074)
-- Fixed NetworkLists not populating on client. NetworkList now uses the most recent list as opposed to the list at the end of previous frame, when sending full updates to dynamically spawned NetworkObject. The difference in behaviour is required as scene management spawns those objects at a different time in the frame, relative to updates. (#2062)
 - Fixed NetworkList Value event on the server. PreviousValue is now set correctly when a new value is set through property setter. (#2067)
-- Fixed NetworkList issue that showed when inserting at the very end of a NetworkList (#2099)
+- Fixed NetworkLists not populating on client. NetworkList now uses the most recent list as opposed to the list at the end of previous frame, when sending full updates to dynamically spawned NetworkObject. The difference in behaviour is required as scene management spawns those objects at a different time in the frame, relative to updates. (#2062)
 
 ## [1.0.0] - 2022-06-27
 

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

@@ -587,14 +587,18 @@ internal void PostNetworkVariableWrite()
             MarkVariablesDirty(false);
         }
 
-        internal void VariableUpdate(ulong targetClientId)
+        internal void PreVariableUpdate()
         {
             if (!m_VarInit)
             {
                 InitializeVariables();
             }
 
             PreNetworkVariableWrite();
+        }
+
+        internal void VariableUpdate(ulong targetClientId)
+        {
             NetworkVariableUpdate(targetClientId, NetworkBehaviourId);
         }
 

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

@@ -30,6 +30,11 @@ internal void NetworkBehaviourUpdate(NetworkManager networkManager)
                 {
                     foreach (var dirtyObj in m_DirtyNetworkObjects)
                     {
+                        for (int k = 0; k < dirtyObj.ChildNetworkBehaviours.Count; k++)
+                        {
+                            dirtyObj.ChildNetworkBehaviours[k].PreVariableUpdate();
+                        }
+
                         for (int i = 0; i < networkManager.ConnectedClientsList.Count; i++)
                         {
                             var client = networkManager.ConnectedClientsList[i];
@@ -56,6 +61,10 @@ internal void NetworkBehaviourUpdate(NetworkManager networkManager)
                     {
                         if (sobj.IsOwner)
                         {
+                            for (int k = 0; k < sobj.ChildNetworkBehaviours.Count; k++)
+                            {
+                                sobj.ChildNetworkBehaviours[k].PreVariableUpdate();
+                            }
                             for (int k = 0; k < sobj.ChildNetworkBehaviours.Count; k++)
                             {
                                 sobj.ChildNetworkBehaviours[k].VariableUpdate(NetworkManager.ServerClientId);

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

@@ -0,0 +1,103 @@
+using System.Collections;
+using System.Collections.Generic;
+using NUnit.Framework;
+using UnityEngine;
+using UnityEngine.TestTools;
+using Unity.Netcode.TestHelpers.Runtime;
+
+namespace Unity.Netcode.RuntimeTests
+{
+    // This is a bit of a quirky test.
+    // Addresses MTT-4386 #2109
+    // Where the NetworkVariable updates would be repeated on some clients.
+    // The twist comes fom the updates needing to happens very specifically for the issue to repro in tests
+
+    public class OwnerModifiedObject : NetworkBehaviour, INetworkUpdateSystem
+    {
+        public NetworkList<int> MyNetworkList;
+
+        static internal int Updates = 0;
+
+        private void Awake()
+        {
+            MyNetworkList = new NetworkList<int>(new List<int>(), NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner);
+            MyNetworkList.OnListChanged += Changed;
+        }
+
+        public void Changed(NetworkListEvent<int> listEvent)
+        {
+            var expected = 0;
+            var listString = "";
+            foreach (var i in MyNetworkList)
+            {
+                Assert.AreEqual(i, expected);
+                expected++;
+                listString += i.ToString();
+            }
+            Debug.Log($"[{NetworkManager.LocalClientId}] Value changed to {listString}");
+            Updates++;
+        }
+
+        public bool AddValues;
+
+        public NetworkUpdateStage NetworkUpdateStageToCheck;
+
+        private int m_ValueToUpdate;
+
+        public void NetworkUpdate(NetworkUpdateStage updateStage)
+        {
+            if (updateStage == NetworkUpdateStageToCheck)
+            {
+                if (AddValues)
+                {
+                    MyNetworkList.Add(m_ValueToUpdate++);
+                    AddValues = false;
+                }
+            }
+        }
+
+        public override void OnDestroy()
+        {
+            NetworkUpdateLoop.UnregisterAllNetworkUpdates(this);
+            base.OnDestroy();
+        }
+
+        public void InitializeLastCient()
+        {
+            NetworkUpdateLoop.RegisterAllNetworkUpdates(this);
+        }
+    }
+
+    public class OwnerModifiedTests : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 2;
+
+        protected override void OnCreatePlayerPrefab()
+        {
+            m_PlayerPrefab.AddComponent<OwnerModifiedObject>();
+        }
+
+        [UnityTest]
+        public IEnumerator OwnerModifiedTest()
+        {
+            // We use this to assure we are the "last client" connected.
+            yield return CreateAndStartNewClient();
+            var ownerModLastClient = m_ClientNetworkManagers[2].LocalClient.PlayerObject.GetComponent<OwnerModifiedObject>();
+            ownerModLastClient.InitializeLastCient();
+
+            // Run through all update loops setting the value once every 5 frames
+            foreach (var updateLoopType in System.Enum.GetValues(typeof(NetworkUpdateStage)))
+            {
+                ownerModLastClient.NetworkUpdateStageToCheck = (NetworkUpdateStage)updateLoopType;
+                Debug.Log($"Testing Update Stage: {ownerModLastClient.NetworkUpdateStageToCheck}");
+                ownerModLastClient.AddValues = true;
+                yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ServerNetworkManager, 5);
+            }
+
+            yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ServerNetworkManager, 5);
+
+            // We'll have at least one update per stage per client, if all goes well.
+            Assert.True(OwnerModifiedObject.Updates > 20);
+        }
+    }
+}