[NOT READY] fix(walker): spirit tree filtering, transport-zone Dijkstra, cross-plane fixes#1786
Open
runsonmypc wants to merge 29 commits into
Open
[NOT READY] fix(walker): spirit tree filtering, transport-zone Dijkstra, cross-plane fixes#1786runsonmypc wants to merge 29 commits into
runsonmypc wants to merge 29 commits into
Conversation
…gate skip Three fixes for Rs2Walker transport and gate handling: 1. Prevent infinite loops with network transports (spirit trees, fairy rings, quetzals, gliders). The current-tile transport handler's "closer to goal" heuristic allowed taking any network transport whose destination was nearer the goal, creating A→B→A loops. Now network transports require the destination to be on the forward path points. 2. Don't skip segment handlers when the next path tile is unreachable. The 15s post-transport optimization blanket-skipped all handlers (including door/gate resolution) when no nearby transport was planned. Adding a reachability check ensures gates are still opened. 3. Widen blocker scan radius from 2 to 4 tiles. Path smoothing can place path points 3+ tiles from a gate object (e.g. Al Kharid gate), causing the blocker scan to filter it out and leaving the walker stuck in recovery clicks that never reach the gate.
The post-click sleepUntil waited up to 2000ms for the player to reach within 2-4 tiles of the click target, but had no check for the player stopping. When the player hit a gate or obstacle mid-walk, the walker sat idle for the remainder of the timeout before re-evaluating. Add !Rs2Player.isMoving() to the wake condition (after the 600ms game tick floor) so the walker reacts immediately when movement stops.
…breaks PATH_VARIANCE_TOLERANCE_CHEBYSHEV raised from 3 to 6. The variance check is the fallback when the BFS-based isNearPath() fails (e.g. a nearby wall blocks BFS despite the player being 2-3 tiles from the path). At 3, normal path-smoothing drift during walking triggered off-path-but-moving breaks that killed the click chain — the walker would watch the player walk without issuing new clicks, causing visible pauses when the player eventually stopped. At 6 tiles the walker keeps clicking through minor drift while still recalculating when genuinely lost.
isTileReachable() dispatches an unbounded BFS to the client thread for every path index in the inner walk loop. With 10+ indices per iteration, each blocking on the client thread round-trip, this caused multi-second delays before the first minimap click (9s observed on a 23-point path). Compute reachable tiles once via getReachableTilesFromTile(radius=45) before the inner loop and use HashMap lookups instead. One client-thread BFS call replaces N sequential blocking calls.
Hoist the cache variables above the outer tail loop and only recompute when the player's position changes. The outer loop can iterate up to 64 times (door settling, recovery clicks, interim yields) — previously each iteration recomputed the full radius-45 BFS on the client thread even when the player was standing still.
- Rewrite getReachableTilesFromTileInternal to use ArrayDeque BFS (O(n)) instead of O(n*d) HashMap scanning with per-tile HashSet allocation - Use raw tile-by-tile path for door/rockfall/transport detection instead of smoothed path, fixing gates missed between widely-spaced smooth points - Add unbounded BFS fallback for reachability check to eliminate false unreachable hits from bounded cache - Always recompute reachable tiles cache each iteration to pick up collision changes (gates opened by other players) - Skip reachable raw edges in door/rockfall scanning for performance; transport scanning always checks all edges (teleports aren't blockages) - Keep smoothed path for click targeting and spatial proximity checks
…outing Transport nodes (fairy rings, spirit trees, etc.) were placed in a separate pending queue ordered by g-cost, only processed when cheaper than the boundary's g-cost. This caused the pathfinder to walk 988 tiles instead of using fairy ring CIR (~260 tiles) because transport nodes at g=58 were starved behind walking nodes at g<58. Now all nodes share the A* boundary queue with proper heuristic, so a fairy ring destination at f=111 is explored immediately over a walking path at f=2475. Also: unbounded reachable tiles cache, remove isTileReachable fallback.
- refreshTransports: re-read config with = instead of &= so fairy rings / spirit trees / gliders recover after early startup calls when player data isn't loaded yet - stallThresholdMs: replace Rs2Player.isInteracting() (can get stuck returning true) with isMoving()||isAnimating() for the interacting multiplier - close bank before processWalk alongside existing closeWorldMap()
…nreachable The BFS in getReachableTilesFromTileInternal only checked 4 cardinal directions, but OSRS supports diagonal movement. Areas where the only path requires diagonal movement (around corners, through diagonal gaps) were falsely flagged unreachable, causing 4-second pauses while the walker ran expensive door/blocker scans on tiles that were actually walkable. - Add NE/NW/SE/SW diagonal expansion to the BFS, matching the collision checks used by the pathfinder's CollisionMap - Refresh the reachable cache before entering the heavy unreachable handler so transient blocks (stale cache) are caught cheaply - Add Taverley wall blocked edges (gate at x=2936 excluded)
…overy A* with Chebyshev heuristic can't discover multi-hop transport chains where the intermediate leg (walking to a fairy ring) moves AWAY from the target. The heuristic inflates f-cost on those nodes so the A* settles on a worse single-hop teleport before ever exploring the chain. Pre-seed the boundary queue with bridge nodes representing chained routes: item teleport → walk → network transport (fairy ring, spirit tree, gnome glider). Each bridge destination enters the queue with combined g-cost and its own heuristic, so the A* naturally evaluates the chain against direct routes. Also close world map before opening bank in banked-transport flows.
The transport refresh snapshot was captured AFTER filterSimilarTransports ran, baking target-specific filtering into the cached data. This forced targetPacked into the cache key, meaning every pathfind to a new destination triggered a full 450ms refresh of 11,738 transports. Move snapshot capture BEFORE filterSimilarTransports so the cached data is target-independent. filterSimilarTransports still runs after every cache restore (which it already did), so target-specific filtering is always applied fresh. Remove targetPacked from the cache key. Sequential pathfinds (birdhouse runs, farming runs) now hit the cache on the 2nd+ destination instead of paying 450ms each time.
walkWithBankedTransportsAndState ran compareRoutes on every walk, which internally calls getWalkPath 3 times + 2 explicit config.refresh calls — 5 full transport refreshes (450ms each) even for a 20-tile path. For short walks (<=200 Chebyshev), skip the comparison entirely and walk directly. Banking detours only make sense for long-distance routes where a teleport item from the bank could save significant tiles.
…ansport The current-tile transport handler oscillated on stairs: after a planned transport UP (segment handler), the current-tile handler saw the stairs at the new tile, the plane filter allowed it (playerPlane != targetPlane), and it took them back DOWN — creating an infinite up/down loop. Block transports whose destination matches lastTransportHandledAtLocation (where the player was before the last transport). This prevents the handler from undoing a transport that just completed.
…e paths The chain bridge injection could pick a teleport→network chain over nearby stairs for cross-plane walks because the heuristic ignores plane changes. A games necklace chain with f=200 beats a stair path whose true cost is 50 but explores 2M nodes to discover. Gate injection behind MIN_CHAIN_INJECT_DISTANCE (500 Chebyshev). Short walks use the normal A* which finds stairs through local expansion. Chains are only beneficial for long-distance routes where walking to a fairy ring after a teleport saves hundreds of tiles.
The chain injection approach was fundamentally wrong — it created phantom parent chains that don't correspond to real transports, could teleport players out of buildings for in-building walks, and required arbitrary distance guards to contain the damage. Reverted.
When start and target are within 200 Chebyshev tiles, skip refreshTeleports entirely so the A* only considers walking and local transports (stairs, doors). Prevents the pathfinder from choosing absurd teleport routes (games necklace to Corp) for in-building cross-plane walks where the stairs are nearby but the collision map makes them expensive to discover.
Tile (1621, 3822, plane=1) is fully blocked in collision-map.zip (N/E/S/W all false) despite being walkable in-game. This made the target unreachable, causing the A* to explore 2M nodes across the entire map and ultimately choose a teleport-to-Corp route for an in-building walk.
The pathfinder used a static collision-map.zip that can have incorrect data (tiles marked blocked when they're walkable in-game). This caused the A* to explore millions of nodes and choose absurd teleport routes for simple in-building walks. On each refresh, snapshot the game engine's live collision flags for all planes of the loaded scene (104x104). CollisionMap.n/e/s/w now check the live snapshot first for tiles within the scene, falling back to the static map only for tiles outside. The live data is always correct — it reflects actual walls, doors, and terrain as the game engine sees them. Removes the ignoreCollisionPacked bandaid for the Arceuus Library tile since the live data handles it properly.
CollisionMap is stored in a ThreadLocal — each thread gets its own instance. The snapshot was captured on the refresh thread's instance but the pathfinder runs on a different thread with a different CollisionMap that never received the snapshot. Making the snapshot fields and setter static ensures all threads see the same live data.
Two bugs found by review: 1. isBlocked() didn't check BLOCK_MOVEMENT_FULL from live data. A tile occupied by an object (rock, tree) with no directional flags set would appear walkable — the pathfinder could route through walls. Now checks BLOCK_MOVEMENT_FULL first. 2. Three separate volatile fields (liveFlags, liveBaseX, liveBaseY) could be read inconsistently during a scene change. Bundle into a single immutable LiveSnapshot object with one volatile reference. Also consolidate the repeated live-flag lookup into liveFlag().
The initial refreshTeleports guard was bypassed by the wilderness level initialization. wildernessLevel starts at 31, so on the FIRST node expansion the A* detected 'not in wilderness' and called refreshTeleports(node, 0) — adding all teleports including games necklace to Corp. This caused the pathfinder to teleport out of buildings for in-building cross-plane walks. For short-distance paths: set wildernessLevel=0 upfront (skipping the initial refresh), and guard the in-loop wilderness refresh with the shortDistance flag so teleports are never injected mid-search.
The live snapshot was REPLACING static data for scene tiles, which introduced a regression: closed doors in live data blocked paths that the static map showed as open, preventing the pathfinder from finding stair routes and forcing teleport detours. Now: if the static map says walkable, trust it. Only consult live data when the static map says blocked — the live data can reveal the tile is actually walkable (fixing stale static data) but can never make a walkable tile appear blocked.
…ort-zone Dijkstra - Revert live collision snapshot (CollisionMap + PathfinderConfig): the "unblock only" overlay had structural bugs (missing BLOCK_MOVEMENT_FULL, s()/w() not checking destination tile) causing paths through furniture. Walker already handles dynamic obstacles via door/transport handlers. - Restore upstream Pathfinder with pending queue (transports sorted by g-cost, walking nodes by f-cost). Our prior merge into boundary with heuristic made transport chain discovery worse, not better. - Transport-zone Dijkstra: walking nodes after a transport landing get heuristic=0 (Dijkstra) so A* freely explores to find the next transport in a chain. Pure walking retains full Chebyshev heuristic for performance. This fixes multi-hop routing (e.g. Ardougne cloak -> fairy ring). - Fix transportsPacked overwrite in refreshTeleports: the packed map was unconditionally overwritten with teleports-only, losing existing transports (stairs/doors) at the player's position. Now uses the merged set.
… short-path clicks - Skip path tiles on a different plane than the player in processWalk loop. These tiles trigger unreachable handlers, BFS refreshes, and door probes that can never succeed from the wrong plane. Eliminates 8-second stalls on cross-plane walks. - Bound getReachableTilesFromTile to HANDLER_RANGE*3 (39 tiles) instead of Integer.MAX_VALUE. Reduces BFS from ~10,816 tiles to ~6,000 max while covering routes around walls. - Add diagonal movement to isTileReachableInternal to match getReachableTilesFromTileInternal. Fixes disagreement between the two methods that caused oscillation on diagonally-reachable tiles. - For paths <= 5 tiles, set nextWalkingDistance to 0 so the path loop can click nearby tiles instead of spinning without issuing movement. - Lower banking skip threshold from 200 to 100 Chebyshev. - Recalculate path instead of giving up on pathfinder-still-null when target is still set.
…re off isSpiritTreeDestinationEnabled only checked the destination, so routes FROM a disabled tree (e.g. Farming Guild) to permanent trees (GE, Gnome Stronghold) still passed. Renamed to isSpiritTreeRouteEnabled and now rejects any route where either endpoint touches a toggled-off tree. Also removes dead pre-quest-gate assignments in refresh() that were immediately overwritten by refreshTransports(), and adds a defensive useSpiritTrees check in the walker's spirit tree dispatch.
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 refines pathfinding heuristics and walker path execution across four modules. Pathfinder adds a post-transport heuristic propagation mechanism to guide frontier ordering after transport nodes or heuristic-zero states. PathfinderConfig introduces a dedicated spirit-tree toggle, refactors spirit-tree route gating to check proximity and destinations, and updates quest-dependent transport refresh logic with adjusted caching behavior. Rs2Tile replaces its reachable-tiles computation with a collision-flag-aware BFS that handles instanced regions. Rs2Walker undergoes substantial refactoring to use raw-path semantics for obstacle handling, introduces per-loop reachable-tile caching, updates door/rockfall/transport handlers, adds spirit-tree config gating, and includes navigation refinements such as minimap distance adjustments, world map closure, and a fast path for close banked-transport targets. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/Pathfinder.java (1)
207-218:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't use
heuristic == 0as the post-transport signal.Lines 244-245 and 260-261 already allow plain walking nodes to get
0from the wrapped underground heuristic. Once one of those nodes is popped, this change propagates0to the surrounding walking region even though no transport was taken, which can blow up the frontier and push long surface↔underground searches into the cutoff path. Carry an explicitafterTransportbit on the node instead of inferring it from the heuristic value.Also applies to: 312-323, 339-350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/Pathfinder.java` around lines 207 - 218, The code is incorrectly using neighbor.heuristic == 0 as a marker for "after taking a transport"; add an explicit boolean flag (e.g., Node.afterTransport) to Node (and set it on TransportNode instances or when you enqueue a neighbor that was reached via a transport) and replace all uses of heuristic==0 in Pathfinder (including the neighbor handling around pending.add(...), the heuristic assignment sites, and any later checks that infer post-transport state) to check that boolean instead; ensure when you pop a transport node or enqueue a neighbor via pending you set neighbor.afterTransport = true, and when computing heuristic for normal walking neighbors set afterTransport = false so the wrapped underground heuristic 0 doesn't get misinterpreted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/tile/Rs2Tile.java`:
- Around line 353-453: The BFS still uses collision flags even when
ignoreCollision is true; update getReachableTilesFromTileInternal to
short-circuit collision logic: skip the BLOCK_MOVEMENT_FULL removal check and
the flag-derived canE/canW/canN/canS calculations when ignoreCollision is true,
and instead enqueue cardinal neighbors by bounds-only checks (use
isWithinBounds(sx±1, sy) / isWithinBounds(sx, sy±1)) and diagonals by
bounds-only checks (isWithinBounds on the diagonal target) without consulting
flags or CollisionDataFlag; keep using tileDistances.putIfAbsent(...) and
queue.add(...) for all enqueues so existing dedup logic remains.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java`:
- Around line 3268-3275: The code is comparing candidate transport destinations
against lastTransportHandledAtLocation (the previous post-landing tile) which is
the last destination, not the last origin, so reverse transports (B -> A after A
-> B) still pass; declare and maintain a separate field (e.g.,
lastTransportOrigin) in Rs2Walker, set it to the player's tile at transport
start (or capture it in the method that begins handling a Transport) and
update/clear it alongside finishHandledTransport(); then change the stream
filter to compare t.getDestination() against this lastTransportOrigin (not
lastTransportHandledAtLocation) to correctly suppress immediate reverse
transport candidates.
---
Outside diff comments:
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/Pathfinder.java`:
- Around line 207-218: The code is incorrectly using neighbor.heuristic == 0 as
a marker for "after taking a transport"; add an explicit boolean flag (e.g.,
Node.afterTransport) to Node (and set it on TransportNode instances or when you
enqueue a neighbor that was reached via a transport) and replace all uses of
heuristic==0 in Pathfinder (including the neighbor handling around
pending.add(...), the heuristic assignment sites, and any later checks that
infer post-transport state) to check that boolean instead; ensure when you pop a
transport node or enqueue a neighbor via pending you set neighbor.afterTransport
= true, and when computing heuristic for normal walking neighbors set
afterTransport = false so the wrapped underground heuristic 0 doesn't get
misinterpreted.
🪄 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: aa32d05a-69b2-45f7-80ef-b73bc0dcfed7
⛔ Files ignored due to path filters (1)
runelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/blocked_edges.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
runelite-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
353
to
+453
| private static HashMap<WorldPoint, Integer> getReachableTilesFromTileInternal(WorldPoint tile, int distance, boolean ignoreCollision) { | ||
| final HashMap<WorldPoint, Integer> tileDistances = new HashMap<>(); | ||
| if (tile == null) return tileDistances; | ||
|
|
||
| final int[][] flags = getFlagsInternal(); | ||
| if (flags == null) return tileDistances; | ||
|
|
||
| final WorldView wv = Microbot.getClient().getTopLevelWorldView(); | ||
| final boolean isInstance = wv.getScene().isInstance(); | ||
|
|
||
| final ArrayDeque<WorldPoint> queue = new ArrayDeque<>(); | ||
| tileDistances.put(tile, 0); | ||
| queue.add(tile); | ||
|
|
||
| for (int i = 0; i < distance + 1; i++) { | ||
| int dist = i; | ||
| for (var kvp : tileDistances.entrySet().stream().filter(x -> x.getValue() == dist).collect(Collectors.toList())) { | ||
| var point = kvp.getKey(); | ||
| LocalPoint localPoint; | ||
| if (Microbot.getClient().getTopLevelWorldView().isInstance()) { | ||
| WorldPoint worldPoint = WorldPoint.toLocalInstance(Microbot.getClient().getTopLevelWorldView(), point).stream().findFirst().orElse(null); | ||
| if (worldPoint == null) break; | ||
| localPoint = LocalPoint.fromWorld(Microbot.getClient().getTopLevelWorldView(), worldPoint); | ||
| } else | ||
| localPoint = LocalPoint.fromWorld(Microbot.getClient().getTopLevelWorldView(), point); | ||
|
|
||
| CollisionData[] collisionMap = Microbot.getClient().getTopLevelWorldView().getCollisionMaps(); | ||
| if (collisionMap != null && localPoint != null) { | ||
| CollisionData collisionData = collisionMap[Microbot.getClient().getTopLevelWorldView().getPlane()]; | ||
| int[][] flags = collisionData.getFlags(); | ||
| int data = flags[localPoint.getSceneX()][localPoint.getSceneY()]; | ||
|
|
||
| Set<MovementFlag> movementFlags = MovementFlag.getSetFlags(data); | ||
|
|
||
| if (!ignoreCollision && !tile.equals(point)) { | ||
| if (movementFlags.contains(MovementFlag.BLOCK_MOVEMENT_FULL) | ||
| || movementFlags.contains(MovementFlag.BLOCK_MOVEMENT_FLOOR)) { | ||
| tileDistances.remove(point); | ||
| continue; | ||
| } | ||
| } | ||
| while (!queue.isEmpty()) { | ||
| final WorldPoint point = queue.poll(); | ||
| final int dist = tileDistances.get(point); | ||
|
|
||
| final LocalPoint lp; | ||
| if (isInstance) { | ||
| WorldPoint instancePoint = WorldPoint.toLocalInstance(wv, point).stream().findFirst().orElse(null); | ||
| if (instancePoint == null) continue; | ||
| lp = LocalPoint.fromWorld(wv, instancePoint); | ||
| } else { | ||
| lp = LocalPoint.fromWorld(wv, point); | ||
| } | ||
| if (lp == null) continue; | ||
|
|
||
| final int sx = lp.getSceneX(); | ||
| final int sy = lp.getSceneY(); | ||
| if (!isWithinBounds(sx, sy)) continue; | ||
|
|
||
| if (kvp.getValue() >= distance) | ||
| continue; | ||
|
|
||
| if (!movementFlags.contains(MovementFlag.BLOCK_MOVEMENT_EAST)) | ||
| tileDistances.putIfAbsent(point.dx(1), dist + 1); | ||
| if (!movementFlags.contains(MovementFlag.BLOCK_MOVEMENT_WEST)) | ||
| tileDistances.putIfAbsent(point.dx(-1), dist + 1); | ||
| if (!movementFlags.contains(MovementFlag.BLOCK_MOVEMENT_NORTH)) | ||
| tileDistances.putIfAbsent(point.dy(1), dist + 1); | ||
| if (!movementFlags.contains(MovementFlag.BLOCK_MOVEMENT_SOUTH)) | ||
| tileDistances.putIfAbsent(point.dy(-1), dist + 1); | ||
| final int data = flags[sx][sy]; | ||
|
|
||
| if (!ignoreCollision && !tile.equals(point)) { | ||
| if ((data & CollisionDataFlag.BLOCK_MOVEMENT_FULL) != 0) { | ||
| tileDistances.remove(point); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (dist >= distance) continue; | ||
|
|
||
| final boolean canE = (data & CollisionDataFlag.BLOCK_MOVEMENT_EAST) == 0; | ||
| final boolean canW = (data & CollisionDataFlag.BLOCK_MOVEMENT_WEST) == 0; | ||
| final boolean canN = (data & CollisionDataFlag.BLOCK_MOVEMENT_NORTH) == 0; | ||
| final boolean canS = (data & CollisionDataFlag.BLOCK_MOVEMENT_SOUTH) == 0; | ||
|
|
||
| if (canE) { | ||
| WorldPoint neighbor = point.dx(1); | ||
| if (tileDistances.putIfAbsent(neighbor, dist + 1) == null) | ||
| queue.add(neighbor); | ||
| } | ||
| if (canW) { | ||
| WorldPoint neighbor = point.dx(-1); | ||
| if (tileDistances.putIfAbsent(neighbor, dist + 1) == null) | ||
| queue.add(neighbor); | ||
| } | ||
| if (canN) { | ||
| WorldPoint neighbor = point.dy(1); | ||
| if (tileDistances.putIfAbsent(neighbor, dist + 1) == null) | ||
| queue.add(neighbor); | ||
| } | ||
| if (canS) { | ||
| WorldPoint neighbor = point.dy(-1); | ||
| if (tileDistances.putIfAbsent(neighbor, dist + 1) == null) | ||
| queue.add(neighbor); | ||
| } | ||
|
|
||
| if (canN && canE && isWithinBounds(sx + 1, sy) && isWithinBounds(sx, sy + 1) && isWithinBounds(sx + 1, sy + 1) | ||
| && (flags[sx + 1][sy] & (CollisionDataFlag.BLOCK_MOVEMENT_FULL | CollisionDataFlag.BLOCK_MOVEMENT_NORTH)) == 0 | ||
| && (flags[sx][sy + 1] & (CollisionDataFlag.BLOCK_MOVEMENT_FULL | CollisionDataFlag.BLOCK_MOVEMENT_EAST)) == 0 | ||
| && (flags[sx + 1][sy + 1] & CollisionDataFlag.BLOCK_MOVEMENT_FULL) == 0) { | ||
| WorldPoint neighbor = new WorldPoint(point.getX() + 1, point.getY() + 1, point.getPlane()); | ||
| if (tileDistances.putIfAbsent(neighbor, dist + 1) == null) | ||
| queue.add(neighbor); | ||
| } | ||
| if (canN && canW && isWithinBounds(sx - 1, sy) && isWithinBounds(sx, sy + 1) && isWithinBounds(sx - 1, sy + 1) | ||
| && (flags[sx - 1][sy] & (CollisionDataFlag.BLOCK_MOVEMENT_FULL | CollisionDataFlag.BLOCK_MOVEMENT_NORTH)) == 0 | ||
| && (flags[sx][sy + 1] & (CollisionDataFlag.BLOCK_MOVEMENT_FULL | CollisionDataFlag.BLOCK_MOVEMENT_WEST)) == 0 | ||
| && (flags[sx - 1][sy + 1] & CollisionDataFlag.BLOCK_MOVEMENT_FULL) == 0) { | ||
| WorldPoint neighbor = new WorldPoint(point.getX() - 1, point.getY() + 1, point.getPlane()); | ||
| if (tileDistances.putIfAbsent(neighbor, dist + 1) == null) | ||
| queue.add(neighbor); | ||
| } | ||
| if (canS && canE && isWithinBounds(sx + 1, sy) && isWithinBounds(sx, sy - 1) && isWithinBounds(sx + 1, sy - 1) | ||
| && (flags[sx + 1][sy] & (CollisionDataFlag.BLOCK_MOVEMENT_FULL | CollisionDataFlag.BLOCK_MOVEMENT_SOUTH)) == 0 | ||
| && (flags[sx][sy - 1] & (CollisionDataFlag.BLOCK_MOVEMENT_FULL | CollisionDataFlag.BLOCK_MOVEMENT_EAST)) == 0 | ||
| && (flags[sx + 1][sy - 1] & CollisionDataFlag.BLOCK_MOVEMENT_FULL) == 0) { | ||
| WorldPoint neighbor = new WorldPoint(point.getX() + 1, point.getY() - 1, point.getPlane()); | ||
| if (tileDistances.putIfAbsent(neighbor, dist + 1) == null) | ||
| queue.add(neighbor); | ||
| } | ||
| if (canS && canW && isWithinBounds(sx - 1, sy) && isWithinBounds(sx, sy - 1) && isWithinBounds(sx - 1, sy - 1) | ||
| && (flags[sx - 1][sy] & (CollisionDataFlag.BLOCK_MOVEMENT_FULL | CollisionDataFlag.BLOCK_MOVEMENT_SOUTH)) == 0 | ||
| && (flags[sx][sy - 1] & (CollisionDataFlag.BLOCK_MOVEMENT_FULL | CollisionDataFlag.BLOCK_MOVEMENT_WEST)) == 0 | ||
| && (flags[sx - 1][sy - 1] & CollisionDataFlag.BLOCK_MOVEMENT_FULL) == 0) { | ||
| WorldPoint neighbor = new WorldPoint(point.getX() - 1, point.getY() - 1, point.getPlane()); | ||
| if (tileDistances.putIfAbsent(neighbor, dist + 1) == null) | ||
| queue.add(neighbor); | ||
| } |
Contributor
There was a problem hiding this comment.
ignoreCollision still obeys collision flags.
When ignoreCollision is true, this BFS still derives canN/canS/canE/canW and all diagonal eligibility from CollisionDataFlag, so getReachableTilesFromTileIgnoreCollision(...) no longer ignores blocked edges. That breaks the method contract and any caller using it as a pure radius/reachability sample. Split the expansion path so the ignoreCollision branch enqueues in-bounds neighbors without consulting movement flags.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/tile/Rs2Tile.java`
around lines 353 - 453, The BFS still uses collision flags even when
ignoreCollision is true; update getReachableTilesFromTileInternal to
short-circuit collision logic: skip the BLOCK_MOVEMENT_FULL removal check and
the flag-derived canE/canW/canN/canS calculations when ignoreCollision is true,
and instead enqueue cardinal neighbors by bounds-only checks (use
isWithinBounds(sx±1, sy) / isWithinBounds(sx, sy±1)) and diagonals by
bounds-only checks (isWithinBounds on the diagonal target) without consulting
flags or CollisionDataFlag; keep using tileDistances.putIfAbsent(...) and
queue.add(...) for all enqueues so existing dedup logic remains.
Comment on lines
+3268
to
+3275
| WorldPoint lastTransportOrigin = lastTransportHandledAtLocation; | ||
| List<Transport> candidates = transports.stream() | ||
| .filter(t -> t.getDestination() != null) | ||
| // Local adjacent same-plane edges (doors/gates) are handled by segment door/object | ||
| // logic; current-tile transport probing can bounce on these and create loops. | ||
| .filter(t -> !isAdjacentSamePlaneTransport(t)) | ||
| .filter(t -> lastTransportOrigin == null | ||
| || !t.getDestination().equals(lastTransportOrigin)) |
Contributor
There was a problem hiding this comment.
Track the previous transport origin separately here.
lastTransportHandledAtLocation is the post-landing tile (finishHandledTransport() writes the player's location after the transport completes), so this filter is comparing against the last destination, not the last origin. After A -> B, the reverse candidate B -> A still passes, which reopens the bounce loop this branch is trying to prevent. Store the last origin explicitly and compare against that instead.
Suggested fix
+ private static volatile WorldPoint lastTransportHandledOrigin = null;
...
- WorldPoint lastTransportOrigin = lastTransportHandledAtLocation;
+ WorldPoint lastTransportOrigin = lastTransportHandledOrigin; private static boolean finishHandledTransport(Transport transport) {
long handoffStartedAt = System.currentTimeMillis();
lastTransportHandledAtMs = handoffStartedAt;
+ lastTransportHandledOrigin = transport != null ? transport.getOrigin() : null;
lastTransportHandledAtLocation = Rs2Player.getWorldLocation();
WorldPoint goal = currentTarget;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java`
around lines 3268 - 3275, The code is comparing candidate transport destinations
against lastTransportHandledAtLocation (the previous post-landing tile) which is
the last destination, not the last origin, so reverse transports (B -> A after A
-> B) still pass; declare and maintain a separate field (e.g.,
lastTransportOrigin) in Rs2Walker, set it to the player's tile at transport
start (or capture it in the method that begins handling a Transport) and
update/clear it alongside finishHandledTransport(); then change the stream
filter to compare t.getDestination() against this lastTransportOrigin (not
lastTransportHandledAtLocation) to correctly suppress immediate reverse
transport candidates.
Pre-existing Rs2Walker/Rs2Tile violations detected by the scanner after rebase onto upstream/development. No new violations introduced.
runsonmypc
changed the title
…heuristic cascade Three fixes: - Rs2Walker: track transport origin separately from landing tile so the reverse-transport filter correctly blocks A→B→A loops (cave entrance ping-pong). New lastTransportOriginLocation field, cleared on walk session start. - Rs2Tile: getReachableTilesFromTileInternal now fully honours ignoreCollision — cardinal direction flags and diagonal corridor flags are short-circuited, leaving only bounds checks. - Pathfinder: remove `|| node.heuristic == 0` from afterTransport checks in all three addNeighbors variants. The zero-heuristic no longer cascades past the first ring of walking nodes after a transport, restoring A* guidance for post-transport paths.
… scripted-walk pathfinder guard - Pathfinder: network-transport-aware admissible heuristic so teleport-> fairy/spirit-tree/glider/quetzal chains are discovered. Each enabled network hub becomes an A* landmark: h = min(directWalk, dist(node->origin) + min(dest->goal)). Stays admissible AND consistent (landmark set fixed per pathfind) so A* optimality holds, while the search is pulled toward useful hubs instead of ignoring them. Fixes the cloak->fairy->CIR route to the Farming Guild being passed over for a worse single teleport. Adds no graph edges (cannot teleport out of buildings) and never zeroes the heuristic (cannot collapse to whole-map Dijkstra), unlike the prior reverted bridge-injection and cascade attempts. - Rs2Walker: current-tile transport handler no longer fires off-path region-crossing transports (boat/ship/charter/canoe) by straight-line distance. A boat destination can be straight-line "closer" to the goal while sitting on another landmass, which made the walker click the Fossil Island rowboat (~17s of landing-timeout retries) before falling through to the planned teleport. These transports are now only used when explicitly on the planned path. - ShortestPathPlugin: skip onGameTick arrival-detection (which nulls the pathfinder) while a scripted Rs2Walker target is active, fixing intermittent pathfinder-still-null stalls.
The recovery-click rewrite in Rs2Walker added two lambda expressions, shifting every subsequent compiler-assigned lambda ordinal by +2. The guardrail baseline keys on those synthetic lambda names, so 26 entries renamed (same callers, same inferred-client-thread API calls) — no new real violations. Regenerated to match.
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
isSpiritTreeDestinationEnabledonly checked the destination side of a route, so routes FROM a disabled tree (e.g. Farming Guild with toggle off) to permanent trees (GE, Gnome Stronghold) still passed. Now checks both origin and destination against the toggle list.getReachableTilesFromTilebounded toHANDLER_RANGE * 3instead of unbounded scene flood.getReachableTilesFromTile, fixing disagreement that caused oscillation.nextWalkingDistanceset to 0 for paths ≤ 5 tiles so the walker doesn't stall on short walks.refreshTeleports()no longer overwrites existing non-teleport transports at the player's position.isUseSpiritTrees()before attempting interaction.refresh()then immediately overwritten inrefreshTransports().Test plan