Fix a crash caused by invalid target in FlyAttack#21479
Fix a crash caused by invalid target in FlyAttack#21479Mailaender merged 1 commit intoOpenRA:bleedfrom
Conversation
| // Move into range of the target. | ||
| if (!target.IsInRange(pos, lastVisibleMaximumRange) || target.IsInRange(pos, minimumRange)) | ||
| QueueChild(aircraft.MoveWithinRange(target, minimumRange, lastVisibleMaximumRange, target.CenterPosition, Color.Red)); | ||
| QueueChild(aircraft.MoveWithinRange(target, minimumRange, lastVisibleMaximumRange, checkTarget.CenterPosition, Color.Red)); |
There was a problem hiding this comment.
So should we be checking for checkTarget.IsInRange... in the condition above? It seems odd to check whether we're in range of one thing, and then move towards a different thing.
(If the condition should change, then there might be other changes since we're already doing some such checks earlier on, but to start with just understanding if the condition should be altered too would be good)
There was a problem hiding this comment.
I see you adjusted the code.
As written now, this seems odd as on line 144 above we have
if (checkTarget.IsInRange(pos, lastVisibleMaximumRange) && !checkTarget.IsInRange(pos, minimumRange) && useLastVisibleTarget)
return true;which is bailing out if it is specifically having to use the last target. If later on we're moving into range of checkTarget per this PR's suggestion, then that feels.... weird? Why can we bail early in the useLastVisibleTarget = true case, but need to keep going for the useLastVisibleTarget = false case?
It might be that this logic is all fine and I just need an explanation as to why this all makes sense in both the useLastVisibleTarget = true and useLastVisibleTarget = false scenarios - or maybe there is something off about this that I can't quite put into words. If you can do anything to clear up my confusion, that would be very helpful.
There was a problem hiding this comment.
The duplicate range check and move order was always present (see FlyAttack before the last relevant commit) and I aimed to preserve the existing order of checks. The check for useLastVisibleTarget could be moved to the final if block, I could experiment to see if the now unskipped takeoff order causes any regressions
tjk-ws
force-pushed
the
flyattack
branch
2 times, most recently
from
8463236 to
5d09b2d
Compare
|
So could I ask how to reproduce this crash? |
Any appreciable amount of air combat should trigger it (see #17688 (comment)) |
RoosterDragon
left a comment
RoosterDragon
left a comment
There was a problem hiding this comment.
I'm struggling to come up with a reproduction case, but based on the stack trace from OpenHV/OpenHV#1060 I'm happy this would resolve the issue by using checkTarget which has a fallback if the target has become invalid, as opposed to moving towards the invalidated target.
5 participants
By moving to checkTarget's position this makes FlyAttack's MoveWithinRange consistent with its counterpart in AttackFollow as has been the approach in #21455.