feat(cgal)!: modernize the CGAL 6.2 integration boundary - #124
Merged
Conversation
- require the exact CGAL baseline and its supported compiler floors - adopt canonical CGAL simplex types, finite ranges, and metadata safeguards - make optional oneTBB concurrency and lock-grid ownership explicit - document and benchmark the supported integration contract BREAKING CHANGE: CDT++ now requires exactly CGAL 6.2, GCC 13.3 or Clang 22 on those toolchains, and removes the legacy Face_handle_t alias. Closes #102
acgetchell
enabled auto-merge
July 24, 2026 00:10
WalkthroughThis change pins CGAL 6.2, updates compiler and parallel-triangulation configuration, modernizes CGAL handle and range usage, strengthens metadata and derived-state validation, adds integration tests and a benchmark, and documents the supported integration contract. ChangesCGAL integration modernization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Benchmark
participant Delaunay_state
participant FoliatedTriangulation
participant Manifold
Benchmark->>Delaunay_state: Construct insertion state
Delaunay_state->>FoliatedTriangulation: Create detached triangulation
FoliatedTriangulation->>Manifold: Reconstruct and validate manifold
Manifold-->>Benchmark: Return topology counts and timings
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@include/Foliated_triangulation.hpp`:
- Around line 1377-1426: Add a lightweight invariant-check path for move
validation and update detail::check_move()/Manifold::is_correct() to use it,
avoiding has_consistent_derived_state() and its cache-rebuilding scans on every
move. Keep has_consistent_derived_state() available for full validation, while
the fast path verifies only the essential structural invariants needed after a
move.
In `@tests/CGAL_integration_test.cpp`:
- Around line 92-100: Extract the repeated labelled-point verification loop into
a shared test helper, such as expect_labels_preserved, reusable by both
scenarios in CGAL_integration_test.cpp and Parallel_triangulation_test.cpp. The
helper should look up each point with foliated_triangulations::find_vertex<3>,
assert the vertex exists, and verify its info() matches the expected label;
replace each duplicated loop with calls to this helper.
In `@tests/Parallel_triangulation_test.cpp`:
- Around line 139-167: Document or explicitly validate the lock-grid colocation
assumption in the contention setup around candidate, contested_point, and
could_lock_zone. Prefer asserting that both points map to the same lock-grid
cell using the existing grid-cell/index API; otherwise add a concise comment
explaining the dependency on locking_box resolution and GV_BOUNDING_BOX_SIZE so
failures clearly identify the violated prerequisite.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c0d76de-b078-4b88-9dd6-2c901752ce8e
📒 Files selected for processing (18)
.github/CONTRIBUTING.mdCMakeLists.txtJustfileREADME.mdcmake/StandardProjectSettings.cmakedocs/api-boundary.mddocs/cgal-integration.mdinclude/Foliated_triangulation.hppinclude/Settings.hppinclude/Triangulation_traits.hppinclude/Utilities.hppscripts/clang-tidy.shscripts/slurm.shtests/CGAL_benchmark.cpptests/CGAL_integration_test.cpptests/CMakeLists.txttests/Parallel_triangulation_test.cpptests/Tetrahedron_test.cpp
💤 Files with no reviewable changes (2)
- cmake/StandardProjectSettings.cmake
- include/Settings.hpp
- keep move validation on structural, TDS, causal, and tracked-count checks - provide opt-in full derived-cache diagnostics outside hot paths BREAKING CHANGE: is_correct() now checks essential structural and causal invariants only. Call is_correct_with_diagnostics() when full derived-cache consistency is required.
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 `@include/Foliated_triangulation.hpp`:
- Around line 1377-1392: Update has_consistent_structure() to directly verify
that the cached m_cells, m_faces, and m_edges counts match the corresponding
Delaunay topology counts, while retaining the existing partition and invariant
checks. Ensure these O(1) cache-size comparisons are included in the fast
validation path used by is_structurally_correct() and check_move().
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d81f899-794c-49d0-a34a-4ccd43e7f001
📒 Files selected for processing (9)
docs/cgal-integration.mddocs/ergodic-moves.mdinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Manifold.hpptests/Apply_move_test.cpptests/CGAL_integration_test.cpptests/CGAL_test_helpers.hpptests/Parallel_triangulation_test.cpp
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.
BREAKING CHANGE: CDT++ now requires exactly CGAL 6.2, GCC 13.3 or Clang 22 on those toolchains, and removes the legacy Face_handle_t alias.
Closes #102
Summary by CodeRabbit