KSPModdingLibs/KSPCommunityFixes

ISP upgrades incorrectly applied on some MOLE engines

gotmachine opened this issue · 1 comments

Repro setup :

  • KSP 1.12.5
  • KSPCF 1.29.0
  • MOLE 1.27 + "Pristine Play mode"

Some MOLE engines ("WB-2 Corvette", "LV-T270 Fulcrum"...) have stock upgrades defined, notably an ISP upgrade applied at the precision propulsion node. With KSPCF installed, these upgrades somehow result in an ISP of 0.001 once applied.

Debugging indeed show that on the PartModule.ApplyUpgradeNode() call, the following node is being applied to the module :

{CURRENTUPGRADE
{
	name__ = CorvetteISP1
	description__ = ISP: 85 (ASL) - 350 (Vac.)
	techRequired__ = propulsionSystems
	maxThrust = 250
	atmosphereCurve
	{
		key = 7 0.001
	}
}
}

However, the node referenced in the upgrade is correct :

{UPGRADE
{
	name__ = CorvetteISP1
	description__ = ISP: 85 (ASL) - 350 (Vac.)
	techRequired__ = propulsionSystems
	atmosphereCurve
	{
		key = 0 350
		key = 1 85
		key = 7 0.001
	}
}
}

The incorrect node is the result of a ConfigNode.CopyTo() call (with overwrite: true), when the upgrade node is copied to the node effectively applied on the PM. KSPCF indeed patches the ConfigNode.CopyToRecursive() method, so I guess we have a bug in there involving multiple same-name values. This piece of code does indeed looks quite sketchy :

if (node._values.values[j].name == value.name)
{
v = node._values.values[j];
v.value = value.value;
v.comment = value.comment;
break;
}

The bug is actually here:
https://github.com/KSPModdingLibs/KSPCommunityFixes/blob/master/KSPCommunityFixes/Performance/ConfigNodePerf.cs#L375-L384

            for (int i = 0, iC = __instance._nodes.nodes.Count; i < iC; ++i)
            {
                ConfigNode sub = __instance.nodes[i];
                if (overwrite)
                {
                    node._nodes.RemoveNode(sub.name);
                }
                ConfigNode newNode = new ConfigNode(string.Empty); // will be set above when we recurse.
                node._nodes.nodes.Add(newNode);
                ConfigNode_CopyToRecursive_Prefix(sub, newNode, overwrite);
            }

In particular, the fact that we recurse passing overwrite. Stock does not. However, stock is subject to a different bug, wherein copying a node without overwrite will lead to duplicate subnodes rather than merged subnodes. Well, that's not necessarily a bug, but either way it's probably behavior we need to retain.