Rs2LootEngine & AIO Fighter looting rework

[See1Duck]

This pull request refactors and modernizes the looting logic for the AIO Fighter plugin by introducing a new, unified looting engine (Rs2LootEngine). The changes replace the old, repetitive looting methods with a flexible builder pattern, allowing for more maintainable and customizable looting behavior. The update also exposes several utility methods in Rs2GroundItem for broader use and improves the handling of custom loot filters.

Looting engine and logic refactor:

• Introduced Rs2LootEngine, a new builder-based looting engine that centralizes and streamlines all looting logic, supports composable looting "intents" (by value, by name, bones, ashes, runes, arrows, coins, untradables, and custom predicates), and handles combined, distance-sorted looting passes. (runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2LootEngine.java)
• Refactored LootScript to use Rs2LootEngine, removing all old, repetitive looting methods and replacing them with a single, configurable looting pipeline. This greatly simplifies the code and makes it easier to add or modify looting behaviors. (runelite-client/src/main/java/net/runelite/client/plugins/microbot/aiofighter/loot/LootScript.java)

Utility method exposure and improvements:

• Changed several previously private methods in Rs2GroundItem (coreLoot, canTakeGroundItem, runWhilePaused) to public, enabling their use from the new looting engine and elsewhere. (runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2GroundItem.java) [1] [2] [3]
• Improved custom loot name handling in LootScript by adding a helper method that parses and normalizes the user-supplied comma-separated list, ensuring robust and case-insensitive matching. (runelite-client/src/main/java/net/runelite/client/plugins/microbot/aiofighter/loot/LootScript.java)

Code cleanup:

• Removed all the old, individual looting methods from LootScript (such as lootBones, lootAshes, lootRunes, lootCoins, lootUntradeableItems, lootArrows, lootItemsByValue, and lootItemsOnName), consolidating their functionality into the new engine. (runelite-client/src/main/java/net/runelite/client/plugins/microbot/aiofighter/loot/LootScript.java)

These changes make the looting system more robust, extensible, and easier to maintain.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR refactors LootScript to use a single parameterized looting pipeline via Rs2LootEngine.Builder, replacing multiple per-item methods and the wait-for-loot workflow. It introduces LootingParameters-driven configuration, optional custom-name filtering (CSV to predicate), and executes one combined loot pass that can pause other scripts upon success. Two helper methods and a core looting method in Rs2GroundItem are made public. A new Rs2LootEngine class is added, providing a fluent Builder with intents (value, names, coins, untradables, arrows, bones, ashes, runes, custom), delayed-loot gating, deduplication, distance sorting, and a final validation pass. Minor logging string updates are included.

Possibly related PRs

• AIO Fighter - Fixes and improvements  #1445: Modifies the same looting pipeline components (LootScript, Rs2GroundItem, LootingParameters), overlapping with this refactor to Rs2LootEngine.
• feat(AIOFighter): Add wait-for-loot feature #1402: Introduces wait-for-loot/state handling in LootScript, which this PR removes in favor of the Rs2LootEngine-based single-pass pipeline.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

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/util/grounditem/Rs2GroundItem.java (1)

37-44: Fix pause/unpause race; use reference counting to avoid premature unpauses.

runWhilePaused can unpause while another concurrent or nested looting action is still running. Convert it to a ref-counted pause with try/finally to guarantee correctness.

Apply this diff and add the field/import shown below:

-    public static boolean runWhilePaused(BooleanSupplier booleanSupplier) {
-        final boolean paused = Microbot.pauseAllScripts.getAndSet(true);
-        final boolean success = booleanSupplier.getAsBoolean();
-        if (!paused && !Microbot.pauseAllScripts.compareAndSet(true, false)) {
-            log.warn("Another script unpaused all scripts");
-        }
-        return success;
-    }
+    public static boolean runWhilePaused(BooleanSupplier booleanSupplier) {
+        final int depth = PAUSE_DEPTH.getAndIncrement();
+        final boolean alreadyPaused = depth > 0 || Microbot.pauseAllScripts.getAndSet(true);
+        boolean success = false;
+        try {
+            success = booleanSupplier.getAsBoolean();
+        } finally {
+            if (PAUSE_DEPTH.decrementAndGet() == 0) {
+                // Only the outermost scope should unpause
+                Microbot.pauseAllScripts.compareAndSet(true, false);
+            } else if (!alreadyPaused && !Microbot.pauseAllScripts.get()) {
+                log.warn("Pause depth mismatch detected; scripts already unpaused");
+            }
+        }
+        return success;
+    }

