test

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

Author: NoelStephensUnity

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

@@ -1,16 +1,13 @@
 using System.Collections;
 using System.Collections.Generic;
+using System.Text;
 using NUnit.Framework;
 using Unity.Netcode.TestHelpers.Runtime;
 using UnityEngine;
 using UnityEngine.TestTools;
 
 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
 
     internal class OwnerModifiedObject : NetworkBehaviour, INetworkUpdateSystem
     {
@@ -74,17 +71,36 @@ public void InitializeLastCient()
         }
     }
 
+    internal class ChangeValueOnAuthority : NetworkBehaviour
+    {
+        public NetworkVariable<int> SomeIntValue = new NetworkVariable<int>();
+
+        public override void OnNetworkSpawn()
+        {
+            if (HasAuthority)
+            {
+                SomeIntValue.Value++;
+            }
+            base.OnNetworkSpawn();
+        }
+
+        protected override void OnNetworkPostSpawn()
+        {
+            if (HasAuthority)
+            {
+                SomeIntValue.Value++;
+            }
+            base.OnNetworkPostSpawn();
+        }
+    }
+
     [TestFixture(HostOrServer.DAHost)]
     [TestFixture(HostOrServer.Host)]
     internal class OwnerModifiedTests : NetcodeIntegrationTest
     {
         protected override int NumberOfClients => 2;
 
-        // TODO: [CmbServiceTests] Adapt to run with the service
-        protected override bool UseCMBService()
-        {
-            return false;
-        }
+        private GameObject m_SpawnObject;
 
         public OwnerModifiedTests(HostOrServer hostOrServer) : base(hostOrServer) { }
 
@@ -93,13 +109,33 @@ protected override void OnCreatePlayerPrefab()
             m_PlayerPrefab.AddComponent<OwnerModifiedObject>();
         }
 
+        protected override void OnServerAndClientsCreated()
+        {
+            m_SpawnObject = CreateNetworkObjectPrefab("SpawnObj");
+            m_SpawnObject.AddComponent<ChangeValueOnAuthority>();
+            base.OnServerAndClientsCreated();
+        }
+
+        private NetworkManager m_LastClient;
+
+        protected override void OnNewClientStartedAndConnected(NetworkManager networkManager)
+        {
+            m_LastClient = networkManager;
+            base.OnNewClientStartedAndConnected(networkManager);
+        }
+
+        /// <summary>
+        /// Addresses MTT-4386 #2109
+        /// Verify NetworkVariable updates are not repeated on some clients.
+        /// TODO: This test needs to be re-written/overhauled.
+        /// </summary>
         [UnityTest]
-        public IEnumerator OwnerModifiedTest()
+        public IEnumerator VerifyDoesNotRepeatOnSomeClients()
         {
             OwnerModifiedObject.EnableVerbose = m_EnableVerboseDebug;
             // We use this to assure we are the "last client" connected.
             yield return CreateAndStartNewClient();
-            var ownerModLastClient = m_ClientNetworkManagers[2].LocalClient.PlayerObject.GetComponent<OwnerModifiedObject>();
+            var ownerModLastClient = m_LastClient.LocalClient.PlayerObject.GetComponent<OwnerModifiedObject>();
             ownerModLastClient.InitializeLastCient();
 
             // Run through all update loops setting the value once every 5 frames
@@ -116,5 +152,51 @@ public IEnumerator OwnerModifiedTest()
             // We'll have at least one update per stage per client, if all goes well.
             Assert.True(OwnerModifiedObject.Updates > 20);
         }
+
+
+        private ChangeValueOnAuthority m_SessionAuthorityInstance;
+        private ChangeValueOnAuthority m_InstanceAuthority;
+
+        private bool NetworkVariablesMatch(StringBuilder errorLog)
+        {
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                if (networkManager == m_InstanceAuthority.NetworkManager)
+                {
+                    continue;
+                }
+
+                var changeValue = networkManager.SpawnManager.SpawnedObjects[m_InstanceAuthority.NetworkObjectId].GetComponent<ChangeValueOnAuthority>();
+                if (changeValue.SomeIntValue.Value != m_InstanceAuthority.SomeIntValue.Value)
+                {
+                    errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] {changeValue.name} value is {changeValue.SomeIntValue.Value} but was expecting {m_InstanceAuthority.SomeIntValue.Value}!");
+                }
+            }
+
+            return errorLog.Length == 0;
+        }
+
+        /// <summary>
+        /// Verifies that when running a distributed authority network topology
+        /// </summary>
+        [UnityTest]
+        public IEnumerator OwnershipSpawnedAndUpdatedDuringSpawn()
+        {
+            var authority = GetAuthorityNetworkManager();
+            var nonAuthority = GetNonAuthorityNetworkManager();
+            m_SessionAuthorityInstance = Object.Instantiate(m_SpawnObject).GetComponent<ChangeValueOnAuthority>();
+
+            SpawnInstanceWithOwnership(m_SessionAuthorityInstance.GetComponent<NetworkObject>(), authority, nonAuthority.LocalClientId);
+            yield return WaitForSpawnedOnAllOrTimeOut(m_SessionAuthorityInstance.NetworkObjectId);
+            AssertOnTimeout($"Failed to spawn {m_SessionAuthorityInstance.name} on all clients!");
+
+            m_InstanceAuthority = nonAuthority.SpawnManager.SpawnedObjects[m_SessionAuthorityInstance.NetworkObjectId].GetComponent<ChangeValueOnAuthority>();
+
+            yield return WaitForConditionOrTimeOut(NetworkVariablesMatch);
+            AssertOnTimeout($"The {nameof(ChangeValueOnAuthority.SomeIntValue)} failed to synchronize on all clients!");
+
+            Assert.IsTrue(m_SessionAuthorityInstance.SomeIntValue.Value == 2, "No values were updated on the spawn authority instance!");
+            Assert.IsTrue(m_InstanceAuthority.SomeIntValue.Value == 2, "No values were updated on the owner's instance!");
+        }
     }
 }

