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 3880 cannot refit clan mechs due to renames #3899

Merged

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Mar 23, 2024

This should fix both Clan configuration changes and custom refits.
Edit: also fixes an NPE when all Techs are occupied and the user wants to start a Refit.

The problem is obviously the requirement to include the Clan unit name in parentheses when doing Clan unit lookups.

I am open to refining this fix further, such as:

  • Adding a Clan-aware lookup within the unit cache
  • Further refining the new utility function so that it can return the filtered list of alternate configurations required by the Refit/Configuration... chooser
  • Figuring out a faster fuzzy search that will return all matching units (possibly by changing the namesMap to a BTree or something)
  • Adding new tests to verify more Clan configs work correctly with Refits,

but this fix works now, and I don't have time to do any more for probably a week.

Testing:

Close #3880

@Sleet01 Sleet01 marked this pull request as draft March 23, 2024 22:37
@Sleet01
Copy link
Collaborator Author

Sleet01 commented Mar 23, 2024

Converted to draft due to CI failures. Will clean up in a bit.

@Sleet01 Sleet01 force-pushed the Fix_3880_cannot_refit_clan_mechs_due_to_renames branch from 0d37319 to f21251e Compare March 24, 2024 03:15
@Sleet01 Sleet01 marked this pull request as ready for review March 24, 2024 04:50
@Sleet01
Copy link
Collaborator Author

Sleet01 commented Mar 24, 2024

Tests broken after pulling latest Planetary Conditions code; should be fixed by /pull/3834

@SJuliez
Copy link
Member

SJuliez commented Mar 24, 2024

Thanks for dealing with the clan name thing.

@Sleet01
Copy link
Collaborator Author

Sleet01 commented Mar 24, 2024

Thanks for dealing with the clan name thing.

There may be some other spots where we'll want to replace getChassis() with getFullChassis() but I figure that can be a separate PR.

Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 10.69%. Comparing base (d1d1d9a) to head (61095a9).
Report is 56 commits behind head on master.

Files Patch % Lines
MekHQ/src/mekhq/Utilities.java 0.00% 10 Missing ⚠️
MekHQ/src/mekhq/gui/dialog/ChooseRefitDialog.java 0.00% 6 Missing ⚠️
MekHQ/src/mekhq/campaign/parts/Refit.java 0.00% 1 Missing ⚠️
MekHQ/src/mekhq/gui/CampaignGUI.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3899      +/-   ##
============================================
+ Coverage     10.64%   10.69%   +0.04%     
- Complexity     5487     5528      +41     
============================================
  Files           834      834              
  Lines        113857   113866       +9     
  Branches      17193    17196       +3     
============================================
+ Hits          12121    12176      +55     
+ Misses       100526   100462      -64     
- Partials       1210     1228      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sleet01 Sleet01 merged commit 5fe672b into MegaMek:master Mar 27, 2024
4 of 6 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.

[0.49.18] MekHQ mech customisation in Mek Lab fails on Clan mechs
3 participants