Add once outside the method (top of class) and import:

import java.util.concurrent.atomic.AtomicInteger;
private static final AtomicInteger PAUSE_DEPTH = new AtomicInteger(0);

🧹 Nitpick comments (5)

runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2GroundItem.java (1)

265-281: Avoid nested pauses; provide a non-pausing loot action for engine usage.

coreLoot wraps its body in runWhilePaused, but Rs2LootEngine.loot() already runs inside runWhilePaused, causing redundant nesting. Provide a variant without pausing and use that from the engine.

Suggested approach:

• Extract the body to coreLootNoPause(GroundItem gi) without runWhilePaused.
• Keep coreLoot as a thin wrapper calling runWhilePaused(() -> coreLootNoPause(gi)).
• Update the engine to call the no-pause variant.

runelite-client/src/main/java/net/runelite/client/plugins/microbot/aiofighter/loot/LootScript.java (3)

26-28: Comment contradicts value; fix arrow threshold note.

DEFAULT_MIN_STACK_EXCLUSIVE_ARROWS = 9 requires stacks > 9 (i.e., 10+), not “2+”.

Apply:

-    private static final int DEFAULT_MIN_STACK_EXCLUSIVE_ARROWS = 9; // allow 2+
+    private static final int DEFAULT_MIN_STACK_EXCLUSIVE_ARROWS = 9; // allow 10+

---

83-85: Log full stack traces for troubleshooting.

Only logging ex.getMessage() drops context.

Apply:

-                Microbot.log("LootScript: " + ex.getMessage());
+                Microbot.log("LootScript: " + ex.getMessage());
+                Microbot.logStackTrace("LootScript", ex);

---

61-63: If you adopt a non-pausing loot action, wire it here.

Once coreLootNoPause exists, switch to it to avoid nested pauses.

Apply:

-                        .withLootAction(Rs2GroundItem::coreLoot);
+                        .withLootAction(Rs2GroundItem::coreLootNoPause);

runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2LootEngine.java (1)

15-16: Deduplicate helpers and avoid import static ...*.

This class re-implements helpers (baseRangeAndOwnershipFilter, passesIgnoredNames, ensureSpaceFor, toLowerTrimmedSet, validateLoot, uniqueKey) also present in Rs2GroundItem, and uses a wildcard static import. Prefer a single source of truth to prevent drift, and avoid wildcard imports.

• Remove import static ...Rs2GroundItem.*;
• Expose shared helpers once (e.g., move to a GroundItemUtils or make the Rs2GroundItem versions public) and call them explicitly.
• Also unify DESPAWN_DELAY_THRESHOLD_TICKS in one place.

Also applies to: 201-256

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe57675 and b085dbb.

📒 Files selected for processing (3)

