add heat method geodesics module #58
Conversation
- HeatGeodesicSolver class with precomputation for repeated queries - heat_geodesic_distances function for single-shot usage - uses CGAL Heat_method_3 with intrinsic Delaunay triangulation - ~30% faster than libigl heat in compas_slicer workflow
|
@jf--- sorry to keep pointing this out, but you need to add an entry to the changelog to make the PR checks pass... |
|
@petrasvestartas can you help review the technical aspects of the binding? thanks! |
There was a problem hiding this comment.
Pull request overview
This PR adds a new geodesics module to compas_cgal that provides geodesic distance computation using CGAL's Heat_method_3 algorithm. The implementation offers approximately 30% performance improvement over libigl's heat method in compas_slicer workflows.
Key changes:
- Added
HeatGeodesicSolverclass for efficient repeated geodesic queries with precomputation - Added
heat_geodesic_distancesfunction for single-shot distance calculations - Integrated CGAL Heat_method_3 with intrinsic Delaunay triangulation for accuracy
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/geodesics.h | C++ header declaring the heat geodesic distance function interface |
| src/geodesics.cpp | C++ implementation with HeatGeodesicSolver class and nanobind bindings |
| src/compas_cgal/geodesics.py | Python wrapper providing Mesh-based API for geodesic distance computation |
| CMakeLists.txt | Build configuration update to include the new geodesics module |
| CHANGELOG.md | Documentation of new features added in this release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tomvanmele
left a comment
tomvanmele
left a comment
There was a problem hiding this comment.
a few thoughts about the C++ docstrings...
other than that LGTM
src/geodesics.cpp
Outdated
| "Parameters\n" | ||
| "----------\n" | ||
| "vertices : array-like\n" | ||
| " Vertex positions as Nx3 matrix (float64)\n" | ||
| "faces : array-like\n" | ||
| " Face indices as Mx3 matrix (int32)\n" | ||
| "sources : list[int]\n" | ||
| " Source vertex indices\n" | ||
| "\n" | ||
| "Returns\n" | ||
| "-------\n" | ||
| "array\n" | ||
| " Geodesic distances from sources to all vertices (Nx1)", | ||
| "vertices"_a, |
There was a problem hiding this comment.
@jf--- although i appreciate the effort, we don't actually use this anywhere. it will not be parsed and it will not be visible to anyone.
and since it is just another docstring to maintain and coordinate, and therefore can get out-of-sync quite easily, i think it would be better to just add a one-liner...
There was a problem hiding this comment.
as far as i am concerned, a docstring is not even needed here. the Python wrapper already provides it. in the C++ code i would prefer to only add developer oriented comments
There was a problem hiding this comment.
fair enough but help aligning things upstream / on the python end
src/geodesics.cpp
Outdated
| "sources"_a); | ||
|
|
||
| nanobind::class_<HeatGeodesicSolver>(m, "HeatGeodesicSolver", | ||
| "Precomputed heat method solver for repeated geodesic queries.\n\n" |
There was a problem hiding this comment.
I dont think removal is a good idea
|
I am doing a few changes to this PR currently, from include cgal new features from this old discussion: CGAL/cgal#5885 |
I added the isocurves and mesh splitting from isocurves.
I remove excessive comments from cpp nanobind wrappers because we wrap them in another python function |
Remove unnecessary docstring from test file.
|
@petrasvestartas absolutely remarkable work! the |
|
oops unintended I'll backport the slicing back to super stoked how this turned out! |
|
@tomvanmele can you approve and merge? |
4 participants
Summary
New API
Test plan