fix: NetworkVariable clearing dirty flags after OnNetworkSpawn (#3779)

NoelStephensUnity · web-flow · commit 710a63a594af · 2025-11-14T09:55:10.000-06:00
* fix

This includes a fix for the forum reported issue where spawning with ownership in a distributed authority topology where the owner is not the spawn authority, the owner/authority sets a NetworkVariable during OnNetworkSpawn, then the changes made would not be synchronized.

* test

This adds an integration test to validate the fix for this PR.

* update

removing an IDE injected namespace that is never usedd.

* fix: second part

This migrates the NetworkList specific script from NetworkBehaviour.InternalOnNetworkSpawn into NetworkList itself.

It also provides (currently) two internal virtual methods, NetworkVariableBase.OnSpawned and NetworkVariableBase.OnPreDespawn, to provide a means for NetworkList to handle cleaning its dirty state up after the instance with write permissions has finished running through the spawn stages (pre-spawn, spawning, post-spawn, gets invoked).

* test

Adding some debug information to NetworkShowHideTests.
Replacing m_ServerNetworkManager in OwnerModifiedTests with GetAuthorityNetworkManager() to become CMB service testing compatible.

* test - fix

Fixing the race condition test instability when running OwnershipPermissionsTests against CMB service until the order in which a message is received will reflect the order in which it is sent relative to messages received prior to and after the given message.

* style

further clarifying the change made here.

* test

Some more instability related fixes.
diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
@@ -797,17 +797,6 @@ internal void InternalOnNetworkSpawn()
             {
                 Debug.LogException(e);
             }
-
-            // Initialize again in case the user's OnNetworkSpawn changed something
-            InitializeVariables();
-
-            if (m_NetworkObject.HasAuthority)
-            {
-                // Since we just spawned the object and since user code might have modified their NetworkVariable, esp.
-                // NetworkList, we need to mark the object as free of updates.
-                // This should happen for all objects on the machine triggering the spawn.
-                PostNetworkVariableWrite(true);
-            }
         }
 
         internal void NetworkPostSpawn()
@@ -821,6 +810,13 @@ internal void NetworkPostSpawn()
             {
                 Debug.LogException(e);
             }
+
+            // Let each NetworkVariableBase derived instance know that
+            // all spawn related methods have been invoked.
+            for (int i = 0; i < NetworkVariableFields.Count; i++)
+            {
+                NetworkVariableFields[i].OnSpawned();
+            }
         }
 
         internal void NetworkSessionSynchronized()
@@ -858,6 +854,13 @@ internal void InternalOnNetworkPreDespawn()
             {
                 Debug.LogException(e);
             }
+
+            // Let each NetworkVariableBase derived instance know that
+            // all spawn related methods have been invoked.
+            for (int i = 0; i < NetworkVariableFields.Count; i++)
+            {
+                NetworkVariableFields[i].OnPreDespawn();
+            }
         }
 
         internal void InternalOnNetworkDespawn()
diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs
@@ -58,6 +58,21 @@ public NetworkList(IEnumerable<T> values = default,
             Dispose();
         }
 
+        internal override void OnSpawned()
+        {
+            // If we are dirty and have write permissions by the time the NetworkObject
+            // is finished spawning (same frame), then go ahead and reset the dirty related
+            // properties for NetworkList in the event user script has made changes when
+            // spawning to prevent duplicate entries.
+            if (IsDirty() && CanSend())
+            {
+                UpdateLastSentTime();
+                ResetDirty();
+                SetDirty(false);
+            }
+            base.OnSpawned();
+        }
+
         /// <inheritdoc cref="NetworkVariable{T}.ResetDirty"/>
         public override void ResetDirty()
         {
diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs
@@ -140,6 +140,28 @@ public void Initialize(NetworkBehaviour networkBehaviour)
             }
         }
 
+        /// TODO-API: After further vetting and alignment on these, we might make them part of the public API.
+        /// Could actually be like an interface that gets automatically registered for these kinds of notifications
+        /// without having to be a NetworkBehaviour.
+        #region OnSpawn and OnPreDespawn (ETC)
+
+        /// <summary>
+        /// Invoked after the associated <see cref="NetworkBehaviour.OnNetworkPostSpawn"/> has been invoked.
+        /// </summary>
+        internal virtual void OnSpawned()
+        {
+
+        }
+
+        /// <summary>
+        /// Invoked after the associated <see cref="NetworkBehaviour.OnNetworkPreDespawn"/> has been invoked.
+        /// </summary>
+        internal virtual void OnPreDespawn()
+        {
+
+        }
+        #endregion
+
         /// <summary>
         /// Deinitialize is invoked when a NetworkObject is despawned.
         /// This allows for a recyled NetworkObject (in-scene or pooled)
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs
@@ -118,7 +118,7 @@ private bool ResetAllStates()
                 else
                 {
                     var attachable = networkManager.SpawnManager.SpawnedObjects[currentAttachableRoot.NetworkObjectId].GetComponentInChildren<TestAttachable>();
-                    attachable.ResetStates();
+                    attachable?.ResetStates();
                 }
 
                 // Target
