fix: NetworkVariable clearing dirty flags after OnNetworkSpawn

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()

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()
         {

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)

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;

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:
             ///////////////////////////////////////////////

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()