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

Crew spots unavailable in VAB when loading Subassemblies with crew capacity #197

Open
Bloodsucker opened this issue Feb 6, 2024 · 5 comments
Labels
kspBug Identified KSP issue

Comments

@Bloodsucker
Copy link

Bloodsucker commented Feb 6, 2024

When attempting to load a saved subassembly with crew spots in the VAB the crew selection interface fails to display any available crew spots. If the subassembly is loaded "directly as a new vessel" the crew selection interface works.

  1. Save a subassembly containing crew spots within the VAB.
  2. Begin a new vessel construction session in the VAB (important!).
  3. Attempt to load the previously saved subassembly into the current vessel.
  4. The crew selection interface fails to show any available crew spots

If the subassembly is loaded as a new vessel, the crew selection interface shows available crew spots.

IMHO, this issue is likely caused by the internals of the game EditorLogic.fetch.SpawnTemplate(shipTemplate); call which I believe is used when loading subassemblies.

@Bloodsucker Bloodsucker changed the title Crew Spots Unavailable in VAB When Loading Subassemblies with Crew Capacity Crew spots unavailable in VAB when loading Subassemblies with crew capacity Feb 6, 2024
@JonnyOThan JonnyOThan added the kspBug Identified KSP issue label Feb 8, 2024
@JonnyOThan
Copy link
Contributor

JonnyOThan commented Feb 8, 2024

There's something a little more to the repro steps. Initially, the subassembly I was testing with was just a single part: the mk1pod. I could not reproduce the issue - the crew tab always updated properly.

Then I tried with a subassembly of two parts: a fuel tank as the root, and the mk1pod - and then I could reproduce the issue.

Notably, when you construct the ship with the fuel tank as the root, the editor will not auto-populate the crew manifest. I thought maybe this was affecting how the subassembly was being saved so I tried again with just the command pod but removed Jeb before saving the subassembly. Still no repro.

Then I figured it probably has something to do with whether the root of the subassembly has any crew capacity. So I made a subassembly with 2 mk1pods. Interestingly, loading this one had BOTH slots show up in the manifest. I tried mk1pod-tank-mk1pod and had the same result.

Then I took that same mk1pod-tank-mk1pod craft, and made the tank the root part. Since you can't save this as a subassembly I used load-merge, attaching it to the side of a tank. No manifest slots.

As far as I can tell, the only determining factor is whether the root of the loaded craft/subassembly has crew capacity - if it does, the bug doesn't occur.

And finally, the workaround I learned from working with TweakScale: if you create and delete any part, the crew manifest will refresh and the slots reappear.

@JonnyOThan
Copy link
Contributor

Well that's pretty surprising....if you pull a crew part off the ship and set it aside (ghosted), it disappears from the manifest. But if you pull off a subtree that includes crewed parts, they still show up in the manifest with a "detached" marker. UNLESS the root of the subtree you pulled off has crew capacity (which is just a more generalized form of the first case).

@JonnyOThan
Copy link
Contributor

In the editor state machine there is some special logic if the part being attached or removed has a >0 crew capacity in the prefab. This might be related to this issue, because in reality what you probably care about is whether the full subtree has any crew capacity.

@gotmachine
Copy link
Contributor

gotmachine commented Feb 10, 2024

The handling of the crew manifest is a total mess, especially in the editor...

As I mentioned in the BetterEditorUndoRedo PR, the crew manifest update is done by copying over the last serialized ship state provided by the undo/redo system, which is quite sketchy to begin with.

The updates are mostly triggered from user part manipulation in the editor FSM, but as you noted, they are usually gated behind the manipulated part having a crew capacity, which lead to various inconsistencies when the manipulated part doesn't have any crew capacity, but has an attached subtree containing parts with crew capacity.

Removing that condition would indeed fix the issue with subassembly loading, and likely make things a bit more consistent in other cases as well. This would however introduce a significant additional overhead on every part manipulation, as the crew manifest rebuild are heavy operations.

I reimplemented the RefreshCrewAssignment() method with a (slightly) faster version using the live state instead of the last serialized state in the BetterEditorUndoRedo patch and replaced the calls made in the on_partPicked and on_partAttached FSM events. Removing the selectedPart.CrewCapacity > 0 condition there seems to fix this subassembly loading issue, I guess we could make a transpiler that just turn the check from > 0 to >= 0 (Changing the opcode from Ble to Blt). But I would suggest making this a separate bugfix patch/transpiler, independent from the BetterEditorUndoRedo patch.

As a side effect, this prevent the "detached" slots from appearing, which might not be desirable from an UX perspective (but at least the behavior is less inconsistent). Making this work again would require checking against detached parts as well, by changing the following :

List<Part> shipParts = EditorLogic.fetch.ship.parts;

to

List<Part> shipParts = Part.allParts;

From an UX perspective, this is nice to have as if we can make it work consistently, this mean detaching and reattaching parts/subtrees would keep crew where previously affected, something that doesn't happen and is quite annoying. But this would likely require patching additional locations from which the editor VCM is being updated.

But given how half assed the stock "detached" implementation seems to be, I would advise some caution and further testing. The detached state of a PCM isn't tracked anywhere, it only happens at the UI level. I'm pretty sure most mods checking the editor provided VCM aren't expecting detached parts/PCM to be included there (see that and that GH searches), even though this can happen occasionally and inconsistently in stock, so enabling this all the time will likely have the side effect of making this rampant bug in many mods appear more often.

So in the end, I would suggest disabling the "detached" altogether to prevent such bugs, although 100% preventing it will likely be difficult, as this can be triggered from a bunch of code paths, including some used by mods (take a look a the usage tree of the GetPartExistsFilter() method).

@JonnyOThan
Copy link
Contributor

As far as performance, it should be reasonable to change the test to "current part has crew or has children." Common case of attaching single parts should be unaffected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kspBug Identified KSP issue
Development

No branches or pull requests

3 participants