Introduce MoveCooldownHelper to prevent lag spikes from failed pathfinding

[RoosterDragon]

Several activities that queue child Move activities can get into a bad scenario where the actor is pathfinding and then gets stuck because the destination is unreachable. When the Move activity then completes, then parent activity sees it has yet to reach the destination and tries to move again. However, the actor is still blocked in the same spot as before and thus the movment finishes immediately. This causes a performance death spiral where the actor attempts to pathfind every tick. The pathfinding attempt can also be very expensive if it must exhaustively check the whole map to determine no route is possible.

In order to prevent blocked actors from running into this scenario, we introduce MoveCooldownHelper. In its default setup it allows the parent activity to bail out if the actor was blocked during a pathfinding attempt. This means the activity will be dropped rather than trying to move endlessly. It also has an option to allow retrying if pathfinding was blocked, but applies a cooldown to avoid the performance penalty. For activities such as Enter, this means the actors will still try and enter their target if it is unreachable, but will only attempt once a second now rather than every tick.

MoveAdjacentTo will now cancel if it fails to reach the destination. This fixes MoveOntoAndTurn to skip the Turn if the move didn't reach the intended destination. Any other derived classes will similarly benefit from skipping follow-up actions.

---

Based on and supersedes #20954. Fixes #20948. Fixes #21187. Fixes #21203. Fixes #21301.

#21164 means that the test case from #21187 no longer has easily visible impact. The repeated pathfinding attempts every tick are there, but they're very cheap now.

I recommend the test case from #21203 instead. This doesn't benefit from the changes in #21164 and the performance impact is very obvious.

Bleed:
Attack moving with units trapped in by a laser fence causes the game to slow to a crawl for a long time.

PR:
Attack moving with units trapped in by a laser fence causes an initial lag spike, but after that the performance is reasonable.

[PunkPun]

Hervester still queues move every tick. As for minelayer, I recon it should be among those units that repeatedly tries to queue move.

This also feels incorrect for handling multiple queued orders / layered orders. One example I found that if I guard a unit in an unreachable position, then killing the guarded units doesn't stop the activity. The only way to end it is to manually stop it or make the position reachable

[anvilvapre]

In the world of web they use the term debounce - where they limit the number of events to x per ms.

[PunkPun]

it might be worth to cancel harvesting when unable to reach the destination and try to pick a new location. FindAndDeliverResources already handles failed searches, the code just needs to propagate the unreachable error from HarvestResource

[RoosterDragon]

Hervester still queues move every tick.
it might be worth to cancel harvesting when unable to reach the destination and try to pick a new location. FindAndDeliverResources already handles failed searches, the code just needs to propagate the unreachable error from HarvestResource

I managed to miss the FindAndDeliverResources activity! This now also has a cooldown helper, and I've set it to retry.

This means if you trap you harvesters in a box and give them a harvest order, they will now retry it once a second instead of every tick. If you break the box they will go to harvest.

I haven't done anything to propogate the failure from HarvestResource up to FindAndDeliverResources, but let me know if the new behaviour is acceptable. Doing some custom wiring between those activites would be nice to avoid if the current changes are sufficient.

As for minelayer, I recon it should be among those units that repeatedly tries to queue move.

Fair, set it to retry.

This also feels incorrect for handling multiple queued orders / layered orders. One example I found that if I guard a unit in an unreachable position, then killing the guarded units doesn't stop the activity. The only way to end it is to manually stop it or make the position reachable

This appears to be an intentional behaviour of AttackFollow and can be reproduced on bleed. It'll try and guard the last known position even if the actor is killed. So I don't think this PR should change that.

In the world of web they use the term debounce - where they limit the number of events to x per ms.

Yes. Do we prefer Debounce instead of Cooldown? Either term works for me.

[PunkPun]

Harvester MoveToDock.MoveOntoAndTurn has a small regression. Now that we cancel the activity, the unit turns when it can no longer find a path.

testcase:
build a refinery, let the harvester out, build walls around the refinery, when the harvester returns and doesn't find a path, it will turn in place

[PunkPun]

otherwise LGTM

[RoosterDragon]

Your testcase recreates for me on bleed. So a pre-existing issue separate from these changes I believe.

[PunkPun]

The MoveOntoAndTurn doesn't turn only when it is cancelled, and here it is cancelled by path being invalid which does not call Cancel(). Basically it needs info on how it was cancelled which is what this PR adds

[RoosterDragon]

I had a go at canceling the activity if the move failed to reach its destination, however this isn't having the intended effect as when the refinery is blocked off, the move to the wall next to it is the destination, so the turn still occurs.

OpenRA/OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs

Lines 102 to 104 in ccb1bd7

	// The last queued childactivity is guaranteed to be the inner move, so if the childactivity
	// queue is empty it means we have reached our destination.
	return TickChild(self);

can be adjusted to

// The last queued child activity is guaranteed to be the inner move,
// so if the child activity queue is empty it means the move completed.
if (!TickChild(self))
	return false;

if (Mobile.MoveResult == MoveResult.CompleteDestinationReached)
	return true;

// The move completed but we didn't reach the destination, so Cancel.
Cancel(self);
return true;

If somebody can suggest a fix I can include it, but otherwise it may as well be deferred to another PR.

[RoosterDragon]

Hmmm, this may indicate a bug of some sort, will have to dig deeper on this one.

[RoosterDragon]

Okay solved. In Move when there is no path we set the destination equals to the mobile.ToCell. Later we need to detect this occured so we can treat the move as Blocked, rather than Completed.

Detecting this case correctly allows us to fix a few things

build a refinery, let the harvester out, build walls around the refinery, when the harvester returns and doesn't find a path, it will turn in place

This is now fixed - the activity will cancel now when it fails to get to the destination, so the turn gets skipped.

This also fixes #21301 - the target will be detected as unreachable. The Attack activity currently does not apply a retry, so the attack will be cancelled.

[PunkPun]

The Attack activity currently does not apply a retry, so the attack will be cancelled.

That seems more like a bug. Something like an aircraft can come into range and back constantly (aircraft circling). We do want to retry attacking

[PunkPun]

when ordering to capture there also seems to be a delay that didn't used to be here before?

[RoosterDragon]

The Attack activity currently does not apply a retry, so the attack will be cancelled.

That seems more like a bug. Something like an aircraft can come into range and back constantly (aircraft circling). We do want to retry attacking

I have AttackFollow set to retry, but I'm less sure about Attack. If you can't reach the target I think it's likely better to drop the target and let AutoTarget pick up a new one when it comes in range - rather than tunnel visioning on the one target. Remember the retry only applies if you can't get to the target.

If a follow-up PR wants to change the RetryIfDestinationBlocked = true/false policy for various activites I'm okay with that.

when ordering to capture there also seems to be a delay that didn't used to be here before?

Indeed, the cooldown was applied too early. Moved it down a few lines of code so the initial move can execute freely.

[PunkPun]

LGT<

[PunkPun]

changelog