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 4801: silent NPE while Princess moving Wraith BA #4802

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Sep 24, 2023

This issue was caused by some confusion on my part when implementing proper Field Gun / Field Artillery expected damage computations for Princess.
The boolean fieldGunsDoDamage is actually checking if a unit can make any non-personal weapons attacks, because either:

  1. the unit is infantry that hasn't moved this turn, or
  2. the unit is literally any other thing.

The root cause was attempting to dereference the linked ammo for BA AP weapons, which doesn't work. That throws an NPE, which is caught in the movement path computation code, which returns a null mp object in that case. But then we try to use the null mp object anyhow, with predictable results.

Fix is to clean up some of the checks in that section of code:

  • Rename the boolean for clarity
  • Formally separate infantry with field weapons from other unit types in comments and code
  • Simplify the field weapon expected damage check; we can let Princess assume only one shot from multi-fire field guns will hit, in which case we can use the rack value for all types instead of trying to compute expected damage more completely. This should also make Princess more likely to use infantry weapons in some cases.

Tested on the save file from the issue, as well as a custom save to verify other infantry and non-infantry predictions still work correctly.

Close #4801

This fixes an issue introduced by my attempt to add Field Gun / Field
Artillery expected damage calculations for infantry units under
Princess.

Unfortunately, I didn't realize that the same code path was used by all
non-infantry units when Princess projects how much damage _any_ unit
will take; in this case, BA attackers trying to compute expected damage
against non-enemy targets.

The root of the issue is trying to dereference linked ammo for BA
weapons, which don't have that concept in some cases.  This throws
an NPE, which gets caught, causing a null 'mp' object to be returned,
which creates a _new_ NPE.

Fix is to clean up that area of code and restrict the special weapon
handling to infantry only.  All others, including BAs, will go back
to the previous method of projecting expected damage.

Testing:

- Re-ran reporter's attached saved game.
- Confirmed the update for field guns / field artillery still works
  correctly.
- Ran all unit tests.
@NickAragua NickAragua merged commit 5fa6bd8 into MegaMek:master Sep 24, 2023
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.15] Silent NPE during Princess Movement Phase for Wraith BA
2 participants