Allow reader generation of abstract NetworkBehaviours by reordering checks in Readers.GenerateReader() (#2808)

* Reorder checks to simplify and allow reader generation of abstract NetworkBehaviours

* Add SyncVar/ClientRpc Tests for abstract NetworkBehaviour

Assets/Mirror/Editor/Weaver/Readers.cs

@@ -74,49 +74,21 @@ static MethodReference GenerateReader(TypeReference variableReference)
             }
 
             TypeDefinition variableDefinition = variableReference.Resolve();
+
+            // check if the type is completely invalid
             if (variableDefinition == null)
             {
                 Weaver.Error($"{variableReference.Name} is not a supported type", variableReference);
                 return null;
             }
-            if (variableDefinition.IsDerivedFrom<UnityEngine.Component>() &&
+            else if (variableReference.IsByReference)
-                !variableReference.IsDerivedFrom<NetworkBehaviour>())
-            {
-                Weaver.Error($"Cannot generate reader for component type {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
-                return null;
-            }
-            if (variableReference.Is<UnityEngine.Object>())
-            {
-                Weaver.Error($"Cannot generate reader for {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
-                return null;
-            }
-            if (variableReference.Is<UnityEngine.ScriptableObject>())
-            {
-                Weaver.Error($"Cannot generate reader for {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
-                return null;
-            }
-            if (variableReference.IsByReference)
             {
                 // error??
                 Weaver.Error($"Cannot pass type {variableReference.Name} by reference", variableReference);
                 return null;
             }
-            if (variableDefinition.HasGenericParameters && !variableDefinition.Is(typeof(ArraySegment<>)) && !variableDefinition.Is(typeof(List<>)))
-            {
-                Weaver.Error($"Cannot generate reader for generic variable {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
-                return null;
-            }
-            if (variableDefinition.IsInterface)
-            {
-                Weaver.Error($"Cannot generate reader for interface {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
-                return null;
-            }
-            if (variableDefinition.IsAbstract)
-            {
-                Weaver.Error($"Cannot generate reader for abstract class {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
-                return null;
-            }
 
+            // use existing func for known types
             if (variableDefinition.IsEnum)
             {
                 return GenerateEnumReadFunc(variableReference);
@@ -137,6 +109,38 @@ static MethodReference GenerateReader(TypeReference variableReference)
                 return GetNetworkBehaviourReader(variableReference);
             }
 
+            // check if reader generation is applicable on this type
+            if (variableDefinition.IsDerivedFrom<UnityEngine.Component>())
+            {
+                Weaver.Error($"Cannot generate reader for component type {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
+                return null;
+            }
+            if (variableReference.Is<UnityEngine.Object>())
+            {
+                Weaver.Error($"Cannot generate reader for {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
+                return null;
+            }
+            if (variableReference.Is<UnityEngine.ScriptableObject>())
+            {
+                Weaver.Error($"Cannot generate reader for {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
+                return null;
+            }
+            if (variableDefinition.HasGenericParameters)
+            {
+                Weaver.Error($"Cannot generate reader for generic variable {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
+                return null;
+            }
+            if (variableDefinition.IsInterface)
+            {
+                Weaver.Error($"Cannot generate reader for interface {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
+                return null;
+            }
+            if (variableDefinition.IsAbstract)
+            {
+                Weaver.Error($"Cannot generate reader for abstract class {variableReference.Name}. Use a supported type or provide a custom reader", variableReference);
+                return null;
+            }
+
             return GenerateClassOrStructReadFunction(variableReference);
         }
 

Assets/Mirror/Tests/Editor/ClientRpcTest.cs

@@ -26,6 +26,38 @@ public void RpcSendInt(int someInt)
         }
     }
 
+    class AbstractNetworkBehaviourClientRpcBehaviour : NetworkBehaviour
+    {
+        public abstract class MockMonsterBase : NetworkBehaviour
+        {
+            public abstract string GetName();
+        }
+
+        public class MockZombie : MockMonsterBase
+        {
+            public override string GetName()
+            {
+                return "Zombie";
+            }
+        }
+
+        public class MockWolf : MockMonsterBase
+        {
+            public override string GetName()
+            {
+                return "Wolf";
+            }
+        }
+
+        public event Action<MockMonsterBase> onSendMonsterBase;
+
+        [ClientRpc]
+        public void RpcSendMonster(MockMonsterBase someMonster)
+        {
+            onSendMonsterBase?.Invoke(someMonster);
+        }
+    }
+
     public class ClientRpcTest : RemoteTestBase
     {
         [Test]
@@ -84,5 +116,35 @@ public void RpcNotCalledForOwner()
             ProcessMessages();
             Assert.That(callCount, Is.EqualTo(0));
         }
+
+        [Test]
+        public void RpcIsCalledWithAbstractNetworkBehaviourParameter()
+        {
+            // spawn with owner
+            CreateNetworkedAndSpawn(out GameObject _, out NetworkIdentity _, out AbstractNetworkBehaviourClientRpcBehaviour hostBehaviour, NetworkServer.localConnection);
+
+            // spawn clientrpc parameter targets
+            CreateNetworkedAndSpawn(out GameObject _, out NetworkIdentity wolfIdentity, out AbstractNetworkBehaviourClientRpcBehaviour.MockWolf wolf, NetworkServer.localConnection);
+            CreateNetworkedAndSpawn(out GameObject _, out NetworkIdentity zombieIdentity, out AbstractNetworkBehaviourClientRpcBehaviour.MockZombie zombie, NetworkServer.localConnection);
+
+            AbstractNetworkBehaviourClientRpcBehaviour.MockMonsterBase currentMonster = null;
+
+            int callCount = 0;
+            hostBehaviour.onSendMonsterBase += incomingMonster =>
+            {
+                callCount++;
+                Assert.That(incomingMonster, Is.EqualTo(currentMonster));
+            };
+
+            currentMonster = wolf;
+            hostBehaviour.RpcSendMonster(currentMonster);
+            ProcessMessages();
+            Assert.That(callCount, Is.EqualTo(1));
+
+            currentMonster = zombie;
+            hostBehaviour.RpcSendMonster(currentMonster);
+            ProcessMessages();
+            Assert.That(callCount, Is.EqualTo(2));
+        }
     }
 }

Assets/Mirror/Tests/Editor/SyncVarTest.cs

@@ -35,6 +35,35 @@ class SyncVarNetworkBehaviour : NetworkBehaviour
         [SyncVar]
         public SyncVarNetworkBehaviour value;
     }
+    class SyncVarAbstractNetworkBehaviour : NetworkBehaviour
+    {
+        public abstract class MockMonsterBase : NetworkBehaviour
+        {
+            public abstract string GetName();
+        }
+
+        public class MockZombie : MockMonsterBase
+        {
+            public override string GetName()
+            {
+                return "Zombie";
+            }
+        }
+
+        public class MockWolf : MockMonsterBase
+        {
+            public override string GetName()
+            {
+                return "Wolf";
+            }
+        }
+
+        [SyncVar]
+        public MockMonsterBase monster1;
+
+        [SyncVar]
+        public MockMonsterBase monster2;
+    }
 
     public class SyncVarTest : SyncVarTestBase
     {
@@ -373,5 +402,39 @@ public void SyncVarCacheNetidForBehaviour(bool initialState)
             // check field finds value
             Assert.That(clientObject.value, Is.EqualTo(serverValue), "fields should return serverValue");
         }
+
+        [Test]
+        public void TestSyncingAbstractNetworkBehaviour()
+        {
+            // set up a "server" object
+            CreateNetworked(out GameObject _, out NetworkIdentity serverIdentity, out SyncVarAbstractNetworkBehaviour serverBehaviour);
+
+            // spawn syncvar targets
+            CreateNetworked(out GameObject _, out NetworkIdentity wolfIdentity, out SyncVarAbstractNetworkBehaviour.MockWolf wolf);
+            CreateNetworked(out GameObject _, out NetworkIdentity zombieIdentity, out SyncVarAbstractNetworkBehaviour.MockZombie zombie);
+
+            wolfIdentity.netId = 135;
+            zombieIdentity.netId = 246;
+
+            serverBehaviour.monster1 = wolf;
+            serverBehaviour.monster2 = zombie;
+
+            // serialize all the data as we would for the network
+            NetworkWriter ownerWriter = new NetworkWriter();
+            // not really used in this Test
+            NetworkWriter observersWriter = new NetworkWriter();
+            serverIdentity.OnSerializeAllSafely(true, ownerWriter, out int ownerWritten, observersWriter, out int observersWritten);
+
+            // set up a "client" object
+            CreateNetworked(out GameObject _, out NetworkIdentity clientIdentity, out SyncVarAbstractNetworkBehaviour clientBehaviour);
+
+            // apply all the data from the server object
+            NetworkReader reader = new NetworkReader(ownerWriter.ToArray());
+            clientIdentity.OnDeserializeAllSafely(reader, true);
+
+            // check that the syncvars got updated
+            Assert.That(clientBehaviour.monster1, Is.EqualTo(serverBehaviour.monster1), "Data should be synchronized");
+            Assert.That(clientBehaviour.monster2, Is.EqualTo(serverBehaviour.monster2), "Data should be synchronized");
+        }
     }
 }