Conversation
|
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 Walkthrough
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. 🪧 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
🧹 Nitpick comments (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java (2)
1954-1956: Clarify parameter order and bounds in Javadoc.Explicitly state indexing and inclusivity to avoid ambiguity.
- * @param worldPoints an array of two world points of the SE and NW corners of the area + * @param worldPoints exactly two points: worldPoints[0] = SE, worldPoints[1] = NW; bounds are inclusive + * @throws IllegalArgumentException if fewer than two points are provided or any point is null
1975-1977: Handle negative range defensively.Using Math.abs avoids inverted corners from accidental negative input.
- WorldPoint seCorner = new WorldPoint(centerOfArea.getX() + range, centerOfArea.getY() - range, centerOfArea.getPlane()); - WorldPoint nwCorner = new WorldPoint(centerOfArea.getX() - range, centerOfArea.getY() + range, centerOfArea.getPlane()); + int r = Math.abs(range); + WorldPoint seCorner = new WorldPoint(centerOfArea.getX() + r, centerOfArea.getY() - r, centerOfArea.getPlane()); + WorldPoint nwCorner = new WorldPoint(centerOfArea.getX() - r, centerOfArea.getY() + r, centerOfArea.getPlane());
📜 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 selected for processing (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/player/Rs2Player.java (1)
Rs2Player(67-2070)
⏰ 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
| public static boolean isInArea(WorldPoint... worldPoints) { | ||
| WorldPoint playerLocation = Rs2Player.getWorldLocation(); | ||
| return playerLocation.getX() <= worldPoints[0].getX() && // NW corner x | ||
| playerLocation.getY() >= worldPoints[0].getY() && // NW corner y | ||
| playerLocation.getX() >= worldPoints[1].getX() && // SE corner x | ||
| playerLocation.getY() <= worldPoints[1].getY(); // SE corner Y | ||
| return playerLocation.getX() <= worldPoints[0].getX() && // SE corner x | ||
| playerLocation.getY() >= worldPoints[0].getY() && // SE corner y | ||
| playerLocation.getX() >= worldPoints[1].getX() && // NW corner x | ||
| playerLocation.getY() <= worldPoints[1].getY(); // NW corner Y |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and make corner order robust (prevents AIOOBE/NPE and avoids subtle call-site breakage).
Currently this will throw if fewer than two points are passed; also strictly requiring SE/NW order is brittle. Compute min/max instead.
- public static boolean isInArea(WorldPoint... worldPoints) {
- WorldPoint playerLocation = Rs2Player.getWorldLocation();
- return playerLocation.getX() <= worldPoints[0].getX() && // SE corner x
- playerLocation.getY() >= worldPoints[0].getY() && // SE corner y
- playerLocation.getX() >= worldPoints[1].getX() && // NW corner x
- playerLocation.getY() <= worldPoints[1].getY(); // NW corner Y
- // draws box from 2 points to check against all variations of player X,Y from said points.
- }
+ public static boolean isInArea(WorldPoint... worldPoints) {
+ if (worldPoints == null || worldPoints.length < 2 || worldPoints[0] == null || worldPoints[1] == null) {
+ throw new IllegalArgumentException("isInArea requires two WorldPoints (SE, NW).");
+ }
+ final WorldPoint playerLocation = Rs2Player.getWorldLocation();
+ final WorldPoint a = worldPoints[0];
+ final WorldPoint b = worldPoints[1];
+ final int minX = Math.min(a.getX(), b.getX());
+ final int maxX = Math.max(a.getX(), b.getX());
+ final int minY = Math.min(a.getY(), b.getY());
+ final int maxY = Math.max(a.getY(), b.getY());
+ return playerLocation.getX() >= minX && playerLocation.getX() <= maxX
+ && playerLocation.getY() >= minY && playerLocation.getY() <= maxY;
+ // draws box from 2 points to check against all variations of player X,Y from said points.
+ }Optional: also require planes to match if that’s the intended semantic for “area” on multi-level maps.
📝 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.
| public static boolean isInArea(WorldPoint... worldPoints) { | |
| WorldPoint playerLocation = Rs2Player.getWorldLocation(); | |
| return playerLocation.getX() <= worldPoints[0].getX() && // NW corner x | |
| playerLocation.getY() >= worldPoints[0].getY() && // NW corner y | |
| playerLocation.getX() >= worldPoints[1].getX() && // SE corner x | |
| playerLocation.getY() <= worldPoints[1].getY(); // SE corner Y | |
| return playerLocation.getX() <= worldPoints[0].getX() && // SE corner x | |
| playerLocation.getY() >= worldPoints[0].getY() && // SE corner y | |
| playerLocation.getX() >= worldPoints[1].getX() && // NW corner x | |
| playerLocation.getY() <= worldPoints[1].getY(); // NW corner Y | |
| public static boolean isInArea(WorldPoint... worldPoints) { | |
| if (worldPoints == null || worldPoints.length < 2 || worldPoints[0] == null || worldPoints[1] == null) { | |
| throw new IllegalArgumentException("isInArea requires two WorldPoints (SE, NW)."); | |
| } | |
| final WorldPoint playerLocation = Rs2Player.getWorldLocation(); | |
| final WorldPoint a = worldPoints[0]; | |
| final WorldPoint b = worldPoints[1]; | |
| final int minX = Math.min(a.getX(), b.getX()); | |
| final int maxX = Math.max(a.getX(), b.getX()); | |
| final int minY = Math.min(a.getY(), b.getY()); | |
| final int maxY = Math.max(a.getY(), b.getY()); | |
| return playerLocation.getX() >= minX && playerLocation.getX() <= maxX | |
| && playerLocation.getY() >= minY && playerLocation.getY() <= maxY; | |
| // draws box from 2 points to check against all variations of player X,Y from said points. | |
| } |
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java
around lines 1957 to 1962, the isInArea(WorldPoint... worldPoints) method lacks
input validation and assumes the two points are provided in SE/NW order; add a
check that worldPoints is non-null and has at least two non-null elements and
throw IllegalArgumentException (or return false) on invalid input, then compute
int minX = Math.min(p0.getX(), p1.getX()), maxX = Math.max(p0.getX(),
p1.getX()), and similarly minY/maxY and test playerLocation.getX()/getY()
against those bounds so corner order is irrelevant; optionally also compare
getPlane() for equality if multi-level area semantics are required.
Checks to ensure two world points are used. Cached values from getter methods to keep as performant as possible. Updated method parameter description again to the more naturally read: NW then SE.
|
New commit should make isInArea(WorldPoint... worldPoints) more robust as it no longer matters which order we pass through our points. |
damn nice coderabbitai fix <3 that really makes this PR golden |
Haha yeah, wasn't a bad suggestion. Tweaked it a little, but it was reasonable |
Reasonable is putting it very lightly |
| */ | ||
| @Deprecated(since = "1.5.5", forRemoval = true) | ||
| public static boolean isInArea(WorldPoint centerOfArea, int range) { | ||
| WorldPoint nwCorner = new WorldPoint(centerOfArea.getX() + range + range, centerOfArea.getY() - range, centerOfArea.getPlane()); |
There was a problem hiding this comment.
should we not change it here to follow the logic of WorldArea, calculating the
northeast corner
ne = new WorldPoint(centerOfArea.getX() + radius, centerOfArea.getY() + radius, centerOfArea.getPlane()); and
southwest corner
sw = new WorldPoint(centerOfArea.getX() - radius, centerOfArea.getY() - radius, centerOfArea.getPlane()); range -> radius ?
or better directly use use WorldArea
| */ | ||
| public static boolean isInArea(WorldPoint... worldPoints) { | ||
| WorldPoint playerLocation = Rs2Player.getWorldLocation(); | ||
| return playerLocation.getX() <= worldPoints[0].getX() && // NW corner x |
There was a problem hiding this comment.
Change the logic here too.
playerLocation.getX() >= worldPoints[0].getX() && // SW corner x
playerLocation.getY() >= worldPoints[0].getY() && // SW corner y
playerLocation.getX() <= worldPoints[1].getX() && // NE corner x
playerLocation.getY() <= worldPoints[1].getY(); // NE corner YThere was a problem hiding this comment.
Change the logic here too.
playerLocation.getX() >= worldPoints[0].getX() && // SW corner x playerLocation.getY() >= worldPoints[0].getY() && // SW corner y playerLocation.getX() <= worldPoints[1].getX() && // NE corner x playerLocation.getY() <= worldPoints[1].getY(); // NE corner Y
Im confused as to what you're referring to here. This was changed to not be so static in its approach.
4 participants
The logic for the isInArea() methods are backwards.
This change doesn't change the actual logic, but changes the method description to properly reflect the logic that's applied.
The overloaded isInArea() method that takes in a centre point and a range previously created a rectangle with larger offsets on the x value, rather than an equally sided square with the calculated corners. This is now changed to create an equally sided square around the centre point. Was there a reason we were creating horizontally favoured rectangle areas?
These pedantic changes better reflect the applied logic without changing the original logic (besides creating an equal-sided area, instead of a horizontally favoured rectangle), so this shouldn't have any affect on the current usages of these methods in other scripts/plugins.
I originally became aware of this flawed logic when I tried to use the
isInArea(WorldPoint... worldPoints)method to determine if I had the player in a certain area using custom points, instead of the simpler to use overloaded methodisInArea(WorldPoint centerOfArea, int range), and of course I ran into problems before looking into how the methods were operating.