-
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 5870: princess engaging targets outside visual range in heavy fog #6457
Fix 5870: princess engaging targets outside visual range in heavy fog #6457
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6457 +/- ##
============================================
- Coverage 28.59% 28.59% -0.01%
Complexity 14377 14377
============================================
Files 2798 2798
Lines 274872 274895 +23
Branches 48626 48630 +4
============================================
+ Hits 78592 78593 +1
- Misses 191649 191669 +20
- Partials 4631 4633 +2 ☔ 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.
Nice!! I think someone will at some point want to add low-vis conditions this to Princess's fire control unit tests (I like double blind so I might).
Re: C3 & WAA changes: I was thinking of expanding the WAA JUnit tests I added last week during 50.04 if it really is a “stability” release - covering C3 in the manner you described would be a good test to add.
@@ -305,6 +306,11 @@ ToHitData guessToHitModifierHelperForAnyAttack(final Entity shooter, | |||
return new ToHitData(TH_RNG_TOO_FAR); | |||
} | |||
|
|||
final int maxVisRange = game.getPlanetaryConditions().getVisualRange(shooter, shooter.isUsingSearchlight()); |
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.
Will Princess still be able to consider Entities illuminated by a different source as valid - like a different Mek with a searchlight illuminating the enemy?
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.
She will, but for the fast approximation path she's only going to consider the current unit's searchlight state and any cached illumination state on target units.
This path is used when planning where to move a unit, not just for the moving unit but for predicting the likely actions of all known enemies and allies as well.
So I wanted to add the planetary environment check but keep it as minimal as possible to avoid introducing a big slowdown.
The attack planning code switches to using the same to-hit calc code as player sees when using the GUI; there should be fewer options at that point, so it's fast enough.
And that contains the full illumination check code.
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.
Looks clean.
Please see #5870 for analysis of the root cause.
This fix lets Princess use knowledge of the current visual range when guessing at attack effectiveness for movement planning.
It also removes the ability for C3-equipped units to completely bypass LOS restrictions as long as one C3 network member has a target within their sensor ranges.
Based on forum posts and TacOps, it looks like at least one member of the network needs to have visual LOS, including any Planetary Conditions that restrict visual range, for the network to provide targeting capabilities to its members. Otherwise it's just sharing the already-detected sensor pings.
Testing:
Close #5870