From cf55333a072c0ffe36a2972ca0a5122a48d708d0 Mon Sep 17 00:00:00 2001 From: James Frowen Date: Mon, 23 Mar 2020 13:06:24 +0000 Subject: [PATCH] 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 --- .../Processors/NetworkBehaviourProcessor.cs | 4 +- .../Weaver/Processors/SyncVarProcessor.cs | 2 +- .../Mirror/Tests/Editor/SyncVarVirtualTest.cs | 178 ++++++++++++++++++ .../Tests/Editor/SyncVarVirtualTest.cs.meta | 11 ++ 4 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 Assets/Mirror/Tests/Editor/SyncVarVirtualTest.cs create mode 100644 Assets/Mirror/Tests/Editor/SyncVarVirtualTest.cs.meta diff --git a/Assets/Mirror/Editor/Weaver/Processors/NetworkBehaviourProcessor.cs b/Assets/Mirror/Editor/Weaver/Processors/NetworkBehaviourProcessor.cs index b226d1077..59f5b32dd 100644 --- a/Assets/Mirror/Editor/Weaver/Processors/NetworkBehaviourProcessor.cs +++ b/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); diff --git a/Assets/Mirror/Editor/Weaver/Processors/SyncVarProcessor.cs b/Assets/Mirror/Editor/Weaver/Processors/SyncVarProcessor.cs index b44b392dc..09f61709b 100644 --- a/Assets/Mirror/Editor/Weaver/Processors/SyncVarProcessor.cs +++ b/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)); diff --git a/Assets/Mirror/Tests/Editor/SyncVarVirtualTest.cs b/Assets/Mirror/Tests/Editor/SyncVarVirtualTest.cs new file mode 100644 index 000000000..00f242218 --- /dev/null +++ b/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(); + serverTester = gameObject1.AddComponent(); + + GameObject gameObject2 = new GameObject(); + netIdClient = gameObject2.AddComponent(); + clientTester = gameObject2.AddComponent(); + + 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"); + } + } +} diff --git a/Assets/Mirror/Tests/Editor/SyncVarVirtualTest.cs.meta b/Assets/Mirror/Tests/Editor/SyncVarVirtualTest.cs.meta new file mode 100644 index 000000000..4f78a6cd7 --- /dev/null +++ b/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: