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 5419: iATM ranges incorrect #5424

Merged
merged 4 commits into from
Apr 28, 2024

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Apr 27, 2024

Initial issue was that iATM weapon bays did not accurately reflect their selected ammo.
In the process of investigating this I discovered a number of issues:

  1. iATMs are not covered by most code that handles ATMs
  2. iATMs are not considered Streak weapons for Princess planning purposes
  3. iATM ammo types are not considered by Princess when guessing at expected damage from prospective attacks
  4. ATM and iATM weapon ranges are retrieved, re-calculated, overwritten, and recreated from magic numbers multiple times every turn for each launcher due to redundant code throughout WeaponType, WeaponFireInfo, WeaponAttackAction, and the WeaponPanel GUI elements.
  5. The merge of PR Mounted rework #5406 reverted at least some of the Princess ammo selection code; any other code merged after 02/08/2024 is also suspect.

This PR fixes 1-4 and part of 5.

Testing:

  • Re-ran all projects' unit tests (note: MHQ unit tests were already failing for other reasons)
  • Tested extensively with player-controlled and bot-controlled Aerospace units mounting iATMs
  • Built custom Dropships to confirm iATM bays functioning as expected.

Note: appears to be blocked by #5423

Close #5419

@SJuliez
Copy link
Member

SJuliez commented Apr 28, 2024

@Sleet01 Not sure from your inital comment: Is this safer to merge?

@Sleet01
Copy link
Collaborator Author

Sleet01 commented Apr 28, 2024

@Sleet01 Not sure from your inital comment: Is this safer to merge?

Yes.
This will fix the reported issue and restore Princess ammo selection in the current .20 dev branch.
It should also be fine to re-apply if we do end up pulling out the last week of merges and re-building everything.
Also, it appears that a fix for #5423 has already been merged so this should no longer be blocked.

@SJuliez SJuliez merged commit 3f351e3 into MegaMek:master Apr 28, 2024
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.

iATMs do not work correctly in weapon bays
3 participants