Fix #521 (#527)

Fixing the issue of multiple actions calling the same method was (partially) fixed by syncing both the hashed method's description and the hashed original target's type description.

This only fixes the issue partially, as it will only work if the type we're syncing is a generic type and each possible action uses a different generic arguments. However, it should be enough to handle everything in the game, as well as most (if not all) mods.

As for fixing the issue where an action we sync is opening a confirmation dialog - the fix is a bit more complex.

I've added a new delegate that will work as a wrapper around the original action. It may return either null or the synced method as-is, in which case nothing will happen. However, the wrapper may return a different action that will be called instead of immediately syncing the method.

The wrapper is passed the current instance of the object, its arguments, the original `Action` that was going to be called, as well as the `Action` that, when invoked, will sync the data. The arguments are (currently) unused, but I've decided to include them for the case of future-proofing this code.

As for the wrappers I've included:

The wrapper for Caravan actions will return null (no wrapper) unless the method's declaring type is `CaravanArrivalActionUtility.<>c__DisplayClass0_1<T>`.

If the type matches then we will replace the original action with our own which will access the `action` field from the target type, and:
- If we're in a synced call, it'll execute that action.
- If not in a synced call it'll replace the action with our syncing action and call the original method, displaying the confirmation dialog (which, if accepted, will sync the call).

As for the wrapper for Transport Pods, it will return null (no wrapper) unless the method's declaring type is `TransportPodsArrivalActionUtility.<>c__DisplayClass0_0<T>`.

If the type matches then we will replace the original action with our own which will access the `uiConfirmationCallback` field.
- If executing commands, we set the confirmation to null and call the original method
  - This is done so the original method calls the actual action without the confirmation dialog
- If not executing commands and the field is null, just sync the call
- If not executing commands and the field is not null, call the confirmation callback with our syncing action as the action we pass to it

As an additional note, we access the inner compiler-generated types directly. This is due to `MpMethodUtil` not supporting generic types and methods. If we decide to make it support those, we'll need to change how we access those types.

Author: SokyranTheDragon

Source/Client/Syncing/Game/SyncActions.cs

@@ -3,6 +3,7 @@
 using System;
 using System.Collections.Generic;
 using System.Reflection;
+using HarmonyLib;
 using Verse;
 
 namespace Multiplayer.Client
@@ -12,23 +13,96 @@ static class SyncActions
         static SyncAction<FloatMenuOption, WorldObject, Caravan, object> SyncWorldObjCaravanMenus;
         static SyncAction<FloatMenuOption, WorldObject, IEnumerable<IThingHolder>, CompLaunchable> SyncTransportPodMenus;
 
+        static Type CaravanActionConfirmationType;
+        static Type TransportPodActionConfirmationType;
+
         public static void Init()
         {
-            SyncWorldObjCaravanMenus = RegisterActions((WorldObject obj, Caravan c) => obj.GetFloatMenuOptions(c), o => ref o.action);
+            void Error(string error)
+            {
+                Multiplayer.loadingErrors = true;
+                Log.Error(error);
+            }
+
+            // TODO: Use MpMethodUtil instead if we decide to make it work with generic types/methods (already in MP Compat, so use it). Or remove this TODO if we decide not to.
+            CaravanActionConfirmationType = AccessTools.Inner(typeof(CaravanArrivalActionUtility), "<>c__DisplayClass0_1`1");
+            TransportPodActionConfirmationType = AccessTools.Inner(typeof(TransportPodsArrivalActionUtility), "<>c__DisplayClass0_0`1");
+
+            if (CaravanActionConfirmationType == null) Error($"Could not find type: {nameof(CaravanArrivalActionUtility)}.<>c__DisplayClass0_1<T>");
+            if (TransportPodActionConfirmationType == null) Error($"Could not find type: {nameof(TransportPodsArrivalActionUtility)}.<>c__DisplayClass0_0<T>");
+
+            SyncWorldObjCaravanMenus = RegisterActions((WorldObject obj, Caravan c) => obj.GetFloatMenuOptions(c), ActionGetter, WorldObjectCaravanMenuWrapper);
             SyncWorldObjCaravanMenus.PatchAll(nameof(WorldObject.GetFloatMenuOptions));
 
-            SyncTransportPodMenus = RegisterActions((WorldObject obj, IEnumerable<IThingHolder> p, CompLaunchable r) => obj.GetTransportPodsFloatMenuOptions(p, r), o => ref o.action);
+            SyncTransportPodMenus = RegisterActions((WorldObject obj, IEnumerable<IThingHolder> p, CompLaunchable r) => obj.GetTransportPodsFloatMenuOptions(p, r), o => ref o.action, TransportPodMenuWrapper);
             SyncTransportPodMenus.PatchAll(nameof(WorldObject.GetTransportPodsFloatMenuOptions));
         }
 
-        static SyncAction<T, A, B, object> RegisterActions<T, A, B>(Func<A, B, IEnumerable<T>> func, ActionGetter<T> actionGetter)
+        private static ref Action ActionGetter(FloatMenuOption o) => ref o.action;
+
+        private static Action WorldObjectCaravanMenuWrapper(FloatMenuOption instance, WorldObject a, Caravan b, object c, Action original, Action sync)
+        {
+            if (instance.action.Method.DeclaringType is not { IsGenericType: true })
+                return null;
+
+            if (instance.action.Method.DeclaringType.GetGenericTypeDefinition() != CaravanActionConfirmationType)
+                return null;
+
+            return () =>
+            {
+                var field = AccessTools.DeclaredField(original.Target.GetType(), "action");
+                if (!Multiplayer.ExecutingCmds)
+                {
+                    // If not in a synced call then replace the method that will be
+                    // called by confirmation dialog with our synced method call.
+                    field.SetValue(original.Target, sync);
+                    original();
+                    return;
+                }
+
+                // If we're in a synced call, just call the action itself (after confirmation dialog)
+                ((Action)field.GetValue(original.Target))();
+            };
+        }
+
+        public static Action TransportPodMenuWrapper(FloatMenuOption instance, WorldObject worldObject, IEnumerable<IThingHolder> thingHolders, CompLaunchable compLaunchable, Action original, Action sync)
+        {
+            if (instance.action.Method.DeclaringType is not { IsGenericType: true })
+                return null;
+
+            if (instance.action.Method.DeclaringType.GetGenericTypeDefinition() != TransportPodActionConfirmationType)
+                return null;
+
+            return () =>
+            {
+                var field = AccessTools.DeclaredField(original.Target.GetType(), "uiConfirmationCallback");
+
+                if (Multiplayer.ExecutingCmds)
+                {
+                    // Remove UI confirmation during synced commands so the method is just called directly
+                    field.SetValue(original.Target, null);
+                    original();
+                    return;
+                }
+
+                var confirmation = (Action<Action>)field.GetValue(original.Target);
+                // If no confirmation dialog, just sync
+                if (confirmation == null)
+                    sync();
+                // If there's a confirmation dialog, call it with sync as its synced method
+                else
+                    confirmation(sync);
+            };
+        }
+
+        static SyncAction<T, A, B, object> RegisterActions<T, A, B>(Func<A, B, IEnumerable<T>> func, ActionGetter<T> actionGetter, ActionWrapper<T, A, B, object> actionWrapper = null)
         {
-            return RegisterActions<T, A, B, object>((a, b, c) => func(a, b), actionGetter);
+            return RegisterActions<T, A, B, object>((a, b, c) => func(a, b), actionGetter, actionWrapper);
         }
 
-        static SyncAction<T, A, B, C> RegisterActions<T, A, B, C>(Func<A, B, C, IEnumerable<T>> func, ActionGetter<T> actionGetter)
+        static SyncAction<T, A, B, C> RegisterActions<T, A, B, C>(Func<A, B, C, IEnumerable<T>> func, ActionGetter<T> actionGetter, ActionWrapper<T, A, B, C> actionWrapper = null)
         {
-            var sync = new SyncAction<T, A, B, C>(func, actionGetter);
+            var sync = new SyncAction<T, A, B, C>(func, actionGetter, actionWrapper);
             Sync.handlers.Add(sync);
 
             return sync;

Source/Client/Syncing/Handler/SyncAction.cs

@@ -9,7 +9,8 @@
 
 namespace Multiplayer.Client
 {
-    public delegate ref Action ActionGetter<T>(T t);
+    public delegate ref Action ActionGetter<in T>(T t);
+    public delegate Action ActionWrapper<in T, in A, in B, in C>(T instance, A a, B b, C c, Action original, Action syncAction);
 
     public interface ISyncAction
     {
@@ -20,11 +21,13 @@ public class SyncAction<T, A, B, C> : SyncHandler, ISyncAction
     {
         private Func<A, B, C, IEnumerable<T>> func;
         private ActionGetter<T> actionGetter;
+        private ActionWrapper<T, A, B, C> actionWrapper;
 
-        public SyncAction(Func<A, B, C, IEnumerable<T>> func, ActionGetter<T> actionGetter)
+        public SyncAction(Func<A, B, C, IEnumerable<T>> func, ActionGetter<T> actionGetter, ActionWrapper<T, A, B, C> actionWrapper = null)
         {
             this.func = func;
             this.actionGetter = actionGetter;
+            this.actionWrapper = actionWrapper ?? ((_, _, _, _, _, _) => null);
         }
 
         public IEnumerable<T> DoSync(A target, B arg0, C arg1)
@@ -40,7 +43,9 @@ public IEnumerable<T> DoSync(A target, B arg0, C arg1)
                     int j = i;
                     i++;
                     var original = actionGetter(t);
-                    actionGetter(t) = () => ActualSync(target, arg0, arg1, original);
+                    var sync = () => ActualSync(target, arg0, arg1, original);
+                    var wrapper = actionWrapper(t, target, arg0, arg1, original, sync);
+                    actionGetter(t) = wrapper ?? sync;
 
                     yield return t;
                 }
@@ -68,12 +73,18 @@ private void ActualSync(A target, B arg0, C arg1, Action original)
             SyncSerialization.WriteSync(writer, arg0);
             SyncSerialization.WriteSync(writer, arg1);
 
-            writer.WriteInt32(GenText.StableStringHash(original.Method.MethodDesc()));
-            Log.Message(original.Method.MethodDesc());
+            var methodDesc = original.Method.MethodDesc();
+            writer.Log.Node($"Method desc: {methodDesc}");
+            writer.WriteInt32(GenText.StableStringHash(methodDesc));
+
+            // If target is null then just sync the hash for null
+            var typeDesc = (original.Target?.GetType()).FullDescription();
+            writer.Log.Node($"Type desc: {typeDesc}");
+            writer.WriteInt32(GenText.StableStringHash(typeDesc));
 
             int mapId = writer.MpContext().map?.uniqueID ?? -1;
 
-            writer.Log.Node("Map id: " + mapId);
+            writer.Log.Node($"Map id: {mapId}");
             Multiplayer.WriterLog.AddCurrentNode(writer);
 
             SendSyncCommand(mapId, writer);
@@ -85,9 +96,26 @@ public override void Handle(ByteReader data)
             B arg0 = SyncSerialization.ReadSync<B>(data);
             C arg1 = SyncSerialization.ReadSync<C>(data);
 
-            int descHash = data.ReadInt32();
+            int methodDescHash = data.ReadInt32();
+            int typeDescHash = data.ReadInt32();
+
+            var action = func(target, arg0, arg1)
+                .Where(t =>
+                {
+                    var a = actionGetter(t);
+                    // Match both the method description and target type description (including generics), or "null" string for the type
+                    return GenText.StableStringHash(a.Method.MethodDesc()) == methodDescHash &&
+                           GenText.StableStringHash((a.Target?.GetType()).FullDescription()) == typeDescHash;
+                })
+                .Select(t =>
+                {
+                    var a = actionGetter(t);
+                    var w = actionWrapper(t, target, arg0, arg1, a, null);
+                    // Return the wrapper (if present) or the action itself
+                    return w ?? a;
+                })
+                .FirstOrDefault();
 
-            var action = func(target, arg0, arg1).Select(t => actionGetter(t)).FirstOrDefault(a => GenText.StableStringHash(a.Method.MethodDesc()) == descHash);
             action?.Invoke();
         }