Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISP upgrades incorrectly applied on some MOLE engines #152

Closed
gotmachine opened this issue Jun 18, 2023 · 1 comment
Closed

ISP upgrades incorrectly applied on some MOLE engines #152

gotmachine opened this issue Jun 18, 2023 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@gotmachine
Copy link
Contributor

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;
}

@gotmachine gotmachine added the bug Something isn't working label Jun 18, 2023
@NathanKell
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants