Conversation
|
Warning Rate limit exceeded@g-mason0 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
WalkthroughRemoves the “The Mess” plugin (config, overlay, plugin, script). Refactors shortest-path overlays to use static accessors and adjusts path-clear handling to route via ShortestPathScript. Adds collision exclusions, updates walker enums/coordinates, tweaks door handling to ignore imposters, and bumps microbot.version in the client POM. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as Client UI
participant SPP as ShortestPathPlugin
participant SPS as ShortestPathScript
participant W as Rs2Walker
U->>C: Select "Clear Path" / press CTRL+X
C->>SPP: onMenuOptionClicked / keyPressed
Note right of SPP: New behavior
SPP->>SPS: setTriggerWalker(null)
alt point is null
SPS->>W: setTarget(null)
SPS-->>SPP: trigger cleared
end
sequenceDiagram
participant O as Overlay (Map/Tile/Tooltip/Debug)
participant SPP as ShortestPathPlugin (static)
participant PF as Pathfinder
participant T as Transports Map
O->>SPP: getPathfinder()
SPP-->>O: PF or null
O->>SPP: getTransports()
SPP-->>O: T or null
opt PF available
O->>PF: getPath(), isDone()
PF-->>O: path, status
O->>O: render path with status color
end
opt T available
O->>T: iterate transports
T-->>O: transport entries
O->>O: render transport overlays/tooltips
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
runelite-client/pom.xml (1)
472-481: Stray character in shade plugin config likely to break the buildThere’s an unexpected "§y" immediately following the DSA exclude entry, which can cause XML parsing or plugin config issues.
Apply this diff to remove it:
- <exclude>META-INF/*.DSA</exclude>§y + <exclude>META-INF/*.DSA</exclude>Also, note there are two separate blocks under the same execution; Maven tolerates this, but consolidating them would be cleaner.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java (1)
1074-1089: Filtering out all impostor-capable objects can suppress valid door interactionsMany doors and passable objects in OSRS use impostor transforms (varbit-driven). Ignoring every composition with non-null impostorIds risks skipping real, actionable doors, causing stuck paths. Prefer resolving the impostor first, then filtering by name “null”.
Use the resolved composition for action/name checks:
- ObjectComposition comp = Rs2GameObject.convertToObjectComposition(object); - // Ignore imposter objects - if (comp == null || comp.getImpostorIds() != null || comp.getName().equals("null")) continue; + ObjectComposition comp = Rs2GameObject.convertToObjectComposition(object); + // Resolve impostor (varbit-driven) to get the actionable composition + ObjectComposition effective = comp != null && comp.getImpostor() != null ? comp.getImpostor() : comp; + if (effective == null || "null".equals(effective.getName())) continue; + + // Use effective for subsequent logic - String action = Arrays.stream(comp.getActions()) + String action = Arrays.stream(effective.getActions()) .filter(Objects::nonNull) .filter(act -> doorActions.stream().anyMatch(dact -> act.toLowerCase().startsWith(dact.toLowerCase()))) .min(Comparator.comparing(act -> doorActions.indexOf(doorActions.stream().filter(dact -> act.toLowerCase().startsWith(dact)).findFirst().orElse("")))) .orElse(null); ... - final String name = comp.getName(); + final String name = effective.getName();This maintains the intent to ignore meaningless “null” names while still recognizing valid, transformed door objects.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathPlugin.java (1)
420-424: Bug: PLUGIN_MESSAGE_CLEAR clears only the path overlay, not the walker.External “clear” messages call setTarget(null) but do not clear ShortestPathScript.triggerWalker, so walking may continue. Mirror the menu/hotkey behavior to also stop the walker.
- } else if (PLUGIN_MESSAGE_CLEAR.equals(action)) { - ShortestPathPlugin.configOverride.clear(); - cacheConfigValues(); - setTarget(null); - } + } else if (PLUGIN_MESSAGE_CLEAR.equals(action)) { + ShortestPathPlugin.configOverride.clear(); + cacheConfigValues(); + // Clear walker trigger and underlying Rs2Walker target + if (shortestPathScript != null) { + shortestPathScript.setTriggerWalker(null); + } else { + // Fallback: keep existing behavior if script not ready + setTarget(null); + } + }runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PathMapOverlay.java (1)
66-83: Do not abort the entire overlay render when transports/pathfinder are unavailableReturning from render() here prevents other layers (e.g., path, collision map) from drawing. Wrap the transports drawing in guards instead of exiting early. Also avoid repeated lookups and per-iteration allocations; iterate entrySet and use Collections.emptySet().
Apply this diff to confine the early-exit to the transports section and reduce allocations/lookups:
- if (ShortestPathPlugin.getTransports() == null) return null; - if (ShortestPathPlugin.getPathfinder() == null || !ShortestPathPlugin.getPathfinder().isDone()) return null; - for (WorldPoint a : ShortestPathPlugin.getTransports().keySet()) { - Point mapA = worldMapOverlay.mapWorldPointToGraphicsPoint(a); - if (mapA == null || !worldMapClipArea.contains(mapA.getX(), mapA.getY())) { - continue; - } - - for (Transport b : ShortestPathPlugin.getTransports().getOrDefault(a, new HashSet<>())) { - Point mapB = worldMapOverlay.mapWorldPointToGraphicsPoint(b.getDestination()); - if (mapB == null || !worldMapClipArea.contains(mapB.getX(), mapB.getY())) { - continue; - } - - graphics.drawLine(mapA.getX(), mapA.getY(), mapB.getX(), mapB.getY()); - } - } + final java.util.Map<WorldPoint, java.util.Set<Transport>> transports = ShortestPathPlugin.getTransports(); + if (transports != null && ShortestPathPlugin.getPathfinder() != null && ShortestPathPlugin.getPathfinder().isDone()) { + for (java.util.Map.Entry<WorldPoint, java.util.Set<Transport>> entry : transports.entrySet()) { + final WorldPoint a = entry.getKey(); + final Point mapA = worldMapOverlay.mapWorldPointToGraphicsPoint(a); + if (mapA == null || !worldMapClipArea.contains(mapA.getX(), mapA.getY())) { + continue; + } + for (Transport b : entry.getValue() != null ? entry.getValue() : java.util.Collections.<Transport>emptySet()) { + final Point mapB = worldMapOverlay.mapWorldPointToGraphicsPoint(b.getDestination()); + if (mapB == null || !worldMapClipArea.contains(mapB.getX(), mapB.getY())) { + continue; + } + graphics.drawLine(mapA.getX(), mapA.getY(), mapB.getX(), mapB.getY()); + } + } + }Note: This also aligns gating with PathTileOverlay’s transport rendering strategy (guard and continue), while keeping behavior consistent and avoiding NPEs without disabling the rest of the overlay.
🧹 Nitpick comments (10)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/enums/Herbs.java (1)
19-21: Make worldPoint immutable for enum constantsworldPoint doesn’t change per constant. Mark it final to prevent accidental mutation and reflect intent.
Apply:
- private WorldPoint worldPoint; + private final WorldPoint worldPoint;Same suggestion applies to FruitTrees and Trees enums.
cache/src/main/java/net/runelite/cache/CollisionMapDumper.java (1)
363-365: Confirm intent: new exclusions default to “blocked” (TILE_BLOCKED).Both BARROWS_BRICK_WALL(53256) and BURGH_DE_ROTT_FENCE(18411) use the single-arg ctor, which sets tile = TILE_BLOCKED. If the fix’s intent is to prevent routing through these objects, this is correct. If instead these were meant to be passable, switch to the 2-arg ctor with TILE_DEFAULT.
For self-documentation, consider being explicit:
- BARROWS_BRICK_WALL(53256), - BURGH_DE_ROTT_FENCE(18411), + BARROWS_BRICK_WALL(53256 /* TILE_BLOCKED */), + BURGH_DE_ROTT_FENCE(18411 /* TILE_BLOCKED */),runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/DebugOverlayPanel.java (1)
41-41: Guard against null path to avoid NPE in path length.pathfinder.getStats() can be non-null while getPath() is still null. Add a null check and avoid calling size() on a null path.
- Pathfinder pathfinder = ShortestPathPlugin.getPathfinder(); - Pathfinder.PathfinderStats stats; - if (pathfinder == null || (stats = pathfinder.getStats()) == null) { + Pathfinder pathfinder = ShortestPathPlugin.getPathfinder(); + Pathfinder.PathfinderStats stats; + if (pathfinder == null || (stats = pathfinder.getStats()) == null) { return null; } ... - String pathLength = Integer.toString(pathfinder.getPath().size()); + int len = pathfinder.getPath() != null ? pathfinder.getPath().size() : 0; + String pathLength = Integer.toString(len);Also applies to: 58-60
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PathMapTooltipOverlay.java (2)
49-51: Reduce race/NPE risk by caching pathfinder and path.ShortestPathPlugin.getPathfinder() is volatile, but calling it twice and dereferencing getPath() without a null check can still race. Cache both and guard.
- if (ShortestPathPlugin.getPathfinder() != null) { - List<WorldPoint> path = ShortestPathPlugin.getPathfinder().getPath(); + Pathfinder pf = ShortestPathPlugin.getPathfinder(); + if (pf != null) { + List<WorldPoint> path = pf.getPath(); + if (path == null) { + return null; + }
81-84: Use the cached path and transports; avoid repeated static lookups.This avoids minor jitter and ensures consistent size/iteration across a single render pass.
- List<String> rows = new ArrayList<>(Arrays.asList("Shortest path:", "Step " + n + " of " + ShortestPathPlugin.getPathfinder().getPath().size())); + List<String> rows = new ArrayList<>(Arrays.asList("Shortest path:", "Step " + n + " of " + path.size())); ... - for (Transport transport : ShortestPathPlugin.getTransports().getOrDefault(point, new HashSet<>())) { + for (Transport transport : ShortestPathPlugin.getTransports() + .getOrDefault(point, new HashSet<>())) {runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PathMapOverlay.java (1)
85-87: Cache pathfinder locally and guard against null path; reduce repeated static callsMultiple calls to ShortestPathPlugin.getPathfinder() are unnecessary. Cache a local reference, then fetch the path once. This reduces risk of subtle race/readability issues and clarifies intent.
- if (ShortestPathPlugin.getPathfinder() != null) { - Color colour = ShortestPathPlugin.getPathfinder().isDone() ? plugin.colourPath : plugin.colourPathCalculating; - List<WorldPoint> path = ShortestPathPlugin.getPathfinder().getPath(); + final net.runelite.client.plugins.microbot.shortestpath.pathfinder.Pathfinder pf = ShortestPathPlugin.getPathfinder(); + if (pf != null) { + final Color colour = pf.isDone() ? plugin.colourPath : plugin.colourPathCalculating; + final List<WorldPoint> path = pf.getPath();Nitpick: Since colour is constant within the loop below, set graphics.setColor(colour) once before the loop to avoid per-iteration calls.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PathTileOverlay.java (4)
40-42: Transport rendering is gated on pathfinderFuture; consider removing or aligning gatingRequiring getPathfinderFuture().isDone() to draw transports can hide useful transport visualization unrelated to the path computation and is inconsistent with PathMapOverlay (which checks isDone() on the pathfinder instance). Prefer either:
- No pathfinder gating for transports, or
- Consistent gating across overlays (choose one approach and apply uniformly).
If you opt to remove the gating here:
- if (ShortestPathPlugin.getPathfinderFuture() == null || !ShortestPathPlugin.getPathfinderFuture().isDone()) return;
51-51: Avoid per-call allocations: use Collections.emptySet() instead of new HashSet<>()getOrDefault(a, new HashSet<>()) allocates on misses. Use Collections.emptySet() to avoid allocations.
- for (Transport b : ShortestPathPlugin.getTransports().getOrDefault(a, new HashSet<>())) { + for (Transport b : ShortestPathPlugin.getTransports().getOrDefault(a, java.util.Collections.emptySet())) {
239-243: Cache pathfinder and path once; guard against null pathMinimize repeated static calls and handle a potential null path defensively.
- if (ShortestPathPlugin.getPathfinder() == null) return; + final Pathfinder pf = ShortestPathPlugin.getPathfinder(); + if (pf == null) return; if (counter >= 0 && !TileCounter.DISABLED.equals(plugin.showTileCounter)) { int n = plugin.tileCounterStep > 0 ? plugin.tileCounterStep : 1; - int s = ShortestPathPlugin.getPathfinder().getPath().size(); + final List<WorldPoint> path = pf.getPath(); + final int s = (path != null) ? path.size() : 0; if ((counter % n != 0) && (s != (counter + 1))) { return; }
272-272: Avoid per-call allocations when fetching transports for a pointSame allocation nit as above: prefer Collections.emptySet() over new HashSet<>().
- for (Transport transport : ShortestPathPlugin.getTransports().getOrDefault(point, new HashSet<>())) { + for (Transport transport : ShortestPathPlugin.getTransports().getOrDefault(point, java.util.Collections.emptySet())) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
runelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/collision-map.zipis excluded by!**/*.zip
📒 Files selected for processing (16)
cache/src/main/java/net/runelite/cache/CollisionMapDumper.java(1 hunks)runelite-client/pom.xml(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/mess/TheMessConfig.java(0 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/mess/TheMessOverlay.java(0 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/mess/TheMessPlugin.java(0 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/mess/TheMessScript.java(0 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/DebugOverlayPanel.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PathMapOverlay.java(2 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PathMapTooltipOverlay.java(2 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PathTileOverlay.java(4 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathPlugin.java(2 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathScript.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/enums/FruitTrees.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/enums/Herbs.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/enums/Trees.java(1 hunks)
💤 Files with no reviewable changes (4)
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/mess/TheMessOverlay.java
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/mess/TheMessScript.java
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/mess/TheMessPlugin.java
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/mess/TheMessConfig.java
🧰 Additional context used
🧬 Code graph analysis (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathScript.java (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/revKiller/revKillerScript.java (9)
getAwayFromPker(606-707)Rs2Player(261-261)revKillerScript(49-1453)Microbot(83-174)Rs2Player(304-304)kiteTheKnight(224-373)moveCameraToTile(375-393)Rs2Player(360-360)Rs2Player(294-294)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (6)
runelite-client/pom.xml (1)
44-46: Bump to microbot.version 1.9.8.6 looks goodProperty update is consistent with the shaded output file path and snapshot repo. No functional concerns.
If there are downstream scripts or release notes that parse the jar name, make sure they’re aware of the new version tag.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/enums/Herbs.java (1)
15-16: Coordinate update for Morytania herb patch — please confirm target tile semanticsThe new WorldPoint(3604, 3529, 0) is fine on shape, but double-check it represents the intended interaction/entry tile the walker uses (not a blocked tile or off-by-one). If it’s the center of the patch rather than an adjacent walkable tile, certain reachability checks may fail.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/enums/FruitTrees.java (1)
13-16: New FruitTrees.KASTORI entry — verify map location and accessibilityAddition looks consistent. Please ensure (1350, 3055, 0) is inside the correct region and is a walkable/interactable tile for the panel’s usage, not just the visual tree center behind collision.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/enums/Trees.java (1)
9-12: New Trees.AUBURNVALE entry — verify coordinates against Auburnvale hubCoordinates (1365, 3319, 0) look plausible for Varlamore; confirm the walker can reach/interact from that tile and that the display name matches how it’s surfaced in the panel.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathPlugin.java (2)
636-637: Good: unify CLEAR action in menu with script-level clearing.Routing CLEAR PATH through shortestPathScript.setTriggerWalker(null) ensures Rs2Walker gets cleared as well. This aligns user actions with walker state.
893-895: Good: CTRL+X hotkey now clears via script setter.Consistent with the CLEAR menu action, this prevents desyncs between path overlay and walker target.
...ient/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathScript.java
Show resolved
Hide resolved
…iana herb patch worldpoint
…r minimap chore(shortest-path): use static reference for pathfinder & transports in overlays
1 participant
Features
Fixes
Chores
Summary by CodeRabbit
New Features
Bug Fixes
Chores