@@ -129,7 +129,7 @@ private bool ResetAllStates()
                 else
                 {
                     var node = networkManager.SpawnManager.SpawnedObjects[m_TargetInstance.NetworkObjectId].GetComponentInChildren<TestNode>();
-                    node.ResetStates();
+                    node?.ResetStates();
                 }
 
                 // Target B
@@ -140,7 +140,7 @@ private bool ResetAllStates()
                 else
                 {
                     var node = networkManager.SpawnManager.SpawnedObjects[m_TargetInstanceB.NetworkObjectId].GetComponentInChildren<TestNode>();
-                    node.ResetStates();
+                    node?.ResetStates();
                 }
             }
             return m_ErrorLog.Length == 0;
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs
@@ -67,7 +67,7 @@ private bool WaitForOneClientToBeApproved(OwnershipPermissionsTestHelper[] clien
                 {
                     approvedClients++;
                 }
-                else if (helper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.RequestInProgress)
+                else if (helper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.RequestInProgress || helper.OwnershipRequestResponseStatus == NetworkObject.OwnershipRequestResponseStatus.Denied)
                 {
                     requestInProgressClients++;
                 }
@@ -279,8 +279,16 @@ public IEnumerator ValidateOwnershipPermissionsTest()
             var fourthInstance = fourthClient.SpawnManager.SpawnedObjects[networkObjectId];
             var fourthInstanceHelper = fourthInstance.GetComponent<OwnershipPermissionsTestHelper>();
 
+            // Mock race condition scenario where the second instance request arrives before the rest of the requests.
+            // This could be on the same frame all requests are received or where they stagger over several frames.
+            // Until we resolve the CMB service issue where the order in which messages are received does not always
+            // reflect the order in which they are forwarded to their destination (relative to other messages received from
+            // other clients).
+            firstInstanceHelper.OnlyAllowTargetClientId = true;
+            firstInstanceHelper.ClientToAllowOwnership = secondClient.LocalClientId;
+
             // Send out a request from three clients at the same time
-            // The first one sent (and received for this test) gets ownership
+            // The first one received gets ownership
             requestStatus = secondInstance.RequestOwnership();
             Assert.True(requestStatus == NetworkObject.OwnershipRequestStatus.RequestSent, $"Client-{secondClient.LocalClientId} was unable to send a request for ownership because: {requestStatus}!");
             requestStatus = thirdInstance.RequestOwnership();
@@ -291,7 +299,7 @@ public IEnumerator ValidateOwnershipPermissionsTest()
             // The 2nd and 3rd client should be denied and the 4th client should be approved
             yield return WaitForConditionOrTimeOut(() => WaitForOneClientToBeApproved(new[] { secondInstanceHelper, thirdInstanceHelper, fourthInstanceHelper }));
             AssertOnTimeout("[Targeted Owner] A client received an incorrect response. " +
-                            $"Expected one client to have {NetworkObject.OwnershipRequestResponseStatus.Approved} and the others to have {NetworkObject.OwnershipRequestResponseStatus.RequestInProgress}!."
+                            $"Expected one client to have {NetworkObject.OwnershipRequestResponseStatus.Approved} and the others to have {NetworkObject.OwnershipRequestResponseStatus.RequestInProgress} or {NetworkObject.OwnershipRequestResponseStatus.Denied}!."
                             + $"\n Client-{fourthClient.LocalClientId}: has {fourthInstanceHelper.OwnershipRequestResponseStatus}!"
                             + $"\n Client-{thirdClient.LocalClientId}: has {thirdInstanceHelper.OwnershipRequestResponseStatus}!"
                             + $"\n Client-{secondClient.LocalClientId}: has {secondInstanceHelper.OwnershipRequestResponseStatus}!");
@@ -305,6 +313,9 @@ public IEnumerator ValidateOwnershipPermissionsTest()
             yield return WaitForConditionOrTimeOut(ValidatePermissionsOnAllClients);
             AssertOnTimeout($"[Multiple request race condition][Permissions Mismatch] {secondInstance.name}");
 
+            // Reset this value once this part of the test is complete
+            firstInstanceHelper.OnlyAllowTargetClientId = false;
+
             ///////////////////////////////////////////////
             // Test for targeted ownership request:
             ///////////////////////////////////////////////
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs
@@ -31,6 +31,8 @@ public static NetworkObject GetNetworkObjectById(ulong networkObjectId)
 
         public override void OnNetworkSpawn()
         {
+            MyNetworkVariable.OnValueChanged += Changed;
+            MyOwnerReadNetworkVariable.OnValueChanged += OwnerReadChanged;
             if (NetworkManager.LocalClientId == ClientIdToTarget)
             {
                 ClientTargetedNetworkObjects.Add(this);
@@ -42,7 +44,7 @@ public override void OnNetworkSpawn()
             }
             else
             {
-                Debug.Assert(MyListSetOnSpawn.Count == 1);
+                Debug.Assert(MyListSetOnSpawn.Count == 1, $"[Session Authority][Client-{NetworkManager.LocalClientId}][{name}] Count = {MyListSetOnSpawn.Count} when expecting only 1!");
                 Debug.Assert(MyListSetOnSpawn[0] == 45);
             }
 
@@ -53,34 +55,23 @@ public override void OnNetworkSpawn()
 
         public override void OnNetworkDespawn()
         {
+            MyNetworkVariable.OnValueChanged -= Changed;
+            MyOwnerReadNetworkVariable.OnValueChanged -= OwnerReadChanged;
             if (ClientTargetedNetworkObjects.Contains(this))
             {
                 ClientTargetedNetworkObjects.Remove(this);
             }
             base.OnNetworkDespawn();
         }
 
-        public NetworkVariable<int> MyNetworkVariable;
-        public NetworkList<int> MyListSetOnSpawn;
-        public NetworkVariable<int> MyOwnerReadNetworkVariable;
-        public NetworkList<int> MyList;
+        public NetworkVariable<int> MyNetworkVariable = new NetworkVariable<int>();
+        public NetworkList<int> MyListSetOnSpawn = new NetworkList<int>();
+        public NetworkVariable<int> MyOwnerReadNetworkVariable = new NetworkVariable<int>(readPerm: NetworkVariableReadPermission.Owner);
+        public NetworkList<int> MyList = new NetworkList<int>();
         public static NetworkManager NetworkManagerOfInterest;
 
         internal static int GainOwnershipCount = 0;
 
-        private void Awake()
-        {
-            // Debug.Log($"Awake {NetworkManager.LocalClientId}");
-            MyNetworkVariable = new NetworkVariable<int>();
-            MyNetworkVariable.OnValueChanged += Changed;
-
-            MyListSetOnSpawn = new NetworkList<int>();
-            MyList = new NetworkList<int>();
-
-            MyOwnerReadNetworkVariable = new NetworkVariable<int>(readPerm: NetworkVariableReadPermission.Owner);
-            MyOwnerReadNetworkVariable.OnValueChanged += OwnerReadChanged;
-        }
-
         public override void OnGainedOwnership()
         {
             GainOwnershipCount++;
@@ -532,6 +523,7 @@ public IEnumerator NetworkHideChangeOwnership()
             AssertOnTimeout($"NetworkObject is still visible to Client-{m_ClientWithoutVisibility} or other clients think it is still visible to Client-{m_ClientWithoutVisibility}:\n {m_ErrorLog}");
 
             yield return WaitForConditionOrTimeOut(() => ShowHideObject.ClientTargetedNetworkObjects.Count == 0);
+            AssertOnTimeout($"Timed out waiting for ShowHideObject.ClientTargetedNetworkObjects to have a count of 0 but was {ShowHideObject.ClientTargetedNetworkObjects.Count}!");
 
             foreach (var client in m_ClientNetworkManagers)
             {
@@ -564,8 +556,31 @@ public IEnumerator NetworkHideChangeOwnership()
             }
 
             yield return WaitForConditionOrTimeOut(() => ShowHideObject.ClientTargetedNetworkObjects.Count == 1);
+            AssertOnTimeout($"Timed out waiting for ShowHideObject.ClientTargetedNetworkObjects to have a count of 1 but was {ShowHideObject.ClientTargetedNetworkObjects.Count}!");
+
+            m_ClientIdToCheck = firstClient.LocalClientId;
+            yield return WaitForConditionOrTimeOut(CheckIsClientOwner);
+            AssertOnTimeout($"Timed out waiting for client owner check!");
+        }
 
-            Assert.True(ShowHideObject.ClientTargetedNetworkObjects[0].OwnerClientId == firstClient.LocalClientId);
+        private ulong m_ClientIdToCheck;
+        private bool CheckIsClientOwner(StringBuilder errorLog)
+        {
+            if (ShowHideObject.ClientTargetedNetworkObjects[0].OwnerClientId != m_ClientIdToCheck)
+            {
+                errorLog.AppendLine($"[CheckIsClientOwner][Index: 0][{ShowHideObject.ClientTargetedNetworkObjects[0].name}] OwnerClientId is {ShowHideObject.ClientTargetedNetworkObjects[0].OwnerClientId} when it was expected to be {m_ClientIdToCheck}!");
+                if (ShowHideObject.ClientTargetedNetworkObjects.Count > 1)
+                {
+                    for (int i = 1; i < ShowHideObject.ClientTargetedNetworkObjects.Count; i++)
+                    {
+                        var target = ShowHideObject.ClientTargetedNetworkObjects[i];
+                        errorLog.AppendLine($"[CheckIsClientOwner][Index: {i}][{target.name}] OwnerClientId is {target.OwnerClientId}.");
+                    }
+                }
+                return false;
+            }
+
+            return true;
         }
 
         private bool AllClientsSpawnedObject1()
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/OwnerModifiedTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/OwnerModifiedTests.cs
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/OwnerPermissionTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/OwnerPermissionTests.cs
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs

Original file line number	Diff line number	Diff line change
`@@ -797,17 +797,6 @@ internal void InternalOnNetworkSpawn()`
`797`	`797`	`{`
`798`	`798`	`Debug.LogException(e);`
`799`	`799`	`}`
`800`		`-`
`801`		`- // Initialize again in case the user's OnNetworkSpawn changed something`
`802`		`- InitializeVariables();`
`803`		`-`
`804`		`- if (m_NetworkObject.HasAuthority)`
`805`		`- {`
`806`		`- // Since we just spawned the object and since user code might have modified their NetworkVariable, esp.`
`807`		`- // NetworkList, we need to mark the object as free of updates.`
`808`		`- // This should happen for all objects on the machine triggering the spawn.`
`809`		`- PostNetworkVariableWrite(true);`
`810`		`- }`
`811`	`800`	`}`
`812`	`801`
`813`	`802`	`internal void NetworkPostSpawn()`
`@@ -821,6 +810,13 @@ internal void NetworkPostSpawn()`
`821`	`810`	`{`
`822`	`811`	`Debug.LogException(e);`
`823`	`812`	`}`
	`813`	`+`
	`814`	`+ // Let each NetworkVariableBase derived instance know that`
	`815`	`+ // all spawn related methods have been invoked.`
	`816`	`+ for (int i = 0; i < NetworkVariableFields.Count; i++)`
	`817`	`+ {`
	`818`	`+ NetworkVariableFields[i].OnSpawned();`
	`819`	`+ }`
`824`	`820`	`}`
`825`	`821`
`826`	`822`	`internal void NetworkSessionSynchronized()`
`@@ -858,6 +854,13 @@ internal void InternalOnNetworkPreDespawn()`
`858`	`854`	`{`
`859`	`855`	`Debug.LogException(e);`
`860`	`856`	`}`
	`857`	`+`
	`858`	`+ // Let each NetworkVariableBase derived instance know that`
	`859`	`+ // all spawn related methods have been invoked.`
	`860`	`+ for (int i = 0; i < NetworkVariableFields.Count; i++)`
	`861`	`+ {`
	`862`	`+ NetworkVariableFields[i].OnPreDespawn();`
	`863`	`+ }`
`861`	`864`	`}`
`862`	`865`
`863`	`866`	`internal void InternalOnNetworkDespawn()`
Original file line number	Diff line number	Diff line change
`@@ -118,7 +118,7 @@ private bool ResetAllStates()`
`118`	`118`	`else`
`119`	`119`	`{`
`120`	`120`	`var attachable = networkManager.SpawnManager.SpawnedObjects[currentAttachableRoot.NetworkObjectId].GetComponentInChildren<TestAttachable>();`
`121`		`- attachable.ResetStates();`
	`121`	`+ attachable?.ResetStates();`
`122`	`122`	`}`
`123`	`123`
`124`	`124`	`// Target`
`@@ -129,7 +129,7 @@ private bool ResetAllStates()`
`129`	`129`	`else`
`130`	`130`	`{`
`131`	`131`	`var node = networkManager.SpawnManager.SpawnedObjects[m_TargetInstance.NetworkObjectId].GetComponentInChildren<TestNode>();`
`132`		`- node.ResetStates();`
	`132`	`+ node?.ResetStates();`
`133`	`133`	`}`
`134`	`134`
`135`	`135`	`// Target B`
`@@ -140,7 +140,7 @@ private bool ResetAllStates()`
`140`	`140`	`else`
`141`	`141`	`{`
`142`	`142`	`var node = networkManager.SpawnManager.SpawnedObjects[m_TargetInstanceB.NetworkObjectId].GetComponentInChildren<TestNode>();`
`143`		`- node.ResetStates();`
	`143`	`+ node?.ResetStates();`
`144`	`144`	`}`
`145`	`145`	`}`
`146`	`146`	`return m_ErrorLog.Length == 0;`