Fix many issues with upgrades

[NathanKell]

These patches fix a large number of issues with upgrades. One default to enabled; the other, which requires it, fixes the weird case where creating a PartListTooltip for a part with upgrades enabled applies those upgrades (irreversibly!) to the prefab.

To fix this, the patch will create a new instantiation of the prefab and use that for the PartListTooltip instead of the prefab itself. This means when the upgrades are applied, they will be applied to the new instance, not the base prefab.

As an optimization, the substitute part will only be created when the prefab has upgrades enabled for it.

The substitute parts are destroyed on scene change, but otherwise cached so this cost only applies the first time you mouse over a part with upgrades.

[NathanKell]

@gotmachine
cc @DRVeyl

[gotmachine]

TBH, I'm a bit torn on this one.

The stock issue it's trying to solve is definitely annoying, but this has a lot of potential for causing problems.

We can't possibly anticipate what is happening in modules OnAwake()/OnLoad() (and even OnSave()). Potential side effects will be sneaky and are likely to result in hard to track down issues.

And even if they are reported, I don't want to get in the situation where plugin authors need to adapt their code to what KSPCF is doing, and having to manage per-mod compatibility issues from the KSPCF side isn't very viable either.

I would prefer to put that patch in the "disabled by default" category. At least people enabling it will have a vague awareness they did it, which might help in having issues actually reported.

[NathanKell]

I'm actually awake now and back at work at this. But (per discord) it turns out that large swathes of the upgrade system got broken after I left, so I need to fix them all first. :(

[NathanKell]

TBH, I'm a bit torn on this one.

The stock issue it's trying to solve is definitely annoying, but this has a lot of potential for causing problems.

We can't possibly anticipate what is happening in modules OnAwake()/OnLoad() (and even OnSave()).

Potential side effects will be sneaky and are likely to result in hard to track down issues.
OnAwake/OnLoad already run when you load a ship construct in the editor (and OnAwake runs even when you spawn a part), and OnSave when you save it, so I'm not sure what side effects those might cause. I agree it's plausible there's danger doing so in the SpaceCenter scene, although I am not entirely sure what that could be, but I take your point.

And even if they are reported, I don't want to get in the situation where plugin authors need to adapt their code to what KSPCF is doing, and having to manage per-mod compatibility issues from the KSPCF side isn't very viable either.

This I 100% agree with.

I would prefer to put that patch in the "disabled by default" category. At least people enabling it will have a vague awareness they did it, which might help in having issues actually reported.

I am not actually sure how many folks use upgrades outside of the RO community--honestly it can't be that many, given how broken they have apparently become since I left Squad--so I think in terms of RO impact, starting it disabled and flipping it true in RO/RP-1 works for us. Though I feel bad for anyone else who would want to use them. I'll ask around at the Hole.

[NathanKell]

Had to add OnStart (in another try/catch) because Kerbalism's Configure throws in GetInfo if OnStart isn't run (on a clone part, since it deserializes in OnStart instead of OnLoad).

[NathanKell]

Updated PR title and comment. Note that the substitute-part fix depends on the basic UpgradeBugs fix, but I'm not sure how to have KSPCF not apply it if the latter is disabled.

[gotmachine]

OnAwake/OnLoad already run when you load a ship construct in the editor (and OnAwake runs even when you spawn a part), and OnSave when you save it, so I'm not sure what side effects those might cause. I agree it's plausible there's danger doing so in the SpaceCenter scene, although I am not entirely sure what that could be, but I take your point.

Had to add OnStart

My main concern is patterns where PartModule instances are registering themselves from OnAwake/OnLoad/OnStart to higher level data/logic. Obvious case is GameEvents, but there are other possible patterns, for example instances adding themselves to a collection in a top level singleton class like a KSPAddon.

I agree that the probability of that causing actual problems is fairly low, but in any case there will definitely be PartModules that won't produce expected GetInfo() results, as it's impossible to reproduce their initialization sequence reliably and without some modules just throwing an exception for one reason or another. This is a topic I explored extensively when trying to implement generalized module stats/config switching (@blowfishpro also did a fair bit of experiments there), and there are just too many edge cases relying on tiny implementation details (for example the execution order of Start() vs OnStart() vs OnStartFinished()) for anything 100% reliable to be achieved. Not to mention mods hacking their way to alter the results of stock modules GetInfo() by hijacking its results at a latter point of execution.

