fix: OnNetworkSpawn timing (#1299)

* fix: OnNetworkSpawn timing

Fixes MTT-1431 and MTT-1432 - ensures that OnNetworkSpawn is called before RPCs sent during OnNetworkSpawn on the client, and that OnValueChanged is not executed for initialization of values.

* - Added comment per review feedback
- standards fix

Author: ShadauxCat

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

@@ -119,7 +119,7 @@ public override void ReadDelta(FastBufferReader reader, bool keepDirtyDelta)
         /// <inheritdoc />
         public override void ReadField(FastBufferReader reader)
         {
-            ReadDelta(reader, false);
+            reader.ReadValueSafe(out m_InternalValue);
         }
 
         /// <inheritdoc />

com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

@@ -475,6 +475,10 @@ private void SpawnNetworkObjectLocallyCommon(NetworkObject networkObject, ulong
             networkObject.ApplyNetworkParenting();
             NetworkObject.CheckOrphanChildren();
 
+            networkObject.InvokeBehaviourNetworkSpawn();
+
+            // This must happen after InvokeBehaviourNetworkSpawn, otherwise ClientRPCs and other messages can be
+            // processed before the object is fully spawned. This must be the last thing done in the spawn process.
             if (m_Triggers.ContainsKey(networkId))
             {
                 var triggerInfo = m_Triggers[networkId];
@@ -487,8 +491,6 @@ private void SpawnNetworkObjectLocallyCommon(NetworkObject networkObject, ulong
                 triggerInfo.TriggerData.Dispose();
                 m_Triggers.Remove(networkId);
             }
-
-            networkObject.InvokeBehaviourNetworkSpawn();
         }
 
         internal void SendSpawnCallForObject(ulong clientId, NetworkObject networkObject)

testproject/Assets/Tests/Runtime/MessageOrdering.cs

@@ -19,6 +19,7 @@ public IEnumerator SetUp()
             Support.SpawnRpcDespawn.ClientUpdateCount = 0;
             Support.SpawnRpcDespawn.ServerUpdateCount = 0;
             Support.SpawnRpcDespawn.ClientNetworkSpawnRpcCalled = false;
+            Support.SpawnRpcDespawn.ExecuteClientRpc = false;
             yield break;
         }
 
@@ -34,6 +35,7 @@ public IEnumerator Teardown()
                 Support.SpawnRpcDespawn.ClientUpdateCount = 0;
                 Support.SpawnRpcDespawn.ServerUpdateCount = 0;
                 Support.SpawnRpcDespawn.ClientNetworkSpawnRpcCalled = false;
+                Support.SpawnRpcDespawn.ExecuteClientRpc = false;
             }
             yield break;
         }
@@ -175,6 +177,7 @@ public IEnumerator SpawnRpcDespawn()
         [UnityTest]
         public IEnumerator RpcOnNetworkSpawn()
         {
+            Support.SpawnRpcDespawn.ExecuteClientRpc = true;
             // Must be 1 for this test.
             const int numClients = 1;
             Assert.True(MultiInstanceHelpers.Create(numClients, out NetworkManager server, out NetworkManager[] clients));

testproject/Assets/Tests/Runtime/NetworkVariableInitializationOnNetworkSpawnTest.cs

@@ -18,6 +18,7 @@ public IEnumerator SetUp()
             // Make sure these static values are reset
             NetworkVariableInitOnNetworkSpawn.NetworkSpawnCalledOnClient = false;
             NetworkVariableInitOnNetworkSpawn.NetworkSpawnCalledOnServer = false;
+            NetworkVariableInitOnNetworkSpawn.OnValueChangedCalledOnClient = false;
             yield break;
         }
 
@@ -33,12 +34,11 @@ public IEnumerator Teardown()
             }
             NetworkVariableInitOnNetworkSpawn.NetworkSpawnCalledOnClient = false;
             NetworkVariableInitOnNetworkSpawn.NetworkSpawnCalledOnServer = false;
+            NetworkVariableInitOnNetworkSpawn.OnValueChangedCalledOnClient = false;
             yield break;
         }
 
