feat: allowing events to be used with syncvar hook (#991)

Assets/Mirage/Weaver/Processors/SyncVarProcessor.cs

@@ -231,7 +231,7 @@ MethodDefinition GenerateSyncVarSetter(FoundSyncVar syncVar)
             worker.Append(worker.Create(OpCodes.Ldc_I8, syncVar.DirtyBit));
             worker.Append(worker.Create<NetworkBehaviour>(OpCodes.Call, nb => nb.SetDirtyBit(default)));
 
-            if (syncVar.HasHookMethod)
+            if (syncVar.HasHook)
             {
                 //if (base.isLocalClient && !getSyncVarHookGuard(dirtyBit))
                 Instruction label = worker.Create(OpCodes.Nop);
@@ -251,7 +251,7 @@ MethodDefinition GenerateSyncVarSetter(FoundSyncVar syncVar)
 
                 // call hook (oldValue, newValue)
                 // Generates: OnValueChanged(oldValue, value)
-                WriteCallHookMethodUsingArgument(worker, syncVar.HookMethod, oldValue);
+                WriteCallHookMethodUsingArgument(worker, syncVar.Hook, oldValue);
 
                 // setSyncVarHookGuard(dirtyBit, false)
                 worker.Append(worker.Create(OpCodes.Ldarg_0));
@@ -333,19 +333,27 @@ void WriteStoreField(ILProcessor worker, ParameterDefinition valueParam, FoundSy
         }
 
 
-        void WriteCallHookMethodUsingArgument(ILProcessor worker, MethodDefinition hookMethod, VariableDefinition oldValue)
+        void WriteCallHookMethodUsingArgument(ILProcessor worker, SyncVarHook hook, VariableDefinition oldValue)
         {
-            WriteCallHookMethod(worker, hookMethod, oldValue, null);
+            WriteCallHook(worker, hook, oldValue, null);
         }
 
-        void WriteCallHookMethodUsingField(ILProcessor worker, MethodDefinition hookMethod, VariableDefinition oldValue, FoundSyncVar syncVarField)
+        void WriteCallHookMethodUsingField(ILProcessor worker, SyncVarHook hook, VariableDefinition oldValue, FoundSyncVar syncVarField)
         {
             if (syncVarField == null)
             {
                 throw new ArgumentNullException(nameof(syncVarField));
             }
 
-            WriteCallHookMethod(worker, hookMethod, oldValue, syncVarField);
+            WriteCallHook(worker, hook, oldValue, syncVarField);
+        }
+
+        void WriteCallHook(ILProcessor worker, SyncVarHook hook, VariableDefinition oldValue, FoundSyncVar syncVarField)
+        {
+            if (hook.Method != null)
+                WriteCallHookMethod(worker, hook.Method, oldValue, syncVarField);
+            if (hook.Event != null)
+                WriteCallHookEvent(worker, hook.Event, oldValue, syncVarField);
         }
 
         void WriteCallHookMethod(ILProcessor worker, MethodDefinition hookMethod, VariableDefinition oldValue, FoundSyncVar syncVarField)
@@ -410,6 +418,53 @@ void WriteEndFunctionCall()
             }
         }
 
