fix(shortestpath): TransportNode cost double-count + overflow; remove MoA audit temp plugin#1751
Draft
runsonmypc wants to merge 4 commits intochsami:developmentfrom
Draft
Conversation
Contributor
WalkthroughThis PR adds support for seasonal League transports, specifically the Map of Alacrity mechanic, to the shortest path pathfinder. Changes span configuration updates, transport type classification, pathfinding logic, and interactive transport handling. The implementation includes marking seasonal transports as teleport-like, tracking Map of Alacrity transports with cost calculations, filtering based on destination availability and configuration, and adding comprehensive widget-based interaction for the Map of Alacrity picker in Rs2Walker with session blacklist management for locked destinations and regions. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
🧹 Nitpick comments (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java (1)
2593-2601: Log-level inconsistency in the MoA path.Entry logging at Line 2595 and the destination-selection log at Line 2712 are
log.info, while the surrounding MoA diagnostics (region resolution, hotkey press, parsing) arelog.debug. SincehandleSeasonalTransportis called speculatively for every candidate transport during a walk,log.info("[MoA] entry: ...")will fire even for non-MoA seasonal entries (early-returned at Line 2598-2601) and pollute the normal walk log. Demote both todebugto match the rest of this handler.As per coding guidelines: "Keep logging minimal; avoid PII/session identifiers and respect existing log levels/patterns".
Proposed diff
- log.info("[MoA] entry: displayInfo='{}'", displayInfo); + log.debug("[MoA] entry: displayInfo='{}'", displayInfo);- log.info("[MoA] selecting destination '{}' (text='{}')", shortName, destText); + log.debug("[MoA] selecting destination '{}' (text='{}')", shortName, destText);Also applies to: 2710-2714
🤖 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 2593 - 2601, The entry and destination-selection log calls in handleSeasonalTransport are using log.info and should be demoted to log.debug to match surrounding MoA diagnostics and avoid noisy logs; update the log invocation that currently logs "[MoA] entry: displayInfo='{}'" and the later "[MoA] destination selected ..." (the destination-selection log in the same method) to use log.debug instead of log.info so all MoA-related diagnostics remain at debug level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java`:
- Around line 2593-2601: The entry and destination-selection log calls in
handleSeasonalTransport are using log.info and should be demoted to log.debug to
match surrounding MoA diagnostics and avoid noisy logs; update the log
invocation that currently logs "[MoA] entry: displayInfo='{}'" and the later
"[MoA] destination selected ..." (the destination-selection log in the same
method) to use log.debug instead of log.info so all MoA-related diagnostics
remain at debug level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1a6b02e9-76a5-431d-b556-024581d6e89b
⛔ Files ignored due to path filters (2)
runelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/agility_shortcuts.tsvis excluded by!**/*.tsvrunelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/seasonal_transports.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathConfig.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/TransportType.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/PathfinderConfig.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/TransportNode.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java
TransportNode's constructor was pre-adding previous.cost to travelTime,
then passing that sum as `wait` to Node(WorldPoint, Node, int) — which
re-adds previous.cost. The upshot: any TransportNode chained off another
TransportNode had its cost double-counted. This only surfaced once a
real transport->transport chain became the only viable path (e.g. Map
of Alacrity -> agility shortcut), which is why it stayed latent.
Concrete impact: "go to nearest bank" from outside Nemus retreat picked
Shilo (cost 55) over Nemus (observed 74, should be 40 = 34 MoA + 3
climb + 3 walk). The shortcut's TransportNode was being stamped at g=71
instead of g=37, tripling the cost of every agility-shortcut edge
downstream of a teleport.
Passing the final absolute cost through Node(int, Node, int cost)
instead of the wait-based constructor also sidesteps a second bug: for
plane-crossing transports with travelTime=0 (e.g. Slayer Tower chains),
the wait<=0 branch of Node.cost falls back to
WorldPointUtil.distanceBetween, which returns Integer.MAX_VALUE across
planes — overflowing previousCost + distance into Integer.MIN_VALUE
territory and making those transports look "free". The absolute-cost
path never consults distanceBetween.
Also in this pass:
- CollisionMap [MoA] debug log: report actual per-transport costs
instead of the hardcoded `+ 4` (all MoA durations happened to be
4, but the log was incidental to the value, not tracking it).
- Rs2Walker.findMoaWidget: token-set membership instead of
substring match — avoids false positives like token "log"
matching a hypothetical "logstrum" widget label.
runsonmypc
force-pushed
the
fix/transportnode-cost-overflow
branch
from
770d57f to
2cbb2af
Compare
MoaAuditPlugin and the runMoaAudit()/closeMoaWidgetIfOpen() helpers in Rs2Walker were intended as a temporary offline landing-coord audit tool (self-labelled [TEMP] "Delete when done") and were supposed to be held on a separate debug branch, not merged. They slipped into upstream with PR chsami#1750 because the amend that removed them locally was never pushed before the PR was merged. Scope: delete-only. No functional changes to MoA handling — handleSeasonalTransport, lockedMoaRegions, and blacklistedMoaDestinations are untouched.
…d guardrail - collectMoaChildren called getDynamicChildren/getNestedChildren/getStaticChildren directly; the static guardrail scanner can't follow through to the callers' runOnClientThreadOptional wrappers, so it flagged a new violation. - Inlined the three child-array fetches into findMoaWidget and computeMoaHotkeyByIndex — they now happen inside the client-thread lambda where the scanner can see the wrapper. - Regenerated client-thread-guardrail-baseline.txt: combined effect of the MoA edits and the audit-plugin removal shifted pre-existing Rs2Walker lambda indices, plus a few entries the old baseline was missing.
runsonmypc
force-pushed
the
fix/transportnode-cost-overflow
branch
from
7800f54 to
58b11f5
Compare
3 tasks
runsonmypc
changed the title
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
Three bundled changes on one branch to avoid back-to-back Rs2Walker baseline-regen conflicts:
1.
TransportNodecost fix (main)TransportNodewas stamping chained-transport costs at2 * previous.cost + travelTimeinstead ofprevious.cost + travelTime, tripling the effective cost of any agility shortcut (or other non-teleport transport) reached via a teleport. Invisible on first-hop TransportNodes (sinceprevious.cost == 0at the start node), which is why it was latent for a long time.getNearestBank()picked Shilo Village (actual cost 55) over Nemus Retreat (reported cost 74, true cost 40 = MoA teleport 34 + east broken-wall climb 3 + 3-tile walk). The shortcut's TransportNode was gettingg=71instead ofg=37.travelTime=0(e.g. Slayer Tower chains).Node.costfalls back toWorldPointUtil.distanceBetween, which returnsInteger.MAX_VALUEacross planes, andpreviousCost + Integer.MAX_VALUEwraps intoInteger.MIN_VALUE-territory, making those transports look free. Passing the absolute cost directly throughNode(int packed, Node prev, int cost)never consultsdistanceBetween.2. Remove MoA audit temp debug plugin
MoaAuditPlugin+ itsrunMoaAudit/closeMoaWidgetIfOpenhelpers inRs2Walkerwere supposed to stay on a side branch and got merged by accident via #1750. Pure deletion — plugin was only ever enabled manually and wrote to logs. Supersedes #1752.3. Leftover /simplify findings from PR #1750 review
CollisionMapMoA debug log now reports actual per-edge costs instead of hardcoded+ 4.Rs2Walker.findMoaWidgetuses token-set membership instead of substring match — avoids false positives like tokenloghitting a hypotheticallogstrumlabel.[MoA]log.infocalls tolog.debug(entry + destination selection) — noisy in normal operation.Rs2Walker.collectMoaChildrenhelper inlined into its two callers so the client-thread guardrail scanner sees therunOnClientThreadOptionalwrapper.client-thread-guardrail-baseline.txt— Rs2Walker edits shifted pre-existing lambda indices; no new offenders introduced.Diagnosis trail for the TransportNode bug
Pathfinder/CollisionMapwith per-pop logs → shortcut dest(1389, 3309, 0)was appearing in pending withg=71, not the expectedg=37.TransportNodeconstructor chain → staticcost(previous, travelTime)computesprevious.cost + travelTime, passes that aswaittoNode(WorldPoint, Node, int), which addsprevious.costagain via its instancecost()method.super(point, previous, travelTime)) compiled fine but blew up on plane-crossing transports with duration 0 — the log showedg=-2147483615for the Slayer Tower chain tile, which led to the absolute-cost constructor swap.Verification (limited — only on a League 6 account)
go to nearest bankfrom Ardougne picks Shilo Village, Nemus bank target shows cost 74 when reachable via MoA east + broken-wall shortcut.(1386, 3309, 0)atcost=40, walker actually selectsMap of Alacrity: Varlamore - Nemus retreat (east)and teleports.(3421, 3550, 0)↔(3421, 3550, 1)) no longer produce overflowedInteger.MIN_VALUE-ish costs.Only manually tested on one League 6 account (all MoA regions unlocked). Haven't confirmed main-game behavior; the TransportNode change is generic enough that other transport->transport chains (boat-after-teleport, shortcut-after-teleport) should now cost correctly too — would appreciate a second set of eyes on that.
Test plan
./gradlew :client:runTest, log in, MoA in inventory, click "go to nearest bank" from mainland → expect Nemus Retreat to be chosen and the walker to fireMap of Alacrity: Varlamore - Nemus retreat (east).previous.cost == 0.MoaAuditPluginis gone from the plugin list.