Note that the substitute-part fix depends on the basic UpgradeBugs fix, but I'm not sure how to have KSPCF not apply it if the latter is disabled.

You can achieve that by defining a [PatchPriority(Order = x)] attribute on both BasePatch class definitions, having the substitute one with a higher Order. Then in the substitute patch, override CanApplyPatch(), returning false if KSPCommunityFixes.GetPatchInstance<UpgradeBugs>() returns null.

[NathanKell]

Thanks, added the ordering and the check.

Will follow up on discord re: any other ways to do the patch.

[gotmachine]

For the record, we had some discussion on discord about how to handle getting up-to-date upgrade info in a safer way than what the PartTooltipUpgradesApplyToSubstituteParts patch is doing. The current solution is to instantiate "hidden parts" on part tooltip mouseover to apply upgrades on them, and have the tooltip get part/modules info from that part. This is not a very satisfactory solution because :

• This has a quite noticeable impact on the part list UI reactivity and is overall quite resource intensive.
• We can't guarantee that modules will react correctly to the instantiation/init sequence and provide the expected GetInfo() results.
• Triggering the instantiation/init sequence of modules on "phantom parts" has quite some potential for deeper sneaky side-issues. Main concern is modules registering themselves to GameEvents or higher level logic components.

For these reasons, the PartTooltipUpgradesApplyToSubstituteParts will stay disabled by default. That fix might a good solution for RP1 as it is a somewhat controlled environment, and the RP-1 people would likely be able to identify and fix such issues quickly. But for the general modding jungle, this is simply too risky. Those side issues would likely not even get reported and even if they do, implementing per-module compatibility fixes isn't a sane option

There was additional discussion about alternatives to work around the whole issue, and I think two options emerged :

• Removing entirely the application of upgrades to prefabs, and just appending the applied upgrades name/description to the original GetInfo results. This is a big step backwards in terms of user experience when upgrades are involved, but it is the only 100% safe fix to that issue.
• Let the issue happen, but keep track of which upgrades are applied to which prefabs, and then check that there is no mismatch every time a part is instantiated. In case of mismatch, inform the user and allow him to exit the game before the save is corrupted. This is an awful user experience too, so probably not a great option.

I would personally lean on implementing the first option as the default one for KSPCF. RP1 aside, upgrades are far from being widely used (when they are, it is usually through B9PS and the issue doesn't apply in that case).

In the meantime, I guess we can merge this as is ? Further refinements can come latter.

One question : the PR removes the HidePartUpgradeExtendedInfo patch. Isn't that patch still useful when the PartTooltipUpgradesApplyToSubstituteParts patch is disabled ?

[NathanKell]

I asked on the Hole and there's actually a fair few folks who use upgrades. But I agree that it's too dangerous to default on.

As to your question, no. The issue is that I wrote an Extended Info panel for upgrades back in 1.2 (which I forgot about until I started working on this patch), but at some point (probably 1.3, and likely as a copy-pasta error) the code triggering it (i.e. calling CreateExtendedUpgradeInfo instead of CreateExtendedInfo) was replaced with the standard call.
Since this patch fixes that, too, extended info should no longer be disabled--it's now (back to) working properly, showing all the parts that make use of the upgrade.

[NathanKell]

Oh, to clarify further: there are two patches in this PR, in case it wasn't clear: one that does the substitution (that defaults off) and one that fixes a ton of bugs with Upgrades that crept in over the years (and likely a few of mine from the start :D ). The latter patch is the one that fixes the extended info, and so completely superceded the old hide-extended-info thing.

Oh! That reminds me. Should probably fix the thing where partInfo.entryCost is used instead of upgrade.entryCost if the latter is 0. That was due to someone seeing my typo (when I copypasted the tooltip I left that line as it was by mistake) and thinking I meant something by it, I guess.

[NathanKell]

Fixed the merge conflicts. Rebase choked so there is (alas) a merge commit.