Rewritten astar.rs#58
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe codebase refactors A\* pathfinding from a stateful struct with grid caching to a stateless function accepting a passability callback. Bot core integrates the new API, changes walk/path signatures from Changes
Sequence DiagramsequenceDiagram
participant Bot as Bot Core
participant AStar as A* Pathfinding
participant Callback as Tile Passability<br/>Callback
participant World as World/Tile System
Bot->>Bot: compute_path(goal_x, goal_y)
Bot->>AStar: find_path(ox, oy, tx, ty, is_passable_cb)
AStar->>AStar: Initialize open list with start
loop While open list not empty
AStar->>AStar: Pop lowest f-cost node
alt At goal
AStar->>AStar: Reconstruct path via parent pointers
AStar->>Bot: Return Some(path)
else Expand neighbors (4-directional)
AStar->>Callback: is_passable_cb(nx, ny, direction)
Callback->>World: Check tile type & flags
Callback->>World: Check world permissions
Callback->>AStar: Return bool
alt Passable & unvisited
AStar->>AStar: Add to open list (f, h, pos)
end
end
end
alt Open list exhausted
AStar->>Bot: Return None
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/bot/core.rs (2)
2916-2955: Minor: Typo in comment.Line 2945: "Onw way" should be "One way".
📝 Fix typo
- 5 => { // Onw way + 5 => { // One way🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot/core.rs` around lines 2916 - 2955, In is_tile_passable (function) fix the comment typo "Onw way" to "One way" near the collision_type 5 branch; locate the match arm for item_info.collision_type == 5 inside is_tile_passable in src/bot/core.rs and update the inline comment to read "One way" so the code comment is correct and clear.
2305-2320: Minor: Redundant type casts.
nodeis already(u32, u32)fromcompute_path, so castingnode.0 as u32andnode.1 as u32is unnecessary.🔧 Remove redundant casts
for node in nodes { - let nx = node.0 as u32; - let ny = node.1 as u32; + let (nx, ny) = node; let cx = (self.pos_x / 32.0) as u32; let cy = (self.pos_y / 32.0) as u32; if nx == cx && ny == cy { continue; } // TODO: Optimize this. // I think the impact is tolerable if self.compute_path(nx, ny).is_none() { self.pathfind_recalc = true; break; } self.walk(nx, ny); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot/core.rs` around lines 2305 - 2320, The variables nx and ny are redundantly cast to u32 even though node is already (u32, u32); update the block to use the tuple values directly (e.g., let (nx, ny) = node;) or assign nx = node.0 and ny = node.1 without "as u32", keep the existing comparisons against cx/cy (which come from pos_x/pos_y) and the subsequent calls to self.compute_path(nx, ny) and self.walk(nx, ny) unchanged so behavior stays the same.src/astar.rs (1)
1-3: Unnecessaryvecimport.The
vec!macro is available in the Rust prelude and doesn't need to be explicitly imported.🔧 Remove redundant import
use std::{ - cmp::Ordering, collections::{BinaryHeap, HashMap}, vec + cmp::Ordering, collections::{BinaryHeap, HashMap}, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astar.rs` around lines 1 - 3, The use statement in src/astar.rs unnecessarily imports the vec symbol (vec macro) from the prelude; remove vec from the grouped import so the line becomes: use std::{cmp::Ordering, collections::{BinaryHeap, HashMap}} (or otherwise drop only the vec token), then run cargo check to ensure no other changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bot/core.rs`:
- Around line 2607-2610: Movement offset x/y is being applied twice: it's added
when computing cx/cy from self.pos_x/pos_y and then added again when calling
self.walk; update the call to self.walk to pass cx and cy (converted to u32)
directly (e.g., self.walk(cx as u32, cy as u32)), keep the negative check on
cx/cy as-is, and remove the extra + x and + y in the walk invocation so the bot
moves the intended single offset.
---
Nitpick comments:
In `@src/astar.rs`:
- Around line 1-3: The use statement in src/astar.rs unnecessarily imports the
vec symbol (vec macro) from the prelude; remove vec from the grouped import so
the line becomes: use std::{cmp::Ordering, collections::{BinaryHeap, HashMap}}
(or otherwise drop only the vec token), then run cargo check to ensure no other
changes are needed.
In `@src/bot/core.rs`:
- Around line 2916-2955: In is_tile_passable (function) fix the comment typo
"Onw way" to "One way" near the collision_type 5 branch; locate the match arm
for item_info.collision_type == 5 inside is_tile_passable in src/bot/core.rs and
update the inline comment to read "One way" so the code comment is correct and
clear.
- Around line 2305-2320: The variables nx and ny are redundantly cast to u32
even though node is already (u32, u32); update the block to use the tuple values
directly (e.g., let (nx, ny) = node;) or assign nx = node.0 and ny = node.1
without "as u32", keep the existing comparisons against cx/cy (which come from
pos_x/pos_y) and the subsequent calls to self.compute_path(nx, ny) and
self.walk(nx, ny) unchanged so behavior stays the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 458e9b96-6c2f-4c2a-b6a2-1d83b17bd49f
📒 Files selected for processing (2)
src/astar.rssrc/bot/core.rs
Comment on lines
+2607
to
+2610
| let cx = ((self.pos_x / 32.0).round() as i32) + x; | ||
| let cy = ((self.pos_y / 32.0).round() as i32) + y; | ||
| if cx < 0 || cy < 0 { return; } | ||
| self.walk((cx + x) as u32, (cy + y) as u32); |
Contributor
There was a problem hiding this comment.
Bug: Movement offset applied twice.
The offset x is added to compute cx, then added again in the walk() call. Same for y. This causes the bot to move twice the intended distance.
🐛 Proposed fix
BotCommand::Move { x, y } => {
let cx = ((self.pos_x / 32.0).round() as i32) + x;
let cy = ((self.pos_y / 32.0).round() as i32) + y;
if cx < 0 || cy < 0 { return; }
- self.walk((cx + x) as u32, (cy + y) as u32);
+ self.walk(cx as u32, cy as u32);
}📝 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.
Suggested change
| let cx = ((self.pos_x / 32.0).round() as i32) + x; | |
| let cy = ((self.pos_y / 32.0).round() as i32) + y; | |
| if cx < 0 || cy < 0 { return; } | |
| self.walk((cx + x) as u32, (cy + y) as u32); | |
| let cx = ((self.pos_x / 32.0).round() as i32) + x; | |
| let cy = ((self.pos_y / 32.0).round() as i32) + y; | |
| if cx < 0 || cy < 0 { return; } | |
| self.walk(cx as u32, cy as u32); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/core.rs` around lines 2607 - 2610, Movement offset x/y is being
applied twice: it's added when computing cx/cy from self.pos_x/pos_y and then
added again when calling self.walk; update the call to self.walk to pass cx and
cy (converted to u32) directly (e.g., self.walk(cx as u32, cy as u32)), keep the
negative check on cx/cy as-is, and remove the extra + x and + y in the walk
invocation so the bot moves the intended single offset.
Owner
|
Nicely done! thank you for your hardwork! |
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
2 participants
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.
Now it should be generic enough to be reused in other project :)
Summary by CodeRabbit