fix: #3447 make cached SyncVar fallback to original field value when …

…there is no network context (#3449) * Add Tests * Fix tests relying on undefined behaviors - GetSyncVarGameObjectOnClient() The test relies on the behavior that GameObject SyncVar lookup will behave as client if both isServer and isClient is false. The test is modified in a way that removes unnecessary object creation and uses a single object that is made sure it's context is client-side. - TestSyncingAbstractNetworkBehaviour() This test has been comparing null against null. The test is modified so it simulates each context better. * Make cached SyncVar getters fallback on no network context * Remove unnecessary assertions Gone under my radar doing mindless copy & pasting * Update Assets/Mirror/Core/NetworkBehaviour.cs * Update Assets/Mirror/Core/NetworkBehaviour.cs * Update Assets/Mirror/Core/NetworkBehaviour.cs --------- Co-authored-by: mischa <[email protected]>
MirrorNetworking · Apr 8, 2023 · 88170ed · 88170ed
1 parent c3b7fe6
commit 88170ed
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 7 deletions.
diff --git a/Assets/Mirror/Core/NetworkBehaviour.cs b/Assets/Mirror/Core/NetworkBehaviour.cs
@@ -648,7 +648,9 @@ protected void SetSyncVarGameObject(GameObject newGameObject, ref GameObject gam
         protected GameObject GetSyncVarGameObject(uint netId, ref GameObject gameObjectField)
         {
             // server always uses the field
-            if (isServer)
+            // if neither, fallback to original field
+            // fixes: https://github.com/MirrorNetworking/Mirror/issues/3447
+            if (isServer || !isClient)
             {
                 return gameObjectField;
             }
@@ -956,7 +958,9 @@ protected void SetSyncVarNetworkIdentity(NetworkIdentity newIdentity, ref Networ
         protected NetworkIdentity GetSyncVarNetworkIdentity(uint netId, ref NetworkIdentity identityField)
         {
             // server always uses the field
-            if (isServer)
+            // if neither, fallback to original field
+            // fixes: https://github.com/MirrorNetworking/Mirror/issues/3447
+            if (isServer || !isClient)
             {
                 return identityField;
             }
@@ -1019,7 +1023,9 @@ protected void SetSyncVarNetworkBehaviour<T>(T newBehaviour, ref T behaviourFiel
         protected T GetSyncVarNetworkBehaviour<T>(NetworkBehaviourSyncVar syncNetBehaviour, ref T behaviourField) where T : NetworkBehaviour
         {
             // server always uses the field
-            if (isServer)
+            // if neither, fallback to original field
+            // fixes: https://github.com/MirrorNetworking/Mirror/issues/3447
+            if (isServer || !isClient)
             {
                 return behaviourField;
             }

diff --git a/Assets/Mirror/Tests/Editor/NetworkBehaviourTests.cs b/Assets/Mirror/Tests/Editor/NetworkBehaviourTests.cs
@@ -597,15 +597,13 @@ public void GetSyncVarGameObjectOnClient()
             NetworkServer.Listen(1);
             ConnectClientBlockingAuthenticatedAndReady(out _);
 
-            CreateNetworked(out GameObject _, out NetworkIdentity identity);
+            // create a networked object with test component
+            CreateNetworked(out GameObject _, out NetworkIdentity identity, out NetworkBehaviourGetSyncVarGameObjectComponent comp);
 
             // are we on client and not on server?
             identity.isClient = true;
             Assert.That(identity.isServer, Is.False);
 
-            // create a networked object with test component
-            CreateNetworked(out GameObject _, out NetworkIdentity _, out NetworkBehaviourGetSyncVarGameObjectComponent comp);
-
             // create a spawned, syncable GameObject
             // (client tries to look up via netid, so needs to be spawned)
             CreateNetworkedAndSpawn(

diff --git a/Assets/Mirror/Tests/Editor/SyncVarAttributeTest.cs b/Assets/Mirror/Tests/Editor/SyncVarAttributeTest.cs
@@ -392,6 +392,7 @@ public void TestSyncingAbstractNetworkBehaviour()
         {
             // set up a "server" object
             CreateNetworked(out _, out NetworkIdentity serverIdentity, out SyncVarAbstractNetworkBehaviour serverBehaviour);
+            serverIdentity.isServer = true;
 
             // spawn syncvar targets
             CreateNetworked(out _, out NetworkIdentity wolfIdentity, out SyncVarAbstractNetworkBehaviour.MockWolf wolf);
@@ -400,6 +401,10 @@ public void TestSyncingAbstractNetworkBehaviour()
             wolfIdentity.netId = 135;
             zombieIdentity.netId = 246;
 
+            // add to spawned as if they were spawned on clients
+            NetworkClient.spawned.Add(wolfIdentity.netId, wolfIdentity);
+            NetworkClient.spawned.Add(zombieIdentity.netId, zombieIdentity);
+
             serverBehaviour.monster1 = wolf;
             serverBehaviour.monster2 = zombie;
 
@@ -411,14 +416,62 @@ public void TestSyncingAbstractNetworkBehaviour()
 
             // set up a "client" object
             CreateNetworked(out _, out NetworkIdentity clientIdentity, out SyncVarAbstractNetworkBehaviour clientBehaviour);
+            clientIdentity.isClient = true;
 
             // apply all the data from the server object
             NetworkReader reader = new NetworkReader(ownerWriter.ToArray());
             clientIdentity.DeserializeClient(reader, true);
 
             // check that the syncvars got updated
+            Debug.Log($"{clientBehaviour.monster1} and {serverBehaviour.monster1}");
             Assert.That(clientBehaviour.monster1, Is.EqualTo(serverBehaviour.monster1), "Data should be synchronized");
             Assert.That(clientBehaviour.monster2, Is.EqualTo(serverBehaviour.monster2), "Data should be synchronized");
+
+            // remove spawned objects
+            NetworkClient.spawned.Remove(wolfIdentity.netId);
+            NetworkClient.spawned.Remove(zombieIdentity.netId);
+        }
+
+        // Tests if getter for GameObject SyncVar field returns proper value on server before the containing object is spawned.
+        [Test]
+        public void SyncVarGameObjectGetterOnServerBeforeSpawn()
+        {
+            // The test should only need server objects, but at the same time this belongs in SyncVar tests,
+            // and objects in the tests defined here need client objects to spawn.
+            CreateNetworkedAndSpawn(
+                out GameObject serverGO, out NetworkIdentity serverIdentity, out SyncVarNetworkBehaviour serverNB,
+                out _, out _, out _);
+
+            CreateNetworked(out _, out _, out SyncVarGameObject serverComponent);
+
+            serverComponent.value = serverGO;
+            Assert.That(serverComponent.value, Is.EqualTo(serverGO), "getter should return original field value on server");
+        }
+
+        [Test]
+        public void SyncVarNetworkIdentityGetterOnServerBeforeSpawn()
+        {
+            CreateNetworkedAndSpawn(
+                out GameObject serverGO, out NetworkIdentity serverIdentity, out SyncVarNetworkBehaviour serverNB,
+                out _, out _, out _);
+
+            CreateNetworked(out _, out _, out SyncVarNetworkIdentity serverComponent);
+
+            serverComponent.value = serverIdentity;
+            Assert.That(serverComponent.value, Is.EqualTo(serverIdentity), "getter should return original field value on server");
+        }
+
+        [Test]
+        public void SyncVarNetworkBehaviourGetterOnServerBeforeSpawn()
+        {
+            CreateNetworkedAndSpawn(
+                out GameObject serverGO, out NetworkIdentity serverIdentity, out SyncVarNetworkBehaviour serverNB,
+                out _, out _, out _);
+
+            CreateNetworked(out _, out _, out SyncVarNetworkBehaviour serverComponent);
+
+            serverComponent.value = serverNB;
+            Assert.That(serverComponent.value, Is.EqualTo(serverNB), "getter should return original field value on server");
         }
     }
 }