Repository navigation
fix(barrows): tunnels, Ferox RoD, POH travel, and banking rewrite - #565
Conversation
chsami
left a comment
There was a problem hiding this comment.
Reviewed head c5827c929bc53e16f0cd96e6c5343db1a6682243. ./gradlew build -PpluginList=BarrowsPlugin passes against client 2.6.26, and the version bump to 2.1.3 is present.
Target branch. Hub changes go to development, not main, and PR CI only runs for development, which is why this PR has no checks. The branch is cut from main and carries a merge of chsami:main. Only c5827c9 belongs to this PR. Please cherry-pick or rebase that commit onto development and retarget. Please also add a short description of the intended behaviour changes.
P1: the rune-tier downgrade never sticks, so banking loops. Case: the bank has no Wrath runes and the inventory has no rune stack above the minimum. downgradeRuneTier (BarrowsScript.java:1213-1228) sets neededRune = "Blood rune", and the banking branch around line 556 returns without withdrawing. On the next loop, gettheRune() (line 166 → 1158) runs unconditionally. switchToInventoryRuneTier finds nothing, so it resets neededRune = getHighestCastableRune() to Wrath. The downgrade repeats forever and Blood runes are never withdrawn. The removed if (!neededRune.equals("unknown")) return; guard used to prevent this. Fix: withdraw the downgraded tier in the same pass. Alternatively, keep gettheRune() from overriding a downgrade until the higher tier is available in the bank again.
P2: supply checks switch to the Magic tab mid-fight in POH mode. canCastHouseTeleport()/teleToPoh() (around 757-775) call Rs2Magic.canCast(...), which switches to the Magic tab and sleeps. suppliesCheck calls it (around 1096) from the tunnel and crypt loops (480, 708, 1294). Decide from Rs2Magic.hasRequiredRunes(...) plus a spellbook check, and call canCast/cast only when actually teleporting.
P2: POH mode can stall. canCastHouseTeleport() is true when either canCast or hasRequiredRunes is true, but teleToPoh() only casts when canCast is true. With runes present, canCast false and no tabs, shouldBank is set, banking withdraws no tabs because it believes the cast is possible, and the cycle repeats. Use one predicate in both places, and fall back to tabs whenever the cast cannot happen.
- runes fallback repaired - banking withdrawal repaired - tunnel walking repaired
Withdraw the downgraded rune tier in the same banking pass, use a non-UI house-tele check for supplies, and fall back to house tabs whenever cast cannot happen. Co-authored-by: Cursor <cursoragent@cursor.com>
c5827c9 to
0508784
Compare
- Updated BarrowsConfig to allow optional inventory setup selection. - Improved BarrowsOverlay to handle null values gracefully for tunnel and pieces display. - Enhanced BarrowsPlugin to manage bank trip configurations and restore settings on shutdown. - Refactored BarrowsScript to prevent duplicate script executions and improve error logging.
chsami
left a comment
There was a problem hiding this comment.
Reviewed head 56250dcc647ecf9070ab063185af8db0c326fbc5.
Fixed since the last review:
- The PR now targets
developmentand contains only barrows files. - The rune-tier downgrade now withdraws the lower tier in the same bank pass and sticks (
BarrowsScript.java:771-783,2363-2433). suppliesCheckno longer callsRs2Magic.canCastmid-fight.
compileBarrowsJava passes against client 2.6.26. The red CI is the unrelated development breakage (22 errors in other plugins, none in barrows), so that part isn't on you.
56250dc rewrote about 2,500 lines of BarrowsScript: Ferox ring-of-dueling banking, Inventory Setup gear, tunnel targeting, shortest-path overrides and auto-retaliate. That brought in these blockers:
P0: route planning on the client thread. findTunnelMonster (BarrowsScript.java:1748-1752) puts isNpcOnWayToChest inside a .where(...) that ends in .nearestOnClientThread(). where is a lazy Stream.filter, so the predicate runs inside ClientThread.invoke, and isNpcOnWayToChest → safeTotalTiles → Rs2Walker.getTotalTiles (line 1793) runs a full Rs2PathApi.plan twice per candidate NPC. In tunnels with several skeletons or bloodworms loaded, this freezes the client. It is also re-evaluated through shouldDeferChestWalk during the chest walk. Collect the candidates with the cache query first, then do path math on the script thread.
P1: an equipped ring of dueling passes the supply check but can't be used to leave. hasDuelingRingAvailable() (2583) accepts an equipped ring, so suppliesCheck (2284) doesn't bank. But tryFeroxTeleportViaRingOfDueling (2675-2691) only rubs inventory rings. A user upgrading from 2.0.9 with the ring equipped hits the out-of-supplies branch (723-733), which returns every tick without teleporting, so the bot stands at Barrows or in the tunnels indefinitely. Either teleport with the equipped ring or require an inventory ring in the supply check.
P2 (from the last review, still open): POH mode can still stall. The two checks still disagree:
suppliesCheckusescanCastHouseTeleport(), which only checks that the runes are present.teleToPohalso requiresRs2Magic.canCast.
With runes present, canCast false and no house tabs, teleToPoh() fails and sets shouldBank = true (lines 231 and 670). The next outOfSupplies → suppliesCheck pass (214 or 716) clears it again before banking. So ensurePohTravelSupplies never runs, and the bot loops on the Magic tab. Make the "can travel to POH" decision a single predicate used in both places, or keep the bank request until tabs are in the inventory.
Please also update the PR description. It doesn't mention the rewrite and says 2.1.4, but the descriptor is 2.5.18.
Non-blocking:
- The
shutdown()override nullsmainScheduledFuturebeforesuper.shutdown(), which skips the base cleanup (ShortestPathPlugin.exit(), the pause reset and the spec reset). - Auto-retaliate is forced off in
run()(fromstartUp) and never restored. - The global shortest-path config overrides persist if
shutDowndoesn't run. loadStartingEquipmentretries forever if the selected setup's gear isn't banked.- There is a fair amount of now-dead code:
walkToChest,solvePuzzle,gainRPand others.
Live testing isn't required. A targeted build, plus a short explanation of how each case above is handled, is enough.
…eze path Collect tunnel NPC candidates before path math, tele with equipped RoD, and stick POH banking until house tabs land when cast fails. Co-authored-by: Cursor <cursoragent@cursor.com>
|
P0 — client freeze: findTunnelMonster now pulls candidates with toListOnClientThread(), then runs safeTotalTiles / en-route checks on the script thread before picking nearest. P1 — equipped RoD: tryFeroxTeleportViaRingOfDueling still prefers inventory, then falls back to Rs2Equipment.interact(RING, Ferox) so an equipped-only ring can leave. P2 — POH stall: canTravelToPoh() is shared by suppliesCheck. Failed teleToPoh sets sticky requireHouseTabsToTravel until a house tablet is present, so shouldBank no longer flaps. Also: shutdown() no longer nulls mainScheduledFuture before super.shutdown(); auto-retaliate restored on stop. |
- Bumped BarrowsPlugin version to 2.5.21. - Introduced new pathfinding methods in BarrowsScript to improve NPC detection and route planning towards chests. - Added constants for path proximity and slack to refine NPC interactions during gameplay.
- Updated BarrowsPlugin version to 2.5.22. - Added constants for maximum scene distance and path steps ahead to enhance NPC detection logic in BarrowsScript. - Improved conditions for determining NPC engagement and proximity during gameplay.
chsami
left a comment
There was a problem hiding this comment.
Re-reviewed head 7d9b0048f50379e8adf81256798667791ebc493d. All three blockers from my last review are fixed:
- P0 client freeze:
findTunnelMonstercollects candidates withtoListOnClientThread()and does the path math on the script thread. It reuses the active chest route when one exists and plans withgetWalkPathotherwise, so nothing in the.where(...)predicates plans a route any more. TheRs2NpcModelgetters it now calls off-thread (getWorldLocation,getInteracting) already route through the client thread. - P1 equipped ring:
tryEquippedRingToFerox()is a fallback intryFeroxTeleportViaRingOfDueling, so an equipped-only ring can leave Barrows or the tunnels. - P2 POH stall:
suppliesCheckusescanTravelToPoh(), and a failedteleToPoh()sets the stickyrequireHouseTabsToTravel.shouldBankno longer flaps, andensurePohTravelSupplieswithdraws house tabs.
The shutdown() override now leaves mainScheduledFuture to Script.shutdown(), so the base cleanup runs again.
Validation: I merged this head with current development (after #566 fixed the client 2.6.26 compile breakage). ./gradlew clean build passes and produces BarrowsPlugin-2.5.22.jar. The red CI on this head is the old development breakage (22 errors, none in barrows). I've triggered a fresh run against the fixed base.
Non-blocking follow-ups:
- POH mode, when
requireHouseTabsToTravelis set and the bank holds fewer house tabs thantargetBarrowsTeleports:ensurePohTravelSuppliesneither withdraws tabs nor stops, because the stop branch only fires when the cast is impossible. The bot then repeats bank trips. Consider withdrawing whatever tabs exist, or stopping with a message. shutdown()sets auto-retaliate totrueunconditionally rather than restoring the user's previous setting.isNpcOnWayToChestandsafeTotalTilesare now unused.- The PR description says 2.5.19; the descriptor is 2.5.22.
Brothers are now defend-only: a brother is engaged only when it has the hint arrow or is attacking us. That's a sensible ownership rule, and I'm treating it as intended.
Summary
Rs2Walker.getTotalTiles), shortest-path bank-trip overrides, auto-retaliate off while running, puzzle/RP/piece tracking fixes.canTravelToPoh()predicate (runes/tabs only — no Magic-tabcanCastmid-fight). After a failed cast with no tabs, banking sticks until a house tablet is withdrawn.getTotalTilesinsidenearestOnClientThread).Test plan
./gradlew compileBarrowsJava -PpluginList=BarrowsPluginagainst client 2.6.26canCastfalse, no house tabs: banks, withdraws tabs, teleports (no Magic-tab loop)