Refactor/mesh algorithm dependency inversion #90
Conversation
ryancinsight
commented
Jul 10, 2025
ryancinsight
commented
- Move convex_hull.rs and smoothing.rs into algorithm/ subdirectory
- Split algorithms into serial and parallel implementations
- Add trait definitions for algorithm interfaces
- Update module structure and imports in mesh/mod.rs and connectivity.rs
- Implement dependency inversion pattern using BspOps trait - Create separate SerialBspOps and ParallelBspOps implementations - Replace manual loops with iterator patterns throughout - Maintain backward compatibility with existing Node API - Add SplittingPlaneStrategy trait for algorithm flexibility - Organize code into clean submodule structure: - traits.rs: Core trait definitions - node.rs: Node data structure only - serial.rs: Serial BSP operations - parallel.rs: Parallel BSP operations (with rayon) - mod.rs: Public API and backward compatibility - Fix doctest import issues and create missing directories - All tests passing (98 lib tests + 28 doc tests) This refactoring separates algorithms from data structures, enables easy testing of different BSP strategies, and follows modern Rust patterns while maintaining full API compatibility.
- Implement dependency inversion pattern using BspOps trait - Create separate SerialBspOps and ParallelBspOps implementations - Replace manual loops with iterator patterns throughout - Maintain backward compatibility with existing Node API - Add SplittingPlaneStrategy trait for algorithm flexibility - Organize code into clean submodule structure: - traits.rs: Core trait definitions - node.rs: Node data structure only - serial.rs: Serial BSP operations - parallel.rs: Parallel BSP operations (with rayon) - mod.rs: Public API and backward compatibility - Fix doctest import issues and create missing directories - All tests passing (98 lib tests + 28 doc tests) This refactoring separates algorithms from data structures, enables easy testing of different BSP strategies, and follows modern Rust patterns while maintaining full API compatibility.
- Implement dependency inversion pattern using BspOps trait - Create separate SerialBspOps and ParallelBspOps implementations - Replace manual loops with iterator patterns throughout - Maintain backward compatibility with existing Node API - Add SplittingPlaneStrategy trait for algorithm flexibility - Organize code into clean submodule structure: - traits.rs: Core trait definitions - node.rs: Node data structure only - serial.rs: Serial BSP operations - parallel.rs: Parallel BSP operations (with rayon) - mod.rs: Public API and backward compatibility - Fix doctest import issues and create missing directories - All tests passing (98 lib tests + 28 doc tests) This refactoring separates algorithms from data structures, enables easy testing of different BSP strategies, and follows modern Rust patterns while maintaining full API compatibility.
…m/ryancinsight/csgrs into refactor/bsp-dependency-inversion
…m/ryancinsight/csgrs into refactor/bsp-dependency-inversion
…m/ryancinsight/csgrs into refactor/bsp-dependency-inversion
…m/ryancinsight/csgrs into refactor/bsp-dependency-inversion
…m/ryancinsight/csgrs into refactor/bsp-dependency-inversion
…m/ryancinsight/csgrs into refactor/bsp-dependency-inversion
…m/ryancinsight/csgrs into refactor/bsp-dependency-inversion
- Split monolithic sdf.rs into focused submodules - Add grid.rs for SDF grid data structures - Add traits.rs for common SDF interfaces - Add serial.rs and parallel.rs for different computation strategies - Update readme.md with new module structure
- Split monolithic sdf.rs into focused submodules - Add grid.rs for SDF grid data structures - Add traits.rs for common SDF interfaces - Add serial.rs and parallel.rs for different computation strategies - Update readme.md with new module structure
…into refactor-sdf
…into refactor-sdf
…into refactor-sdf
…into refactor-sdf
…into refactor-sdf
…into refactor-sdfc fmt
- Move convex_hull.rs and smoothing.rs into algorithm/ subdirectory - Split algorithms into serial and parallel implementations - Add trait definitions for algorithm interfaces - Update module structure and imports in mesh/mod.rs and connectivity.rs This refactoring improves code organization and enables better dependency inversion for mesh processing algorithms.
|
I'm actually going to implement this differently |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 6d87c97..faf8043
| Severity | Location | Issue | Delete |
|---|---|---|---|
 |
src/mesh/algorithm/smoothing/parallel.rs:317 | Runtime panic from unwrap |  |
✅ Files analyzed, no issues (34)
• examples/adjacency_demo.rs
• readme.md
• src/io/stl.rs
• src/lib.rs
• src/main.rs
• src/mesh/algorithm/convex_hull/mod.rs
• src/mesh/algorithm/convex_hull/parallel.rs
• src/mesh/algorithm/convex_hull/serial.rs
• src/mesh/algorithm/convex_hull/traits.rs
• src/mesh/algorithm/mod.rs
• src/mesh/algorithm/smoothing/mod.rs
• src/mesh/algorithm/smoothing/serial.rs
• src/mesh/algorithm/smoothing/traits.rs
• src/mesh/bsp.rs
• src/mesh/bsp/mod.rs
• src/mesh/bsp/node.rs
• src/mesh/bsp/parallel.rs
• src/mesh/bsp/serial.rs
• src/mesh/bsp/traits.rs
• src/mesh/bsp_parallel.rs
• src/mesh/connectivity.rs
• src/mesh/metaballs.rs
• src/mesh/mod.rs
• src/mesh/sdf.rs
• src/mesh/sdf/grid.rs
• src/mesh/sdf/mod.rs
• src/mesh/sdf/parallel.rs
• src/mesh/sdf/serial.rs
• src/mesh/sdf/traits.rs
• src/nurbs/mod.rs
• src/sketch/extrudes.rs
• src/sketch/hershey.rs
• src/sketch/shapes.rs
• src/tests.rs
| // Curvature check | ||
| if !should_refine { | ||
| 'edge_loop: for edge in polygon.edges() { | ||
| let v1_idx = vertex_map.get_index(&edge.0.pos).unwrap(); |
There was a problem hiding this comment.
Potential runtime panic in parallel adaptive_refine implementation. The code calls .unwrap() on vertex_map.get_index() which returns Option. If the epsilon-based floating-point comparison fails to match a vertex position, or if the pre-population step misses any vertices, this will panic at runtime.
The serial implementation (serial.rs line 272) uses get_or_create_index() which handles missing vertices gracefully by creating them. The parallel implementation cannot use get_or_create_index() due to Rust's borrow checker restrictions in parallel contexts, but it should handle the None case instead of unwrapping.
This bug will cause crashes when:
- Using the 'parallel' feature flag
- Calling adaptive_refine() on meshes
- When floating-point precision causes vertex positions to not match within epsilon tolerance
Error will be: thread panicked at 'called Option::unwrap() on a None value'
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
1 participant