+        void WriteCallHookEvent(ILProcessor worker, EventDefinition @event, VariableDefinition oldValue, FoundSyncVar syncVarField)
+        {
+            FieldReference eventField = @event.DeclaringType.GetField(@event.Name);
+            Instruction nop = worker.Create(OpCodes.Nop);
+            worker.Append(worker.Create(OpCodes.Ldarg_0));
+            worker.Append(worker.Create(OpCodes.Ldfld, eventField));
+            // jump to nop if null
+            worker.Append(worker.Create(OpCodes.Brfalse_S, nop));
+
+            worker.Append(worker.Create(OpCodes.Ldarg_0));
+            worker.Append(worker.Create(OpCodes.Ldfld, eventField));
+            WriteOldValue();
+            WriteNewValue();
+
+            MethodReference invokeNonGeneric = @event.Module.ImportReference(typeof(Action<,>).GetMethod("Invoke"));
+            MethodReference invoke = invokeNonGeneric.MakeHostInstanceGeneric((GenericInstanceType)@event.EventType);
+            worker.Append(worker.Create(OpCodes.Call, invoke));
+
+            // after if (event!=null)
+            worker.Append(nop);
+
+
+
+            // *** Local functions used to write OpCodes ***
+            // Local functions have access to function variables, no need to pass in args
+
+            void WriteOldValue()
+            {
+                worker.Append(worker.Create(OpCodes.Ldloc, oldValue));
+            }
+
+            void WriteNewValue()
+            {
+                // write arg1 or this.field
+                if (syncVarField == null)
+                {
+                    worker.Append(worker.Create(OpCodes.Ldarg_1));
+                }
+                else
+                {
+                    WriteLoadField(worker, syncVarField);
+                }
+            }
+        }
+
+
+
         void GenerateSerialization()
         {
             Weaver.DebugLog(behaviour.TypeDefinition, "  GenerateSerialization");
@@ -546,7 +601,7 @@ private VariableDefinition StartHook(ILProcessor worker, MethodDefinition deseri
             // Store old value in local variable, we need it for Hook
             // T oldValue = value
             VariableDefinition oldValue = null;
-            if (syncVar.HasHookMethod)
+            if (syncVar.HasHook)
             {
                 oldValue = deserialize.AddLocal(originalType);
                 WriteLoadField(worker, syncVar);
@@ -566,7 +621,7 @@ private VariableDefinition StartHook(ILProcessor worker, MethodDefinition deseri
         /// <param name="oldValue"></param>
         private void EndHook(ILProcessor worker, FoundSyncVar syncVar, TypeReference originalType, VariableDefinition oldValue)
         {
-            if (syncVar.HasHookMethod)
+            if (syncVar.HasHook)
             {
                 // call hook
                 // but only if SyncVar changed. otherwise a client would
@@ -592,7 +647,7 @@ private void EndHook(ILProcessor worker, FoundSyncVar syncVar, TypeReference ori
 
                 // call the hook
                 // Generates: OnValueChanged(oldValue, this.syncVar)
-                WriteCallHookMethodUsingField(worker, syncVar.HookMethod, oldValue, syncVar);
+                WriteCallHookMethodUsingField(worker, syncVar.Hook, oldValue, syncVar);
 
                 // Generates: end if (!SyncVarEqual)
                 worker.Append(syncVarEqualLabel);

Assets/Mirage/Weaver/Processors/SyncVars/FoundSyncVar.cs

@@ -34,8 +34,8 @@ public FoundSyncVar(ModuleDefinition module, FoundNetworkBehaviour behaviour, Fi
         public bool IsWrapped { get; private set; }
 
 
-        public bool HasHookMethod { get; private set; }
-        public MethodDefinition HookMethod { get; private set; }
+        public bool HasHook { get; private set; }
+        public SyncVarHook Hook { get; private set; }
         public bool InitialOnly { get; private set; }
 
         /// <summary>
@@ -86,8 +86,10 @@ private bool CheckWrapType(TypeReference originalType, out TypeReference wrapTyp
         /// <param name="module"></param>
         public void ProcessAttributes(Writers writers, Readers readers)
         {
-            HookMethod = HookMethodFinder.GetHookMethod(FieldDefinition, OriginalType);
-            HasHookMethod = HookMethod != null;
+            SyncVarHook hook = HookMethodFinder.GetHookMethod(FieldDefinition, OriginalType);
+            Hook = hook;
+            HasHook = hook != null;
+
             InitialOnly = GetInitialOnly(FieldDefinition);
 
             ValueSerializer = ValueSerializerFinder.GetSerializer(this, writers, readers);

Assets/Mirage/Weaver/Processors/SyncVars/HookMethodFinder.cs

@@ -1,15 +1,20 @@
-using System;
-using System.Collections.Generic;
+using System;
 using System.Linq;
 using Mono.Cecil;
+using Mono.Collections.Generic;
 
 namespace Mirage.Weaver.SyncVars
 {
+    class SyncVarHook
+    {
+        public MethodDefinition Method;
+        public EventDefinition Event;
+    }
     internal static class HookMethodFinder
     {
         /// <returns>Found Hook method or null</returns>
         /// <exception cref="HookMethodException">Throws if users sets hook in attribute but method could not be found</exception>
-        public static MethodDefinition GetHookMethod(FieldDefinition syncVar, TypeReference originalType)
+        public static SyncVarHook GetHookMethod(FieldDefinition syncVar, TypeReference originalType)
         {
             CustomAttribute syncVarAttr = syncVar.GetCustomAttribute<SyncVarAttribute>();
 
@@ -24,13 +29,19 @@ public static MethodDefinition GetHookMethod(FieldDefinition syncVar, TypeRefere
             return FindHookMethod(syncVar, hookFunctionName, originalType);
         }
 
-        static MethodDefinition FindHookMethod(FieldDefinition syncVar, string hookFunctionName, TypeReference originalType)
+        static SyncVarHook FindHookMethod(FieldDefinition syncVar, string hookFunctionName, TypeReference originalType)
         {
-            List<MethodDefinition> methods = syncVar.DeclaringType.GetMethods(hookFunctionName);
+            // check event first
+            EventDefinition @event = syncVar.DeclaringType.Events.Where(x => x.Name == hookFunctionName).FirstOrDefault();
+            if (@event != null)
+            {
+                return ValidateEvent(syncVar, originalType, @event);
+            }
 
-            var methodsWith2Param = new List<MethodDefinition>(methods.Where(m => m.Parameters.Count == 2));
+            MethodDefinition[] methods = syncVar.DeclaringType.GetMethods(hookFunctionName);
+            MethodDefinition[] methodsWith2Param = methods.Where(m => m.Parameters.Count == 2).ToArray();
 
-            if (methodsWith2Param.Count == 0)
+            if (methodsWith2Param.Length == 0)
             {
                 throw new HookMethodException($"Could not find hook for '{syncVar.Name}', hook name '{hookFunctionName}'. " +
                     $"Method signature should be {HookParameterMessage(hookFunctionName, originalType)}",
@@ -41,7 +52,7 @@ static MethodDefinition FindHookMethod(FieldDefinition syncVar, string hookFunct
             {
                 if (MatchesParameters(method, originalType))
                 {
-                    return method;
+                    return new SyncVarHook { Method = method };
                 }
             }
 
@@ -50,6 +61,39 @@ static MethodDefinition FindHookMethod(FieldDefinition syncVar, string hookFunct
                 syncVar);
         }
 
+        private static SyncVarHook ValidateEvent(FieldDefinition syncVar, TypeReference originalType, EventDefinition @event)
+        {
+            TypeReference eventType = @event.EventType;
+            if (!eventType.FullName.Contains("System.Action"))
+            {
+                ThrowWrongHookType(syncVar, @event, eventType);
+            }
+
+            if (!eventType.IsGenericInstance)
+            {
+                ThrowWrongHookType(syncVar, @event, eventType);
+            }
+
+            var genericEvent = (GenericInstanceType)eventType;
+            Collection<TypeReference> args = genericEvent.GenericArguments;
+            if (args.Count != 2)
+            {
+                ThrowWrongHookType(syncVar, @event, eventType);
+            }
+
+            if (args[0].FullName != originalType.FullName || args[1].FullName != originalType.FullName)
+            {
+                ThrowWrongHookType(syncVar, @event, eventType);
+            }
+
+            return new SyncVarHook { Event = @event };
+        }
+
+        private static void ThrowWrongHookType(FieldDefinition syncVar, EventDefinition @event, TypeReference eventType)
+        {
+            throw new HookMethodException($"Hook Event for '{syncVar.Name}' needs to be type 'System.Action<,>' but was '{eventType.FullName}' instead", @event);
+        }
+
         static string HookParameterMessage(string hookName, TypeReference ValueType)
             => string.Format("void {0}({1} oldValue, {1} newValue)", hookName, ValueType);
 

Assets/Mirage/Weaver/TypeExtensions.cs

@@ -18,10 +18,10 @@ public static MethodDefinition GetMethod(this TypeDefinition td, string methodNa
             return td.Methods.FirstOrDefault(method => method.Name == methodName);
         }
 
-        public static List<MethodDefinition> GetMethods(this TypeDefinition td, string methodName)
+        public static MethodDefinition[] GetMethods(this TypeDefinition td, string methodName)
         {
             // Linq allocations don't matter in weaver
-            return td.Methods.Where(method => method.Name == methodName).ToList();
+            return td.Methods.Where(method => method.Name == methodName).ToArray();
         }
 
         /// <summary>

Assets/Tests/Runtime/ClientServer/SyncVarEventHook.cs

@@ -0,0 +1,42 @@
+using System;
+using System.Collections;
+using NUnit.Framework;
+using UnityEngine.TestTools;
+
+namespace Mirage.Tests.Runtime.ClientServer
+{
+    public class BehaviourWithSyncVarEvent : NetworkBehaviour
+    {
+        [SyncVar(hook = nameof(OnHealthChanged))]
+        public int health;
+
+        public event Action<int, int> OnHealthChanged;
+    }
+
+    public class SyncVarEventHook : ClientServerSetup<BehaviourWithSyncVarEvent>
+    {
+        [UnityTest]
+        public IEnumerator SyncVarHookEventIsCalled()
+        {
+            const int SValue = 10;
+            const int CValue = 2;
+            int clientA = default;
+            int clientB = default;
+            int called = 0;
+            clientComponent.health = CValue;
+            clientComponent.OnHealthChanged += (a, b) =>
+            {
+                clientA = a;
+                clientB = b;
+                called++;
+            };
+            serverComponent.health = SValue;
+            yield return null;
+            yield return null;
+
+            Assert.That(called, Is.EqualTo(1));
+            Assert.That(clientA, Is.EqualTo(CValue));
+            Assert.That(clientB, Is.EqualTo(SValue));
+        }
+    }
+}

Assets/Tests/Weaver/SyncVarHookTests.cs

@@ -78,5 +78,32 @@ public void ErrorForWrongTypeNewParameter()
             HasError($"Wrong type for Parameter in hook for 'health', hook name 'onChangeHealth'. Method signature should be {OldNewMethodFormat("onChangeHealth", "System.Int32")}",
                 "System.Int32 SyncVarHookTests.ErrorForWrongTypeNewParameter.ErrorForWrongTypeNewParameter::health");
         }
+
+        [Test]
+        public void FindsHookEvent()
+        {
+            IsSuccess();
+        }
+
+        [Test]
+        public void ErrorWhenHookNotAction()
+        {
+            HasError($"Hook Event for 'health' needs to be type 'System.Action<,>' but was 'SyncVarHookTests.ErrorWhenHookNotAction.DoStuff' instead",
+                "SyncVarHookTests.ErrorWhenHookNotAction.DoStuff SyncVarHookTests.ErrorWhenHookNotAction.ErrorWhenHookNotAction::OnChangeHealth");
+        }
+
+        [Test]
+        public void ErrorWhenNotGenericAction()
+        {
+            HasError($"Hook Event for 'health' needs to be type 'System.Action<,>' but was 'System.Action' instead",
+                "System.Action SyncVarHookTests.ErrorWhenNotGenericAction.ErrorWhenNotGenericAction::OnChangeHealth");
+        }
+
+        [Test]
+        public void ErrorWhenEventArgsAreWrong()
+        {
+            HasError($"Hook Event for 'health' needs to be type 'System.Action<,>' but was 'System.Action`2<System.Int32,System.Single>' instead",
+                "System.Action`2<System.Int32,System.Single> SyncVarHookTests.ErrorWhenEventArgsAreWrong.ErrorWhenEventArgsAreWrong::OnChangeHealth");
+        }
     }
 }

Assets/Tests/Weaver/SyncVarHookTests~/ErrorWhenEventArgsAreWrong.cs

@@ -0,0 +1,13 @@
+using Mirage;
+using System;
+
+namespace SyncVarHookTests.ErrorWhenEventArgsAreWrong
+{
+    class ErrorWhenEventArgsAreWrong : NetworkBehaviour
+    {
+        [SyncVar(hook = nameof(OnChangeHealth))]
+        int health;
+
+        public event Action<int, float> OnChangeHealth;
+    }
+}

Assets/Tests/Weaver/SyncVarHookTests~/ErrorWhenHookNotAction.cs

@@ -0,0 +1,14 @@
+using Mirage;
+using System;
+
+namespace SyncVarHookTests.ErrorWhenHookNotAction
+{
+    delegate void DoStuff(int a, int b);
+    class ErrorWhenHookNotAction : NetworkBehaviour
+    {
+        [SyncVar(hook = nameof(OnChangeHealth))]
+        int health;
+
+        public event DoStuff OnChangeHealth;
+    }
+}

Assets/Tests/Weaver/SyncVarHookTests~/ErrorWhenNotGenericAction.cs

@@ -0,0 +1,13 @@
+using Mirage;
+using System;
+
+namespace SyncVarHookTests.ErrorWhenNotGenericAction
+{
+    class ErrorWhenNotGenericAction : NetworkBehaviour
+    {
+        [SyncVar(hook = nameof(OnChangeHealth))]
+        int health;
+
+        public event Action OnChangeHealth;
+    }
+}