Page MenuHomeWildfire Games

Forced attack on moving targets bug
AbandonedPublic

Authored by temple on Nov 6 2017, 6:48 AM.

Details

Reviewers
wraitii
Summary

There's a bug with forced attacks on a moving target that moves out of vision range.

Usually the logic is in UnitAI, in this case combat approaching, where in Timer we decide if we should abandon the chase (for forced commands we don't want to stop chasing; even if he goes out of vision range we want to follow the path to where we saw him last) and in MoveCompleted we decide if we're in range or need to move closer to the target or have to give up.

However, in UnitMotion, if we're out of waypoints, i.e. done with the approach, there's a piece of code that tests if the target is moving and if so to move to target range, i.e. get a new path. So it decides what to do itself rather than sending a move completed message to UnitAI. (UnitAI stays in the approach Timer.)

The particular problem here is that the UnitMotion code doesn't take into account visibility (UnitAI does), so units will continue to chase after the target, even in the fog of war and shroud of darkness, as long as the target continues to move.

I don't know why the code's there, but it's wrong and seems out of place.

Test Plan

Probably needs to be thought about carefully.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

temple created this revision.Nov 6 2017, 6:48 AM
wraitii added a subscriber: wraitii.EditedNov 6 2017, 8:15 AM

That's definitely buggy, but it's only one of like 15 bugs in our existing implementation. I'm fine with committing it (easy rebase iirc I just remove all that code), but I think it'd be worth more looking at D13 directly.

I do think this code might lead to stuck units though, so I think it'd be preferable to explicitly check for visibility of the target

temple added a comment.Nov 6 2017, 4:26 PM

Unfortunately this isn't the whole story. If units get obstructed then they can ask for new paths...

wraitii added a comment.EditedNov 6 2017, 4:28 PM
In D1015#39916, @temple wrote:

Unfortunately this isn't the whole story. If units get obstructed then they can ask for new paths...

Erh, honestly just check out D13 instead you'll waste less time. The existing unit motion is an absolute mess.

This is more of a UnitAI bug anyway.

temple added a comment.Nov 6 2017, 4:49 PM

This is more of a UnitAI bug anyway.

Not sure about that, because UnitMotion keeps creating new paths rather than sending a MoveCompleted message back to UnitAI. Unless the UnitAI Timer should check if the unit's still visible and if so change to MoveToPoint of the target's last known position. I suppose that could work.

Not sure about that, because UnitMotion keeps creating new paths rather than sending a MoveCompleted message back to UnitAI. Unless the UnitAI Timer should check if the unit's still visible and if so change to MoveToPoint of the target's last known position. I suppose that could work.

In general, I believe unitMotion shouldn't decide to arbitrarily stop on its own, because you can't expect unitAI to always handle it (and indeed we had… a ton of bugs with units being "stuck" because of that). I'm much more in favour of having unitAI decide that it can't follow a unit anymore.
That being said it's a little inconvenient to have unitAI keep updating unitMotion on a moving target, so I think this can be a slight exception to the rule (and indeed in D13 I believe the behaviour is the same as UnitMotion tries to predict where the target will be, when this is more of a unitAI thing).

Ideal behaviour here would be for unitMotion to stand still, still trying to path to the unit, but since it doesn't know where the unit is, do nothing. Then UnitAI, on its own, should realise that we can't reach our target and do something intelligent. If unitAI doesn't do that, and the unit someday reappears, unitMotion should be ready to go and path again. D13 makes that possible, current unit motion doesn't.

wraitii requested changes to this revision.May 14 2018, 4:44 PM
wraitii added a reviewer: wraitii.

Still think time is better spent reviewing D13. In D13 there's a TODO for LOS, since after more thinking I do think LOS should be handled in unitMotion. UnitMotion will now be able to warn unitAI that the move will fail, and furthermore UnitAI was updated to do range checks in "Timer" instead of relying on Move Completed (which is far more correct).

If you want to fix this now, you should just change the range check to Timer instead of MoveCompleted, that's what will be done in D13.

This revision now requires changes to proceed.May 14 2018, 4:44 PM
bb abandoned this revision.Sep 2 2020, 8:27 PM
bb added a subscriber: bb.

Obsolete after D2698