Skip to content
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

Introduce MoveCooldownHelper to prevent lag spikes from failed pathfinding #21391

Open
wants to merge 1 commit into
base: bleed
Choose a base branch
from

Conversation

RoosterDragon
Copy link
Member

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.


Based on and supersedes #20954. Fixes #21187, #21203.

#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.

…nding

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.
Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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

@PunkPun
Copy link
Member

PunkPun commented Apr 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enclosed units attacking unreachable causes large perf loss
3 participants