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

Tweak automatic munition selection for low-ammo-count Autocannons #5860

Merged

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Aug 5, 2024

QA testing revealed that some units with single bins of AC10 and AC5 ammo would get Precision Autocannon munitions loaded, despite this significantly decreasing their mission endurance.

The issue was caused by a couple things:

  1. Precision and Armor-Piercing munitions were not listed in the category "AMMO_REDUCING_MUNITIONS", so reducing the weight of the category would not reduce the weights of these munitions.
  2. Standard munitions might not be selected as the primary ammo when two or more munitions tie for highest weight, so even if 1 is fixed, Standard munitions might not be selected.

Testing fixes for 1 and 2 revealed an opportunity to extend the Caseless ammo selection code for AC/20s to specific AC/10 and AC/5 situations as well.

Testing:

  • Tested on MHQ campaign submitted by reporter of original bug (Thanks @Thom293 !)
  • Ran all 3 projects' unit tests

@Sleet01 Sleet01 requested a review from NickAragua August 5, 2024 19:33
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.98%. Comparing base (a55a508) to head (a4b3e17).
Report is 24 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5860      +/-   ##
============================================
- Coverage     28.99%   28.98%   -0.02%     
+ Complexity    13930    13908      -22     
============================================
  Files          2512     2512              
  Lines        267289   267254      -35     
  Branches      47837    47787      -50     
============================================
- Hits          77510    77467      -43     
- Misses       185829   185830       +1     
- Partials       3950     3957       +7     

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

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.

Let's reduce copy-pasta a little bit.

@Sleet01 Sleet01 requested a review from IllianiCBT August 9, 2024 18:04
@Sleet01
Copy link
Collaborator Author

Sleet01 commented Aug 9, 2024

Roping in @IllianiCBT as they are some fresher eyes on the project, and have not worked on this code before.

Do you mind taking a look at my proposed code, and @NickAragua 's suggested fix?

private static boolean setAC10Imperatives(Entity e, MunitionTree mt, ReconfigurationParameters rp) {
int ac10Count = getACWeaponCount(e, "10");

// TODO: remove this block when implementing new anti-ground Aero errata
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know @Algebro7 is implementing some of the new Aero errata, but I don't know if they're doing the anti-ground errata. Tagging them here so that they're aware of this TODO (in case there is any cross-over).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Side note: anyone working on this will also need to touch the other MM, MML, and MHQ ammo validation code - it's not a small lift.

@IllianiCBT
Copy link
Collaborator

Do you mind taking a look at my proposed code, and @NickAragua 's suggested fix?

So I took a look, but I really don't feel qualified to make a decision given the substantially more experience you both have over me.

@Sleet01
Copy link
Collaborator Author

Sleet01 commented Aug 9, 2024

Do you mind taking a look at my proposed code, and @NickAragua 's suggested fix?

So I took a look, but I really don't feel qualified to make a decision given the substantially more experience you both have over me.

Alright, thanks for giving it a look-over.

@Sleet01 Sleet01 force-pushed the Fix_Add_AC_ammo_reducing_munitions_to_list branch from a4b3e17 to 4a08954 Compare August 9, 2024 22:35
@Sleet01 Sleet01 merged commit 11bd938 into MegaMek:master Aug 9, 2024
3 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.

3 participants