-
Notifications
You must be signed in to change notification settings - Fork 293
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 5590: princess firing on 13s due to intervening trees, etc. #6477
Fix 5590: princess firing on 13s due to intervening trees, etc. #6477
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6477 +/- ##
============================================
- Coverage 28.59% 28.58% -0.01%
- Complexity 14377 14405 +28
============================================
Files 2798 2798
Lines 274895 275141 +246
Branches 48630 48676 +46
============================================
+ Hits 78593 78636 +43
- Misses 191669 191831 +162
- Partials 4633 4674 +41 ☔ View full report in Codecov by Sentry. |
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.
Overall looks fine. For logging methods I think we should be moving to I18n messages using varargs for passing in data.
Not needed for this change as I'm not sure we have I18n proper and refined in place just yet.
logger.warn(shooter.getDisplayName() + " tried to load " | ||
+ currentWeapon.getName() + " with ammo " + | ||
mountedAmmo.getDesc() + " but this would have caused it to miss; skipping."); |
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.
Warn should support varargs with %s.
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.
Had both Co-Pilot and IDEA AI check this and they see no issues. GOing to include in .03
This patch adds a few bits of extra knowledge to Princess in order to avoid her wasting shots where she shouldn't.
The key issues it fixes are:
In more detail:
The fix here was to add a single light-weight LOS modifier check prior to the more in-depth calculations to get better LOS data but allow a fast exit if LOS is blocked.
Princess should now have a better idea of LOS mods from trees and smoke when predicting all units' turns.
This means that, e.g., an LB-10X shot might be switched from cluster to solid in attempt to bypass special armor, but in doing so the shot goes from a 12 to hit up to a 13 due to the Cluster bonus being lost.
The fix is to clone the existing WeaponAttackAction, give it the suggested new ammo, confirm that it does not have a higher to-hit number, and only then load the new WAA into the queue of attacks to be sent to the server.
The fix is to add a new section in the WeaponFireInfo class (responsible for assessing potential attack effectiveness) to mark such attacks as dealing zero damage if the rule is enabled. This is a fast but not comprehensive check, and may need updating for full coverage in the future.
Note:
It appears that there is still the possibility for Princess to be forced to take impossible shots, when ammo usage during the Attack Phase causes one ammo bin to go bingo and the replacement ammo has different to-hit characteristics.
The chief offender is IS LRM Dead-Fire, where switching from D-F rounds to normal rounds at range 1 adds a +5 penalty for minimum range.
Unfortunately I don't see a way to fix this without adding a lot of extra ammo bin checks prior to attacks.
Testing:
Close #5590