fix: overriden hooks are invoked (fixes #1581) (#1584)

* failing tests for virutal methods used by syncvar hook

* replacing Assert.fail to stop OnDeserialize failed error

* adding tests to make sure classes are set up correctly

* fix: call overriden hooks

Use a virtual call for hooks.  fixes #1581

Co-authored-by: Paul Pacheco <paulpach@gmail.com>

Assets/Mirror/Editor/Weaver/Processors/NetworkBehaviourProcessor.cs

@@ -565,7 +565,7 @@ void DeserializeField(FieldDefinition syncVar, ILProcessor serWorker, MethodDefi
                     serWorker.Append(serWorker.Create(OpCodes.Ldarg_0));
                     // syncvar.get (finds current GO/NI from netId)
                     serWorker.Append(serWorker.Create(OpCodes.Ldfld, syncVar));
-                    serWorker.Append(serWorker.Create(OpCodes.Call, foundMethod));
+                    serWorker.Append(serWorker.Create(OpCodes.Callvirt, foundMethod));
 
                     // Generates: end if (!SyncVarEqual);
                     serWorker.Append(syncVarEqualLabel);
@@ -649,7 +649,7 @@ void DeserializeField(FieldDefinition syncVar, ILProcessor serWorker, MethodDefi
                     serWorker.Append(serWorker.Create(OpCodes.Ldarg_0));
                     // syncvar.get
                     serWorker.Append(serWorker.Create(OpCodes.Ldfld, syncVar));
-                    serWorker.Append(serWorker.Create(OpCodes.Call, foundMethod));
+                    serWorker.Append(serWorker.Create(OpCodes.Callvirt, foundMethod));
 
                     // Generates: end if (!SyncVarEqual);
                     serWorker.Append(syncVarEqualLabel);

Assets/Mirror/Editor/Weaver/Processors/SyncVarProcessor.cs

@@ -222,7 +222,7 @@ public static MethodDefinition ProcessSyncVarSet(TypeDefinition td, FieldDefinit
                 setWorker.Append(setWorker.Create(OpCodes.Ldarg_0));
                 setWorker.Append(setWorker.Create(OpCodes.Ldloc, oldValue));
                 setWorker.Append(setWorker.Create(OpCodes.Ldarg_1));
-                setWorker.Append(setWorker.Create(OpCodes.Call, hookFunctionMethod));
+                setWorker.Append(setWorker.Create(OpCodes.Callvirt, hookFunctionMethod));
 
                 // setSyncVarHookGuard(dirtyBit, false);
                 setWorker.Append(setWorker.Create(OpCodes.Ldarg_0));

Assets/Mirror/Tests/Editor/SyncVarVirtualTest.cs

@@ -0,0 +1,178 @@
+using System;
+using NUnit.Framework;
+using UnityEngine;
+
+namespace Mirror.Tests
+{
+    abstract class SyncVarHookTesterBase : NetworkBehaviour
+    {
+        [SyncVar(hook = nameof(onValue1Changed))]
+        public float value1;
+        [SyncVar(hook = nameof(onValue2Changed))]
+        public float value2;
+
+        public event Action OnValue2ChangedVirtualCalled;
+
+        public abstract void onValue1Changed(float old, float newValue);
+        public virtual void onValue2Changed(float old, float newValue)
+        {
+            OnValue2ChangedVirtualCalled?.Invoke();
+        }
+
+        public void ChangeValues()
+        {
+            value1 += 1f;
+            value2 += 1f;
+        }
+
+        public void CallOnValue2Changed()
+        {
+            this.onValue2Changed(1, 1);
+        }
+    }
+
+    class SyncVarHookTester : SyncVarHookTesterBase
+    {
+        public event Action OnValue1ChangedOverrideCalled;
+        public event Action OnValue2ChangedOverrideCalled;
+        public override void onValue1Changed(float old, float newValue)
+        {
+            OnValue1ChangedOverrideCalled?.Invoke();
+        }
+        public override void onValue2Changed(float old, float newValue)
+        {
+            OnValue2ChangedOverrideCalled?.Invoke();
+        }
+    }
+    [TestFixture]
+    public class SyncVarVirtualTest
+    {
+        private SyncVarHookTester serverTester;
+        private NetworkIdentity netIdServer;
+        private SyncVarHookTester clientTester;
+        private NetworkIdentity netIdClient;
+
+        [SetUp]
+        public void Setup()
+        {
+            // create server and client objects and sync inital values
+
+            GameObject gameObject1 = new GameObject();
+            netIdServer = gameObject1.AddComponent<NetworkIdentity>();
+            serverTester = gameObject1.AddComponent<SyncVarHookTester>();
+
+            GameObject gameObject2 = new GameObject();
+            netIdClient = gameObject2.AddComponent<NetworkIdentity>();
+            clientTester = gameObject2.AddComponent<SyncVarHookTester>();
+
+            serverTester.value1 = 1;
+            serverTester.value2 = 2;
+
+            SyncValuesWithClient();
+        }
+
+        private void SyncValuesWithClient()
+        {
+            // serialize all the data as we would for the network
+            NetworkWriter ownerWriter = new NetworkWriter();
+            // not really used in this Test
+            NetworkWriter observersWriter = new NetworkWriter();
+            netIdServer.OnSerializeAllSafely(true, ownerWriter, out int ownerWritten, observersWriter, out int observersWritten);
+
+
+            // apply all the data from the server object
+            NetworkReader reader = new NetworkReader(ownerWriter.ToArray());
+            netIdClient.OnDeserializeAllSafely(reader, true);
+        }
+
+        [TearDown]
+        public void TearDown()
+        {
+            UnityEngine.Object.DestroyImmediate(serverTester.gameObject);
+            UnityEngine.Object.DestroyImmediate(clientTester.gameObject);
+        }
+        [Test]
+        public void abstractMethodOnChangeWorkWithHooks()
+        {
+            serverTester.ChangeValues();
+
+            bool value1OverrideCalled = false;
+            clientTester.OnValue1ChangedOverrideCalled += () => {
+                value1OverrideCalled = true;
+            };
+
+            SyncValuesWithClient();
+
+            Assert.AreEqual(serverTester.value1, serverTester.value1);
+            Assert.IsTrue(value1OverrideCalled);
+        }
+        [Test]
+        public void virtualMethodOnChangeWorkWithHooks()
+        {
+            serverTester.ChangeValues();
+
+            bool value2OverrideCalled = false;
+            clientTester.OnValue2ChangedOverrideCalled += () => {
+                value2OverrideCalled = true;
+            };
+
+            bool value2VirtualCalled = false;
+            clientTester.OnValue2ChangedVirtualCalled += () => {
+                value2VirtualCalled = true;
+            };
+
+            SyncValuesWithClient();
+
+            Assert.AreEqual(serverTester.value2, serverTester.value2);
+            Assert.IsTrue(value2OverrideCalled, "Override method not called");
+            Assert.IsFalse(value2VirtualCalled, "Virtual method called when Override exists");
+        }
+
+        [Test]
+        public void manuallyCallingVirtualMethodCallsOverride()
+        {
+            // this to check that class are set up correct for tests above
+            serverTester.ChangeValues();
+
+            bool value2OverrideCalled = false;
+            clientTester.OnValue2ChangedOverrideCalled += () => {
+                value2OverrideCalled = true;
+            };
+
+            bool value2VirtualCalled = false;
+            clientTester.OnValue2ChangedVirtualCalled += () => {
+                value2VirtualCalled = true;
+            };
+
+            SyncVarHookTesterBase baseClass = clientTester as SyncVarHookTesterBase;
+            baseClass.onValue2Changed(1, 1);
+
+            Assert.AreEqual(serverTester.value2, serverTester.value2);
+            Assert.IsTrue(value2OverrideCalled, "Override method not called");
+            Assert.IsFalse(value2VirtualCalled, "Virtual method called when Override exists");
+        }
+        [Test]
+        public void manuallyCallingVirtualMethodInsideBaseClassCallsOverride()
+        {
+            // this to check that class are set up correct for tests above
+            serverTester.ChangeValues();
+
+            bool value2OverrideCalled = false;
+            clientTester.OnValue2ChangedOverrideCalled += () => {
+                value2OverrideCalled = true;
+            };
+
+            bool value2VirtualCalled = false;
+            clientTester.OnValue2ChangedVirtualCalled += () => {
+                value2VirtualCalled = true;
+            };
+
+            SyncVarHookTesterBase baseClass = clientTester as SyncVarHookTesterBase;
+            baseClass.CallOnValue2Changed();
+
+            Assert.AreEqual(serverTester.value2, serverTester.value2);
+            Assert.IsTrue(value2OverrideCalled, "Override method not called");
+            Assert.IsFalse(value2VirtualCalled, "Virtual method called when Override exists");
+        }
+    }
+}

Assets/Mirror/Tests/Editor/SyncVarVirtualTest.cs.meta

@@ -0,0 +1,11 @@
+fileFormatVersion: 2
+guid: c3e3ffcfc0a8d764e8a2cdb0473afab9
+MonoImporter:
+  externalObjects: {}
+  serializedVersion: 2
+  defaultReferences: []
+  executionOrder: 0
+  icon: {instanceID: 0}
+  userData: 
+  assetBundleName: 
+  assetBundleVariant: