Fix #195: clone the srfAttachNode when reversing a surface attachment during re-root

GameData/KSPCommunityFixes/Settings.cfg

@@ -174,7 +174,10 @@ KSP_COMMUNITY_FIXES
 
   // Disable the stock behavior of altering surface attachment nodes on re-rooting, which is 
   // unnecessary and doesn't work correctly, leading to permanently borked attachement nodes.
-  ReRootPreserveSurfaceAttach = true
+  // Replaces ReRootPreserveSurfaceAttach with some better (but more complex) logic.
+  // If this patch causes an issue, try turning it off and turning on ReRootPreserveSurfaceAttach
+  ReRootCloneSurfaceAttach = true
+  ReRootPreserveSurfaceAttach = false
 
   // Fix leaking a camera and spotlight created by the thumbnail system on certain failures
   ThumbnailSpotlight = true

KSPCommunityFixes/BugFixes/ReRootCloneSurfaceAttach.cs

@@ -0,0 +1,106 @@
+using Expansions.Missions.Editor;
+using HarmonyLib;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using UnityEngine;
+
+namespace KSPCommunityFixes.BugFixes
+{
+    class ReRootCloneSurfaceAttach : BasePatch
+    {
+        // the name here isn't important, but if anyone is debugging an issue I'd like to make it clear where it came from.
+        // I'm pretty sure the empty string has some special meaning, and we need to be sure it doesn't collide with any existing node IDs
+        // we also use this to fix up attach nodes when loading a craft file.
+        const string CLONED_NODE_ID = "KSPCF-reroot-srfAttachNode";
+
+        protected override void ApplyPatches(List<PatchInfo> patches)
+        {
+            patches.Add(new PatchInfo(
+                PatchMethodType.Prefix,
+                AccessTools.Method(typeof(AttachNode), nameof(AttachNode.ReverseSrfNodeDirection)),
+                this));
+
+            patches.Add(new PatchInfo(
+                PatchMethodType.Prefix,
+                AccessTools.Method(typeof(AttachNode), nameof(AttachNode.ChangeSrfNodePosition)),
+                this));
+
+            patches.Add(new PatchInfo(
+                PatchMethodType.Postfix,
+                AccessTools.Method(typeof(Part), nameof(Part.FindAttachNode)),
+                this));
+
+            patches.Add(new PatchInfo(
+                PatchMethodType.Postfix,
+                AccessTools.Method(typeof(EditorLogicBase), nameof(EditorLogicBase.clearAttachNodes)),
+                this));
+        }
+
+        // In stock, this function is called after reversing a surface attachment during a re-root operation.
+        // it tries to alter a part's surface attachment so that it mirrors the surface attach node of its parent.
+        // But that's not a great idea, because a lot of things depend on the surface attach node never changing.
+        // For example, if the user then picks the part back up, it won't attach the same way to anything else
+        // To fix this, instead of using the child's actual srfAttachNode we create a new surface attach node and
+        // just stick it in the regular AttachNode list.
+        static bool AttachNode_ReverseSrfNodeDirection_Prefix(AttachNode __instance, AttachNode fromNode)
+        {
+            // Ideally we would be cloning fromNode in order to match the original attachment as closely as possible.
+            // however when loading a saved craft file, we won't know anything about that node, possibly leading
+            // to differing behavior.  So we'll clone this part's attachnode instead.
+            AttachNode newSrfAttachNode = AttachNode.Clone(__instance);
+            newSrfAttachNode.attachedPart = fromNode.owner;
+            newSrfAttachNode.id = CLONED_NODE_ID;
+
+            // convert the position, orientation from the other part's local space into ours
+            Vector3 positionWorld = fromNode.owner.transform.TransformPoint(fromNode.position);
+            Vector3 orientationWorld = fromNode.owner.transform.TransformDirection(fromNode.orientation);
+            newSrfAttachNode.originalPosition = newSrfAttachNode.owner.transform.InverseTransformPoint(positionWorld);
+            newSrfAttachNode.originalOrientation = -newSrfAttachNode.owner.transform.InverseTransformDirection(orientationWorld);
+            newSrfAttachNode.position = newSrfAttachNode.originalPosition;
+            newSrfAttachNode.orientation = newSrfAttachNode.originalOrientation;
+            newSrfAttachNode.owner.attachNodes.Add(newSrfAttachNode);
+
+            // now clear the srfAttachNodes from both parts
+            __instance.attachedPart = null;
+            fromNode.attachedPart = null;
+
+            return false;
+        }
+
+        // this function is just horribly broken and no one could call it, ever
+        static bool AttachNode_ChangeSrfNodePosition_Prefix()
+        {
+            return false;
+        }
+
+        static void Part_FindAttachNode_Postfix(Part __instance, string nodeId, ref AttachNode __result)
+        {
+            // TODO: ideally we would only do this if we know we're in the process of loading a craft file.  Is there any way to check that?
+            if (__result == null && nodeId == CLONED_NODE_ID)
+            {
+                AttachNode newSrfAttachNode = AttachNode.Clone(__instance.srfAttachNode);
+                newSrfAttachNode.id = CLONED_NODE_ID;
+                __instance.attachNodes.Add(newSrfAttachNode);
+                __result = newSrfAttachNode;
+            }
+        }
+
+        // In the editor, when detaching parts, remove the extra attachnode we added
+        static void EditorLogicBase_clearAttachNodes_Postfix(Part part)
+        {
+            for (int i = 0; i < part.attachNodes.Count; i++)
+            {
+                AttachNode attachNode = part.attachNodes[i];
+
+                if (attachNode.id == CLONED_NODE_ID && attachNode.attachedPart.IsNullRef())
+                {
+                    part.attachNodes.RemoveAt(i);
+                    return;
+                }
+            }
+        }
+    }
+}

