Skip to content

Add CoACD support - #28

Merged
jschueller merged 8 commits into
masterfrom
coacd
Jul 27, 2026
Merged

Add CoACD support#28
jschueller merged 8 commits into
masterfrom
coacd

Conversation

@jschueller

@jschueller jschueller commented Jun 22, 2026

Copy link
Copy Markdown
Member
  • turns out the convex decomposition in coacd is well parallelized!
  • examples/plot_full.py: t=2662.1s => t=15.2s
  • new api to avoid decomposition/recomposition:
convexPiecesAllCylinders = algoInter.buildCylinderConvex([C_1, C_2, C_3]) # => Collection<Sample>
globalMesh = algoInter.buildWithConvexParts(mesh, convexPiecesAllCylinders)

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

  • VolumeMesher
  • use ConvexHullMesher + VolumeMesher in IntersectionMesher for speed & reduce number of simplices
  • Use Qhull by default in CloudMesher

Summary by CodeRabbit

  • New Features

    • Added VolumeMesher to generate volumetric meshes from surface meshes, with CENTROID and FIRST_VERTEX apex strategies.
    • Enabled optional CoACD-based approximate convex decomposition (and reported CoACD feature availability when present).
    • Extended IntersectionMesher with a workflow that accepts precomputed convex pieces and improves convex-intersection meshing.
  • Documentation

    • Added Python docs and new volumetric meshing examples; refreshed the full workflow example.
  • Bug Fixes

    • Improved convex decomposition/intersection handling across supported dimensional cases and unsupported requests.
  • Tests

    • Updated 3D volumetric/convex decomposition expectations and added end-to-end VolumeMesher coverage.

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jschueller, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 84491759-9345-4aa9-904a-1e635c953092

📥 Commits

Reviewing files that changed from the base of the PR and between 26e86ee and 3027d2b.

📒 Files selected for processing (6)
  • CMakeLists.txt
  • ChangeLog
  • VERSION
  • distro/debian/changelog
  • distro/rpm/otmeshing.spec
  • python/src/__init__.py
📝 Walkthrough

Walkthrough

Adds optional CoACD convex decomposition, a persistent VolumeMesher C++/Python API, reusable convex intersection workflows, updated build integration, documentation, examples, benchmarks, and tests.

Changes

Meshing features and integration

