Add CoACD support#28
Open
jschueller wants to merge 6 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults 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)
📝 WalkthroughWalkthroughAdds optional CoACD convex decomposition, a persistent VolumeMesher C++/Python API, reusable convex intersection workflows, updated build integration, documentation, examples, benchmarks, and tests. ChangesMeshing algorithms and build integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PythonExample
participant IntersectionMesher
participant ConvexDecompositionMesher
participant CloudMesher
PythonExample->>IntersectionMesher: buildCylinderConvex(cylinders)
IntersectionMesher->>ConvexDecompositionMesher: decompose cylinder intersections
ConvexDecompositionMesher-->>IntersectionMesher: convex pieces
PythonExample->>IntersectionMesher: buildWithConvexParts(mesh, convex pieces)
IntersectionMesher->>CloudMesher: mesh convex intersections
CloudMesher-->>IntersectionMesher: intersection meshes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CMakeLists.txt`:
- Around line 48-52: The find_package call for CoACD is optional and does not
verify that the package was actually found before logging the success message.
Modify the code block to check the CoACD_FOUND variable after calling
find_package, and only log the "Found CoACD" message when CoACD_FOUND is true.
Alternatively, add REQUIRED to the find_package call to make it fail explicitly
if CoACD is not available when USE_COACD is enabled, or add an error or warning
message when USE_COACD is ON but CoACD_FOUND is false.
In `@lib/src/ConvexDecompositionMesher.cxx`:
- Around line 348-366: The loop that populates facetMap from tetrahedra
simplices lacks the volume filter that exists in the fallback branch, allowing
degenerate tetrahedra to corrupt the face-counting map. Add a guard condition at
the start of the for loop (where i iterates through simplices from 0 to
simplices.getSize()) to skip processing any simplex where simplicesVolume[i] is
less than or equal to zero, which will prevent inverted or degenerate cells from
being included in the facet extraction and maintain consistency with the
fallback path starting around line 564.
In `@lib/src/IntersectionMesher.cxx`:
- Line 536: The resource key "ConvexDecompositionMesher-Threshold" is currently
registered in IntersectionMesher initialization, but it is consumed by
ConvexDecompositionMesher::build, creating a fragile cross-unit dependency. Move
the ResourceMap::AddAsScalar("ConvexDecompositionMesher-Threshold", 0.05)
registration from IntersectionMesher.cxx into the ConvexDecompositionMesher
class initialization (such as in its constructor or init method) to keep
resource ownership local and predictable.
🪄 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 Plus
Run ID: 7d51ea20-b1d5-4b88-86d5-0cd4415342e7
📒 Files selected for processing (9)
CMakeLists.txtlib/src/CMakeLists.txtlib/src/ConvexDecompositionMesher.cxxlib/src/IntersectionMesher.cxxlib/test/t_CloudMesher_std.cxxlib/test/t_ConvexDecompositionMesher_std.cxxpython/doc/examples/plot_full.pypython/src/ConvexDecompositionMesher_doc.ipython/test/t_ConvexDecompositionMesher_std.py
jschueller
force-pushed
the
coacd
branch
3 times, most recently
from
June 23, 2026 15:38
01a65b2 to
f81228d
Compare
jschueller
force-pushed
the
coacd
branch
2 times, most recently
from
July 10, 2026 16:07
7f84752 to
78e0378
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/IntersectionMesher.cxx (1)
498-511: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse chunked block processing for memory efficiency.
In
buildWithConvexPartsandbuild, the intersections are processed in blocks of sizeIntersectionMesher-BlockSizeto bound memory allocation for the intermediateCollection<Sample>(which can become massive iftoDoSizeis large). Here,result(toDoSize)allocates for the entire Cartesian product at once, which could lead to high memory usage or OOM for highly decomposed meshes.Consider adopting the same chunked approach used in
buildWithConvexParts.♻️ Proposed fix to add block chunking
// build lists of intersections to compute const UnsignedInteger toDoSize = unionCurrent.getSize() * unionNext.getSize(); + const UnsignedInteger blockSize = ResourceMap::GetAsUnsignedInteger("IntersectionMesher-BlockSize"); // loop over intersections - Collection<Sample> result(toDoSize); - const IntersectionMesherConvexSamplePolicy policy(*this, unionCurrent, unionNext, 0, result); - TBBImplementation::ParallelFor(0, toDoSize, policy); - - // prune empty intersections - unionCurrent.resize(0); - for (UnsignedInteger i0 = 0; i0 < result.getSize(); ++ i0) - if (result[i0].getSize()) - unionCurrent.add(result[i0]); + Collection<Sample> result(0); + Collection<Sample> resultChunk(blockSize); + for (UnsignedInteger done = 0; done < toDoSize; done += blockSize) + { + const UnsignedInteger actualBlockSize = std::min(blockSize, toDoSize - done); + const IntersectionMesherConvexSamplePolicy policy(*this, unionCurrent, unionNext, done, resultChunk); + TBBImplementation::ParallelFor(0, actualBlockSize, policy); + + // prune empty intersections + for (UnsignedInteger i0 = 0; i0 < actualBlockSize; ++ i0) + if (resultChunk[i0].getSize()) + result.add(resultChunk[i0]); + } + unionCurrent = result;🤖 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 `@lib/src/IntersectionMesher.cxx` around lines 498 - 511, Update buildWithConvexParts/build’s intersection processing to use the existing IntersectionMesher-BlockSize chunking pattern instead of allocating result for the entire Cartesian product. Process each block through IntersectionMesherConvexSamplePolicy, append non-empty samples to unionCurrent, and preserve the current pruning behavior while bounding intermediate Collection<Sample> memory.
🧹 Nitpick comments (4)
python/test/t_ConvexDecompositionMesher_std.py (1)
219-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused loop control variable.
The variable
iandenumerateare not used in the loop body. You can simplify this by iterating overdecompositiondirectly.♻️ Proposed refactor
-for i, convex in enumerate(decomposition): +for convex in decomposition: volume_sum += convex.getVolume() assert otmeshing.ConvexDecompositionMesher.IsConvex(convex)🤖 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 `@python/test/t_ConvexDecompositionMesher_std.py` around lines 219 - 222, Remove the unused loop control variable by changing the loop over decomposition to iterate directly over each convex item, while preserving the volume accumulation and convexity assertion.Source: Linters/SAST tools
python/test/t_VolumeMesher_std.py (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the unit cube interval bounds explicit.
While
ot.Interval(3)may produce the correct default bounds in your environment, explicitly passing the lower and upper bounds improves readability and explicitly guarantees that the resulting volume will be exactly1.0(which is asserted on line 51).♻️ Proposed refactor for clarity
-cube_corners = ot.IntervalMesher([1] * 3).build(ot.Interval(3)).getVertices() +cube_corners = ot.IntervalMesher([1] * 3).build(ot.Interval([0.0]*3, [1.0]*3)).getVertices()🤖 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 `@python/test/t_VolumeMesher_std.py` at line 44, Update the IntervalMesher setup in the cube test to construct ot.Interval with explicit lower and upper bounds for a unit cube, while preserving the existing vertex generation and volume assertion.lib/src/VolumeMesher.cxx (2)
153-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify point extraction.
OpenTURNS
Sampleprovides anoperator[]that returns aPoint. You can simplify this inner loop by leveraging it directly.♻️ Proposed refactor
if (usedVertices[i]) { - Point p(dimension); - for (UnsignedInteger j = 0; j < dimension; ++ j) - p[j] = verticesWithApex(i, j); - verticesCompact.add(p); + verticesCompact.add(verticesWithApex[i]); oldToNew[i] = verticesCompact.getSize() - 1; }🤖 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 `@lib/src/VolumeMesher.cxx` around lines 153 - 159, In the usedVertices handling of VolumeMesher, replace the manual Point construction and dimension-indexed copy loop with the Sample operator[] access on verticesWithApex. Preserve the existing verticesCompact insertion and oldToNew mapping behavior.
86-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated vertex initialization loop.
The logic that copies
verticesintoverticesWithApexis duplicated in both branches of the conditional. You can move this loop outside to improve maintainability.♻️ Proposed refactor
// Compute apex UnsignedInteger apexIndex = 0; Sample verticesWithApex(nbVertices + 1, dimension); + // Initialize with original vertices + for (UnsignedInteger i = 0; i < nbVertices; ++ i) + for (UnsignedInteger j = 0; j < dimension; ++ j) + verticesWithApex(i, j) = vertices(i, j); + if (apexStrategy_ == CENTROID) { Point centroid(dimension); for (UnsignedInteger i = 0; i < nbVertices; ++ i) for (UnsignedInteger j = 0; j < dimension; ++ j) centroid[j] += vertices(i, j); centroid /= static_cast<Scalar>(nbVertices); apexIndex = nbVertices; - for (UnsignedInteger i = 0; i < nbVertices; ++ i) - for (UnsignedInteger j = 0; j < dimension; ++ j) - verticesWithApex(i, j) = vertices(i, j); for (UnsignedInteger j = 0; j < dimension; ++ j) verticesWithApex(apexIndex, j) = centroid[j]; } else { apexIndex = 0; - for (UnsignedInteger i = 0; i < nbVertices; ++ i) - for (UnsignedInteger j = 0; j < dimension; ++ j) - verticesWithApex(i, j) = vertices(i, j); }🤖 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 `@lib/src/VolumeMesher.cxx` around lines 86 - 111, Consolidate the duplicated vertices-to-verticesWithApex copy loop in the apexStrategy_ conditional: perform the copy once before or after the CENTROID branch, while preserving apexIndex assignment and centroid apex population behavior.
🤖 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 `@lib/src/ConvexDecompositionMesher.cxx`:
- Around line 276-340: Replace the boundary-vertex apex logic in the simplex
construction around apexIndex with the new VolumeMesher, or compute an interior
centroid apex and create one tetrahedron for every triangulated boundary
triangle. Ensure generated tetrahedra use the interior point and cannot be
zero-volume, removing the current facet filter that skips faces containing the
selected boundary vertex.
- Around line 422-505: The current edge-based grouping in the component-building
logic splits outer and inner cavity shells, causing CoACD to fill cavities;
replace it with components derived from tetrahedron face adjacency, retaining
all boundary shells belonging to each volumetric component. Ensure nested-shell
cases use the exact existing path instead of independently decomposing cavity
shells, while preserving the subsequent local vertex/index construction.
In `@lib/src/VolumeMesher.cxx`:
- Around line 71-75: Update the dimension validation in VolumeMesher to check
dimension == 0 before computing or displaying dimension - 1, and throw
InvalidArgumentException with an appropriate message for zero-dimensional
meshes. Preserve the existing intrinsic-dimension validation for dimensions
greater than zero.
In `@lib/test/t_ConvexDecompositionMesher_std.cxx`:
- Around line 193-194: Update the Interval used to build cube2Mesh so its Y and
Z bounds match cube1Mesh’s [0,2] ranges while keeping its X range [2,4], making
the cubes share the face at x=2.
In `@python/src/VolumeMesher_doc.i`:
- Around line 9-12: Update the apexStrategy documentation in VolumeMesher to
describe the exposed ApexStrategy enum, using the CENTROID and FIRST_VERTEX
constants (or their integer values) instead of the string values "centroid" and
"firstVertex". Preserve the existing descriptions of each strategy’s behavior.
---
Outside diff comments:
In `@lib/src/IntersectionMesher.cxx`:
- Around line 498-511: Update buildWithConvexParts/build’s intersection
processing to use the existing IntersectionMesher-BlockSize chunking pattern
instead of allocating result for the entire Cartesian product. Process each
block through IntersectionMesherConvexSamplePolicy, append non-empty samples to
unionCurrent, and preserve the current pruning behavior while bounding
intermediate Collection<Sample> memory.
---
Nitpick comments:
In `@lib/src/VolumeMesher.cxx`:
- Around line 153-159: In the usedVertices handling of VolumeMesher, replace the
manual Point construction and dimension-indexed copy loop with the Sample
operator[] access on verticesWithApex. Preserve the existing verticesCompact
insertion and oldToNew mapping behavior.
- Around line 86-111: Consolidate the duplicated vertices-to-verticesWithApex
copy loop in the apexStrategy_ conditional: perform the copy once before or
after the CENTROID branch, while preserving apexIndex assignment and centroid
apex population behavior.
In `@python/test/t_ConvexDecompositionMesher_std.py`:
- Around line 219-222: Remove the unused loop control variable by changing the
loop over decomposition to iterate directly over each convex item, while
preserving the volume accumulation and convexity assertion.
In `@python/test/t_VolumeMesher_std.py`:
- Line 44: Update the IntervalMesher setup in the cube test to construct
ot.Interval with explicit lower and upper bounds for a unit cube, while
preserving the existing vertex generation and volume assertion.
🪄 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 Plus
Run ID: db9f64e8-e5d3-45dc-bd1b-cd7722237f88
📒 Files selected for processing (26)
.ci_support/run_docker_linux.sh.ci_support/run_docker_mingw.shCMakeLists.txtlib/src/CMakeLists.txtlib/src/ConvexDecompositionMesher.cxxlib/src/IntersectionMesher.cxxlib/src/VolumeMesher.cxxlib/src/otmeshing/ConvexDecompositionMesher.hxxlib/src/otmeshing/IntersectionMesher.hxxlib/src/otmeshing/VolumeMesher.hxxlib/test/t_ConvexDecompositionMesher_std.cxxpython/doc/examples/plot_full.pypython/doc/examples/plot_volume.pypython/doc/pyplots/VolumeMesher.pypython/doc/user_manual/user_manual.rstpython/src/CMakeLists.txtpython/src/ConvexDecompositionMesher_doc.ipython/src/IntersectionMesher.ipython/src/IntersectionMesher_doc.ipython/src/VolumeMesher.ipython/src/VolumeMesher_doc.ipython/src/otmeshing_module.ipython/test/CMakeLists.txtpython/test/t_ConvexDecompositionMesher_std.pypython/test/t_IntersectionMesher_bench.pypython/test/t_VolumeMesher_std.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/src/ConvexDecompositionMesher_doc.i
- lib/src/CMakeLists.txt
- CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/src/VolumeMesher.cxx`:
- Around line 69-80: Update VolumeMesher::build to validate that the input
surface is a closed, convex triangular mesh before constructing the apex fan.
Reject unsupported open or non-convex surfaces through the existing
InvalidArgumentException path, preserving the current dimension and
minimum-vertex checks.
🪄 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 Plus
Run ID: a66d2bd6-68de-4b88-82ff-ab4fb63b92b1
📒 Files selected for processing (15)
lib/src/CMakeLists.txtlib/src/IntersectionMesher.cxxlib/src/VolumeMesher.cxxlib/src/otmeshing/VolumeMesher.hxxlib/test/CMakeLists.txtlib/test/t_VolumeMesher_std.cxxpython/doc/examples/plot_volume.pypython/doc/pyplots/VolumeMesher.pypython/doc/user_manual/user_manual.rstpython/src/CMakeLists.txtpython/src/VolumeMesher.ipython/src/VolumeMesher_doc.ipython/src/otmeshing_module.ipython/test/CMakeLists.txtpython/test/t_VolumeMesher_std.py
🚧 Files skipped from review as they are similar to previous changes (9)
- python/doc/pyplots/VolumeMesher.py
- python/src/VolumeMesher.i
- python/doc/user_manual/user_manual.rst
- python/src/otmeshing_module.i
- python/doc/examples/plot_volume.py
- lib/src/otmeshing/VolumeMesher.hxx
- python/test/CMakeLists.txt
- python/test/t_VolumeMesher_std.py
- lib/src/IntersectionMesher.cxx
jschueller
force-pushed
the
coacd
branch
2 times, most recently
from
July 16, 2026 09:51
972dbee to
c64aa68
Compare
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
1 participant
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.
this avoid union of independently triangulated meshes that do not form a simplicial mesh (coacd errors: The mesh is not a 2-manifold!), instead we intersect the mesh by pairwise intersection with the pre-decomposed pieces
Summary by CodeRabbit
VolumeMesherfor generating volumetric meshes from surface meshes, with centroid and first-vertex apex strategies.