Repository navigation
feat: bfs dijkstra flow map - #97
JeongGyeom wants to merge 5 commits into
Conversation
|
Warning Review limit reachedNext included review available in 56 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis PR adds BFS pathfinding to ChangesPathfinding Algorithms and Documentation
Estimated code review effort: 4 (Complex) | ~50 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains; the change is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Example
participant BfsMap
participant BaseMap
participant Console
Example->>BfsMap: build map from start and target tiles
BfsMap->>BaseMap: query exits during BFS
BaseMap-->>BfsMap: return exits and costs
BfsMap-->>Example: return tile depth values
Example->>Console: render walls and depth colors
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Out of Scope Changes checkExplanation The BFS implementation, Dijkstra correction, examples, and related documentation match the stated PR objectives. The FOV image alt-text change is unrelated to issues Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
4d8ab73 to
bf96536
Compare
![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 (3)
bracket-pathfinding/src/bfs.rs (3)
249-249: 💤 Low value
partial_cmp().unwrap()will panic if BFS map contains NaN.While the map is initialized to
f32::MAX(safe for comparison), the publicmapfield allows external modification. If corrupted with NaN,partial_cmpreturnsNoneand the unwrap panics.Consider using
unwrap_or(Ordering::Equal)for defensive handling:🛡️ Suggested fix
- exits.par_sort_by(|a, b| bfs.map[a.0].partial_cmp(&bfs.map[b.0]).unwrap()); + exits.par_sort_by(|a, b| { + bfs.map[a.0] + .partial_cmp(&bfs.map[b.0]) + .unwrap_or(std::cmp::Ordering::Equal) + });Apply similarly to all four helper variants.
Also applies to: 262-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-pathfinding/src/bfs.rs` at line 249, The sort comparator currently calls bfs.map[a.0].partial_cmp(&bfs.map[b.0]).unwrap(), which will panic if either value is NaN; change the comparator to handle None defensively by using .unwrap_or(std::cmp::Ordering::Equal) (or Ordering::Equal via a use) instead of unwrap so NaNs don't cause a panic; replace the unwrap in exits.par_sort_by and the three other helper variants that use partial_cmp on bfs.map with this defensive pattern referencing the exits.par_sort_by call and the bfs.map field.
298-324: ⚡ Quick winConsider expanding test coverage for BFS module.
The existing test validates that BFS counts edges rather than costs, which is good. However, coverage is minimal for a new module. Consider adding tests for:
find_lowest_exit/find_highest_exithelpersmax_depthcutoff behavior (verify tiles beyond cutoff remainf32::MAX)new_emptyandclearmethods- Parallel build path (under
#[cfg(feature = "threaded")])As per coding guidelines: "Add tests for new functionality in the relevant module; for split domains, prefer colocated
tests.rs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-pathfinding/src/bfs.rs` around lines 298 - 324, Add unit tests in the BFS test module to increase coverage: write tests that call find_lowest_exit and find_highest_exit on small maps with known exits to assert correct indices are returned; test max_depth cutoff by creating a BfsMap with a tight max_depth and assert tiles beyond the cutoff remain f32::MAX; add tests for BfsMap::new_empty (or new_empty) and BfsMap::clear to verify they initialize and reset the internal map state as expected; and, if the threaded feature exists, add a test exercising the parallel build path (e.g., BfsMap::new with threaded feature) to ensure it produces identical results to the single-threaded build. Ensure each test references the BfsMap methods and helper functions by name so they’re easy to locate.
204-210: 💤 Low valueParallel BFS hardcodes start depth to 0.0, unlike serial
build_weighted.The serial
build_weightedaccepts(usize, f32)tuples allowing variable start depths, but the parallel path hardcodes0.0for all starts. The condition0.0 >= l.map[start]is always false (sincel.map[start]isf32::MAX), making the first check dead code.Currently safe since
build_parallelis only invoked frombuild()which uses depth 0.0 for all starts. Consider a TODO if future support for parallel weighted starts is desired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-pathfinding/src/bfs.rs` around lines 204 - 210, The parallel BFS currently hardcodes start depth to 0.0 and leaves a dead branch; update build_parallel (and any callers that populate l.starts) to accept start depth tuples like the serial build_weighted by changing l.starts to hold (usize, f32) pairs, then in the loop use the provided depth (e.g., for (start, depth) in l.starts.iter().copied()) and replace the checks/assignments to compare and assign depth (if depth >= l.map[start] || depth >= l.max_depth { continue } l.map[start] = depth; open_list.push_back((start, depth));). Also update any call sites that construct l.starts and add a TODO comment if you prefer to postpone full API changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manual/src/ex_path.md`:
- Line 39: Replace the empty alt text in the image markdown
 with a concise, descriptive alt string that explains the
image content for accessibility (for example: );
locate the image token in manual/src/ex_path.md and update only the alt text
between the brackets to a brief descriptive phrase that conveys the image's
meaning to screen readers.
---
Nitpick comments:
In `@bracket-pathfinding/src/bfs.rs`:
- Line 249: The sort comparator currently calls
bfs.map[a.0].partial_cmp(&bfs.map[b.0]).unwrap(), which will panic if either
value is NaN; change the comparator to handle None defensively by using
.unwrap_or(std::cmp::Ordering::Equal) (or Ordering::Equal via a use) instead of
unwrap so NaNs don't cause a panic; replace the unwrap in exits.par_sort_by and
the three other helper variants that use partial_cmp on bfs.map with this
defensive pattern referencing the exits.par_sort_by call and the bfs.map field.
- Around line 298-324: Add unit tests in the BFS test module to increase
coverage: write tests that call find_lowest_exit and find_highest_exit on small
maps with known exits to assert correct indices are returned; test max_depth
cutoff by creating a BfsMap with a tight max_depth and assert tiles beyond the
cutoff remain f32::MAX; add tests for BfsMap::new_empty (or new_empty) and
BfsMap::clear to verify they initialize and reset the internal map state as
expected; and, if the threaded feature exists, add a test exercising the
parallel build path (e.g., BfsMap::new with threaded feature) to ensure it
produces identical results to the single-threaded build. Ensure each test
references the BfsMap methods and helper functions by name so they’re easy to
locate.
- Around line 204-210: The parallel BFS currently hardcodes start depth to 0.0
and leaves a dead branch; update build_parallel (and any callers that populate
l.starts) to accept start depth tuples like the serial build_weighted by
changing l.starts to hold (usize, f32) pairs, then in the loop use the provided
depth (e.g., for (start, depth) in l.starts.iter().copied()) and replace the
checks/assignments to compare and assign depth (if depth >= l.map[start] ||
depth >= l.max_depth { continue } l.map[start] = depth;
open_list.push_back((start, depth));). Also update any call sites that construct
l.starts and add a TODO comment if you prefer to postpone full API changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3cf71e9-216e-480e-8afa-cd5ffeb3d125
📒 Files selected for processing (9)
bracket-pathfinding/Cargo.tomlbracket-pathfinding/README.mdbracket-pathfinding/examples/bfs/main.rsbracket-pathfinding/src/bfs.rsbracket-pathfinding/src/dijkstra.rsbracket-pathfinding/src/lib.rsmanual/src/ex_path.mdmanual/src/individual_parts.mdsrc/lib.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format all Rust code usingcargo fmt --all
Fix allcargo clippywarnings — the CI enforces-D warnings
Add tests for new functionality in the relevant module; for split domains, prefer colocatedtests.rs
Files:
bracket-pathfinding/examples/bfs/main.rsbracket-pathfinding/src/lib.rssrc/lib.rsbracket-pathfinding/src/dijkstra.rsbracket-pathfinding/src/bfs.rs
🪛 LanguageTool
manual/src/individual_parts.md
[style] ~9-~9: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...erating things. * bracket-pathfinding provides A-Star, BFS and Dijkstra mapping soluti...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
bracket-pathfinding/README.md
[style] ~3-~3: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ... the overall bracket-lib system, and (in conjunction with bracket-algorithm-traits) provides pa...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
🪛 markdownlint-cli2 (0.22.1)
manual/src/ex_path.md
[warning] 39-39: Images should have alternate text (alt text)
(MD045, no-alt-text)
🔇 Additional comments (18)
bracket-pathfinding/Cargo.toml (1)
7-7: LGTM!src/lib.rs (1)
40-40: LGTM!bracket-pathfinding/README.md (1)
3-3: LGTM!Also applies to: 29-29, 91-107, 121-121, 137-137, 141-141, 144-147
manual/src/ex_path.md (1)
19-24: LGTM!manual/src/individual_parts.md (1)
9-9: LGTM!bracket-pathfinding/src/bfs.rs (5)
267-295: Samepartial_cmp().unwrap()consideration applies here as noted above.
1-40: LGTM!
42-110: LGTM!
112-135: LGTM!
137-179: LGTM!bracket-pathfinding/src/lib.rs (1)
4-4: LGTM!Also applies to: 10-10
bracket-pathfinding/examples/bfs/main.rs (1)
1-47: LGTM!bracket-pathfinding/src/dijkstra.rs (6)
6-7: LGTM!The
DijkstraNodeordering correctly implements min-heap semantics by reversing the cost comparison (other.cost.partial_cmp(&self.cost)), which is the standard pattern for Dijkstra with Rust's max-heapBinaryHeap. Tie-breaking by index ensures deterministic behavior.Also applies to: 29-57
178-179: LGTM!
185-219: LGTM!The priority-queue Dijkstra implementation is correct: seeds starts into the heap, pops minimum-cost nodes, skips stale entries, and relaxes neighbors with proper cost accumulation. This fixes the previous BFS-like behavior that could produce incorrect results with non-uniform exit costs.
223-286: LGTM!The parallel implementation correctly uses per-layer
BinaryHeapwith proper stale-entry checks and min-recombination. Pre-computing exits outside the parallel loop is a good optimization to avoid redundantget_available_exitscalls.
382-392: LGTM!
457-466: LGTM!This test validates the core correctness fix: with the priority-queue implementation, Dijkstra correctly discovers that path 0→2→1 (cost 2.0) is cheaper than direct path 0→1 (cost 10.0). This was the exact scenario that would fail with the previous BFS-like traversal.
2 participants
What
Add a dedicated BFS flow map, update Dijkstra maps to use a priority queue for weighted shortest-path costs, and add BFS example/docs coverage.
Why
The previous
DijkstraMapimplementation used FIFO queue traversal, which behaves more like BFS/flood fill and can produce incorrect results with non-uniform exit costs. Splitting BFS intoBfsMapand movingDijkstraMaptoBinaryHeapkeeps the algorithm roles clear and makes weighted path costs correct.Closes #32
Checklist
Required
cargo check --allpassescargo fmt --all -- --checkpassescargo clippy --workspace --all-targets -- -D warnings -A clippy::multiple-crate-versionspassescargo test --allpassesCloses #123)Functional Validation
Configuration & Docs
README.md,ARCHITECTURE.md, or relevant manual pages, if applicable)If Applicable
cargo auditlocally if needed)Summary by CodeRabbit
New Features
Bug Fixes
Documentation