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 1182 mixed tech ds do not save ba tech type #4709

Merged

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Aug 15, 2023

Fairly extensive overhaul of Transporter bay encoding / decoding to fix MML 1182 so that mixed-tech vehicles with BA transport bays can utilize the correct type and save/load correctly.

Overview:

  1. Replaced the current variable-length "Numbers" field encoding transporter bays with fixed-length encoding that uses 6 fields. This simplifies Transporter bay loading significantly (when reading the new format).
  2. Moved infantry platoon type and facing to separate fields; previously, either could be found in field 2 or 3.
  3. Removed 'f' prefix from the "Facing" data, but still allow each entity type to store arbitrary integers in field 4 as there does not appear to be a set of unified "Facing" values used by all Entities.
  4. Made the bay type field (field 5) a bitmap to store various states, e.g. ComStar, Clan, or IS tech base, compactly.
  5. Added conversion function to read in older BLK files but store new files in the updated format.
  6. Updated all Bay types to output their data in the new format.
  7. Added unit tests of the new conversion functions.
  8. Added a unit test that verifies that we now save/load data as expected per MML 1182 above.

New bay "numbers" format:

// Field 0 is the size of the bay, in tons or # of units and is required
// Field 1 is the number of doors in the bay, and is required
// Field 2 is the bay number and is required; default value of "-1" is "unset"
// Field 3 is used to record infantry platoon type; default is an empty string
// Field 4 is an int recording facing; default is string representation of entity.LOC_NONE
// Field 5 is a bitmap recording status like tech type, ComStar bay; default is "0"

Compatibility:

  • 49.14 BLK file -> 49.XX load: OK
  • 49.XX BLK file -> 49.XX load: OK
  • 49.XX BLK file -> 49.14 (or earlier) load: Entities with bays will load but some data may be incorrect, specifically mixed-tech BA bay weights (they will all be converted to the base tech type), ComStar BA bays (won't be correct type), and any bays with facings (e.g. in Space Stations).

Initial commit to fix MML issue MegaMek#1182.
Adds more fields to bay representation in strings and objects;
normalizes string representations so all bays use the same fields
in the same ways - no more bay #, _or_ infantry type, _or_ facing
in one field.
Added a test file for BLKDropshipFile.java that reads a BLK-file
string in and confirms new BattleArmor bay behavior.
I ended up not needing Mockito after all.
@Sleet01
Copy link
Collaborator Author

Sleet01 commented Aug 15, 2023

I'll update the PR to fix the above issues tonight.

Add some extra exception handling, fix string equality check,
make bit-wise operation safer.
It appears that we have been writing empty <transporters> blocks
into every vehicle for the past 6 or so years.  These blocks should
not exist in, e.g., combat vehicles, but apparently went unnoticed
due to loose handling of the relevant block when empty.

This change adds a new test prior to processing these empty blocks
on file load, a function to support this test, and a conditional
around writing the <transporters> block to prevent writing empty
blocks in every .BLK file.
New function "containsData" encompasses this call already.
Copy link
Member

@NickAragua NickAragua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this do ok loading both "before" and "after" blk files?
Edit: reading the PR notes, apparently mostly yes.

Copy link
Member

@NickAragua NickAragua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, but I'll leave this up for a little bit for others to take a look at as it is fairly extensive.

@HammerGS
Copy link
Member

Merging so QA has access to test.

@HammerGS HammerGS merged commit 956b7c1 into MegaMek:master Aug 23, 2023
4 checks passed
HammerGS added a commit that referenced this pull request Aug 23, 2023
+ Issue #3956: First Succession Wars Rats seem incorrect
+ PR# 4716: Camo rotation and scaling
+ PR #4709: Mixed tech dropships do not save BA Tech type
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.

Mixed tech dropships force change battle armor bays when saved and reloaded
3 participants