-        [UnityTest]
-        [Description("When a network variable is initialized in OnNetworkSpawn on the server, the spawned object's NetworkVariable on the client is initialized with the same value.")]
-        public IEnumerator WhenANetworkVariableIsInitializedInOnNetworkSpawnOnTheServer_TheSpawnedObjectsNetworkVariableOnTheClientIsInitializedWithTheSameValue()
+        private IEnumerator RunTest()
         {
             const int numClients = 1;
             Assert.True(MultiInstanceHelpers.Create(numClients, out NetworkManager server, out NetworkManager[] clients));
@@ -95,8 +95,88 @@ public IEnumerator WhenANetworkVariableIsInitializedInOnNetworkSpawnOnTheServer_
                 var nextFrameNumber = Time.frameCount + 1;
                 yield return new WaitUntil(() => Time.frameCount >= nextFrameNumber);
             }
+        }
+
+        [UnityTest]
+        [Description("When a network variable is initialized in OnNetworkSpawn on the server, the spawned object's NetworkVariable on the client is initialized with the same value.")]
+        public IEnumerator WhenANetworkVariableIsInitializedInOnNetworkSpawnOnTheServer_TheSpawnedObjectsNetworkVariableOnTheClientIsInitializedWithTheSameValue()
+        {
+            yield return RunTest();
             Assert.IsTrue(NetworkVariableInitOnNetworkSpawn.NetworkSpawnCalledOnServer);
             Assert.IsTrue(NetworkVariableInitOnNetworkSpawn.NetworkSpawnCalledOnClient);
         }
+
+        [UnityTest]
+        [Description("When a network variable is initialized in OnNetworkSpawn on the server, OnValueChanged is not called")]
+        public IEnumerator WhenANetworkVariableIsInitializedInOnNetworkSpawnOnTheServer_OnValueChangedIsNotCalled()
+        {
+            yield return RunTest();
+            Assert.IsFalse(NetworkVariableInitOnNetworkSpawn.OnValueChangedCalledOnClient);
+        }
+
+        [UnityTest]
+        [Description("When a network variable is changed just after OnNetworkSpawn on the server, OnValueChanged is called after OnNetworkSpawn")]
+        public IEnumerator WhenANetworkVariableIsInitializedJustAfterOnNetworkSpawnOnTheServer_OnValueChangedIsCalledAfterOnNetworkSpawn()
+        {
+            const int numClients = 1;
+            Assert.True(MultiInstanceHelpers.Create(numClients, out NetworkManager server, out NetworkManager[] clients));
+            m_Prefab = new GameObject("Object");
+            var networkObject = m_Prefab.AddComponent<NetworkObject>();
+            m_Prefab.AddComponent<NetworkVariableInitOnNetworkSpawn>();
+
+            // Make it a prefab
+            MultiInstanceHelpers.MakeNetworkObjectTestPrefab(networkObject);
+
+            var validNetworkPrefab = new NetworkPrefab();
+            validNetworkPrefab.Prefab = m_Prefab;
+            server.NetworkConfig.NetworkPrefabs.Add(validNetworkPrefab);
+            foreach (var client in clients)
+            {
+                client.NetworkConfig.NetworkPrefabs.Add(validNetworkPrefab);
+            }
+
+            // Start the instances
+            if (!MultiInstanceHelpers.Start(true, server, clients))
+            {
+                Debug.LogError("Failed to start instances");
+                Assert.Fail("Failed to start instances");
+            }
+
+            // [Client-Side] Wait for a connection to the server
+            yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnected(clients, null, 512));
+
+            // [Host-Side] Check to make sure all clients are connected
+            yield return MultiInstanceHelpers.Run(
+                MultiInstanceHelpers.WaitForClientsConnectedToServer(server, clients.Length + 1, null, 512));
+
+            var serverObject = Object.Instantiate(m_Prefab, Vector3.zero, Quaternion.identity);
+            NetworkObject serverNetworkObject = serverObject.GetComponent<NetworkObject>();
+            serverNetworkObject.NetworkManagerOwner = server;
+            serverNetworkObject.Spawn();
+            serverNetworkObject.GetComponent<NetworkVariableInitOnNetworkSpawn>().Variable.Value = 10;
+            Assert.IsFalse(NetworkVariableInitOnNetworkSpawn.OnValueChangedCalledOnClient);
+
+            // Wait until all objects have spawned.
+            //const int expectedNetworkObjects = numClients + 2; // +2 = one for prefab, one for server.
+            const int maxFrames = 240;
+            var doubleCheckTime = Time.realtimeSinceStartup + 5.0f;
+            while (!NetworkVariableInitOnNetworkSpawn.OnValueChangedCalledOnClient)
+            {
+                if (Time.frameCount > maxFrames)
+                {
+                    // This is here in the event a platform is running at a higher
+                    // frame rate than expected
+                    if (doubleCheckTime < Time.realtimeSinceStartup)
+                    {
+                        Assert.Fail("Did not successfully spawn all expected NetworkObjects");
+                        break;
+                    }
+                }
+                var nextFrameNumber = Time.frameCount + 1;
+                yield return new WaitUntil(() => Time.frameCount >= nextFrameNumber);
+            }
+            // Test of ordering is handled by an assert within OnNetworkSpawn
+            Assert.IsTrue(NetworkVariableInitOnNetworkSpawn.OnValueChangedCalledOnClient);
+        }
     }
 }

