fix(pathfinding): prevent corrupted and stale mob waypoints#2519
Draft
RedstoneGithub wants to merge 4 commits into
Draft
fix(pathfinding): prevent corrupted and stale mob waypoints#2519RedstoneGithub wants to merge 4 commits into
RedstoneGithub wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the navigator’s A* relaxation logic to avoid overwriting a node’s predecessor with a higher/equal-cost route, preventing cycles during path reconstruction (which can manifest as mobs briefly pathing “backwards” after attacks).
Changes:
- Introduces helper functions to centralize “should relax” (strictly lower g) and to fetch a node’s known g from either open or closed sets.
- Updates neighbor relaxation to consider both open and closed sets and to reopen a closed node only for strictly lower-cost paths.
- Adds unit tests covering the relaxation predicate and a closed-set lookup scenario.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+484
to
+499
|
|
||
| #[test] | ||
| fn does_not_reopen_closed_node_with_worse_cost() { | ||
| let closed_node = Node { | ||
| g: 2.0, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let mut closed_set = HashMap::new(); | ||
| closed_set.insert(closed_node.pos.0, closed_node); | ||
|
|
||
| let known_g = known_node_g(&BinaryHeap::new(), &closed_set, &closed_node); | ||
| assert_eq!(known_g, Some(2.0)); | ||
| assert!(!should_relax_node(4.0, known_g)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pumpkin/src/entity/ai/pathfinder/mod.rs:433
- After skipping a waypoint via
should_target_next_node_in_direction,tickreturns immediately without recomputing yaw/movement for the newly advanced next node. That can leave the mob targeting the old (now-behind) node for one tick, causing brief backtracking/jitter and undermining the intent of skipping the behind waypoint. Advance the path index but continue through the rest oftickso control outputs are computed for the updated next node in the same tick.
let current_pos = entity.entity.pos.load();
let navigation_pos =
Vector3::new(current_pos.x, (current_pos.y + 0.5).floor(), current_pos.z);
if should_target_next_node_in_direction(path, navigation_pos) {
path.advance();
self.current_goal = Some(goal);
return;
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pumpkin/src/entity/ai/pathfinder/mod.rs:430
- This adds a new path-following heuristic that advances the path when the next waypoint is "behind" the mob. The PR description and title focus on A* node relaxation / closed-set reopening, so this behavioral change looks out of scope and makes it harder to evaluate risk and intent. Either update the PR description to explicitly cover this new behavior (and why it’s needed), or move this change into a separate PR.
let current_pos = entity.entity.pos.load();
let navigation_pos =
Vector3::new(current_pos.x, (current_pos.y + 0.5).floor(), current_pos.z);
if should_target_next_node_in_direction(path, navigation_pos) {
path.advance();
Comment on lines
+136
to
+144
| let to_current = node_center(current).sub(&mob_pos); | ||
| let to_next = node_center(next).sub(&mob_pos); | ||
| let current_distance_sq = | ||
| to_current.x * to_current.x + to_current.y * to_current.y + to_current.z * to_current.z; | ||
| let next_distance_sq = to_next.x * to_next.x + to_next.y * to_next.y + to_next.z * to_next.z; | ||
|
|
||
| current_distance_sq < 4.0 | ||
| && (next_distance_sq < current_distance_sq || current_distance_sq < 0.5) | ||
| && to_current.x * to_next.x + to_current.y * to_next.y + to_current.z * to_next.z < 0.0 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Marking as draft, looking for feedback.
What Changed
Updated navigator A* node relaxation so an existing node’s predecessor is replaced only when the newly discovered route has a strictly lower g cost.
Added Vanilla-style directional waypoint skipping. When the current waypoint is behind the mob relative to the following waypoint, navigation advances past the stale waypoint instead of turning back toward it. This is disabled for fire-adjacent, damage-adjacent, and walkable-door nodes.
Added regression coverage for:
Why
After creating #2442, I realized when a mob would hit the player, it would turn away and walk a few blocks backward before pathing to the player again. The mobs would also unnecessarily spin at times. I noticed two reasons that caused this, one was incorrect A* behavior, the second was fixed by adding waypoint skipping (where possible, Vanilla also does this).
came_frompredecessor with a worse route. This could create cycles during reconstruction and produce routes beginning away from the mob.Impact
Mob navigation paths retain an acyclic predecessor chain back to their start node.
While following valid paths, mobs can advance past stale waypoints behind them, matching Vanilla ground-navigation behavior and preventing the post-attack backward correction/spin.
Known Limitations
This does not attempt full parity with Vanilla for navigation. It builds on top of Pumpkin's existing nav behavior. I am looking for people to test pathing in-game to ensure there are no regressions.
Validation
Passed locally:
AI was used to help with this PR.