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

Fix Locomotor.CanMoveFreelyInto when using ignoreSelf. #21165

Merged
merged 1 commit into from Oct 30, 2023

Conversation

RoosterDragon
Copy link
Member

The ignoreSelf flag is intended to allow the current actor to be ignored when checking for blocking actors. This check worked correctly for cells occupied by a single actor. When a cell was occupied by multiple actors, the check was only working if the current actor happened to be the first actor. This is incorrect, if the current actor is anywhere in the cell then this flag should apply.

This flag failing to be as effective as intended meant that checks in methods such as PathFinder.FindPathToTargetCells would consider the source cell inaccessible, when it should have considered the cell accessible. This is a disaster for performance as an inaccessible cell requires a slow fallback path that performs a local path search. This means pathfinding was unexpectedly slow when this occurred. One scenario is force attacking with a group of infantry sharing the same cell. They should benefit from this check to do a fast path search, but failed to benefit from this check and the search would be slow instead.

Applying the flag correctly resolves the performance impact.


This resolves poor performance in #21132, where the AI is trying to order many units at once, and in particular groups of infantry cause a large slowdown due to this bug. Note attempting to run the replay with this PR applied will cause a desync. This is expected as the paths the pathfinder returns will be different.

Testcase:

  • Create 5 infantry and move them to the same cell.
  • Activate the /path-debug command.
  • Select the 5th unit and force-attack (ctrl+click) across the map.

With this PR applied, you will see a search that looks like this. The green lines indicate the hierarchical pathfinder has been used. This is good as it makes the path search cheaper. You can see a reasonable amount of yellow lines that show the explored route. This is also the same outcome you will get if you force-attack when the unit is standing on its own.

image

Without this PR, the search will look like this. No green lines means the hierarchical pathfinder did not get used. The local pathfinder had to do everything on its own. You will see more yellow lines indicating it had to explore more cells. More explored cells = more expensive.

image

pchote
pchote previously approved these changes Oct 28, 2023
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

Code changes look sensible and confirmed behaviour fix ingame.

The ignoreSelf flag is intended to allow the current actor to be ignored when checking for blocking actors. This check worked correctly for cells occupied by a single actor. When a cell was occupied by multiple actors, the check was only working if the current actor happened to be the first actor. This is incorrect, if the current actor is anywhere in the cell then this flag should apply.

This flag failing to be as effective as intended meant that checks in methods such as PathFinder.FindPathToTargetCells would consider the source cell inaccessible, when it should have considered the cell accessible. This is a disaster for performance as an inaccessible cell requires a slow fallback path that performs a local path search. This means pathfinding was unexpectedly slow when this occurred. One scenario is force attacking with a group of infantry sharing the same cell. They should benefit from this check to do a fast path search, but failed to benefit from this check and the search would be slow instead.

Applying the flag correctly resolves the performance impact.
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.

LGTM

@PunkPun PunkPun merged commit 216758d into OpenRA:bleed Oct 30, 2023
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Oct 30, 2023

Changelog

@RoosterDragon RoosterDragon deleted the pathfinder-ignoreself-fix branch October 30, 2023 09:38
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.

None yet

5 participants