Potential new KSP bugfix : ReRootPreserveSurfaceAttach (#142)

* New bugfix patch : ReRootPreserveSurfaceAttach

* ReRootPreserveSurfaceAttach : prepare for release

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,65 @@
+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
+    {
+        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));
+        }
+
+        // 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)
+        {
+            // 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;
+            // 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
+            newSrfAttachNode.id = "KSPCF-reroot-srfAttachNode";
+
+            // 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;
+        }
+    }
+}

KSPCommunityFixes/BugFixes/ReRootPreserveSurfaceAttach.cs

@@ -6,8 +6,6 @@
 using System.Collections.Generic;
 using System.Reflection;
 using System.Reflection.Emit;
-using UnityEngine.UIElements;
-using UnityEngine;
 
 #if REROOT_DEBUG_MODULE
 using UnityEngine;
@@ -20,53 +18,23 @@ class ReRootPreserveSurfaceAttach : BasePatch
         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)),
+                PatchMethodType.Transpiler,
+                AccessTools.Method(typeof(Part), nameof(Part.SetHierarchyRoot)),
                 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)
+        // 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)
         {
-            // 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;
-            // 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
-            newSrfAttachNode.id = "KSPCF-reroot-srfAttachNode";
-
-            // 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;
-        }
+            MethodInfo m_Part_SetHierarchyRoot = AccessTools.Method(typeof(Part), nameof(Part.SetHierarchyRoot));
 
-        // this function is just horribly broken and no one could call it, ever
-        static bool AttachNode_ChangeSrfNodePosition_Prefix()
-        {
-            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);
+            }
         }
     }
 

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" />