-
Notifications
You must be signed in to change notification settings - Fork 287
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 aerospace attack values for LBX autocannons #5565
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I seem to recall this previously causing issues with aerospace units not doing the correct amount of damage with LBX autocannons. LBXHandler.java in particular contains code (lines 67-74) that will reduce LBX attack values to 60% of their specified value. If it is preferred to have the correct values in the weapon definition files, removing that code might be best. Either way, this could use testing to confirm that aerospace units are doing the expected amount of damage with LBX autocannons after the changes. |
Tested the Gradle artifact for the PR; I can now confirm that this causes LBX autocannons to do less damage than intended. Does seem to be following the If you decide to remove the LBXHandler code and leave the adjusted weapon AVs (a perfectly reasonable choice), also fix the Silver Bullet Gauss Rifle AV, since it counts as an LBX weapon. |
Pretty sure this is the way it is because we have multiple consumers of the AV values, only some of which are actually Aerospace units. Just changing the AV values won't produce correct behavior in all cases. |
Also note that I changed the SB Gauss AV value to 15 from 9 specifically because of this code. Which has been in place for 11 years. Was this PR submitted due to an issue, or just because the values looked wrong? |
That must have been where I remembered it from, I was looking for the issue report or change history for a LBX change and couldn't find anything, heh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like us to hold off on this unless it also includes updating the LBX handler that already lowers the AV value, as well as any other weapons currently affected by it (such as the Silver Bullet Gauss).
It may be that we no longer need to have both a full and an Aero-specific AV value, but I don't think that question has been answered satisfactorily yet.
It feels like we can close this, right? |
I opted to withhold the review requested from me, as I don't know enough about the rules to make a reliable judgement call. Also, this was before I opted to avoid reviewing MM content too much due to my unfamiliarity with the codebase. |
Closing this as per the above comments (seems unnecessary or at least, it would require the LBX handler to be updated) |
The AV for LBX autocannons (other than the LB2X) are incorrect. See TW, pp. 303-4.