refactor!: flatten triangulation modules into focused APIs#399
Merged
Conversation
- Move Delaunay-facing APIs from the old triangulation facade to crate-root modules and focused preludes such as construction, insertion, flips, repair, validation, and delaunayize. - Split generic Triangulation behavior into construction, insertion, query, orientation, repair, and validation modules with colocated tests and errors. - Keep the broad prelude for exploratory use while making focused preludes orthogonal and workflow-specific. - Refresh docs, examples, benchmarks, and API export tests for the new module layout and 7,500-vertex 3D debug-scale default. BREAKING CHANGE: Delaunay workflow imports now use crate-root modules and focused preludes instead of nested triangulation workflow paths. For example, use delaunay::prelude::construction::* instead of delaunay::prelude::triangulation::construction::*, and use root modules such as delaunay::construction, delaunay::repair, and delaunay::validation. BREAKING CHANGE: BistellarFlips now uses the const dimension as its trait parameter and exposes scalar and vertex-data types as associated types. Closes #381
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughMassive rename/refactor from triangulation to delaunay: adds new Delaunay/core modules, restructures preludes/imports, updates docs/examples/tests/benchmarks/CI, introduces query/repair/validation/orientation features, adds error variants and flips trait change, and adjusts large-scale 3D defaults (8000→7500). ChangesDelaunay namespace migration and API surface updates
Sequence Diagram(s)sequenceDiagram
participant User as User code
participant Builder as DelaunayTriangulationBuilder
participant DT as DelaunayTriangulation
participant Core as Triangulation/Tds
participant Flips as BistellarFlips
User->>Builder: build via prelude::construction
Builder->>DT: construct
User->>DT: insert/remove
DT->>Flips: repair with k2/k3 or inverse k1
DT->>Core: mutate Tds
DT->>DT: validate (L1–L4) + orientation normalize
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 1050 |
🟢 Coverage 89.74% diff coverage · -0.01% coverage variation
Metric Results Coverage variation ✅ -0.01% coverage variation (-1.00%) Diff coverage ✅ 89.74% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (d89961c) 61626 55755 90.47% Head commit (d8c8666) 61899 (+273) 55993 (+238) 90.46% (-0.01%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#399) 11291 10133 89.74% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
semgrep.yaml (1)
557-579: ⚡ Quick winInclude
insertionin prelude-enforcement regexes.Both prelude-preference rules miss
delaunay::insertion::..., so deep imports from the insertion API won’t be flagged in examples/doctests after this namespace migration.Proposed patch
- - pattern-regex: '^\s*use\s+delaunay::(core|builder|construction|delaunayize|diagnostics|flips|geometry|repair|topology|validation)::' + - pattern-regex: '^\s*use\s+delaunay::(core|builder|construction|delaunayize|diagnostics|flips|geometry|insertion|repair|topology|validation)::' @@ - - pattern-regex: '^\s*//[/!]\s*(?:#\s*)?use\s+delaunay::(core|builder|construction|delaunayize|diagnostics|flips|geometry|repair|topology|validation)::' + - pattern-regex: '^\s*//[/!]\s*(?:#\s*)?use\s+delaunay::(core|builder|construction|delaunayize|diagnostics|flips|geometry|insertion|repair|topology|validation)::'🤖 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 `@semgrep.yaml` around lines 557 - 579, Update both regex lists that enumerate allowed deep-module namespaces to include the insertion namespace so deep imports from the insertion API are flagged; specifically add "insertion" to the alternation in the pattern-regex that currently matches '^\s*use\s+delaunay::(core|builder|construction|delaunayize|diagnostics|flips|geometry|repair|topology|validation)::' and also add "insertion" to the doctest/commented-use pattern-regex '^\s*//[/!]\s*(?:#\s*)?use\s+delaunay::(core|builder|construction|delaunayize|diagnostics|flips|geometry|repair|topology|validation)::' so both the rule for normal imports and the rule id delaunay.rust.prefer-prelude-imports-in-delaunay-doctests will catch insertion::* uses.src/core/construction.rs (1)
174-183: ⚡ Quick winAdd the required literature citation for the bootstrap/orientation strategy.
The conditioning note is useful, but this new construction path still needs a numbered
REFERENCES.mdcitation near therobust_orientationdesign note so the algorithm/predicate choice is traceable in the way the repo expects. As per coding guidelines, "Algorithms in Rust must cite their source (Shewchuk, Bowyer–Watson, Edelsbrunner, Preparata–Shamos, etc.) inREFERENCES.mdand document their conditioning behaviour" and "Reference literature viaREFERENCES.mdnumbered citations in Rust code and documentation".🤖 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 `@src/core/construction.rs` around lines 174 - 183, Add a numbered literature citation in REFERENCES.md for the bootstrap/orientation strategy referenced by robust_orientation (e.g., Shewchuk or another primary source), then insert the corresponding numbered reference marker next to the "robust_orientation" design note in the Build initial D-simplex doc comment inside construction.rs (the method that creates the initial TDS and calls robust_orientation). Ensure the doc comment includes the REFERENCES.md citation number and a short bibliographic key so readers can trace the algorithm/predicate choice per project guidelines.src/delaunay/locality.rs (1)
29-34: ⚡ Quick winKeep these seed-set helpers crate-private unless you intend to support them as public API.
Making these helpers
publeaksVec<SimplexKey>/FastHashSet<SimplexKey>bookkeeping into the public surface and makes future locality-internals refactors needlessly breaking. If this widening is only for cross-module reuse,pub(crate)is enough.Suggested visibility narrowing
-pub fn accumulate_live_simplex_seeds<T, U, V, const D: usize>( +pub(crate) fn accumulate_live_simplex_seeds<T, U, V, const D: usize>( ... -pub fn retain_live_simplex_seeds<T, U, V, const D: usize>( +pub(crate) fn retain_live_simplex_seeds<T, U, V, const D: usize>( ... -pub fn clear_simplex_seed_set( +pub(crate) fn clear_simplex_seed_set(Also applies to: 79-99
🤖 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 `@src/delaunay/locality.rs` around lines 29 - 34, Change the public visibility of the seed-set helper(s) to crate-private to avoid leaking internal bookkeeping types: replace the pub on accumulate_live_simplex_seeds (and the other helper functions mentioned around lines 79-99) with pub(crate) (or remove pub entirely if only used in the same module) so Vec<SimplexKey>/FastHashSet<SimplexKey> remain internal to the crate and future refactors won't be breaking.src/delaunay/query.rs (1)
308-315: ⚡ Quick winDrop
constfrom the mutating setters/helpers.These methods do not benefit from const evaluation, but they do freeze the API into const-context usability and make later validation/cache-invalidation changes harder than they need to be. Plain
fnkeeps the surface more flexible.Suggested change
- pub(crate) const fn invalidate_locate_hint_cache(&mut self) { + pub(crate) fn invalidate_locate_hint_cache(&mut self) { self.insertion_state.last_inserted_simplex = None; } ... - pub const fn set_delaunay_repair_policy(&mut self, policy: DelaunayRepairPolicy) { + pub fn set_delaunay_repair_policy(&mut self, policy: DelaunayRepairPolicy) { self.insertion_state.delaunay_repair_policy = policy; } ... - pub const fn set_delaunay_check_policy(&mut self, policy: DelaunayCheckPolicy) { + pub fn set_delaunay_check_policy(&mut self, policy: DelaunayCheckPolicy) { self.insertion_state.delaunay_check_policy = policy; } ... - pub const fn set_global_topology(&mut self, global_topology: GlobalTopology<D>) { + pub fn set_global_topology(&mut self, global_topology: GlobalTopology<D>) { self.tri.set_global_topology(global_topology); }As per coding guidelines, "Use
const fnin Rust for pure-math helpers (sign_to_orientation,sign_to_insphere, coordinate conversions) where the inputs allow. Do not twist mutating APIs intoconst fnfor its own sake".Also applies to: 430-445, 534-535
🤖 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 `@src/delaunay/query.rs` around lines 308 - 315, The two mutating helper methods are incorrectly declared const; change the signatures of pub(crate) const fn invalidate_locate_hint_cache(...) and pub(crate) const fn invalidate_repair_caches(...) to plain pub(crate) fn so they can mutate state freely; also scan other mutating helpers (the additional mutating setters mentioned around the other ranges) and remove const from their signatures as well, then rebuild to ensure no const-context usage remains.src/delaunay/repair.rs (2)
776-779: 💤 Low valueRemove dead code or clarify intent.
Lines 697-701 save
rebuild_repair_policyandrebuild_check_policy, but they're immediately overwritten and never used. Line 779 explicitly discards them withlet _ = .... If these are truly unused, remove the assignments entirely rather than suppressing the warning.♻️ Suggested cleanup
- // During rebuild, force local repair after every insertion. We'll restore the caller's - // policies after we have a repaired candidate. - let rebuild_repair_policy = candidate.insertion_state.delaunay_repair_policy; - let rebuild_check_policy = candidate.insertion_state.delaunay_check_policy; + // During rebuild, force local repair after every insertion. candidate.insertion_state.delaunay_repair_policy = DelaunayRepairPolicy::EveryInsertion; candidate.insertion_state.delaunay_check_policy = DelaunayCheckPolicy::EndOnly;And remove line 779:
- // Restore prior rebuild-only policies (kept for completeness; currently overwritten above). - let _ = (rebuild_repair_policy, rebuild_check_policy);🤖 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 `@src/delaunay/repair.rs` around lines 776 - 779, The saved variables rebuild_repair_policy and rebuild_check_policy are created but never used (then explicitly discarded), so remove the unused assignments where those policies are stored and delete the final discard line `let _ = (rebuild_repair_policy, rebuild_check_policy);`; if the original intent was to preserve prior policies, instead replace the no-op with actual restoration logic referencing those variables (e.g., restoring into candidate.insertion_state or the relevant policy fields) or add a clear comment explaining why they must be preserved rather than removed.
41-66: 💤 Low valueRecursion guard tracks depth but doesn't enforce a limit.
The
HeuristicRebuildRecursionGuardincrements and restoresHEURISTIC_REBUILD_DEPTH, but I don't see where the depth value is checked to actually prevent unbounded recursion. The guard is created at line 676 but the depth isn't tested before proceeding.If this is intentional tracking for diagnostics only (not enforcement), consider documenting that in the struct's doc comment. If enforcement is intended, add a check like:
fn enter() -> Result<Self, DelaunayRepairError> { let prior_depth = HEURISTIC_REBUILD_DEPTH.with(|depth| { let prior = depth.get(); if prior >= MAX_HEURISTIC_REBUILD_DEPTH { return Err(/* error */); } depth.set(prior.saturating_add(1)); Ok(prior) })?; Ok(Self { prior_depth }) }🤖 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 `@src/delaunay/repair.rs` around lines 41 - 66, The recursion guard currently only increments/restores HEURISTIC_REBUILD_DEPTH in HeuristicRebuildRecursionGuard::enter and Drop but never enforces a limit; either document that the guard is diagnostics-only in the struct doc comment or change enter to enforce a max depth by checking HEURISTIC_REBUILD_DEPTH before incrementing (use a MAX_HEURISTIC_REBUILD_DEPTH constant) and return a Result<HeuristicRebuildRecursionGuard, DelaunayRepairError> on overflow; update all call sites that construct the guard (where enter() is used) to handle the Result and ensure Drop still restores prior_depth via the existing Drop impl.src/delaunay/validation.rs (1)
183-194: 💤 Low valueSimplify unreachable branch.
In
from_optional_every, theSome(every)arm whereevery > 0(sinceSome(0)is matched earlier) callsNonZeroUsize::new(every). This will always returnSome(_)becauseeveryis guaranteed non-zero at this point. Theelsebranch returningSelf::Neveris unreachable.♻️ Suggested simplification
pub const fn from_optional_every(validate_every: Option<usize>) -> Self { match validate_every { None | Some(0) => Self::Never, Some(every) => { - if let Some(every) = NonZeroUsize::new(every) { - Self::EveryN(every) - } else { - Self::Never - } + // SAFETY: every > 0 is guaranteed by the match arm + Self::EveryN( + // Use expect in const context when NonZeroUsize::new_unchecked + // isn't available, or use unwrap() if panicking is acceptable + match NonZeroUsize::new(every) { + Some(n) => n, + None => unreachable!(), + } + ) } } }Or simply keep the current form with a comment noting the branch is logically unreachable but required by the compiler for
const fnexhaustiveness.🤖 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 `@src/delaunay/validation.rs` around lines 183 - 194, The match in from_optional_every has an unreachable else branch when matching Some(every) because every>0; replace the inner if-let with a direct construction using NonZeroUsize::new_unchecked to avoid the unreachable branch (e.g., in the Some(every) arm return Self::EveryN(unsafe { NonZeroUsize::new_unchecked(every) })), or alternatively keep the current code but add a brief comment in from_optional_every explaining that the else branch is logically unreachable but present for exhaustiveness in const fn.
🤖 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 `@src/core/adjacency.rs`:
- Line 354: Update the obsolete import path: replace the test import line that
currently references crate::triangulation::DelaunayTriangulation with the
re-exported symbol at the crate root by using crate::DelaunayTriangulation
instead; locate the import statement in src/core/adjacency.rs (the line that
imports DelaunayTriangulation) and change it to use crate::DelaunayTriangulation
so it matches the refactored module exports and existing test patterns.
In `@src/core/orientation.rs`:
- Around line 327-354: The code currently logs residual negative-orientation
simplices after simplices_require_positive_orientation_promotion() and then
proceeds, which accepts an invalid triangulation; instead, return a typed
non-convergence error so callers know promotion failed. Replace the debug-only
path in normalize_and_promote_positive_orientation (the block after evaluating
orientations via evaluate_simplex_orientation_for_context) with constructing and
returning an Err variant (e.g., an OrientationPromotionNonConvergence or similar
error added to the function's Result error enum) that includes residual_count
and a sample of SimplexKey values (sampled), do not call
canonicalize_global_orientation_sign() on failure, and ensure the error type
ties to Tds::is_valid/validate_geometric_simplex_orientation semantics so
callers can detect and handle non-convergence.
In `@src/core/query.rs`:
- Around line 215-222: Replace the panic in boundary_facets by adding a fallible
API: implement a new try_boundary_facets(&self) -> Result<BoundaryFacetsIter<'_,
K::Scalar, U, V, D>, QueryError> that calls
self.tds.build_facet_to_simplices_map().map_err(|e|
QueryError::TriangulationCorrupted(format!("{}", e)))? and returns
BoundaryFacetsIter::new(&self.tds, facet_map) on success; update or deprecate
the existing boundary_facets() to either call
try_boundary_facets().expect_with_doc() (if you must keep it) or remove it so
callers use the Result-returning try_boundary_facets; ensure you define/extend a
QueryError enum variant (e.g. TriangulationCorrupted) to represent the mapped
error and use the build_facet_to_simplices_map and BoundaryFacetsIter symbols to
locate the change.
In `@src/core/repair.rs`:
- Around line 775-874: Add an explicit removal budget parameter and exhaustion
error: change the public API of repair_local_facet_issues to accept
max_simplices_removed: usize and return a typed budget-exhaustion error (e.g.
add InsertionError::MaxSimplicesRemovedExceeded or similar). Enforce the budget
by passing it into repair_local_facet_issues_with_frontier (or by checking the
Outcome.removed_count against max_simplices_removed immediately after that call)
and early-return an Err(InsertionError::MaxSimplicesRemovedExceeded) when the
budget would be exceeded; ensure downstream logic
(repair_neighbors_after_local_simplex_removal, assign_incident_simplices,
validate) only runs when within budget. Update any call sites to supply the new
parameter and add the new error variant handling in the InsertionError enum and
its conversions.
In `@src/core/tds.rs`:
- Line 6915: Replace the long import path for the DelaunayTriangulation type
with the module's re-export: change the use statement that references
crate::triangulation::DelaunayTriangulation to use crate::DelaunayTriangulation
so the test imports match the shorter, consistent re-export style; update the
use declaration where DelaunayTriangulation is imported.
In `@src/delaunay/insertion.rs`:
- Around line 1-5: Update the module-level documentation in insertion.rs to add
numbered inline citations (e.g. [1], [2], [3], [4]) next to the described
algorithms—cavity insertion, flip-based repair, robust fallback, and fan
retriangulation—and add matching bibliographic entries to REFERENCES.md for
Shewchuk, Bowyer–Watson, Edelsbrunner, and any other cited sources; explicitly
document the conditioning model for each workflow (which steps require exact
geometric predicates vs. heuristic/local repairs), mark where correctness
depends on exact predicates (e.g. in the cavity identification and in_sphere
tests used by insert_vertex/repair_cavity/flip_retriangulation), and note where
robust fallback or non-exact heuristics are invoked (e.g. the robust fallback
path and fan retriangulation routines) so readers know which parts are proved vs
heuristic.
---
Nitpick comments:
In `@semgrep.yaml`:
- Around line 557-579: Update both regex lists that enumerate allowed
deep-module namespaces to include the insertion namespace so deep imports from
the insertion API are flagged; specifically add "insertion" to the alternation
in the pattern-regex that currently matches
'^\s*use\s+delaunay::(core|builder|construction|delaunayize|diagnostics|flips|geometry|repair|topology|validation)::'
and also add "insertion" to the doctest/commented-use pattern-regex
'^\s*//[/!]\s*(?:#\s*)?use\s+delaunay::(core|builder|construction|delaunayize|diagnostics|flips|geometry|repair|topology|validation)::'
so both the rule for normal imports and the rule id
delaunay.rust.prefer-prelude-imports-in-delaunay-doctests will catch
insertion::* uses.
In `@src/core/construction.rs`:
- Around line 174-183: Add a numbered literature citation in REFERENCES.md for
the bootstrap/orientation strategy referenced by robust_orientation (e.g.,
Shewchuk or another primary source), then insert the corresponding numbered
reference marker next to the "robust_orientation" design note in the Build
initial D-simplex doc comment inside construction.rs (the method that creates
the initial TDS and calls robust_orientation). Ensure the doc comment includes
the REFERENCES.md citation number and a short bibliographic key so readers can
trace the algorithm/predicate choice per project guidelines.
In `@src/delaunay/locality.rs`:
- Around line 29-34: Change the public visibility of the seed-set helper(s) to
crate-private to avoid leaking internal bookkeeping types: replace the pub on
accumulate_live_simplex_seeds (and the other helper functions mentioned around
lines 79-99) with pub(crate) (or remove pub entirely if only used in the same
module) so Vec<SimplexKey>/FastHashSet<SimplexKey> remain internal to the crate
and future refactors won't be breaking.
In `@src/delaunay/query.rs`:
- Around line 308-315: The two mutating helper methods are incorrectly declared
const; change the signatures of pub(crate) const fn
invalidate_locate_hint_cache(...) and pub(crate) const fn
invalidate_repair_caches(...) to plain pub(crate) fn so they can mutate state
freely; also scan other mutating helpers (the additional mutating setters
mentioned around the other ranges) and remove const from their signatures as
well, then rebuild to ensure no const-context usage remains.
In `@src/delaunay/repair.rs`:
- Around line 776-779: The saved variables rebuild_repair_policy and
rebuild_check_policy are created but never used (then explicitly discarded), so
remove the unused assignments where those policies are stored and delete the
final discard line `let _ = (rebuild_repair_policy, rebuild_check_policy);`; if
the original intent was to preserve prior policies, instead replace the no-op
with actual restoration logic referencing those variables (e.g., restoring into
candidate.insertion_state or the relevant policy fields) or add a clear comment
explaining why they must be preserved rather than removed.
- Around line 41-66: The recursion guard currently only increments/restores
HEURISTIC_REBUILD_DEPTH in HeuristicRebuildRecursionGuard::enter and Drop but
never enforces a limit; either document that the guard is diagnostics-only in
the struct doc comment or change enter to enforce a max depth by checking
HEURISTIC_REBUILD_DEPTH before incrementing (use a MAX_HEURISTIC_REBUILD_DEPTH
constant) and return a Result<HeuristicRebuildRecursionGuard,
DelaunayRepairError> on overflow; update all call sites that construct the guard
(where enter() is used) to handle the Result and ensure Drop still restores
prior_depth via the existing Drop impl.
In `@src/delaunay/validation.rs`:
- Around line 183-194: The match in from_optional_every has an unreachable else
branch when matching Some(every) because every>0; replace the inner if-let with
a direct construction using NonZeroUsize::new_unchecked to avoid the unreachable
branch (e.g., in the Some(every) arm return Self::EveryN(unsafe {
NonZeroUsize::new_unchecked(every) })), or alternatively keep the current code
but add a brief comment in from_optional_every explaining that the else branch
is logically unreachable but present for exhaustiveness in const fn.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 12cf1616-bbf3-471f-8ab2-8f801081afec
📒 Files selected for processing (105)
README.mdbenches/PERFORMANCE_RESULTS.mdbenches/README.mdbenches/ci_performance_suite.rsbenches/profiling_suite.rsbenches/tds_clone.rsbenches/topology_guarantee_construction.rsdocs/ORIENTATION_SPEC.mddocs/api_design.mddocs/archive/issue_204_investigation.mddocs/archive/todo_2026-04-23.mddocs/archive/topology_integration_design_historical.mddocs/code_organization.mddocs/dev/debug_env_vars.mddocs/dev/rust.mddocs/dev/tooling-alignment.mddocs/diagnostics.mddocs/invariants.mddocs/limitations.mddocs/numerical_robustness_guide.mddocs/production_review_remediation_checklist.mddocs/roadmap.mddocs/topology.mddocs/validation.mddocs/workflows.mdexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/numerical_robustness.rsexamples/topology_editing.rsexamples/triangulation_and_hull.rsjustfilesemgrep.yamlsrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/collections/key_maps.rssrc/core/collections/secondary_maps.rssrc/core/construction.rssrc/core/facet.rssrc/core/insertion.rssrc/core/operations.rssrc/core/orientation.rssrc/core/query.rssrc/core/repair.rssrc/core/simplex.rssrc/core/tds.rssrc/core/traits/data_type.rssrc/core/traits/facet_cache.rssrc/core/triangulation.rssrc/core/util/deduplication.rssrc/core/util/delaunay_validation.rssrc/core/util/facet_keys.rssrc/core/util/facet_utils.rssrc/core/util/jaccard.rssrc/core/validation.rssrc/core/vertex.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/diagnostics.rssrc/delaunay/flips.rssrc/delaunay/insertion.rssrc/delaunay/locality.rssrc/delaunay/query.rssrc/delaunay/repair.rssrc/delaunay/serialization.rssrc/delaunay/triangulation.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/kernel.rssrc/geometry/quality.rssrc/geometry/util/measures.rssrc/geometry/util/triangulation_generation.rssrc/lib.rssrc/topology/manifold.rssrc/triangulation.rssrc/triangulation/validation.rstests/README.mdtests/allocation_api.rstests/circumsphere_debug_tools.rstests/dedup_batch_construction.rstests/delaunay_edge_cases.rstests/delaunay_incremental_insertion.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/euler_characteristic.rstests/example_workflows.rstests/insert_with_statistics.rstests/large_scale_debug.rstests/pachner_roundtrip.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.rstests/proptest_euler_characteristic.rstests/proptest_flips.rstests/proptest_orientation.rstests/proptest_triangulation.rstests/proptest_vertex.rstests/public_topology_api.rstests/regressions.rstests/semgrep/src/project_rules/rust_style.rstests/serialization_vertex_preservation.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
💤 Files with no reviewable changes (2)
- src/triangulation/validation.rs
- src/triangulation.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #399 +/- ##
==========================================
- Coverage 90.46% 90.44% -0.02%
==========================================
Files 61 71 +10
Lines 61422 61695 +273
==========================================
+ Hits 55565 55803 +238
- Misses 5857 5892 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Return `QueryError` from boundary facet queries instead of panicking when facet-map construction detects corrupted topology. - Add a simplex-removal budget to local facet repair and preserve budget-exceeded failures through insertion, construction, and flip-wiring error paths. - Report orientation-promotion non-convergence as a typed validation failure with residual simplex diagnostics. - Update examples, preludes, benchmarks, and workflow/tooling pins for the fallible query and repair APIs. BREAKING CHANGE: `Triangulation::boundary_facets` and `DelaunayTriangulation::boundary_facets` now return `Result<BoundaryFacetsIter, QueryError>`. BREAKING CHANGE: `Triangulation::repair_local_facet_issues` now requires a `max_simplices_removed` argument and can return `InsertionError::MaxSimplicesRemovedExceeded`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/core/algorithms/incremental_insertion.rs (2)
4227-4228: ⚡ Quick winMove this test import to the renamed Delaunay path.
The test still pulls
DelaunayTriangulationfromcrate::triangulation, so it is not validating the flatteneddelaunayAPI surface this PR is introducing. If the legacy path is removed, this breaks; if it remains, the test stops catching stale imports.Based on learnings: In the acgetchell/delaunay pre-v1.0.0 Rust crate, treat public API renames as intentionally breaking changes. When code review finds renamed/moved public items, do not recommend adding deprecated compatibility aliases, deprecated re-exports, or shim types/functions.
🤖 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 `@src/core/algorithms/incremental_insertion.rs` around lines 4227 - 4228, The test is importing DelaunayTriangulation from the legacy path crate::triangulation; update the test to import the type from the new flattened API path (e.g., crate::delaunay::DelaunayTriangulation or the publicly re-exported symbol in crate::delaunay) so it validates the renamed public API surface instead of relying on the old module; locate the import statements referencing DelaunayTriangulation and vertex and change their paths to the renamed delaunay module exports.
1430-1444: ⚡ Quick winAdd a regression test for
MaxSimplicesRemovedExceeded.This new public variant now feeds both
InsertionErrorSummary::fromandInsertionError::is_retryable(), but the test module does not exercise that branch. A small case asserting the summarykindandretryable == falsewould pin the intended contract.As per coding guidelines "Unit tests in Rust must cover known values, error paths, and dimension-generic correctness".
🤖 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 `@src/core/algorithms/incremental_insertion.rs` around lines 1430 - 1444, Add a unit test that exercises the new MaxSimplicesRemovedExceeded variant and verifies how it flows through InsertionErrorSummary::from and InsertionError::is_retryable(): construct or simulate an insertion error that produces Triangulation::repair_local_facet_issues emitting MaxSimplicesRemovedExceeded (with specific max_simplices_removed and attempted values), call InsertionErrorSummary::from on that error and assert the summary.kind matches the expected variant, and assert InsertionError::is_retryable() returns false; add this test to the existing test module covering error paths and dimension-generic behavior so the regression is pinned.src/core/tds.rs (1)
1233-1234: ⚡ Quick winAdd regression coverage for the new
OrientationPromotionNonConvergencemapping.
TriangulationValidationErrorKindand itsFrom<&TriangulationValidationError>mapping were extended, but the existing “preserves all variants” test should also include this new variant to keep that contract enforced.As per coding guidelines, "Unit tests in Rust must cover known values, error paths, and dimension-generic correctness."
Also applies to: 1255-1257
🤖 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 `@src/core/tds.rs` around lines 1233 - 1234, Update the unit test that verifies the From<&TriangulationValidationError> -> TriangulationValidationErrorKind mapping (the "preserves all variants" test) to include the new OrientationPromotionNonConvergence variant: construct or reference a TriangulationValidationError representing OrientationPromotionNonConvergence and assert it maps to TriangulationValidationErrorKind::OrientationPromotionNonConvergence; ensure the test touches the TriangulationValidationErrorKind enum and the From<&TriangulationValidationError> impl so the new variant is covered alongside the other existing variants.
🤖 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 `@src/core/algorithms/flips.rs`:
- Line 9503: The import in tests references the removed facade path; change the
use statement to import the struct from its new module by replacing the old
import of DelaunayTriangulation from crate::triangulation with the new path
crate::DelaunayTriangulation so tests refer to the relocated struct (update the
use line that currently names DelaunayTriangulation).
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 602-610: The current conversion collapses
TriangulationConstructionError::LocalRepairBudgetExceeded into
CavityFillingError::InitialSimplexConstruction::UnexpectedInsertionStage with
only a message; instead preserve the typed budget error so upstream code can
pattern-match. Modify the conversion logic that handles
TriangulationConstructionError::LocalRepairBudgetExceeded to either (a) add a
new CavityFillingError variant (e.g., LocalRepairBudgetExceeded {
max_simplices_removed, attempted }) and map to that, or (b) change
InitialSimplexConstruction to wrap the original TriangulationConstructionError
(or an enum payload) so the max_simplices_removed and attempted fields are
retained; update the match arm for
TriangulationConstructionError::LocalRepairBudgetExceeded to populate those
fields rather than formatting a string. Ensure tests and callers that
pattern-match on CavityFillingError::InitialSimplexConstruction or the new
variant are updated accordingly.
In `@src/core/repair.rs`:
- Around line 254-266: The docstring for remove_vertex and related docs for
repair_local_facet_issues lack citations and conditioning notes; update the
comment blocks for remove_vertex and the documentation around
repair_local_facet_issues to cite the algorithm sources (e.g., Shewchuk,
Bowyer–Watson, Edelsbrunner, Preparata–Shamos) by adding entries to
REFERENCES.md and linking them in the function docs, and add a short paragraph
describing numerical/topological fragility and when the fan-triangulation and
radius-ratio heuristic may fail (e.g., degenerate or nearly-coplanar cavities,
small numeric epsilons, inverted simplices), plus suggested mitigations (robust
predicates, epsilon thresholds, fallbacks). Ensure references in COMMENTS
mention REFERENCES.md and include the function names remove_vertex and
repair_local_facet_issues so reviewers can find the changes.
- Around line 397-402: After calling self.tds.remove_vertex(vertex_key) and
before returning Ok(simplices_removed), run the TDS and triangulation invariant
checks (call Tds::is_valid for Levels 1–3 on self.tds and
DelaunayTriangulation::is_valid for Level 4 on self) and if either check fails
convert that failure into the existing InvariantError path (i.e., return
Err(...) so the transaction/snapshot rollback runs); ensure you reference the
same InvariantError variant style used earlier (e.g., InvariantError::Tds /
appropriate triangulation variant) so the error mapping remains consistent with
the surrounding code.
- Around line 567-569: Update the stale import paths in the doctests and test
module: change the import of InsertionError to use
delaunay::prelude::insertion::InsertionError (instead of
prelude::triangulation), and replace the retired facade path
crate::triangulation::DelaunayTriangulation with the crate-root export
crate::DelaunayTriangulation; keep the other imports like
DelaunayTriangulationConstructionError, FacetIssuesMap, and vertex unchanged so
the doctest and test module compile against the current crate exports.
---
Nitpick comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 4227-4228: The test is importing DelaunayTriangulation from the
legacy path crate::triangulation; update the test to import the type from the
new flattened API path (e.g., crate::delaunay::DelaunayTriangulation or the
publicly re-exported symbol in crate::delaunay) so it validates the renamed
public API surface instead of relying on the old module; locate the import
statements referencing DelaunayTriangulation and vertex and change their paths
to the renamed delaunay module exports.
- Around line 1430-1444: Add a unit test that exercises the new
MaxSimplicesRemovedExceeded variant and verifies how it flows through
InsertionErrorSummary::from and InsertionError::is_retryable(): construct or
simulate an insertion error that produces
Triangulation::repair_local_facet_issues emitting MaxSimplicesRemovedExceeded
(with specific max_simplices_removed and attempted values), call
InsertionErrorSummary::from on that error and assert the summary.kind matches
the expected variant, and assert InsertionError::is_retryable() returns false;
add this test to the existing test module covering error paths and
dimension-generic behavior so the regression is pinned.
In `@src/core/tds.rs`:
- Around line 1233-1234: Update the unit test that verifies the
From<&TriangulationValidationError> -> TriangulationValidationErrorKind mapping
(the "preserves all variants" test) to include the new
OrientationPromotionNonConvergence variant: construct or reference a
TriangulationValidationError representing OrientationPromotionNonConvergence and
assert it maps to
TriangulationValidationErrorKind::OrientationPromotionNonConvergence; ensure the
test touches the TriangulationValidationErrorKind enum and the
From<&TriangulationValidationError> impl so the new variant is covered alongside
the other existing variants.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: e0c9c35b-e2a4-48d6-9d47-29c349c454d1
📒 Files selected for processing (32)
.github/workflows/benchmarks.yml.github/workflows/ci.yml.github/workflows/generate-baseline.yml.github/workflows/semgrep-sarif.ymlREFERENCES.mdbenches/ci_performance_suite.rsdocs/dev/tooling-alignment.mdexamples/triangulation_and_hull.rssemgrep.yamlsrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/boundary.rssrc/core/construction.rssrc/core/orientation.rssrc/core/query.rssrc/core/repair.rssrc/core/tds.rssrc/core/traits/boundary_analysis.rssrc/core/validation.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/insertion.rssrc/delaunay/query.rssrc/delaunay/repair.rssrc/delaunay/validation.rssrc/lib.rstests/example_workflows.rstests/prelude_exports.rstests/proptest_triangulation.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
✅ Files skipped from review due to trivial changes (6)
- .github/workflows/semgrep-sarif.yml
- .github/workflows/generate-baseline.yml
- src/core/traits/boundary_analysis.rs
- REFERENCES.md
- src/core/adjacency.rs
- docs/dev/tooling-alignment.md
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/trait_bound_ergonomics.rs
- benches/ci_performance_suite.rs
- src/core/construction.rs
- semgrep.yaml
- src/core/orientation.rs
- src/delaunay/repair.rs
- src/delaunay/validation.rs
- src/delaunay/insertion.rs
- src/delaunay/query.rs
- src/lib.rs
- Preserve local repair budget exhaustion as a structured initial-simplex error so callers can inspect the attempted and maximum removal counts. - Validate TDS and triangulation invariants after vertex removal so invalid repair states trigger transactional rollback. - Clarify repair documentation around cavity fragility, references, and flattened public import paths.
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: Delaunay workflow imports now use crate-root modules and focused preludes instead of nested triangulation workflow paths. For example, use delaunay::prelude::construction::* instead of
delaunay::prelude::triangulation::construction::*, and use root modules such as delaunay::construction, delaunay::repair, and delaunay::validation. BREAKING CHANGE: BistellarFlips now uses the const dimension as its trait parameter and exposes scalar and vertex-data types as associated types.
Closes #381