post-kerbin-mining-corporation / StationPartsExpansionRedux

Adds an extensive set of space station parts to Kerbal Space Program
54 stars 43 forks source link

MM Patch causing issues #305

Closed zer0Kerbal closed 2 years ago

zer0Kerbal commented 2 years ago

traisjames

Posted 17 hours ago I made a discovery today, probably one already well known but not to me, but mod folders left behind after removing mods such as with CKAN can cause issues. After removing Extraplanetary Launchpads via CKAN I could no longer launch the game due to B9PartSwitch errors thrown with StationPartsExpansionRedux. The error log kept pointing me towards a config file in SSPXr

B9_TANK_TYPE:NEEDS[ExtraplanetaryLaunchpads,!SimpleConstruction,!MKS] Which means somehow B9 believes Extraplanetary Launchpads was still installed. I did manage to find folder ExtraplanetaryLaunchpads with 1 icon files and the settings.cfg file. Deleting that folder makes the game work! Either the folder or the config file left over was enough to cause the game to think the full mod is there. Something for people to think about when having odd issues.

I will post a PR shortly to fix this.

ChrisAdderley commented 2 years ago

This kinda just passes the buck between various patches. The correct answer is to ensure that EPL is removed properly, rather than assume that the lack of the Launchpad dll means the mod is removed. The presence or absence of that file doesn't really ensure the definitions will load correctly; if Launchpad is there but the rest of the mod isn't, things will break, which is extremely possible under install conditions that use EPL as a driver rather than a provider.

zer0Kerbal commented 2 years ago

EPL is removed properly

can't control that. that is a PEBCAK issue.

This fixes the thing(s) that are within our control. πŸ‘

ChrisAdderley commented 2 years ago

Eh, the patch you proposed is definitely a solution. However, it potentially causes a failure mode that is arguably harder to detect. The root cause of the issue isn't that EPL is missing, it's that EPL's resource definitions are missing. If we are detecting the EPL dll, we are are not detecting the presence of the resources. It's relatively easy to tell a user to ensure that a folder is deleted vs tell a user to look for a dll that may or may not be present, particularly since some mods use EPL as a backend for construction with different resources - EPL's dll presence in that case will be a false positive and cause the patch to fail.

zer0Kerbal commented 2 years ago

easy to tell what .dll's are installed; look in the first part of the KSP.log - lists not only all the .dll's (and if there are duplicates), but also all the directories under GameData.

Also - all the resources (IIRC) that are added either through Squad/SquadExpansion and others are also listed in the KSP.log

I understand the issue about missing resources, which should present a different error message (if not then that is an issue with B9PS which should be addressed).

I use a combination of VSCodium/NotePad++/and LogFusion (Mostly LogFusion) to quickly skim a KSP.log - usually doesn't take long to find issues. Too bad B9PS doesn't also do a log. chuckle

I use :NEEDS[folder] as well, but always try to use the dll name; just seems to reduce module manager flakiness.

Concerning the PEBCAK issues; CKAN takes care of most of those issues (removal) except for mac and linux? (I don't know about this). This one issue stems from a MacOS install - which didn't remove. πŸ€·β€β™‚οΈ can't win them all.

ChrisAdderley commented 2 years ago

That's not really what I'm describing. Targeting the presence of the dll for this particular case of patching is bad, because the patch will break if the dll exists but the rest of the mod does not. The patch's functioning relies on the resource definitions present in the EPL folder, not the EPL dll. There have been mods in the past that specifically redistribute the dll without the rest of EPL, so this isn't a crazy hypothetical.

The patch dependency should be as close to the requirement as possible. Since I can't easily do a needs presence of resource xyz, checking for the folder that should contain the definition is as close as I can get (actually there is probably a way to check for the resource def, but it's not really worth it).

On the other hand, if I was patching something that purely depended on a dll presence - sure, better to target that. This isn't the case here though!

Lastly, anything that requires the user to look at a log or a text file is basically second level diagnosis. Most users understand what folders are. Fewer users understand what to look for in a log. Even fewer are going to understand the specifics of dll/version matching. Pick the solution that requires the least expertise. I've been modding this game for a long time. I might know what I'm doing.

zer0Kerbal commented 2 years ago

I am curious which mod you are referring to, the only one I can think of that uses launchpad.dll like that is SimpleConstruction! (the name has an exclamation mark); and I cannot ever remember it not including the resource definitions, at least since I've been using it or authoring it.

I see your point.

A lifelong friend of mine recently brought up the concept of 'legislating the exception' a legal concept (this friend wears black robes for their day job) It means writing a law to handle an exception, an outlier; less than 0.1% of situations. Means it is folly and wasteful to write a law to handle an outlier instead of using existing laws or a new law to handle >99.9999% of situations.

So 'patching the outlier' would mean about the same in our situation. The situation where the dll is installed but 'MetalOre' is not installed is an outlier; and checking for the folder doesn't fix the underlying outlying situation you reference above. The situation should cause B9 to scream and stop the game load.

If the resource is missing, then it will break B9. Checking for launchpad and not finding the RESOURCE_DEFINITION will be only way the patch breaks. Either way of checking the user will come to you (or somebody else) blaming you or somebody for breaking their game. Most can't/won't diagnose it; so you (or somebody else) will do what happened here: ask for help and provide the KSP.log (not needed in this case).

What I am saying is that 'patching the outlier' seems to be a waste of your time; patch for the 99.99% not the 0.01%

Ideally the situation should cause more than one fix; it should be used to maximum effect and fix any and all root causes (other than PEBCAK, which is not really practical to fix) πŸ˜€ πŸ˜‰ 😜

With that said:

Too bad this wouldn't work (it might, I haven't tried combined it like this) :NEEDS[@RESOURCE_DEFINITION[MetalOre],Launchpad,!SimpleConstruction,!MKS]

can one stack NEEDS? so

:NEEDS[@RESOURCE_DEFINITION[MetalOre]]:NEEDS[Launchpad,!SimpleConstruction,!MKS]

FYI: the next version of SimpleConstruction! (or the one after) will break your patch because there won't be a EL folder, just a SimpleConstruction/Plugins/Launchpad.dll; because having both and EL and SC folder is a headache - major cause of installation issues (other than PEBCAK). Fortunately SCON! provides its own BPTankTypes and tank switches.

SimpleConstruction already handles the situation where CRP|!CRP https://github.com/zer0Kerbal/SimpleConstruction/blob/master/GameData/SimpleConstruction/Config/Resources.cfg

not suggesting you do that, rather that β€œthere are more ways to heaven than one.”

Have a blessed and enjoyable weekend!

zed'K