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 :
KSPCommunityFixes/KSPCommunityFixes/Performance/ConfigNodePerf.cs
Lines 361 to 367 in 213177b
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.