Poh transports added on runtime#1506
Conversation
# Conflicts: # runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java
# Conflicts: # runelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/teleportation_poh.tsv
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR restructures POH transport handling across the shortest path system. It introduces PohTransport, adds PohTeleports.getTransportsToPoh(), and updates Rs2PohCache with new public APIs, fairy-ring integration, and spawn/despawn handling. PathfinderConfig now merges TSV, PoH, and world-to-PoH transports, adds Spirit Tree gating, and refines consumable-teleport filtering. Transport gains several public constructors, removes POH enum-tracing fields, and stops loading POH TSVs. Rs2Walker refactors transport selection and executes PohTransport directly. HouseStyle renames its POH location field to pohExitWorldPoint, adds getPohExitPoint, and HouseLocation updates TSV mapping. Minor formatting and serialization-variable renaming are included. Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/PathfinderConfig.java (2)
268-315: Method createMergedList() should be staticThe method doesn't access any instance fields and is only called from within the instance method
refreshTransports. Making it static would improve clarity about its dependencies.- private Map<WorldPoint, Set<Transport>> createMergedList() { + private static Map<WorldPoint, Set<Transport>> createMergedList(Map<WorldPoint, Set<Transport>> allTransports) { Map<WorldPoint, Set<Transport>> mergedTransports = new HashMap<>(); // Start with putting all the TSV imported persistent transportsAnd update the call site at line 268:
- for (Map.Entry<WorldPoint, Set<Transport>> entry : createMergedList().entrySet()) { + for (Map.Entry<WorldPoint, Set<Transport>> entry : createMergedList(allTransports).entrySet()) {
475-476: Simplify conditional logic for Spirit Tree checkThe Spirit Tree check can be simplified by directly returning the result.
- // Check Spirit Tree specific requirements (farming state for farmable trees) - if (transport.getType() == TransportType.SPIRIT_TREE) return isSpiritTreeUsable(transport); + // Check Spirit Tree specific requirements (farming state for farmable trees) + if (transport.getType() == TransportType.SPIRIT_TREE) { + return isSpiritTreeUsable(transport); + }runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2PohCache.java (2)
136-145: Inconsistent logging between addTeleport and removeTeleportThe
removeTeleportmethod logs "Removing X Teleport from Y" whileaddTeleportlogs "Adding X Teleport to Y". The remove log should use past tense for consistency with the operation's completion.- log.info("Removing {} Teleport from {}", teleport.name(), key); + log.info("Removed {} Teleport from {}", teleport.name(), key);
147-156: Inconsistent logging in addTeleportSimilar to removeTeleport, the log message should use past tense.
- log.info("Adding {} Teleport to {}", teleport.name(), key); + log.info("Added {} Teleport to {}", teleport.name(), key);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
runelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/fairy_rings.tsvis excluded by!**/*.tsvrunelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/teleportation_items.tsvis excluded by!**/*.tsvrunelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/teleportation_poh.tsvis excluded by!**/*.tsvrunelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/teleportation_portal_poh.tsvis excluded by!**/*.tsvrunelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/teleportation_spells.tsvis excluded by!**/*.tsv
📒 Files selected for processing (10)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathPlugin.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/Transport.java(3 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/PathfinderConfig.java(4 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2PohCache.java(5 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/serialization/PohTeleportDataAdapter.java(2 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTeleports.java(3 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTransport.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/data/HouseLocation.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/data/HouseStyle.java(2 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/serialization/PohTeleportDataAdapter.java (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTransport.java (1)
PohTransport(15-34)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTransport.java (1)
PohTransport(15-34)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTeleports.java (1)
PohTeleports(37-254)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/PathfinderConfig.java (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTransport.java (1)
PohTransport(15-34)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTeleports.java (1)
PohTeleports(37-254)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTeleports.java (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/player/Rs2Player.java (1)
Rs2Player(67-2076)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/Transport.java (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTeleports.java (1)
PohTeleports(37-254)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTransport.java (1)
PohTransport(15-34)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2PohCache.java (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTeleports.java (1)
PohTeleports(37-254)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTransport.java (1)
PohTransport(15-34)
⏰ 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 (10)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/data/HouseStyle.java (3)
30-30: Rename clarifies intent; no issues.
Field rename to pohExitWorldPoint improves semantics and matches downstream usage.
43-43: Deduped exit points is correct.
Mapping via getPohExitWorldPoint() with distinct() avoids duplicate coordinates.
50-53: Helper is fine.
Static accessor reads Varbit and returns the exit point or null; callers already null-check upstream.runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/data/HouseLocation.java (1)
51-51: Destination switch to exit point is correct.
Using style.getPohExitWorldPoint() aligns with the new data model.runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/Transport.java (2)
134-134: Public merge ctor is acceptable.
Surface area increases but behavior is unchanged; no blockers.
11-15: New POH-related imports.
No action; leaving as-is.runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathPlugin.java (1)
203-203: No-op whitespace changeNothing to review here.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/serialization/PohTeleportDataAdapter.java (2)
28-33: Serializer variable rename and field usage look correctUsing the
teleportvariable consistently and emittingclass+namealigns with the deserializer logic.
6-6: No action on added importNo immediate issue detected for this change in isolation.
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTeleports.java (1)
235-252: Confirm null-keyed transport visibility at runtimeThese PoH teleports are stored under a
nullkey.Rs2Walker.handleTransports()currently looks up transports by the current path tile only; unlessShortestPathPlugin.getTransports()merges/null-propagates these, the walker may never consider them. Please verify that null-keyed entries are surfaced to the walker during execution. If not, either:
- also index them under
insidePoint, or- update the walker to union the
null-keyed set when evaluating transports (see suggested change in Rs2Walker below).
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/Transport.java
Outdated
Show resolved
Hide resolved
| Set<PohTransport> pohTransports = getAvailablePohTransports(); | ||
| Map<WorldPoint, Set<Transport>> transportsMap = new HashMap<>(); | ||
| if (pohTransports.isEmpty()) return transportsMap; | ||
| WorldPoint exitPortal = pohTransports.stream().findFirst().get().getOrigin(); |
There was a problem hiding this comment.
Potential NPE when pohTransports is empty
Line 195 assumes pohTransports is not empty after checking isEmpty() returns false. However, between the check on line 194 and access on line 195, the set could theoretically become empty in a concurrent environment.
- if (pohTransports.isEmpty()) return transportsMap;
- WorldPoint exitPortal = pohTransports.stream().findFirst().get().getOrigin();
+ if (pohTransports.isEmpty()) return transportsMap;
+ Optional<PohTransport> firstTransport = pohTransports.stream().findFirst();
+ if (!firstTransport.isPresent()) return transportsMap;
+ WorldPoint exitPortal = firstTransport.get().getOrigin();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WorldPoint exitPortal = pohTransports.stream().findFirst().get().getOrigin(); | |
| if (pohTransports.isEmpty()) return transportsMap; | |
| Optional<PohTransport> firstTransport = pohTransports.stream().findFirst(); | |
| if (!firstTransport.isPresent()) return transportsMap; | |
| WorldPoint exitPortal = firstTransport.get().getOrigin(); |
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTransport.java
Show resolved
Hide resolved
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java
Show resolved
Hide resolved
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java
Show resolved
Hide resolved
|
Similar what I'm doing in my current local version!
Since we're tackling the same core problem with similar approaches, collaborating on this could be really beneficial. Should I make a PR for it ? So you don't tackle the same problem I have already implemented. |
Let's tackle all of them in separate PRs so it doesn't get too large and incomprehensible in a single PR. I'm definitely down to helping with all of those features! |
|
@Voxsylvae After this PR we won't have to deal with annoyances from adding everything in TSV's and can really lay the foundations for a more dynamic web. |
|
Sure, then i will add the cache rework + the rework for the teleports, need for the cache rework, after your PR. if you are fine with it. Im happy when we can collaborate together. |
Krulvis
force-pushed
the
feature/rework-tsv
branch
from
9189a94 to
c937ffb
Compare
3 participants
Since we can't reliably say where PoH is, it required TSV data for all posibilities, creating an enormous amount of useless data. In this PR I rework PoH transports to be dynamically added during runtime when pathfinder refreshes its available transports. Based on Rs2PohCache the available PohTransports are added to the PathfinderConfig. The pathfinder is able to deal with them like regular transports and doesn't require any modifications.
This implementation keeps the ShortestPathfinder plugin as similar as possible to the original plugin. The TSV's remain unaffected. There's only one additional function added to the PathfinderConfig to merge runtime transports with persistent transports. Additionally, some constructors were added to the Transport class and one was made public.