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 #4648: Update AT Classes for MekMortar and IS BA Tube Arty #5027

Merged
merged 2 commits into from
Jan 13, 2024

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Jan 9, 2024

It turns out that Mek Mortars (and, coincidentally, IS BA Tube Artillery) are filtered out of the MekHQ "Spend XP -> SPA" context menu because they have the AT Class "CLASS_NONE". Because MekHQ uses a different code path to validate weapon validity for SPAs, these weapons are filtered out in MekHQ but work fine in MegaMek.

This patch by itself only fixes the issue for IS BA Tube Artillery; the matching MekHQ PR here must also be applied to fix the issue for Mek Mortars as well.

We should probably look at:

  1. removing all AT Class filtering, as it is apparently quite old and mostly unused,
  2. importing the MegaMek SPA validity test, which is much more robust, into MekHQ.

but I have some other stuff to work on right now.

Tested with MegaMek and MekHQ; ran all tests on both packages.

Close #4648

@@ -249,7 +250,7 @@ public class WeaponType extends EquipmentType {
// Used for BA vs BA damage for BA Plasma Rifle
public static final int WEAPON_PLASMA = 15;

public static String[] classNames = { "Unknown", "Laser", "Point Defense", "PPC", "Pulse Laser", "Artilery", "AMS",
public static String[] classNames = { "Unknown", "Laser", "Point Defense", "PPC", "Pulse Laser", "Artillery", "AMS",
Copy link
Member

Choose a reason for hiding this comment

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

Any idea where these values are used and what effect changing it will have?

Also, why do we have two "AMS" (one on line 253, one on line 255)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking at the code and commits, it looks like AT Classes were an attempt at separating AeroTech2 or Naval weapons from other weapons without expanding stuff like WeaponType (due to bitmap size restrictions).

These AT Classes are apparently used for MML printing and Weapon Bay storing / loading, as well as some logic that determines what kind of Bay will be created by adding different WeaponTypes to a Bay. It's mostly very old Aerospace / Dropship-related code. Adding a new entry shouldn't cause any issues; note that CLASS_GAUSS and CLASS_THUNDERBOLT have already been added without classNames entries.

No idea why there are two AMS entries, it looks like a typo - 6 is PLASMA, not AMS. I can update that...

@SJuliez SJuliez merged commit 603e1d7 into MegaMek:master Jan 13, 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.

[0.49.14] RFE - Sandblaster SPA Missing Entry for 'Mech Mortar weapons
3 participants