testproject/Assets/Tests/Runtime/Support/NetworkVariableInitOnNetworkSpawn.cs

@@ -5,23 +5,38 @@ namespace TestProject.RuntimeTests.Support
 {
     public class NetworkVariableInitOnNetworkSpawn : NetworkBehaviour
     {
-        private NetworkVariable<int> m_Variable = new NetworkVariable<int>();
+        public NetworkVariable<int> Variable = new NetworkVariable<int>();
         public static bool NetworkSpawnCalledOnServer;
         public static bool NetworkSpawnCalledOnClient;
+        public static bool OnValueChangedCalledOnClient = false;
+
+        private void Awake()
+        {
+            Variable.OnValueChanged += OnValueChanged;
+        }
+
+        public void OnValueChanged(int previousValue, int newValue)
+        {
+            if (!IsServer)
+            {
+                OnValueChangedCalledOnClient = true;
+            }
+        }
 
         public override void OnNetworkSpawn()
         {
+            Assert.IsFalse(OnValueChangedCalledOnClient);
             base.OnNetworkSpawn();
             if (IsServer)
             {
                 NetworkSpawnCalledOnServer = true;
-                m_Variable.Value = 5;
+                Variable.Value = 5;
             }
             else
             {
                 NetworkSpawnCalledOnClient = true;
             }
-            Assert.AreEqual(5, m_Variable.Value);
+            Assert.AreEqual(5, Variable.Value);
         }
     }
 }

testproject/Assets/Tests/Runtime/Support/SpawnRpcDespawn.cs

@@ -11,6 +11,7 @@ public class SpawnRpcDespawn : NetworkBehaviour, INetworkUpdateSystem
         public static int ClientUpdateCount;
         public static int ServerUpdateCount;
         public static bool ClientNetworkSpawnRpcCalled;
+        public static bool ExecuteClientRpc;
         public static NetworkUpdateStage StageExecutedByReceiver;
 
         private bool m_Active = false;
@@ -42,10 +43,15 @@ public override void OnNetworkSpawn()
         {
             if (!IsServer)
             {
+                // Asserting that the RPC is not called before OnNetworkSpawn
+                Assert.IsFalse(ClientNetworkSpawnRpcCalled);
                 return;
             }
 
-            TestClientRpc();
+            if (ExecuteClientRpc)
+            {
+                TestClientRpc();
+            }
         }
 
         [ClientRpc]
@@ -54,12 +60,6 @@ private void TestClientRpc()
             ClientNetworkSpawnRpcCalled = true;
         }
 
-        public void NetworkStart()
-        {
-            Debug.Log($"Network Start on client {NetworkManager.LocalClientId.ToString()}");
-            Assert.AreEqual(NetworkUpdateStage.EarlyUpdate, NetworkUpdateLoop.UpdateStage);
-        }
-
         public void Awake()
         {
             foreach (NetworkUpdateStage stage in Enum.GetValues(typeof(NetworkUpdateStage)))

testproject/Packages/packages-lock.json

@@ -70,7 +70,7 @@
       "depth": 0,
       "source": "local",
       "dependencies": {
-        "com.unity.netcode.gameobjects": "0.0.1-preview.1",
+        "com.unity.netcode.gameobjects": "0.2.0-preview.1",
         "com.unity.transport": "1.0.0-pre.5"
       }
     },