Layer / File(s) Summary
Build configuration and test wiring
CMakeLists.txt, lib/src/CMakeLists.txt, lib/test/CMakeLists.txt, python/test/CMakeLists.txt, .ci_support/*
Enables CoACD discovery and linking, changes the CDDLIB GMP default, registers VolumeMesher and intersection tests, and updates CI commands.
VolumeMesher implementation and API
lib/src/otmeshing/VolumeMesher.hxx, lib/src/VolumeMesher.cxx, python/src/*VolumeMesher*, python/src/otmeshing_module.i
Adds apex-based surface-to-volume meshing with centroid and first-vertex strategies, persistence, SWIG bindings, and API documentation.
CoACD convex decomposition
lib/src/ConvexDecompositionMesher.cxx, lib/src/otmeshing/ConvexDecompositionMesher.hxx, python/src/ConvexDecompositionMesher_doc.i, *ConvexDecompositionMesher* tests
Reworks 2D and 3D decomposition, adds CoACD feature detection and threshold configuration, and updates adjacent-cube decomposition expectations.
Reusable convex intersection workflow
lib/src/otmeshing/IntersectionMesher.hxx, lib/src/IntersectionMesher.cxx, python/src/IntersectionMesher*
Adds pre-decomposed convex intersection and cylinder decomposition APIs, uses VolumeMesher for larger intersections, and extends Python conversion and documentation support.
Examples and validation
python/doc/*, python/test/*, lib/test/*
Adds VolumeMesher examples and tests, adds an intersection benchmark, updates the full plotting workflow, and expands C++ intersection coverage.

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 geometry
  ConvexDecompositionMesher-->>IntersectionMesher: return convex pieces
  PythonExample->>IntersectionMesher: buildWithConvexParts(mesh, convex pieces)
  IntersectionMesher->>CloudMesher: mesh surviving intersections
  CloudMesher-->>IntersectionMesher: return intersection meshes
Loading

Possibly related PRs

  • openturns/otmeshing#27: Both changes rework ConvexDecompositionMesher.cxx convex decomposition and vertex processing.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 38.10% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding CoACD support to otmeshing.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch coacd

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between dc6b1f2 and 699e552.

📒 Files selected for processing (9)
  • CMakeLists.txt
  • lib/src/CMakeLists.txt
  • lib/src/ConvexDecompositionMesher.cxx
  • lib/src/IntersectionMesher.cxx
  • lib/test/t_CloudMesher_std.cxx
  • lib/test/t_ConvexDecompositionMesher_std.cxx
  • python/doc/examples/plot_full.py
  • python/src/ConvexDecompositionMesher_doc.i
  • python/test/t_ConvexDecompositionMesher_std.py

Comment thread CMakeLists.txt
Comment thread lib/src/ConvexDecompositionMesher.cxx
Comment thread lib/src/IntersectionMesher.cxx
@jschueller
jschueller force-pushed the coacd branch 3 times, most recently from 01a65b2 to f81228d Compare June 23, 2026 15:38
@jschueller
jschueller force-pushed the coacd branch 2 times, most recently from 7f84752 to 78e0378 Compare July 10, 2026 16:07
@jschueller jschueller mentioned this pull request Jul 15, 2026
@jschueller
jschueller marked this pull request as ready for review July 16, 2026 06:16

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Use chunked block processing for memory efficiency.

In buildWithConvexParts and build, the intersections are processed in blocks of size IntersectionMesher-BlockSize to bound memory allocation for the intermediate Collection<Sample> (which can become massive if toDoSize is 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 value

Remove unused loop control variable.

The variable i and enumerate are not used in the loop body. You can simplify this by iterating over decomposition directly.

♻️ 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 value

Make 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 exactly 1.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 value

Simplify point extraction.

OpenTURNS Sample provides an operator[] that returns a Point. 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 win

Consolidate the duplicated vertex initialization loop.

The logic that copies vertices into verticesWithApex is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 699e552 and ad63d4d.

📒 Files selected for processing (26)
  • .ci_support/run_docker_linux.sh
  • .ci_support/run_docker_mingw.sh
  • CMakeLists.txt
  • lib/src/CMakeLists.txt
  • lib/src/ConvexDecompositionMesher.cxx
  • lib/src/IntersectionMesher.cxx
  • lib/src/VolumeMesher.cxx
  • lib/src/otmeshing/ConvexDecompositionMesher.hxx
  • lib/src/otmeshing/IntersectionMesher.hxx
  • lib/src/otmeshing/VolumeMesher.hxx
  • lib/test/t_ConvexDecompositionMesher_std.cxx
  • python/doc/examples/plot_full.py
  • python/doc/examples/plot_volume.py
  • python/doc/pyplots/VolumeMesher.py
  • python/doc/user_manual/user_manual.rst
  • python/src/CMakeLists.txt
  • python/src/ConvexDecompositionMesher_doc.i
  • python/src/IntersectionMesher.i
  • python/src/IntersectionMesher_doc.i
  • python/src/VolumeMesher.i
  • python/src/VolumeMesher_doc.i
  • python/src/otmeshing_module.i
  • python/test/CMakeLists.txt
  • python/test/t_ConvexDecompositionMesher_std.py
  • python/test/t_IntersectionMesher_bench.py
  • python/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

Comment thread lib/src/ConvexDecompositionMesher.cxx
Comment thread lib/src/ConvexDecompositionMesher.cxx
Comment thread lib/src/VolumeMesher.cxx
Comment thread lib/test/t_ConvexDecompositionMesher_std.cxx
Comment thread python/src/VolumeMesher_doc.i Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ad63d4d and e4dc903.

📒 Files selected for processing (15)
  • lib/src/CMakeLists.txt
  • lib/src/IntersectionMesher.cxx
  • lib/src/VolumeMesher.cxx
  • lib/src/otmeshing/VolumeMesher.hxx
  • lib/test/CMakeLists.txt
  • lib/test/t_VolumeMesher_std.cxx
  • python/doc/examples/plot_volume.py
  • python/doc/pyplots/VolumeMesher.py
  • python/doc/user_manual/user_manual.rst
  • python/src/CMakeLists.txt
  • python/src/VolumeMesher.i
  • python/src/VolumeMesher_doc.i
  • python/src/otmeshing_module.i
  • python/test/CMakeLists.txt
  • python/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

Comment thread lib/src/VolumeMesher.cxx
@jschueller
jschueller force-pushed the coacd branch 2 times, most recently from 972dbee to c64aa68 Compare July 16, 2026 09:51

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
lib/test/t_IntersectionMesher_std.cxx (1)

140-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix test case numbering.

The test case numbering skips from 9. (line 115) to 12.. Consider renumbering for sequence consistency.

📝 Proposed fix
-  // 12. buildConvex empty / single / self
+  // 10. buildConvex empty / single / self
🤖 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/test/t_IntersectionMesher_std.cxx` at line 140, Update the test case
comment in lib/test/t_IntersectionMesher_std.cxx so the numbering follows the
preceding `9.` case sequentially; adjust `12. buildConvex empty / single / self`
to the correct next test number without changing test 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.

Nitpick comments:
In `@lib/test/t_IntersectionMesher_std.cxx`:
- Line 140: Update the test case comment in
lib/test/t_IntersectionMesher_std.cxx so the numbering follows the preceding
`9.` case sequentially; adjust `12. buildConvex empty / single / self` to the
correct next test number without changing test behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: caac4d06-3a65-475f-a82b-57f5a4c5c6b5

📥 Commits

Reviewing files that changed from the base of the PR and between a072c16 and c30d990.

📒 Files selected for processing (4)
  • lib/include/otmeshing/otmeshing.hxx
  • lib/src/IntersectionMesher.cxx
  • lib/test/CMakeLists.txt
  • lib/test/t_IntersectionMesher_std.cxx
💤 Files with no reviewable changes (1)
  • lib/src/IntersectionMesher.cxx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
lib/test/t_IntersectionMesher_std.cxx (2)

134-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the actual convex intersection geometry.

These squares intersect in a deterministic four-vertex square. result.getSize() >= 3 also accepts an incorrectly clipped triangle or unrelated polygon. Assert the dimension and expected four vertices, using order-independent comparison if needed.

🤖 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/test/t_IntersectionMesher_std.cxx` around lines 134 - 138, The
buildConvexSample test currently only checks for at least three vertices; update
the assertion around result to verify the expected two-dimensional intersection
and exactly four vertices, comparing vertex coordinates without relying on
ordering if necessary.

167-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the new convex-piece workflow.

This only tests buildCylinder; the PR’s new buildCylinderConvex and buildWithConvexParts APIs are untested. Reuse these convex and non-convex fixtures to build pieces, compose the global mesh, and verify its volume/topology.

🤖 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/test/t_IntersectionMesher_std.cxx` around lines 167 - 234, Extend the
cylinder intersection tests to cover buildCylinderConvex and
buildWithConvexParts using the existing convex and non-convex cylinder fixtures.
Build convex pieces, compose them into the global mesh, and assert the expected
volume and topology, including the disjoint and self-intersection cases where
applicable.
🤖 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.

Nitpick comments:
In `@lib/test/t_IntersectionMesher_std.cxx`:
- Around line 134-138: The buildConvexSample test currently only checks for at
least three vertices; update the assertion around result to verify the expected
two-dimensional intersection and exactly four vertices, comparing vertex
coordinates without relying on ordering if necessary.
- Around line 167-234: Extend the cylinder intersection tests to cover
buildCylinderConvex and buildWithConvexParts using the existing convex and
non-convex cylinder fixtures. Build convex pieces, compose them into the global
mesh, and assert the expected volume and topology, including the disjoint and
self-intersection cases where applicable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e47b05a-5189-4887-a3dd-129d73b8f923

📥 Commits

Reviewing files that changed from the base of the PR and between a072c16 and 26e86ee.

📒 Files selected for processing (4)
  • lib/include/otmeshing/otmeshing.hxx
  • lib/src/IntersectionMesher.cxx
  • lib/test/CMakeLists.txt
  • lib/test/t_IntersectionMesher_std.cxx
💤 Files with no reviewable changes (1)
  • lib/src/IntersectionMesher.cxx
🚧 Files skipped from review as they are similar to previous changes (2)
  • lib/include/otmeshing/otmeshing.hxx
  • lib/test/CMakeLists.txt

@jschueller
jschueller merged commit df50e33 into master Jul 27, 2026
7 checks passed
@jschueller
jschueller deleted the coacd branch July 27, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant