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

Fix 4739 custom protomech bay does not load #4740

Merged

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Sep 2, 2023

It looks like there was a change, or series of changes, to ProtoMechBay.java back in March of 2022 that broke compatibility between saving ProtoMech bays (or "ProtoMekBays") and loading them.

The fix I've proposed here just reads both lines and treats them equally. It's quick and easy and I've tested it with the original file from #4739 successfully.

An alternative fix would be to revert the March 2022 change completely; I feel this warrants consideration as there are several places in the MML UI (not to mention in the filename itself) where we use "ProtoMech".

Close #4739

@HammerGS
Copy link
Member

HammerGS commented Sep 2, 2023

We should move to Mek. We try to avoid Mech.

@Sleet01
Copy link
Collaborator Author

Sleet01 commented Sep 2, 2023

We should move to Mek. We try to avoid Mech.

The MML UI still uses "mech", and extant BLK files do as well. Additionally, the reporter of the bug found that there's a further mismatch between the bay type "ProtoMek" and the unit type itself, so we'd probably need to update a lot more than just BLKFile.java and the MML UI.

I propose this:

  1. roll back the partial change to "ProtoMekBay" that is breaking stuff, keeping the BLKFile.java fix that loads both formats for interim compatibly.
  2. I'll implement (either just for this type, or for all bays and units) the save/load validation unit tests that can confirm saving, loading, and, I guess, that putting units into the bays works correctly.
  3. we do a single big change to replace all instances of "mech" in the UI and code base for all 3 main programs at a later date.

@HammerGS what say you?

@HammerGS
Copy link
Member

HammerGS commented Sep 2, 2023

Sounds good to me. The move to Mek needs to happen, but there is no set timeline for it. But we should make sure there are no impacts to MekHQ as well.

@Sleet01
Copy link
Collaborator Author

Sleet01 commented Sep 3, 2023

Sounds good to me. The move to Mek needs to happen, but there is no set timeline for it. But we should make sure there are no impacts to MekHQ as well.

Well, digging into the code more, I'm finding that all three projects sport an almost-even mix of both forms ("ProtoMech" vs "ProtoMek") so maybe I'll just throw up my hands in despair and try to fix the issue with loading ProtoMechs into ProtoMek bays (or vice-versa) in MekHQ.

I'd estimate that normalizing all remaining references using "ProtoMech" -> "ProtoMek" to be at least a week of full-time work but it looks like about that much work has been done already, so it might be good to set up a longer-running branch to track that.

Sorry for all the hassle; this turned out to be a can full of other cans of worms.

@HammerGS
Copy link
Member

HammerGS commented Sep 3, 2023

Sounds good to me. The move to Mek needs to happen, but there is no set timeline for it. But we should make sure there are no impacts to MekHQ as well.

Well, digging into the code more, I'm finding that all three projects sport an almost-even mix of both forms ("ProtoMech" vs "ProtoMek") so maybe I'll just throw up my hands in despair and try to fix the issue with loading ProtoMechs into ProtoMek bays (or vice-versa) in MekHQ.

I'd estimate that normalizing all remaining references using "ProtoMech" -> "ProtoMek" to be at least a week of full-time work but it looks like about that much work has been done already, so it might be good to set up a longer-running branch to track that.

Sorry for all the hassle; this turned out to be a can full of other cans of worms.

Welcome to MegaMek! :)

A separate branch and a basic fix is good.

It looks like the ProtoMek-specific loading submenu is still very rough:
- logic error prevented checking if non-mechs could load Protos
- even if entities with bays could load Protos, they weren't getting
  added to the menu.

I cribbed some code from the general Load menu builder to add options
other than just Mek Front or Mek Rear as ProtoMek load targets; honestly
this should all get combined back into the loadMenu at some point.

Testing:
- Added regular ProtoMeks to a dropship with ProtoMekBays in MM: works
  now.
- Loaded same ProtoMeks back into the dropship after landing it in a
  game: works fine.
- Confirmed Protos with Magnetic Clamp System can also be loaded onto
  Meks, and unloaded during games.

TODO: add unit tests to validate various ProtoMek loading/unloading
options, separate from UI.
@SJuliez SJuliez merged commit 7476cbf into MegaMek:master Sep 4, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MegaMek does not populate ProtoMech Transport bays in custom dropships.
4 participants