KSPCommunityFixes/BugFixes/ReRootPreserveSurfaceAttach.cs

@@ -6,6 +6,8 @@
 using System.Collections.Generic;
 using System.Reflection;
 using System.Reflection.Emit;
+using UnityEngine.UIElements;
+using UnityEngine;
 
 #if REROOT_DEBUG_MODULE
 using UnityEngine;
@@ -18,23 +20,47 @@ class ReRootPreserveSurfaceAttach : BasePatch
         protected override void ApplyPatches(List<PatchInfo> patches)
         {
             patches.Add(new PatchInfo(
-                PatchMethodType.Transpiler,
-                AccessTools.Method(typeof(Part), nameof(Part.SetHierarchyRoot)),
+                PatchMethodType.Prefix,
+                AccessTools.Method(typeof(AttachNode), nameof(AttachNode.ReverseSrfNodeDirection)),
+                this));
+
+            patches.Add(new PatchInfo(
+                PatchMethodType.Prefix,
+                AccessTools.Method(typeof(AttachNode), nameof(AttachNode.ChangeSrfNodePosition)),
                 this));
         }
 
-        // skip the portion of that method that alter surface nodes position/orientation on re-rooting, 
-        // by returning after the recursive SetHierarchyRoot() call.
-        private static IEnumerable<CodeInstruction> Part_SetHierarchyRoot_Transpiler(IEnumerable<CodeInstruction> instructions)
+        // In stock, this function is called after reversing a surface attachment during a re-root operation.
+        // it tries to alter a part's surface attachment so that it mirrors the surface attach node of its parent.
+        // But that's not a great idea, because a lot of things depend on the surface attach node never changing.
+        // For example, if the user then picks the part back up, it won't attach the same way to anything else
+        // To Fix this, instead of using the child's actual srfAttachNode, we create a new surface attach node and
+        // just stick it in the regular AttachNode list.
+        static bool AttachNode_ReverseSrfNodeDirection_Prefix(AttachNode __instance, AttachNode fromNode)
         {
-            MethodInfo m_Part_SetHierarchyRoot = AccessTools.Method(typeof(Part), nameof(Part.SetHierarchyRoot));
+            // note that instead of cloning the child's srfAttachNode and using its properties, we use the fromNode
+            // because we want to mirror the previous state as much as possible - this node WAS the other part's srfAttachNode
+            AttachNode newSrfAttachNode = AttachNode.Clone(fromNode);
+            newSrfAttachNode.owner = __instance.owner;
+            newSrfAttachNode.attachedPart = fromNode.owner;
+            newSrfAttachNode.id = "KSPCF-reroot-srfAttachNode";
+            Vector3 positionWorld = fromNode.owner.transform.TransformPoint(fromNode.position);
+            Vector3 orientationWorld = fromNode.owner.transform.TransformDirection(fromNode.orientation);
+            newSrfAttachNode.position = newSrfAttachNode.originalPosition = newSrfAttachNode.owner.transform.InverseTransformPoint(positionWorld);
+            newSrfAttachNode.orientation = newSrfAttachNode.originalOrientation = -newSrfAttachNode.owner.transform.InverseTransformDirection(orientationWorld);
+            newSrfAttachNode.owner.attachNodes.Add(newSrfAttachNode);
+
+            // now clear the srfAttachNodes from both parts
+            __instance.attachedPart = null;
+            fromNode.attachedPart = null;
+
+            return false;
+        }
 
-            foreach (CodeInstruction instruction in instructions)
-            {
-                yield return instruction;
-                if (instruction.opcode == OpCodes.Callvirt && ReferenceEquals(instruction.operand, m_Part_SetHierarchyRoot))
-                    yield return new CodeInstruction(OpCodes.Ret);
-            }
+        // this function is just horribly broken and no one could call it, ever
+        static bool AttachNode_ChangeSrfNodePosition_Prefix()
+        {
+            return false;
         }
     }
 

KSPCommunityFixes/KSPCommunityFixes.csproj

@@ -112,6 +112,7 @@
     <Compile Include="BugFixes\MapSOCorrectWrapping.cs" />
     <Compile Include="BugFixes\FixGetUnivseralTime.cs" />
     <Compile Include="BugFixes\ModuleAnimateGenericCrewModSpawnIVA.cs" />
+    <Compile Include="BugFixes\ReRootCloneSurfaceAttach.cs" />
     <Compile Include="BugFixes\ReRootPreserveSurfaceAttach.cs" />
     <Compile Include="BugFixes\RespawnDeadKerbals.cs" />
     <Compile Include="BugFixes\ThumbnailSpotlight.cs" />