com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTest.cs

@@ -11,6 +11,7 @@
 using UnityEngine;
 using UnityEngine.SceneManagement;
 using UnityEngine.TestTools;
+using static UnityEngine.UI.GridLayoutGroup;
 using Object = UnityEngine.Object;
 
 namespace Unity.Netcode.TestHelpers.Runtime
@@ -2171,42 +2172,35 @@ protected GameObject SpawnPlayerObject(GameObject prefabGameObject, NetworkManag
             return SpawnObject(prefabNetworkObject, owner, destroyWithScene, true);
         }
 
-        /// <summary>
-        /// Spawn an already instantiated instance of a network prefab.
-        /// Note: If you pass in the NetworkPrefab itself this method will not create an instance but will spawn the pefab itself. (don't do this)
-        /// </summary>
-        /// <param name="networkObjectToSpawn">the instance of a prefab <see cref="NetworkObject"/> to spawn</param>
-        /// <param name="owner">the owner of the instance</param>
-        /// <param name="destroyWithScene">default is false</param>
-        /// <param name="isPlayerObject">when <see cref="true"/>, the object will be spawned as the <see cref="NetworkManager.LocalClientId"/> owned player.</param>
-        protected void SpawnObjectInstance(NetworkObject networkObjectToSpawn, NetworkManager owner, bool destroyWithScene = false, bool isPlayerObject = false)
+
+        internal void SpawnInstanceWithOwnership(NetworkObject networkObjectToSpawn, NetworkManager spawnAuthority, ulong clientId, bool destroyWithScene = false, bool isPlayerObject = false)
         {
-            if (owner.NetworkConfig.NetworkTopology == NetworkTopologyTypes.DistributedAuthority)
+            if (spawnAuthority.NetworkConfig.NetworkTopology == NetworkTopologyTypes.DistributedAuthority)
             {
-                networkObjectToSpawn.NetworkManagerOwner = owner; // Required to assure the client does the spawning
+                networkObjectToSpawn.NetworkManagerOwner = spawnAuthority; // Required to assure the client does the spawning
                 if (isPlayerObject)
                 {
-                    networkObjectToSpawn.SpawnAsPlayerObject(owner.LocalClientId, destroyWithScene);
+                    networkObjectToSpawn.SpawnAsPlayerObject(clientId, destroyWithScene);
                 }
                 else
                 {
-                    networkObjectToSpawn.SpawnWithOwnership(owner.LocalClientId, destroyWithScene);
+                    networkObjectToSpawn.SpawnWithOwnership(clientId, destroyWithScene);
                 }
             }
             else
             {
                 networkObjectToSpawn.NetworkManagerOwner = m_ServerNetworkManager; // Required to assure the server does the spawning
-                if (owner == m_ServerNetworkManager)
+                if (spawnAuthority == m_ServerNetworkManager)
                 {
                     if (m_UseHost)
                     {
                         if (isPlayerObject)
                         {
-                            networkObjectToSpawn.SpawnAsPlayerObject(owner.LocalClientId, destroyWithScene);
+                            networkObjectToSpawn.SpawnAsPlayerObject(clientId, destroyWithScene);
                         }
                         else
                         {
-                            networkObjectToSpawn.SpawnWithOwnership(owner.LocalClientId, destroyWithScene);
+                            networkObjectToSpawn.SpawnWithOwnership(clientId, destroyWithScene);
                         }
                     }
                     else
@@ -2218,16 +2212,29 @@ protected void SpawnObjectInstance(NetworkObject networkObjectToSpawn, NetworkMa
                 {
                     if (isPlayerObject)
                     {
-                        networkObjectToSpawn.SpawnAsPlayerObject(owner.LocalClientId, destroyWithScene);
+                        networkObjectToSpawn.SpawnAsPlayerObject(clientId, destroyWithScene);
                     }
                     else
                     {
-                        networkObjectToSpawn.SpawnWithOwnership(owner.LocalClientId, destroyWithScene);
+                        networkObjectToSpawn.SpawnWithOwnership(clientId, destroyWithScene);
                     }
                 }
             }
         }
 
+        /// <summary>
+        /// Spawn an already instantiated instance of a network prefab.
+        /// Note: If you pass in the NetworkPrefab itself this method will not create an instance but will spawn the pefab itself. (don't do this)
+        /// </summary>
+        /// <param name="networkObjectToSpawn">the instance of a prefab <see cref="NetworkObject"/> to spawn</param>
+        /// <param name="owner">the owner of the instance</param>
+        /// <param name="destroyWithScene">default is false</param>
+        /// <param name="isPlayerObject">when <see cref="true"/>, the object will be spawned as the <see cref="NetworkManager.LocalClientId"/> owned player.</param>
+        protected void SpawnObjectInstance(NetworkObject networkObjectToSpawn, NetworkManager owner, bool destroyWithScene = false, bool isPlayerObject = false)
+        {
+            SpawnInstanceWithOwnership(networkObjectToSpawn, owner, owner.LocalClientId, destroyWithScene, isPlayerObject);
+        }
+
         /// <summary>
         /// Spawn a NetworkObject prefab instance
         /// </summary>