-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Refactor creature pathfinding, and unify monster pathfinding logic. #70274
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
[JSON]
Changes (can be) made in JSON
Map / Mapgen
Overmap, Mapgen, Map extras, Map display
[C++]
Changes (can be) made in C++. Previously named `Code`
Monsters
Monsters both friendly and unfriendly.
Mechanics: Enchantments / Spells
Enchantments and spells
Code: Infrastructure / Style / Static Analysis
Code internal infrastructure and style
json-styled
JSON lint passed, label assigned by github actions
astyled
astyled PR, label is assigned by github actions
labels
Dec 18, 2023
Ooo. This looks super neat. |
github-actions
bot
added
the
BasicBuildPassed
This PR builds correctly, label assigned by github actions
label
Dec 18, 2023
github-actions
bot
added
the
Code: Tests
Measurement, self-control, statistics, balancing.
label
Dec 29, 2023
prharvey
force-pushed
the
pathfinding_refactor
branch
from
December 29, 2023 09:51
e1a006a
to
083022d
Compare
github-actions
bot
removed
the
BasicBuildPassed
This PR builds correctly, label assigned by github actions
label
Dec 29, 2023
…s no longer needed and is actually slower than using a normal tripoint.
…kes clang-tidy happy.
…d restrictions based on vehicle cargo free space.
github-actions
bot
added
BasicBuildPassed
This PR builds correctly, label assigned by github actions
and removed
BasicBuildPassed
This PR builds correctly, label assigned by github actions
labels
Jan 1, 2024
github-actions
bot
added
the
BasicBuildPassed
This PR builds correctly, label assigned by github actions
label
Jan 1, 2024
This was referenced Jan 1, 2024
…tering a tile if the tile is an obstacle.
github-actions
bot
removed
the
BasicBuildPassed
This PR builds correctly, label assigned by github actions
label
Jan 1, 2024
github-actions
bot
added
the
BasicBuildPassed
This PR builds correctly, label assigned by github actions
label
Jan 2, 2024
…ring when moving long distances.
prharvey
changed the title
[WIP] Refactor creature pathfinding, and unify monster pathfinding logic.
Refactor creature pathfinding, and unify monster pathfinding logic.
Jan 2, 2024
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not bump or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered. |
CLIDragon
added a commit
to CLIDragon/Cataclysm-DDA
that referenced
this pull request
Aug 25, 2024
See pull request for more details.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
astyled
astyled PR, label is assigned by github actions
BasicBuildPassed
This PR builds correctly, label assigned by github actions
[C++]
Changes (can be) made in C++. Previously named `Code`
Code: Infrastructure / Style / Static Analysis
Code internal infrastructure and style
Code: Tests
Measurement, self-control, statistics, balancing.
[JSON]
Changes (can be) made in JSON
json-styled
JSON lint passed, label assigned by github actions
Map / Mapgen
Overmap, Mapgen, Map extras, Map display
Mechanics: Enchantments / Spells
Enchantments and spells
Monsters
Monsters both friendly and unfriendly.
stale
Closed for lack of activity, but still valid.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Infrastructure "Refactor/optimize monster pathfinding."
Purpose of change
Monster pathfinding is the single most expensive thing in high traffic areas. While optimizing this in a previous PR, it quickly became clear that the code path was difficult to work with due to pathfinding logic being spread across multiple different parts of the codebase. Bugs in one area would cover for bugs in others, and due to this it wasn't entirely clear what behavior was originally intended.
Describe the solution
Rewrite creature pathfinding to have all pathfinding logic (i.e. can I move here, how much does it cost to move here, find path between x/y, etc) in a single location, such that no logic needs to be duplicated in order to answer questions regarding pathfinding.
This is done by making the pathfinding implementation generic, and having the pathfinding logic take place at the intersection of two different caches: the pathfinding settings, and the map pathfinding cache.
Pathfinding settings is a per-creature structure that encapsulates pathfinding logic specific to that creature, e.g. if it flies, if it can open doors, what fields it is immune to, etc. Caching this logic beforehand allows for fast rejection of bad tiles using a bitfield, which is important when thousands of tiles are visited.
The map pathfinding cache encapsulates pathfinding logic specific to that tile, e.g. if there is an obstacle there, the cost to move on the tile, the existence of traps/fields, etc. This creates a bitfield for the specific tile that allows for fast rejection of a tile when combined with the pathfinding settings.
Note that in the tested location (TCL) the vast majority of paths end up not being found because monsters are stuck behind glass/bars they cannot bash down, or there is a destination in the next room they can hear, but not move to. This is handled by using a bidirectional A* implementation that can fail fast if either point is in a closed in room, and adding exponential backoff to trying to path while stuck.
Finally, as now the pathfinding logic is unified, it means monsters can act much more intelligently. e.g. flying monsters can actually use the ability to fly when finding paths to a destination. To take advantage of this new ability (and to do integration testing), a new flying variant of mi-go was added: the mi-go shrike. It doesn't spawn anywhere yet however, that will be done in a future PR.
Describe alternatives you've considered
As the vast majority of the CPU usage left is due to paths that cannot be found, it would make sense to preprocess the map into a set of islands so these can be quickly rejected. The problem is that it isn't possible in general to do this as each monster type has different bashing thresholds, door avoidance, field avoidance, etc that could result in the creation of an island for one type of monster but not another. This would require per-monster(type) island generation that could quickly take up a lot of memory and processing power. I think it is worth visiting this as a separate step in a future PR: i.e. detect when a path cannot be found, and detect if the monster is stuck on an island it cannot escape from. This should remove what is left of the pathfinding CPU usage, but would make this PR too complicated.
I also briefly experimented with using a weighted variant of Jump Point Search to further optimize this. While in theory this should result in an order of magnitude speedup, the added complexity wasn't worth it for this PR. It is something to try doing in the future. I would tackle this before trying to tackle islands, as if this really does achieve an order of magnitude speedup it would be more generally useful and less error prone.
In the end though, this was largely solved by just using exponential backoff. Sometimes simple is best.
Caching the pathfinding settings is also an option that wasn't done. I'm not comfortable with properly handling the invalidation of that in the same PR. It would be too prone to breaking in weird ways, and didn't want the added complexity.
Testing
Added extensive unit and integration testing for the new pathfinding logic. The old monster movement behavior wasn't tested at all, so I did my best to look at how monsters already behave with the current pathfinding and try to capture that in the tests.
Additional context
Before:
After: