search

KSPModdingLibs / KSPCommunityFixes

Community patches for bugs in the KSP codebase.
https://github.com/KSPModdingLibs/KSPCommunityFixes/releases/latest
57 stars 19 forks source link

New patch : fix the SAS inconsistency when kerbal XP is off #144

Closed Goufalite closed 1 year ago

Goufalite commented 1 year ago

Hello,

Here's a patch to fix the inconsistent SAS locks between game modes, probes and kerbals. It just puts back SAS as a default career behavior for any game mode and wether the Enable Kerbal experience setting is on or off.

More info and 6-year old bug report. In short scientists and engineers can fully SAS when the Enable Kerbal experience is off when starting a save. According to the ingame KSPedia only pilots (and probes) can SAS and there is no specific way to fine tune this trait in the game. Career players need to add probes (the lightest is the Okto2 which weights 40kg and doesn't have maneuver lock).

Parachutes and part repair have the same problem but these can easily be roleplayed and don't require to check and compare everytime the probes and pilots that are on the ship, so they are not covered by this patch.

I think it's better to leave it deactivated by default to prevent a strict gameplay change for players used to it. Everyone sends Bob on missions because he can SAS, repack parachutes and restore science.

The minimum version is 1.11.1 because I started working on this fix when SAS was completely broken for several months, but it should work on previous versions.

Smoke test :

Quick Note: a mod called BetterSpecializationSettings exists which addresses this problem but goes further on overriding settings like a special setting for all kerbals to SAS or level repair override for engineers, doesn't manage the new 1.11 repair system and looks unmaintained (works until 1.7) .

Thanks!

gotmachine commented 1 year ago

As far as I tested, this what happens in stock in 1.12+ :

I highlighted 2 cases above :

  1. It can feel a bit weird that disabling kerbal experience in career also has the effect of probes always providing max SAS level, but I think it actually somewhat make sense. If SAS levels don't exist for Kerbals, they might also no exist for probes. I'd say it's very much a personal preference.
  2. Enabling the "All SAS modes on all probes" setting having no effect when Kerbal experience is enabled in sandbox/science is a bit more questionable, but I wouldn't qualify that as a bug either.

Overall, I agree that the settings matrix for the SAS level feature is quite messy, but nothing that I would qualify as a bug. And more importantly, changing either of the two points highlighted above (or others, which the patch is definitely doing) is very much a personal preference.

As far as I can tell, your patch is not only an arbitrary revamp of the settings matrix, but also adds SAS capabilities to all command modules that don't require crew, regardless of the presence of ModuleSAS, which is even more a personal preference, and from a technical PoV, is doing it in a hardcoded way that is inconsistent with the KSP API (there are other ways to check SAS level beside the APSkillExtensions.AvailableAtLevel() method), prone to mod compatibility issues (you're doing a strict type check against ModuleCommand, and technically SAS capability can be provided by other modules), and adds additional performance overhead (additional iteration on all the vessel modules).

I would expect changes such as the ones you're suggesting to be somewhat user-configurable, and because KSPCF is a big bag of various stuff, it isn't expected of users to configure anything. Making the patch disabled by default means most people won't ever be aware it exists, and I find the proposed changes very much arbitrary and down to your own preferences.

KSPCF isn't meant to add or changes features, and while it adds some QoL patches, experience has proven that even trivial changes can be controversial, and add potential issues and maintenance burden.

I would suggest to release that work as your own plugin. This being part of KSPCF will have a much lower visibility, doesn't fix any bug and is a plain modification of the stock behavior, the "ignore ModuleSAS" thing specifically is definitely not compatible with KSPCF standards in that regard.

Goufalite commented 1 year ago

Hi, thank you for the explanation. I better understand the way KSPCF works.

You're right, my patch is just one of many ways to address this problem and is not suited for a simple "active/unactive" . Adding SAS to any command module was to handle the Stayputnik (and maybe other modded probes) which don't have a ModuleSAS, an empty Mk1 pod still couldn't SAS without a kerbal. But as you mentionned it might bring conflicts.

Thanks for the review and good luck with KSPCF's maintenance ;)