• runelite-client/src/main/java/net/runelite/client/plugins/microbot/aiofighter/loot/LootScript.java (2 hunks)
• runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2GroundItem.java (3 hunks)
• runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2LootEngine.java (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2LootEngine.java (2)

runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/inventory/Rs2Inventory.java (1)

• Rs2Inventory (60-2244)

runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/player/Rs2Player.java (1)

• Rs2Player (67-2070)

runelite-client/src/main/java/net/runelite/client/plugins/microbot/aiofighter/loot/LootScript.java (1)

runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2LootEngine.java (1)

• Rs2LootEngine (20-256)

⏰ 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 (1)

runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/grounditem/Rs2GroundItem.java (1)

555-565: LGTM: inventory-space check is correct and handles stackables.

[SteffenCarlsen]

LGTM

[Krulvis]

I have some time to go through it tonight. Do you mind keeping it open?

[Krulvis]

With the current setup, I have trouble understanding the coupling of LootingParameters and Rs2LootEngine. The LootingParameter has a constructor with quite some parameters but it is unclear how they will behave in the Rs2LootEngine. Some LootingParameters are added as base filters while others will only have effect on specific calls such as addByValue() and minQuantity is used in the loot() call. At the same time addArrows also has a minStackExclusive that can later be overwritten by the LootParameter's minQuantity.
As of now, Rs2LootEngine presents itself as a declarative builder and stores all of the filters that you add through .addRunes().addAshes() etc and then executes everything at with the .loot() call. If I look at the code, however, this is a facade and it performs a filter on the GroundItems stream on every build step and adds discovered to a list that it later needs to filter for uniqueness.
Wouldn't it make more sense to have the LootingParameters class function as a declarative filter builder and pass that to the Rs2LootEngine to use to filter items? Or alternatively, consider if the LootParameters class becomes redundant with Rs2LootEngine and it can be removed completely to avoid confusion. Instead of mixing LootParameters in with Rs2LootEngine, the Rs2LootEngine class itself can have more descriptive and explicit functions in the Rs2LootEngine. This would help understanding that there are 'base params/filters' that are stacked on top of any other filters (addRunes(), addAshes()).

EDIT: I want to be clear that I like the idea of having a Rs2LootEngine and would love to see it be as useful and extensible as possible while remaining concise and understandable.

Let me show you how I believe Rs2LootEngine would be most useful:

boolean success = Rs2LootEngine.builder()
    .withBaseFilter(item -> item.getLocation().distanceToPlayer() < 8)
    .withBaseFilter(item -> item.getOwnership() == GroundItem.OWNERSHIP_SELF)
    .addIntent("coins", gi -> gi.getId() == ItemID.COINS_995)
    .addIntent("valuable", gi -> gi.getGePrice() > 5000)
    .addIntent("ammo", gi -> gi.isStackable() && gi.getName().contains("arrow"))
    .addIntent("bones", gi -> gi.getName().toLowerCase().contains("bones"))
    .withLootAction(gi -> {
        gi.interact("Take"); / Rs2GroundItem::coreLoot
    })
    .loot();

You can clearly differentiate between base-filters, and intents. Intents do not mix with each other and you can add as many as you want. Obviously you can create pre-defined filters like you did with addCoinsIntent(), but it should focus on the core extensibility like you have with addCustom(Predicate<GroundItem> filter). I personally really believe that the builder should only store filters, and not eagerly query groundItems as soon as you add intents. I.e. only query groundItems (preferably once) when .loot() is called.

[chsami]

With the current setup, I have trouble understanding the coupling of LootingParameters and Rs2LootEngine. The LootingParameter has a constructor with quite some parameters but it is unclear how they will behave in the Rs2LootEngine. Some LootingParameters are added as base filters while others will only have effect on specific calls such as addByValue() and minQuantity is used in the loot() call. At the same time addArrows also has a minStackExclusive that can later be overwritten by the LootParameter's minQuantity. As of now, Rs2LootEngine presents itself as a declarative builder and stores all of the filters that you add through .addRunes().addAshes() etc and then executes everything at with the .loot() call. If I look at the code, however, this is a facade and it performs a filter on the GroundItems stream on every build step and adds discovered to a list that it later needs to filter for uniqueness. Wouldn't it make more sense to have the LootingParameters class function as a declarative filter builder and pass that to the Rs2LootEngine to use to filter items? Or alternatively, consider if the LootParameters class becomes redundant with Rs2LootEngine and it can be removed completely to avoid confusion. Instead of mixing LootParameters in with Rs2LootEngine, the Rs2LootEngine class itself can have more descriptive and explicit functions in the Rs2LootEngine. This would help understanding that there are 'base params/filters' that are stacked on top of any other filters (addRunes(), addAshes()).

EDIT: I want to be clear that I like the idea of having a Rs2LootEngine and would love to see it be as useful and extensible as possible while remaining concise and understandable.

Let me show you how I believe Rs2LootEngine would be most useful:

boolean success = Rs2LootEngine.builder()
    .withBaseFilter(item -> item.getLocation().distanceToPlayer() < 8)
    .withBaseFilter(item -> item.getOwnership() == GroundItem.OWNERSHIP_SELF)
    .addIntent("coins", gi -> gi.getId() == ItemID.COINS_995)
    .addIntent("valuable", gi -> gi.getGePrice() > 5000)
    .addIntent("ammo", gi -> gi.isStackable() && gi.getName().contains("arrow"))
    .addIntent("bones", gi -> gi.getName().toLowerCase().contains("bones"))
    .withLootAction(gi -> {
        gi.interact("Take"); / Rs2GroundItem::coreLoot
    })
    .loot();

You can clearly differentiate between base-filters, and intents. Intents do not mix with each other and you can add as many as you want. Obviously you can create pre-defined filters like you did with addCoinsIntent(), but it should focus on the core extensibility like you have with addCustom(Predicate<GroundItem> filter). I personally really believe that the builder should only store filters, and not eagerly query groundItems as soon as you add intents. I.e. only query groundItems (preferably once) when .loot() is called.

We can take a look at looting later, it does seem that some of it could be simplified. But we have to watch out for regression.