fix(rs2walker): unblock long indoor paths — filtered-edge guard, exhaustion bail, scene doors#1756
Open
runsonmypc wants to merge 10 commits intochsami:developmentfrom
Open
fix(rs2walker): unblock long indoor paths — filtered-edge guard, exhaustion bail, scene doors#1756runsonmypc wants to merge 10 commits intochsami:developmentfrom
runsonmypc wants to merge 10 commits intochsami:developmentfrom
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR enhances the pathfinding and walking system to handle runtime-injected virtual transport edges for doors. It introduces a Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/Pathfinder.java`:
- Around line 307-311: The post-loop exhaustion check can mistakenly mark
searchExhausted true when the loop exited early due to finding the target or
hitting the cutoff after polling the last node; fix by tracking early-exit
conditions and gating the assignment. Introduce booleans (e.g. reachedTarget and
cutoffHit) that are set where the code detects the target (the target-check that
currently breaks the loop) and where the cutoff path breaks, then change the
assignment to: searchExhausted = !cancelled && !reachedTarget && !cutoffHit &&
boundary.isEmpty() && pending.isEmpty(); update references to boundary, pending,
poll, addNeighbors and bestLastNode accordingly so a legitimate bestLastNode for
the target is not discarded.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java`:
- Around line 318-335: The code treats endpoints exactly at distance as
unreachable because later completion logic still uses a strict finalDist <
distance check; change those checks to allow equality (finalDist <= distance) so
endpoints exactly distance tiles away are considered complete, and update both
occurrences referenced (the completion check that compares finalDist and
distance near Rs2Walker and the similar check around lines 676-685) to use <=;
ensure any related retry logic (MAX_NO_PROGRESS_RETRIES) will now short-circuit
by treating the path as successful and returning the appropriate WalkerState
instead of UNREACHABLE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32ee670f-a58c-4b4a-b43b-b95e45915511
📒 Files selected for processing (6)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/Transport.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/CollisionMap.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/Pathfinder.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/PathfinderConfig.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/tile/Rs2Tile.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java
Comment on lines
+307
to
+311
| // Capture exhaustion before finally clears the queues. Both empty | ||
| // after the while loop means BFS terminated by the loop condition | ||
| // (no more nodes to expand) — i.e. dest is unreachable. Cutoff and | ||
| // target-reached exits leave queues non-empty. | ||
| searchExhausted = !cancelled && boundary.isEmpty() && pending.isEmpty(); |
Contributor
There was a problem hiding this comment.
searchExhausted can be set true even when the target was found.
The doc comment claims target-found and cutoff exits leave the queues non-empty, but that's not always the case. The poll happens before the target check (lines 252–256), so a target node that was the last item in boundary (with pending already empty) leaves both queues empty after the break on line 297. Same edge case for the cutoff path (lines 299–302) when it triggers right after polling the last node — addNeighbors doesn't run, so the queues stay empty.
In both edge cases the post-loop assignment evaluates to true, and the walker (which uses this to bail UNREACHABLE) would discard a perfectly valid bestLastNode that points at the target.
🩹 Suggested fix — gate on whether we actually reached a target
- // Capture exhaustion before finally clears the queues. Both empty
- // after the while loop means BFS terminated by the loop condition
- // (no more nodes to expand) — i.e. dest is unreachable. Cutoff and
- // target-reached exits leave queues non-empty.
- searchExhausted = !cancelled && boundary.isEmpty() && pending.isEmpty();
+ // Capture exhaustion before finally clears the queues. Both empty
+ // after the while loop means BFS terminated by the loop condition
+ // (no more nodes to expand) — i.e. dest is unreachable. Guard
+ // against the edge case where the target node itself (or a cutoff)
+ // happens to be the very last polled item, which leaves both
+ // queues empty without the destination actually being unreachable.
+ boolean reachedTarget = bestLastNode != null
+ && Arrays.stream(targetsPacked).anyMatch(t -> t == bestLastNode.packedPosition);
+ searchExhausted = !cancelled && !reachedTarget
+ && boundary.isEmpty() && pending.isEmpty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/Pathfinder.java`
around lines 307 - 311, The post-loop exhaustion check can mistakenly mark
searchExhausted true when the loop exited early due to finding the target or
hitting the cutoff after polling the last node; fix by tracking early-exit
conditions and gating the assignment. Introduce booleans (e.g. reachedTarget and
cutoffHit) that are set where the code detects the target (the target-check that
currently breaks the loop) and where the cutoff path breaks, then change the
assignment to: searchExhausted = !cancelled && !reachedTarget && !cutoffHit &&
boundary.isEmpty() && pending.isEmpty(); update references to boundary, pending,
poll, addNeighbors and bestLastNode accordingly so a legitimate bestLastNode for
the target is not discarded.
Comment on lines
+318
to
+335
| // BFS exhausted both queues without reaching the target — destination is | ||
| // genuinely unreachable (quest-locked gate, walled-off area, etc.). Bail | ||
| // ONLY if endpoint is outside the caller's proximity threshold: when | ||
| // dst is within `distance` of target, the partial path lands the player | ||
| // close enough to satisfy the request (e.g., NPCs accept 1-tile reach, | ||
| // BFS commonly exhausts at adjacent-to-target when the final tile is a | ||
| // throne/altar/object the player can't stand on). Walking the path is | ||
| // correct in that case. Bailing only matters when the closest-found tile | ||
| // is genuinely far — that's what produced the 25+ tile palace tour. | ||
| if (pathfinder.isSearchExhausted() | ||
| && (dst == null || dst.distanceTo(target) > distance)) { | ||
| log.warn("[Walker] Pathfinder exhausted search without reaching target {} (best endpoint {}, dist={}, threshold={}) — bailing UNREACHABLE", | ||
| target, dst, dst == null ? -1 : dst.distanceTo(target), distance); | ||
| Telemetry.recordUnreachable("search-exhausted", Rs2Player.getWorldLocation(), | ||
| target, dst, path == null ? 0 : path.size(), distance, pathfinder); | ||
| setTarget(null); | ||
| return WalkerState.UNREACHABLE; | ||
| } |
Contributor
There was a problem hiding this comment.
distance-exact endpoints still end up as UNREACHABLE.
This new branch correctly keeps exhausted paths whose endpoint is exactly distance tiles from the target, but the completion check later still requires finalDist < distance. With distance == 1, the walker can stop on the valid adjacent tile and then recurse until MAX_NO_PROGRESS_RETRIES trips.
Suggested fix
- if (finalDist < distance) {
+ if (finalDist <= distance) {
setTarget(null);
return WalkerState.ARRIVED;
} else if (partialPath) {Also applies to: 676-685
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java`
around lines 318 - 335, The code treats endpoints exactly at distance as
unreachable because later completion logic still uses a strict finalDist <
distance check; change those checks to allow equality (finalDist <= distance) so
endpoints exactly distance tiles away are considered complete, and update both
occurrences referenced (the completion check that compares finalDist and
distance near Rs2Walker and the similar check around lines 676-685) to use <=;
ensure any related retry logic (MAX_NO_PROGRESS_RETRIES) will now short-circuit
by treating the path as successful and returning the appropriate WalkerState
instead of UNREACHABLE.
Comment on lines
+2249
to
+2263
| boolean reached = sleepUntilTrue(() -> Rs2Player.getWorldLocation().distanceTo(transport.getDestination()) < OFFSET); | ||
| if (!reached) { | ||
| // Static eligibility (member flag, levels, quest, varbits) said this | ||
| // transport was usable, but the click produced no movement (10s timeout). | ||
| // Hidden server-side gate (region unlock, sub-quest progression, etc.). | ||
| // Blacklist the origin so the next pathfinder run routes around it | ||
| // instead of re-emitting the same broken edge — the unbounded retry | ||
| // loop this prevented produced ~3 hours of position oscillation in the | ||
| // wild (240 STALL_RECALC events on one Varrock palace shortcut). | ||
| log.warn("[Walker] Transport {} (type={} origin={} dest={}) failed to deliver player after click — blacklisting origin", | ||
| transport.getDisplayInfo(), transport.getType(), transport.getOrigin(), transport.getDestination()); | ||
| ShortestPathPlugin.getPathfinderConfig().addFailedTransportRestriction(transport.getOrigin()); | ||
| setTarget(null); | ||
| } | ||
| return reached; |
Contributor
There was a problem hiding this comment.
Failed transports are blacklisted, but this walk never replans around them.
After addFailedTransportRestriction(...), calling setTarget(null) cancels the marker/pathfinder for the active walk. The current invocation then falls out on marker-null, and the next recursive pass exits instead of using the new restriction to route around the transport.
Suggested fix
if (!reached) {
// Static eligibility (member flag, levels, quest, varbits) said this
// transport was usable, but the click produced no movement (10s timeout).
// Hidden server-side gate (region unlock, sub-quest progression, etc.).
// Blacklist the origin so the next pathfinder run routes around it
// instead of re-emitting the same broken edge — the unbounded retry
// loop this prevented produced ~3 hours of position oscillation in the
// wild (240 STALL_RECALC events on one Varrock palace shortcut).
log.warn("[Walker] Transport {} (type={} origin={} dest={}) failed to deliver player after click — blacklisting origin",
transport.getDisplayInfo(), transport.getType(), transport.getOrigin(), transport.getDestination());
ShortestPathPlugin.getPathfinderConfig().addFailedTransportRestriction(transport.getOrigin());
- setTarget(null);
+ recalculatePath();
+ return true;
}
return reached;
…Tile bounds
Three independent bugs caused a single account to oscillate at the Varrock
palace north wall for ~3h (240 STALL_RECALC events, 15+ identical wall-door
interactions in 8 min, 29 identical pathfinder calls in 13 min) — exactly the
shape of macro-detection signals.
PathfinderConfig: add a TTL-keyed runtime blacklist of transport origins.
Static eligibility (member flag, levels, quests, varbits) can pass plan-time
while a hidden server-side gate (region unlock, sub-quest progression) makes
the click a no-op at runtime; without this the next pathfinder run keeps
re-emitting the same broken edge.
Rs2Walker.handleTransports: when handleObject returns and the player did not
reach the destination within the wait window, record the origin as failed and
clear the current target so the next walk computes a fresh path that routes
around it.
Rs2Walker.processWalk: thread a no-progress retry counter through the
recursive call chain. After MAX_NO_PROGRESS_RETRIES (5) re-entries where
finalDist did not decrease, return WalkerState.UNREACHABLE instead of looping.
Worst case is now ~50s of stuck behaviour, not unbounded.
Rs2Tile.isTileReachable: bounds-check startX/startY before writing to the
visited[][] grid. Player coordinates outside the loaded scene grid (instance
transitions, stale worldview, base mismatch) previously threw
ArrayIndexOutOfBoundsException ("Index 166 out of bounds for length 104")
and crashed the walker thread mid-loop.
…d transport When a transport is filtered out (member-only, quest gate, F2P, level requirement), the upstream collision map can still permit walking across the underlying edge — the TSV transport entry is the only signal that a fence/jump-gap exists there. BFS would happily route across the obstacle as if it weren't there, producing paths that look valid but require an obstacle the player can't use (canonical case: trellis at (3228,3470) on F2P). For each filtered transport with adjacent (Chebyshev 1) origin and destination on the same plane, register both directions of the edge in a blocked-edges set rebuilt every refreshTransports(). CollisionMap consults it during neighbor expansion and skips blocked edges.
…eshold Pathfinder now records a searchExhausted flag when both boundary and pending queues drain without reaching a target — distinguished from cutoff (queues still hold candidates) and target-found (early break). The walker checks it on entry and bails UNREACHABLE only when the best endpoint is farther than the caller's `distance` threshold. When the endpoint is within threshold (NPC reach, throne/altar approach, etc.) walking the partial path is the correct outcome, so don't bail there — this avoids regressing valid one-tile-short approaches. Also dumps the raw path tiles + every transport hop on each completed run so the actual edge sequence BFS picked is visible in the log.
… for BFS transports.tsv is incomplete — palace internal doors, throne rooms, and generic interior doors aren't listed, so BFS can't route through them and bails UNREACHABLE for destinations a human player could trivially reach. On every refresh, scan the loaded scene for door WallObjects whose ObjectComposition exposes an "Open" action and inject them as bidirectional virtual Transport edges (TSV entries take precedence — existing edges aren't overridden). The whole scan runs in a single client-thread hop. Each WallObject's ObjectComposition lookup needs the client thread; doing those individually from a background thread costs ~26s for ~200 walls. One hop drops it to ~20ms. Bounded to a 52-tile radius around the player to match the rest of the walker's scene-handler range. Existing handleDoors() in Rs2Walker opens these doors at click time — this commit only makes BFS aware of them. Rs2Walker.sessionBlacklistedDoors is promoted to public so the scan can skip blacklisted tiles.
handleTransports() was silently opening Patch H virtual door transports on tiles whose two endpoints both landed somewhere on the path — common in Varrock palace where the path winds past doors connecting rooms it visits. The destIdx >= originIdx ordering check was the only filter, so non-consecutive door pairs would fire and click off-edge. No log line on the open, so it was invisible to debugging. Tag every Patch H virtual door with sceneDiscovered=true at injection time. handleTransports skips tagged transports — handleDoors owns the click, and its WallObject orientation check against path[i]→path[i+1] guarantees only the wall blocking the current step is opened. Add an INFO log right before handleObject() so any future stray opens from this code path are visible in the log.
…2P diagnostic Two small changes folded together since they live in the same area of PathfinderConfig: 1. refresh() can fire on a tick before LocalPlayer is hydrated post-login. getWorldLocation() returned null and NPE'd the client during startup. Skip the tab switch entirely when player location is null. 2. One-shot diagnostic logs the world-type set + agility-shortcut filter decision the first time isFeatureEnabled() sees an AGILITY_SHORTCUT after each refresh(). Used to investigate whether the F2P gate is actually filtering trellises etc. Reset on every refresh() so each cache rebuild logs once.
…p wrong diagonal probes Two related fixes to door handling at click time. (1) Pre-scan: the walker speeds up by clicking far-away minimap targets and advancing the path index past tiles it visually walked over. If a closed door sat between the current tile and the click target, the OSRS server routed the player into the door, and handleDoors() never got called for the skipped indices. Before each skip-click, scan ascending from i for the nearest path tile within scene range that has a closed door; if found, open it and re-enter processWalk so the player's new position is past the door. Bounded by HANDLER_RANGE (not targetIdx) so it still works for interpolated click targets where the indexed range collapses to a single tile. (2) Diagonal corner probes removed: handleDoors() was probing the two shoulder tiles when the path step was diagonal. A wall at the corner facing one of the path tiles passes the orientation check but blocks a different edge entirely, so the OSRS server picks a shoulder for diagonals and opening the wrong shoulder's door is wasted action. Restrict probes to the path tiles themselves (fromWp and toWp via the existing offset loop). Also adds [Walker click] target/result INFO logs so the click sequence is traceable in the log.
…to DEBUG - Remove Pathfinder.java post-run diagnostic dump (per-transport hop log + raw path tile dump). Used during the off-path-door investigation; no longer load-bearing. - Remove [F2P diag] AGILITY_SHORTCUT one-shot in PathfinderConfig and the diagAgilityShortcutLogged AtomicBoolean that backed it. - Demote [Walker click] target/result logs and [Walker] handleTransports interacting log to DEBUG. Useful when actively debugging walker decisions, but noisy at INFO during normal walks. Kept at INFO: [refreshTransports] scene doors added (one line per refresh) and [Walker pre-scan] door-opened (rare event signaling an active behavior worth tracing).
…y-ones Pathfinder: gate searchExhausted on !reachedTarget && !cutoffHit so that polling the last queued node and exiting via target-found or cutoff isn't misread as queue exhaustion; the cutoff variant could otherwise trip a spurious UNREACHABLE downstream. Rs2Walker: arrival check at the end of processWalk used finalDist < distance while the pre-walk early-arrival check at line 225 uses <=. Endpoints exactly at the proximity threshold dropped into the no-progress retry loop instead of returning ARRIVED. Switch to <= for consistency.
runsonmypc
force-pushed
the
fix/rs2walker-failed-transports
branch
from
c81e241 to
de8ec9c
Compare
Regenerated after rebasing onto upstream/development. The earlier baseline refresh (c81e241 on the pre-rebase branch) was skipped during the rebase because both upstream's 98e1cfe and our refresh produced different snapshots — lambda indices and inferred-list violations depend on the combined bytecode, which only exists post-rebase. 57 new entries + 29 obsolete entries removed; reflects upstream's walker refactor (f9e1b9d) layered with our walker fixes. Equivalent to: ./gradlew :client:runUnitTests --tests "*ClientThreadGuardrailTest*" \ -Dmicrobot.guardrail.regenerate-baseline=true
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
1 participant
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
Fixes for Rs2Walker getting stuck, retrying forever, or clicking the wrong door on long indoor paths (Varrock palace was the canonical case driving the work). Builds on the prior Round-1 commit (330485ef33) which already covered failed-transport blacklisting, bounded recursion, and Rs2Tile bounds-checking.
Six commits, each landing one fix:
fix(pathfinder): block walking edges whose only crossing is a filtered transport— when a transport is filtered out (member-only, quest gate, F2P), the upstream collision map can still permit walking across the underlying obstacle. Track those edges in a per-refresh blocked-edge set; CollisionMap consults it during neighbor expansion.fix(walker): bail UNREACHABLE on BFS exhaustion outside proximity threshold— Pathfinder records a `searchExhausted` flag when both queues drain without target reach; walker bails UNREACHABLE only when the best endpoint is farther than the caller's `distance` threshold (preserves valid one-tile-short approaches at NPCs/altars).feat(pathfinder): inject scene-discovered doors as virtual transports for BFS— transports.tsv is incomplete (palace internal doors etc.). On every refresh, scan the loaded scene for door WallObjects and inject bidirectional virtual TRANSPORT edges so BFS can route through them. Whole scan in a single client-thread hop (~20ms vs ~26s for per-wall hops).fix(walker): skip scene-discovered transports in handleTransports— handleTransports' destIdx >= originIdx ordering check was not consecutive; scene-discovered virtuals fired silently for any door whose two endpoints both landed somewhere on the path. Tag the virtuals with `sceneDiscovered=true` and skip them — handleDoors owns the actual click via WallObject orientation against `path[i]→path[i+1]`. Adds INFO log before any handleObject open.chore(pathfinder): null-guard refresh tab switch + AGILITY_SHORTCUT F2P diagnostic— refresh() can fire pre-LocalPlayer-hydration on login and NPE'd. Skip tab switch when player loc is null. One-shot F2P diagnostic on first AGILITY_SHORTCUT seen each refresh.fix(walker): pre-scan path for closed doors before skip-clicking, drop wrong diagonal probes— skip-clicks past closed doors stranded the player; pre-scan ascending from i for the nearest in-range closed path tile and open it before the click. Also drops handleDoors' diagonal corner probes that were opening shoulder-tile doors not actually blocking the fromWp↔toWp edge.Test plan