From 097128021d0265d1400af1d2ded05df3b71ec453 Mon Sep 17 00:00:00 2001 From: Adam Getchell Date: Tue, 28 Apr 2026 19:58:05 -0700 Subject: [PATCH 1/4] refactor: type repair diagnostics and harden invariants (#332) - Replace stringified flip-repair skip samples with typed diagnostic context. - Make vertex removal transactional across post-removal repair and orientation canonicalization. - Deprecate DelaunayTriangulation::as_triangulation_mut ahead of removal in v0.8.0. - Use scale-aware degeneracy checks for low-dimensional simplex and facet measures. - Add regression and property coverage for rollback behavior, typed diagnostics, and scaled valid measures. - Tolerate throughput formatting precision in benchmark baseline round-trip tests. --- scripts/tests/test_benchmark_utils.py | 6 +- src/core/algorithms/flips.rs | 211 ++++++++++++++++---------- src/core/tds.rs | 20 +-- src/core/triangulation.rs | 18 +-- src/geometry/util/measures.rs | 172 +++++++++++---------- src/lib.rs | 6 +- src/triangulation/builder.rs | 2 +- src/triangulation/delaunay.rs | 210 ++++++++++++++----------- src/triangulation/delaunayize.rs | 43 +++--- tests/proptest_geometry.rs | 57 +++++++ tests/proptest_triangulation.rs | 4 +- 11 files changed, 453 insertions(+), 296 deletions(-) diff --git a/scripts/tests/test_benchmark_utils.py b/scripts/tests/test_benchmark_utils.py index defbe9b5..2165e6b4 100644 --- a/scripts/tests/test_benchmark_utils.py +++ b/scripts/tests/test_benchmark_utils.py @@ -1070,9 +1070,9 @@ def test_written_baseline_round_trips_through_parser(self, mock_git, tmp_path) - assert actual.time_mean == pytest.approx(expected.time_mean) assert actual.time_high == pytest.approx(expected.time_high) assert actual.time_unit == expected.time_unit - assert actual.throughput_low == expected.throughput_low - assert actual.throughput_mean == expected.throughput_mean - assert actual.throughput_high == expected.throughput_high + assert actual.throughput_low == pytest.approx(expected.throughput_low) + assert actual.throughput_mean == pytest.approx(expected.throughput_mean) + assert actual.throughput_high == pytest.approx(expected.throughput_high) assert actual.throughput_unit == expected.throughput_unit mock_git.assert_called_once() diff --git a/src/core/algorithms/flips.rs b/src/core/algorithms/flips.rs index 4511a926..da700bfb 100644 --- a/src/core/algorithms/flips.rs +++ b/src/core/algorithms/flips.rs @@ -4344,17 +4344,48 @@ struct RepairDiagnostics { cycle_detections: usize, cycle_samples: Vec, inserted_simplex_skips: usize, - inserted_simplex_sample: Option, + inserted_simplex_sample: Option, invalid_ridge_multiplicity_skips: usize, - invalid_ridge_multiplicity_sample: Option, + invalid_ridge_multiplicity_sample: Option, missing_cell_skips: usize, - missing_cell_sample: Option, + missing_cell_sample: Option, flip_signature_window: VecDeque, flip_signature_counts: FastHashMap, ridge_debug_emitted: usize, postcondition_facet_debug_emitted: usize, } +#[derive(Debug, Clone, PartialEq, Eq)] +struct InsertedSimplexSkipSample { + location: RepairSkipLocation, + removed_face: VertexKeyList, + inserted_face: VertexKeyList, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct RidgeMultiplicitySkipSample { + ridge: RidgeHandle, + multiplicity: usize, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct MissingCellSkipSample { + location: RepairSkipLocation, + cell_key: CellKey, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RepairSkipLocation { + Edge(EdgeKey), + Facet(FacetHandle), + Ridge(RidgeHandle), + Triangle(TriangleHandle), +} + +fn vertex_key_list(vertices: &[VertexKey]) -> VertexKeyList { + vertices.iter().copied().collect() +} + impl RepairDiagnostics { /// Records uncertain predicate sites with bounded samples so diagnostics stay /// actionable on large repairs. @@ -4411,30 +4442,32 @@ impl RepairDiagnostics { } } - /// Captures one duplicate-simplex skip sample without formatting strings on - /// every skipped flip. - fn record_inserted_simplex_skip(&mut self, sample: impl FnOnce() -> String) { + /// Captures one duplicate-simplex skip sample with typed context. + fn record_inserted_simplex_skip(&mut self, sample: InsertedSimplexSkipSample) { self.inserted_simplex_skips = self.inserted_simplex_skips.saturating_add(1); if self.inserted_simplex_sample.is_none() { - self.inserted_simplex_sample = Some(sample()); + self.inserted_simplex_sample = Some(sample); } } - /// Captures one invalid-ridge sample lazily so common skip paths stay cheap. - fn record_invalid_ridge_multiplicity_skip(&mut self, sample: impl FnOnce() -> String) { + /// Captures one invalid-ridge sample with typed context. + const fn record_invalid_ridge_multiplicity_skip( + &mut self, + sample: RidgeMultiplicitySkipSample, + ) { self.invalid_ridge_multiplicity_skips = self.invalid_ridge_multiplicity_skips.saturating_add(1); if self.invalid_ridge_multiplicity_sample.is_none() { - self.invalid_ridge_multiplicity_sample = Some(sample()); + self.invalid_ridge_multiplicity_sample = Some(sample); } } - /// Captures one stale-cell sample lazily so slot-swap churn is visible when + /// Captures one stale-cell sample so slot-swap churn is visible when /// repair diagnostics are inspected. - fn record_missing_cell_skip(&mut self, sample: impl FnOnce() -> String) { + const fn record_missing_cell_skip(&mut self, sample: MissingCellSkipSample) { self.missing_cell_skips = self.missing_cell_skips.saturating_add(1); if self.missing_cell_sample.is_none() { - self.missing_cell_sample = Some(sample()); + self.missing_cell_sample = Some(sample); } } } @@ -5049,9 +5082,12 @@ where ) => { match &err { FlipError::InvalidRidgeMultiplicity { found } => { - diagnostics.record_invalid_ridge_multiplicity_skip(|| { - format!("ridge={ridge:?} multiplicity={found}") - }); + diagnostics.record_invalid_ridge_multiplicity_skip( + RidgeMultiplicitySkipSample { + ridge, + multiplicity: *found, + }, + ); // This is the main #204 failure mode: capture both the local ridge walk // and the full global incidence so we can see whether repair is skipping // a stale handle or a genuinely overshared ridge. @@ -5069,8 +5105,9 @@ where debug_ridge_context(tds, ridge, None, diagnostics, last_applied_flip.as_ref()); } FlipError::MissingCell { cell_key } => { - diagnostics.record_missing_cell_skip(|| { - format!("ridge={ridge:?} missing_cell={cell_key:?}") + diagnostics.record_missing_cell_skip(MissingCellSkipSample { + location: RepairSkipLocation::Ridge(ridge), + cell_key: *cell_key, }); } _ => {} @@ -5153,11 +5190,10 @@ where let applied = match apply_delaunay_flip_k3(tds, &context) { Ok(applied) => applied, Err(err) if let FlipError::InsertedSimplexAlreadyExists { .. } = &err => { - diagnostics.record_inserted_simplex_skip(|| { - format!( - "ridge={ridge:?} removed_face={:?} inserted_face={:?}", - context.removed_face_vertices, context.inserted_face_vertices - ) + diagnostics.record_inserted_simplex_skip(InsertedSimplexSkipSample { + location: RepairSkipLocation::Ridge(ridge), + removed_face: vertex_key_list(&context.removed_face_vertices), + inserted_face: vertex_key_list(&context.inserted_face_vertices), }); log_apply_skip(&err); return Ok(true); @@ -5238,8 +5274,10 @@ where let context = match build_k2_flip_context_from_edge(tds, edge) { Ok(ctx) => ctx, Err(ref err) if let FlipError::MissingCell { cell_key } = err => { - diagnostics - .record_missing_cell_skip(|| format!("edge={edge:?} missing_cell={cell_key:?}")); + diagnostics.record_missing_cell_skip(MissingCellSkipSample { + location: RepairSkipLocation::Edge(edge), + cell_key: *cell_key, + }); log_build_skip(err); return Ok(true); } @@ -5343,11 +5381,10 @@ where let applied = match apply_delaunay_flip_dynamic(tds, D, &context) { Ok(applied) => applied, Err(err) if let FlipError::InsertedSimplexAlreadyExists { .. } = &err => { - diagnostics.record_inserted_simplex_skip(|| { - format!( - "edge={edge:?} removed_face={:?} inserted_face={:?}", - context.removed_face_vertices, context.inserted_face_vertices - ) + diagnostics.record_inserted_simplex_skip(InsertedSimplexSkipSample { + location: RepairSkipLocation::Edge(edge), + removed_face: vertex_key_list(&context.removed_face_vertices), + inserted_face: vertex_key_list(&context.inserted_face_vertices), }); log_apply_skip(&err); return Ok(true); @@ -5430,8 +5467,9 @@ where let context = match build_k3_flip_context_from_triangle(tds, triangle) { Ok(ctx) => ctx, Err(ref err) if let FlipError::MissingCell { cell_key } = err => { - diagnostics.record_missing_cell_skip(|| { - format!("triangle={triangle:?} missing_cell={cell_key:?}") + diagnostics.record_missing_cell_skip(MissingCellSkipSample { + location: RepairSkipLocation::Triangle(triangle), + cell_key: *cell_key, }); log_build_skip(err); return Ok(true); @@ -5525,11 +5563,10 @@ where let applied = match apply_delaunay_flip_dynamic(tds, D - 1, &context) { Ok(applied) => applied, Err(err) if let FlipError::InsertedSimplexAlreadyExists { .. } = &err => { - diagnostics.record_inserted_simplex_skip(|| { - format!( - "triangle={triangle:?} removed_face={:?} inserted_face={:?}", - context.removed_face_vertices, context.inserted_face_vertices - ) + diagnostics.record_inserted_simplex_skip(InsertedSimplexSkipSample { + location: RepairSkipLocation::Triangle(triangle), + removed_face: vertex_key_list(&context.removed_face_vertices), + inserted_face: vertex_key_list(&context.inserted_face_vertices), }); log_apply_skip(&err); return Ok(true); @@ -5614,8 +5651,10 @@ where let context = match build_k2_flip_context(tds, facet) { Ok(ctx) => ctx, Err(ref err) if let FlipError::MissingCell { cell_key } = err => { - diagnostics - .record_missing_cell_skip(|| format!("facet={facet:?} missing_cell={cell_key:?}")); + diagnostics.record_missing_cell_skip(MissingCellSkipSample { + location: RepairSkipLocation::Facet(facet), + cell_key: *cell_key, + }); log_build_skip(err); return Ok(true); } @@ -5711,11 +5750,10 @@ where let applied = match apply_delaunay_flip_k2(tds, &context) { Ok(applied) => applied, Err(err) if let FlipError::InsertedSimplexAlreadyExists { .. } = &err => { - diagnostics.record_inserted_simplex_skip(|| { - format!( - "facet={facet:?} removed_face={:?} inserted_face={:?}", - context.removed_face_vertices, context.inserted_face_vertices - ) + diagnostics.record_inserted_simplex_skip(InsertedSimplexSkipSample { + location: RepairSkipLocation::Facet(facet), + removed_face: vertex_key_list(&context.removed_face_vertices), + inserted_face: vertex_key_list(&context.inserted_face_vertices), }); log_apply_skip(&err); return Ok(true); @@ -6780,10 +6818,7 @@ mod tests { use approx::assert_relative_eq; use rand::{RngExt, SeedableRng, rngs::StdRng}; use slotmap::KeyData; - use std::sync::{ - Once, - atomic::{AtomicUsize, Ordering}, - }; + use std::sync::Once; fn init_tracing() { static INIT: Once = Once::new(); @@ -7880,58 +7915,68 @@ mod tests { } #[test] - fn test_skip_recording_is_lazy() { + fn test_skip_recording_keeps_first_typed_sample() { let mut diagnostics = RepairDiagnostics::default(); - let call_count = AtomicUsize::new(0); + let cell = CellKey::from(KeyData::from_ffi(91)); + let missing_cell = CellKey::from(KeyData::from_ffi(92)); + let v0 = VertexKey::from(KeyData::from_ffi(101)); + let v1 = VertexKey::from(KeyData::from_ffi(102)); + let v2 = VertexKey::from(KeyData::from_ffi(103)); + let edge = EdgeKey::new(v0, v1); + let facet = FacetHandle::new(cell, 0); + let ridge = RidgeHandle::new(cell, 0, 1); + let triangle = TriangleHandle::new(v0, v1, v2); - // First call: sample slot is None, closure must be invoked. - diagnostics.record_inserted_simplex_skip(|| { - call_count.fetch_add(1, Ordering::Relaxed); - "first".to_owned() - }); + let first_inserted_sample = InsertedSimplexSkipSample { + location: RepairSkipLocation::Facet(facet), + removed_face: [v0, v1].into_iter().collect(), + inserted_face: std::iter::once(v2).collect(), + }; + diagnostics.record_inserted_simplex_skip(first_inserted_sample.clone()); assert_eq!(diagnostics.inserted_simplex_skips, 1); assert_eq!( - diagnostics.inserted_simplex_sample.as_deref(), - Some("first") + diagnostics.inserted_simplex_sample, + Some(first_inserted_sample.clone()) ); - assert_eq!(call_count.load(Ordering::Relaxed), 1); - // Second call: sample already set, closure must NOT be invoked. - diagnostics.record_inserted_simplex_skip(|| { - call_count.fetch_add(1, Ordering::Relaxed); - "second".to_owned() + diagnostics.record_inserted_simplex_skip(InsertedSimplexSkipSample { + location: RepairSkipLocation::Edge(edge), + removed_face: std::iter::once(v1).collect(), + inserted_face: [v0, v2].into_iter().collect(), }); assert_eq!(diagnostics.inserted_simplex_skips, 2); assert_eq!( - diagnostics.inserted_simplex_sample.as_deref(), - Some("first") + diagnostics.inserted_simplex_sample, + Some(first_inserted_sample) ); - assert_eq!(call_count.load(Ordering::Relaxed), 1); // Same contract for ridge-multiplicity and missing-cell helpers. - let ridge_calls = AtomicUsize::new(0); - diagnostics.record_invalid_ridge_multiplicity_skip(|| { - ridge_calls.fetch_add(1, Ordering::Relaxed); - "ridge".to_owned() - }); - diagnostics.record_invalid_ridge_multiplicity_skip(|| { - ridge_calls.fetch_add(1, Ordering::Relaxed); - "ridge2".to_owned() + let first_ridge_sample = RidgeMultiplicitySkipSample { + ridge, + multiplicity: 3, + }; + diagnostics.record_invalid_ridge_multiplicity_skip(first_ridge_sample); + diagnostics.record_invalid_ridge_multiplicity_skip(RidgeMultiplicitySkipSample { + ridge: RidgeHandle::new(cell, 1, 2), + multiplicity: 4, }); assert_eq!(diagnostics.invalid_ridge_multiplicity_skips, 2); - assert_eq!(ridge_calls.load(Ordering::Relaxed), 1); + assert_eq!( + diagnostics.invalid_ridge_multiplicity_sample, + Some(first_ridge_sample) + ); - let cell_calls = AtomicUsize::new(0); - diagnostics.record_missing_cell_skip(|| { - cell_calls.fetch_add(1, Ordering::Relaxed); - "cell".to_owned() - }); - diagnostics.record_missing_cell_skip(|| { - cell_calls.fetch_add(1, Ordering::Relaxed); - "cell2".to_owned() + let first_missing_sample = MissingCellSkipSample { + location: RepairSkipLocation::Triangle(triangle), + cell_key: missing_cell, + }; + diagnostics.record_missing_cell_skip(first_missing_sample); + diagnostics.record_missing_cell_skip(MissingCellSkipSample { + location: RepairSkipLocation::Ridge(ridge), + cell_key: CellKey::from(KeyData::from_ffi(93)), }); assert_eq!(diagnostics.missing_cell_skips, 2); - assert_eq!(cell_calls.load(Ordering::Relaxed), 1); + assert_eq!(diagnostics.missing_cell_sample, Some(first_missing_sample)); } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/core/tds.rs b/src/core/tds.rs index 3b79d500..51059538 100644 --- a/src/core/tds.rs +++ b/src/core/tds.rs @@ -8356,7 +8356,7 @@ mod tests { } #[test] - fn test_set_vertex_data_via_triangulation_wrapper() { + fn test_set_vertex_data_via_delaunay_wrapper() { let vertices: [Vertex; 3] = [ vertex!([0.0, 0.0], 10i32), vertex!([1.0, 0.0], 20), @@ -8367,19 +8367,19 @@ mod tests { .unwrap(); let key = dt.vertices().next().unwrap().0; - // Set via Triangulation wrapper - let prev = dt.as_triangulation_mut().set_vertex_data(key, Some(99)); + // Set via Delaunay wrapper + let prev = dt.set_vertex_data(key, Some(99)); assert!(prev.unwrap().is_some()); assert_eq!(dt.tds().get_vertex_by_key(key).unwrap().data, Some(99)); - // Clear via Triangulation wrapper - let prev = dt.as_triangulation_mut().set_vertex_data(key, None); + // Clear via Delaunay wrapper + let prev = dt.set_vertex_data(key, None); assert_eq!(prev, Some(Some(99))); assert_eq!(dt.tds().get_vertex_by_key(key).unwrap().data, None); } #[test] - fn test_set_cell_data_via_triangulation_wrapper() { + fn test_set_cell_data_via_delaunay_wrapper() { let vertices = [ vertex!([0.0, 0.0]), vertex!([1.0, 0.0]), @@ -8390,13 +8390,13 @@ mod tests { .unwrap(); let key = dt.cells().next().unwrap().0; - // Set via Triangulation wrapper - let prev = dt.as_triangulation_mut().set_cell_data(key, Some(42)); + // Set via Delaunay wrapper + let prev = dt.set_cell_data(key, Some(42)); assert_eq!(prev, Some(None)); assert_eq!(dt.tds().get_cell(key).unwrap().data, Some(42)); - // Clear via Triangulation wrapper - let prev = dt.as_triangulation_mut().set_cell_data(key, None); + // Clear via Delaunay wrapper + let prev = dt.set_cell_data(key, None); assert_eq!(prev, Some(Some(42))); assert_eq!(dt.tds().get_cell(key).unwrap().data, None); } diff --git a/src/core/triangulation.rs b/src/core/triangulation.rs index 7369a8ac..a1708cdb 100644 --- a/src/core/triangulation.rs +++ b/src/core/triangulation.rs @@ -1391,11 +1391,11 @@ where /// .build::<()>() /// .unwrap(); /// let key = dt.vertices().next().unwrap().0; - /// let prev = dt.as_triangulation_mut().set_vertex_data(key, Some(99)); + /// let prev = dt.set_vertex_data(key, Some(99)); /// assert!(prev.is_some()); /// /// // Clear data - /// let prev = dt.as_triangulation_mut().set_vertex_data(key, None); + /// let prev = dt.set_vertex_data(key, None); /// assert_eq!(prev, Some(Some(99))); /// assert_eq!(dt.tds().get_vertex_by_key(key).unwrap().data(), None); /// ``` @@ -1428,11 +1428,11 @@ where /// .build::() /// .unwrap(); /// let key = dt.cells().next().unwrap().0; - /// let prev = dt.as_triangulation_mut().set_cell_data(key, Some(42)); + /// let prev = dt.set_cell_data(key, Some(42)); /// assert_eq!(prev, Some(None)); /// /// // Clear data - /// let prev = dt.as_triangulation_mut().set_cell_data(key, None); + /// let prev = dt.set_cell_data(key, None); /// assert_eq!(prev, Some(Some(42))); /// assert_eq!(dt.tds().get_cell(key).unwrap().data(), None); /// ``` @@ -6026,10 +6026,8 @@ where /// let mut dt: DelaunayTriangulation<_, (), (), 2> = DelaunayTriangulation::new(&vertices).unwrap(); /// /// // Empty issues map => nothing to remove. - /// let removed = dt - /// .as_triangulation_mut() - /// .repair_local_facet_issues(&FacetIssuesMap::default()) - /// .unwrap(); + /// let mut tri = dt.as_triangulation().clone(); + /// let removed = tri.repair_local_facet_issues(&FacetIssuesMap::default()).unwrap(); /// assert_eq!(removed, 0); /// ``` /// @@ -10473,9 +10471,9 @@ mod tests { vertex!([0.0, 1.0]), vertex!([1.0, 1.0]), ]; - let mut dt: DelaunayTriangulation<_, (), (), 2> = + let dt: DelaunayTriangulation<_, (), (), 2> = DelaunayTriangulation::new(&vertices).unwrap(); - let tri = dt.as_triangulation_mut(); + let mut tri = dt.as_triangulation().clone(); // Add a duplicate cell with the same vertices as an existing cell. let (_, existing_cell) = tri.tds.cells().next().unwrap(); diff --git a/src/geometry/util/measures.rs b/src/geometry/util/measures.rs index 4f65a78e..aee3c090 100644 --- a/src/geometry/util/measures.rs +++ b/src/geometry/util/measures.rs @@ -19,6 +19,27 @@ use std::ops::AddAssign; // Re-export error types pub use super::{CircumcenterError, SurfaceMeasureError, ValueConversionError}; +const DEGENERACY_EPSILON_FACTOR: f64 = 64.0; + +fn degeneracy_tolerance(scale: T) -> Result { + if scale <= T::zero() { + return Ok(T::zero()); + } + + let factor = safe_scalar_from_f64(DEGENERACY_EPSILON_FACTOR) + .map_err(CircumcenterError::CoordinateConversion)?; + Ok(scale * factor * T::epsilon()) +} + +fn is_zero_or_roundoff(value: T, scale: T) -> Result { + let magnitude = Float::abs(value); + if magnitude == T::zero() { + return Ok(true); + } + + Ok(magnitude <= degeneracy_tolerance(scale)?) +} + /// Calculate the volume of a D-dimensional simplex. /// /// This function computes the D-dimensional volume of a simplex formed by D+1 points. @@ -104,16 +125,8 @@ where let diff = [p1[0] - p0[0]]; let length = Float::abs(diff[0]); - // Check for degeneracy (coincident points) - let epsilon = T::from(1e-12).ok_or_else(|| { - CircumcenterError::ValueConversion(ValueConversionError::ConversionFailed { - value: "1e-12".to_string(), - from_type: "f64", - to_type: std::any::type_name::(), - details: "Failed to convert epsilon threshold".to_string(), - }) - })?; - if length < epsilon { + // Check for degeneracy (coincident points). + if length == T::zero() { return Err(CircumcenterError::MatrixInversionFailed { details: "Degenerate simplex with zero volume (coincident points)".to_string(), }); @@ -135,16 +148,10 @@ where let cross_z = v1[0] * v2[1] - v1[1] * v2[0]; let area = Float::abs(cross_z) / T::from(2).unwrap_or_else(|| T::one() + T::one()); - // Check for degeneracy (collinear points) - let epsilon = T::from(1e-12).ok_or_else(|| { - CircumcenterError::ValueConversion(ValueConversionError::ConversionFailed { - value: "1e-12".to_string(), - from_type: "f64", - to_type: std::any::type_name::(), - details: "Failed to convert epsilon threshold".to_string(), - }) - })?; - if area < epsilon { + // Check for degeneracy (collinear points) using a scale-aware + // determinant threshold instead of an absolute area cutoff. + let cross_scale = Float::abs(v1[0] * v2[1]) + Float::abs(v1[1] * v2[0]); + if is_zero_or_roundoff(cross_z, cross_scale)? { return Err(CircumcenterError::MatrixInversionFailed { details: "Degenerate simplex with zero volume (collinear points)".to_string(), }); @@ -175,16 +182,15 @@ where .unwrap_or_else(|| T::one() + T::one() + T::one() + T::one() + T::one() + T::one()); let volume = Float::abs(triple_product) / six; - // Check for degeneracy (coplanar points) - let epsilon = T::from(1e-12).ok_or_else(|| { - CircumcenterError::ValueConversion(ValueConversionError::ConversionFailed { - value: "1e-12".to_string(), - from_type: "f64", - to_type: std::any::type_name::(), - details: "Failed to convert epsilon threshold".to_string(), - }) - })?; - if volume < epsilon { + // Check for degeneracy (coplanar points) using the expansion scale + // of the 3x3 determinant. + let triple_scale = Float::abs(v1[0] * v2[1] * v3[2]) + + Float::abs(v1[0] * v2[2] * v3[1]) + + Float::abs(v1[1] * v2[2] * v3[0]) + + Float::abs(v1[1] * v2[0] * v3[2]) + + Float::abs(v1[2] * v2[0] * v3[1]) + + Float::abs(v1[2] * v2[1] * v3[0]); + if is_zero_or_roundoff(triple_product, triple_scale)? { return Err(CircumcenterError::MatrixInversionFailed { details: "Degenerate simplex with zero volume (coplanar points)".to_string(), }); @@ -199,38 +205,30 @@ where } } -/// Clamp and validate a Gram determinant. +/// Validate a Gram determinant. /// /// For valid inputs, Gram determinants should be non-negative. /// /// In this crate we compute Gram determinants via a symmetry-exploiting LDLT factorization /// (see [`gram_determinant_ldlt`]), so **negative** determinants should not occur for PSD Gram -/// matrices. We still keep a small negative clamp as a defensive check, since other callers may -/// pass in raw determinants. +/// matrices. Any negative determinant is reported as an error instead of being hidden behind an +/// absolute tolerance. /// /// This function treats any non-positive determinant as a degenerate simplex: /// - non-finite determinants error -/// - sufficiently negative determinants error -/// - determinants in `(-1e-12, 0)` are clamped to `0.0`, and **zero always errors** -/// -/// In other words, clamping does not “allow near-zero volumes”; it just avoids propagating -/// tiny negative values caused by floating-point noise. -fn clamp_gram_determinant(mut det: f64) -> Result { +/// - negative determinants error +/// - zero determinants error +fn validate_gram_determinant(det: f64) -> Result { if !det.is_finite() { return Err(CircumcenterError::MatrixInversionFailed { details: "Gram determinant is non-finite".to_string(), }); } - // Clamp small negative values to zero (numerical tolerance) if det < 0.0 { - if det > -1e-12 { - det = 0.0; - } else { - return Err(CircumcenterError::MatrixInversionFailed { - details: "Gram matrix has negative determinant (degenerate simplex)".to_string(), - }); - } + return Err(CircumcenterError::MatrixInversionFailed { + details: "Gram matrix has negative determinant (degenerate simplex)".to_string(), + }); } // Degenerate case: zero determinant means no volume @@ -301,8 +299,8 @@ where } } - // Compute Gram determinant with clamping (LDLT exploits symmetry / PSD structure). - let det = clamp_gram_determinant(gram_determinant_ldlt(gram_matrix))?; + // Compute Gram determinant with validation (LDLT exploits symmetry / PSD structure). + let det = validate_gram_determinant(gram_determinant_ldlt(gram_matrix))?; let volume_f64 = { let sqrt_det = det.sqrt(); @@ -388,16 +386,7 @@ where // Compute volume let volume = simplex_volume(points)?; - // Check for degenerate simplex (using same epsilon as simplex_volume for consistency) - let epsilon = T::from(1e-12).ok_or_else(|| { - CircumcenterError::ValueConversion(ValueConversionError::ConversionFailed { - value: "1e-12".to_string(), - from_type: "f64", - to_type: std::any::type_name::(), - details: "Failed to convert epsilon threshold".to_string(), - }) - })?; - if volume < epsilon { + if volume <= T::zero() { return Err(CircumcenterError::MatrixInversionFailed { details: format!("Degenerate simplex with volume ≈ {volume:?}"), }); @@ -422,8 +411,8 @@ where surface_area += facet_area; } - // Check for degenerate surface area - if surface_area < epsilon { + // Check for degenerate surface area. + if surface_area <= T::zero() { return Err(CircumcenterError::MatrixInversionFailed { details: format!("Degenerate simplex with surface_area ≈ {surface_area:?}"), }); @@ -527,9 +516,8 @@ where let diff = [p1[0] - p0[0], p1[1] - p0[1]]; let length = hypot(&diff); - // Check for degeneracy (coincident points) - let epsilon = T::from(1e-12).unwrap_or_else(T::zero); - if length < epsilon { + // Check for degeneracy (coincident points). + if length == T::zero() { return Err(CircumcenterError::MatrixInversionFailed { details: "Degenerate facet with zero length (coincident points)".to_string(), }); @@ -558,9 +546,15 @@ where let cross_magnitude = hypot(&cross); let area = cross_magnitude / (T::one() + T::one()); // Divide by 2 - // Check for degeneracy (collinear points) - let epsilon = T::from(1e-12).unwrap_or_else(T::zero); - if area < epsilon { + // Check for degeneracy (collinear points) using the expansion scale + // of the cross-product components. + let cross_scale = Float::abs(v1[1] * v2[2]) + + Float::abs(v1[2] * v2[1]) + + Float::abs(v1[2] * v2[0]) + + Float::abs(v1[0] * v2[2]) + + Float::abs(v1[0] * v2[1]) + + Float::abs(v1[1] * v2[0]); + if is_zero_or_roundoff(cross_magnitude, cross_scale)? { return Err(CircumcenterError::MatrixInversionFailed { details: "Degenerate facet with zero area (collinear points)".to_string(), }); @@ -633,7 +627,7 @@ where *dst = safe_coords_to_f64(p.coords())?; } - // Compute Gram determinant with clamping. + // Compute Gram determinant with validation. // // For a (D-1)-simplex embedded in D dimensions, there are (D-1) edge vectors from // one vertex to the remaining vertices, so the Gram matrix is (D-1)×(D-1). @@ -655,7 +649,7 @@ where } } - clamp_gram_determinant(gram_determinant_ldlt(gram_matrix)) + validate_gram_determinant(gram_determinant_ldlt(gram_matrix)) })?; let volume_f64 = { @@ -837,6 +831,32 @@ mod tests { assert!(result.is_err(), "Degenerate simplex should return an error"); } + #[test] + fn test_simplex_volume_very_small_valid_simplices() { + let small_val = 1e-14; + + let triangle = vec![ + Point::new([0.0, 0.0]), + Point::new([small_val, 0.0]), + Point::new([0.0, small_val]), + ]; + let area = simplex_volume(&triangle).unwrap(); + assert_relative_eq!(area, small_val * small_val / 2.0, max_relative = 1e-12); + + let tetrahedron = vec![ + Point::new([0.0, 0.0, 0.0]), + Point::new([small_val, 0.0, 0.0]), + Point::new([0.0, small_val, 0.0]), + Point::new([0.0, 0.0, small_val]), + ]; + let volume = simplex_volume(&tetrahedron).unwrap(); + assert_relative_eq!( + volume, + small_val * small_val * small_val / 6.0, + max_relative = 1e-12 + ); + } + #[test] fn test_simplex_volume_wrong_point_count() { // Wrong number of points for 2D @@ -968,7 +988,7 @@ mod tests { } } - clamp_gram_determinant(gram_determinant_ldlt(gram_matrix)) + validate_gram_determinant(gram_determinant_ldlt(gram_matrix)) }) } @@ -987,8 +1007,8 @@ mod tests { } #[test] - fn test_clamp_gram_determinant_tiny_negative_errors() { - assert!(clamp_gram_determinant(-1e-13).is_err()); + fn test_validate_gram_determinant_tiny_negative_errors() { + assert!(validate_gram_determinant(-1e-13).is_err()); } // Macro to test orthogonal edges across dimensions @@ -1442,9 +1462,9 @@ mod tests { #[test] fn test_facet_measure_very_small_coordinates() { - // Test with very small but non-zero coordinates - // Use 1e-5 so that area (1e-10/2 = 5e-11) is above epsilon threshold (1e-12) - let small_val = 1e-5; + // Test with very small but non-zero coordinates. The degeneracy check + // should be scale-aware rather than rejecting every tiny valid facet. + let small_val = 1e-14; let points = vec![ Point::new([0.0, 0.0, 0.0]), Point::new([small_val, 0.0, 0.0]), @@ -1458,7 +1478,7 @@ mod tests { assert!(measure.is_finite(), "Measure should be finite"); // Should be area of right triangle: small_val * small_val / 2 let expected = small_val * small_val / 2.0; - assert_relative_eq!(measure, expected, epsilon = 1e-10); + assert_relative_eq!(measure, expected, epsilon = 1e-40); } #[test] diff --git a/src/lib.rs b/src/lib.rs index 0e3d37a4..ad51e40d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -300,9 +300,9 @@ // Allow multiple crate versions due to transitive dependencies #![expect(clippy::multiple_crate_versions)] -// Temporarily allow deprecated warnings during API migration (v0.6.0) -// - Facet -> FacetView migration -// - Tds::new()/add() -> DelaunayTriangulation::new()/insert() +// Temporarily allow deprecated warnings during API migrations. +// - Historical Facet -> FacetView and Tds construction migrations +// - DelaunayTriangulation::as_triangulation_mut() removal planned for v0.8.0 // Forbid unsafe code throughout the entire crate #![forbid(unsafe_code)] diff --git a/src/triangulation/builder.rs b/src/triangulation/builder.rs index f1feab48..91c70254 100644 --- a/src/triangulation/builder.rs +++ b/src/triangulation/builder.rs @@ -1143,7 +1143,7 @@ where self.construction_options, )?; dt.set_global_topology(topology); - dt.as_triangulation_mut() + dt.tri .normalize_and_promote_positive_orientation() .map_err(|e| TriangulationConstructionError::GeometricDegeneracy { message: format!( diff --git a/src/triangulation/delaunay.rs b/src/triangulation/delaunay.rs index 31867248..18b73e17 100644 --- a/src/triangulation/delaunay.rs +++ b/src/triangulation/delaunay.rs @@ -4657,6 +4657,16 @@ where &mut self.tri.tds } + /// Returns mutable TDS access for crate-internal repair algorithms. + /// + /// Repair passes may rewrite topology and invalidate locate hints, so this + /// deliberately clears the ephemeral caches before handing out the borrow. + pub(crate) fn tds_mut_for_repair(&mut self) -> &mut Tds { + self.insertion_state.last_inserted_cell = None; + self.spatial_index = None; + &mut self.tri.tds + } + /// Returns a reference to the underlying `Triangulation` (kernel + tds). /// /// This is useful when you need to pass the triangulation to methods that @@ -4687,60 +4697,19 @@ where /// Returns a mutable reference to the underlying `Triangulation`. /// - /// # ⚠️ WARNING - ADVANCED USE ONLY - /// - /// This method provides direct mutable access to the internal triangulation state. - /// **Modifying the triangulation through this reference can break Delaunay invariants - /// and leave the data structure in an inconsistent state.** - /// - /// ## When to Use - /// - /// This is primarily intended for: - /// - **Testing internal algorithms** (topology validation, repair mechanisms) - /// - **Advanced library development** (implementing custom triangulation operations) - /// - **Research prototyping** (experimenting with new algorithms) - /// - /// ## What Can Go Wrong - /// - /// Direct mutations can violate critical invariants: - /// - **Delaunay property**: Cells may no longer satisfy the empty circumsphere condition - /// - **Manifold topology**: Facets may become over-shared or improperly connected - /// - **Neighbor consistency**: Cell neighbor pointers may become invalid - /// - **Hint caching**: Location hints may point to deleted cells - /// - /// After direct modification, you should: - /// 1. Call `detect_local_facet_issues()` and `repair_local_facet_issues()` if you modified topology - /// 2. Run `dt.as_triangulation().validate()` (Levels 1–3) or `dt.validate()` (Levels 1–4) to verify structural/topological consistency - /// 3. Reserve `dt.is_valid()` for Delaunay-only (Level 4) checks + /// # Deprecated /// - /// ## Safe Alternatives - /// - /// For most use cases, prefer these safe, high-level methods: - /// - [`insert()`](Self::insert) - Add vertices (maintains all invariants) - /// - [`remove_vertex()`](Self::remove_vertex) - Remove vertices safely - /// - [`tds()`](Self::tds) - Read-only access to the data structure - /// - /// # Examples - /// - /// ```rust - /// use delaunay::prelude::triangulation::*; - /// - /// let vertices = vec![ - /// vertex!([0.0, 0.0, 0.0]), - /// vertex!([1.0, 0.0, 0.0]), - /// vertex!([0.0, 1.0, 0.0]), - /// vertex!([0.0, 0.0, 1.0]), - /// ]; - /// let mut dt = DelaunayTriangulation::new(&vertices).unwrap(); - /// - /// // ⚠️ Advanced use: direct access for testing validation - /// let tri = dt.as_triangulation_mut(); - /// // ... perform internal algorithm testing ... - /// - /// // Always validate after direct modifications - /// assert!(dt.validate().is_ok()); - /// ``` + /// Direct mutable access can break the Delaunay, topology, neighbor, and + /// locate-hint invariants owned by `DelaunayTriangulation`. Prefer safe + /// high-level methods such as [`insert()`](Self::insert), + /// [`remove_vertex()`](Self::remove_vertex), [`set_vertex_data`](Self::set_vertex_data), + /// [`set_cell_data`](Self::set_cell_data), and read-only + /// [`as_triangulation()`](Self::as_triangulation) access. #[must_use] + #[deprecated( + since = "0.7.7", + note = "use safe DelaunayTriangulation APIs or read-only as_triangulation(); this mutable escape hatch is planned for removal in 0.8.0" + )] pub fn as_triangulation_mut(&mut self) -> &mut Triangulation { // Direct mutable access can invalidate performance caches. self.insertion_state.last_inserted_cell = None; @@ -6324,47 +6293,76 @@ where return Ok(0); } - // Fast path: inverse k=1 flip when the vertex star is a simplex. - let mut seed_cells: Option = None; - let cells_removed = match apply_bistellar_flip_k1_inverse(&mut self.tri.tds, vertex_key) { - Ok(info) => { - seed_cells = Some(info.new_cells); - info.removed_cells.len() - } - Err(FlipError::NeighborWiring { message }) => { - return Err(TdsError::InvalidNeighbors { - message: format!("inverse k=1 flip failed during remove_vertex: {message}"), + let snapshot = ( + self.tri.tds.clone(), + self.insertion_state, + self.spatial_index.clone(), + ); + + let result = (|| { + // Fast path: inverse k=1 flip when the vertex star is a simplex. + let mut seed_cells: Option = None; + let cells_removed = match apply_bistellar_flip_k1_inverse(&mut self.tri.tds, vertex_key) + { + Ok(info) => { + seed_cells = Some(info.new_cells); + info.removed_cells.len() } - .into()); - } - Err(_) => self.tri.remove_vertex(vertex_key)?, - }; + Err(FlipError::NeighborWiring { message }) => { + return Err(TdsError::InvalidNeighbors { + message: format!("inverse k=1 flip failed during remove_vertex: {message}"), + } + .into()); + } + Err(_) => self.tri.remove_vertex(vertex_key)?, + }; - let topology = self.tri.topology_guarantee(); - if self.should_run_delaunay_repair_for(topology, 0) { - let seed_ref = seed_cells.as_deref(); - let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); - repair_delaunay_with_flips_k2_k3(tds, kernel, seed_ref, topology, None).map_err( - |e| { + let topology = self.tri.topology_guarantee(); + if self.should_run_delaunay_repair_for(topology, 0) { + let seed_ref = seed_cells.as_deref(); + let repair_result = { + let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); + repair_delaunay_with_flips_k2_k3(tds, kernel, seed_ref, topology, None) + }; + + #[cfg(test)] + let repair_result = if test_hooks::force_repair_nonconvergent_enabled() { + Err(test_hooks::synthetic_nonconvergent_error()) + } else { + repair_result + }; + + repair_result.map_err(|e| { InvariantError::Delaunay(DelaunayTriangulationValidationError::RepairFailed { message: format!("Delaunay repair failed after vertex removal: {e}"), }) - }, - )?; - - // Re-canonicalize geometric orientation (#258): flip repair may leave - // the global sign negative. - self.tri - .normalize_and_promote_positive_orientation() - .map_err(|e| { - insertion_error_to_invariant_error( - e, - "Orientation canonicalization failed after vertex removal", - ) })?; - } - Ok(cells_removed) + // Re-canonicalize geometric orientation (#258): flip repair may leave + // the global sign negative. + self.tri + .normalize_and_promote_positive_orientation() + .map_err(|e| { + insertion_error_to_invariant_error( + e, + "Orientation canonicalization failed after vertex removal", + ) + })?; + } + + Ok(cells_removed) + })(); + + match result { + Ok(cells_removed) => Ok(cells_removed), + Err(err) => { + let (tds, insertion_state, spatial_index) = snapshot; + self.tri.tds = tds; + self.insertion_state = insertion_state; + self.spatial_index = spatial_index; + Err(err) + } + } } } @@ -8168,6 +8166,46 @@ mod tests { assert!(dt.vertices().all(|(_, v)| v.uuid() != inserted_uuid)); } + #[test] + fn test_remove_vertex_rolls_back_on_repair_failure() { + init_tracing(); + let vertices: Vec> = vec![ + vertex!([0.0, 0.0, 0.0]), + vertex!([1.0, 0.0, 0.0]), + vertex!([0.0, 1.0, 0.0]), + vertex!([0.0, 0.0, 1.0]), + ]; + + let mut dt: DelaunayTriangulation, (), (), 3> = + DelaunayTriangulation::new(&vertices).unwrap(); + dt.set_topology_guarantee(TopologyGuarantee::PLManifold); + + let cell_key = dt.cells().next().unwrap().0; + let inserted_vertex = vertex!([0.2, 0.2, 0.2]); + let inserted_uuid = inserted_vertex.uuid(); + dt.flip_k1_insert(cell_key, inserted_vertex).unwrap(); + + let vertex_key = dt + .vertices() + .find(|(_, v)| v.uuid() == inserted_uuid) + .map(|(k, _)| k) + .expect("Inserted vertex not found"); + let vertex_count_before = dt.number_of_vertices(); + let cell_count_before = dt.number_of_cells(); + + let _guard = ForceRepairNonconvergentGuard::enable(); + let result = dt.remove_vertex(vertex_key); + assert!( + result.is_err(), + "forced repair failure should make removal fail" + ); + + assert_eq!(dt.number_of_vertices(), vertex_count_before); + assert_eq!(dt.number_of_cells(), cell_count_before); + assert!(dt.vertices().any(|(_, v)| v.uuid() == inserted_uuid)); + assert!(dt.as_triangulation().validate().is_ok()); + } + #[test] fn test_repair_delaunay_with_flips_allows_pl_manifold() { init_tracing(); diff --git a/src/triangulation/delaunayize.rs b/src/triangulation/delaunayize.rs index 9639deba..d2724352 100644 --- a/src/triangulation/delaunayize.rs +++ b/src/triangulation/delaunayize.rs @@ -548,30 +548,29 @@ where max_iterations: config.topology_max_iterations, max_cells_removed: config.topology_max_cells_removed, }; - let topology_stats = - match repair_facet_oversharing(&mut dt.as_triangulation_mut().tds, &pl_config) { - Ok(stats) => stats, - // Topology repair failed but fallback is enabled — try rebuilding. - Err(topo_err) if let Some((ref verts, ref cell_data)) = fallback_state => { - match rebuild_preserving_data(&dt.as_triangulation().kernel, verts, cell_data) { - Ok(rebuilt) => { - *dt = rebuilt; - return Ok(DelaunayizeOutcome { - topology_repair: PlManifoldRepairStats { - succeeded: true, - ..PlManifoldRepairStats::default() - }, - delaunay_repair: DelaunayRepairStats::default(), - used_fallback_rebuild: true, - }); - } - Err(fallback_error) => { - return Err(topology_rebuild_error(topo_err, fallback_error)); - } + let topology_stats = match repair_facet_oversharing(dt.tds_mut_for_repair(), &pl_config) { + Ok(stats) => stats, + // Topology repair failed but fallback is enabled — try rebuilding. + Err(topo_err) if let Some((ref verts, ref cell_data)) = fallback_state => { + match rebuild_preserving_data(&dt.as_triangulation().kernel, verts, cell_data) { + Ok(rebuilt) => { + *dt = rebuilt; + return Ok(DelaunayizeOutcome { + topology_repair: PlManifoldRepairStats { + succeeded: true, + ..PlManifoldRepairStats::default() + }, + delaunay_repair: DelaunayRepairStats::default(), + used_fallback_rebuild: true, + }); + } + Err(fallback_error) => { + return Err(topology_rebuild_error(topo_err, fallback_error)); } } - Err(topo_err) => return Err(topo_err.into()), - }; + } + Err(topo_err) => return Err(topo_err.into()), + }; // Step 2: Flip-based Delaunay repair. let delaunay_result = if let Some(max_flips) = config.delaunay_max_flips { diff --git a/tests/proptest_geometry.rs b/tests/proptest_geometry.rs index 334c9dc5..6c7ddb25 100644 --- a/tests/proptest_geometry.rs +++ b/tests/proptest_geometry.rs @@ -21,6 +21,19 @@ fn finite_coordinate() -> impl Strategy { (-100.0..100.0).prop_filter("must be finite", |x: &f64| x.is_finite()) } +fn nonzero_scale() -> impl Strategy { + (-90.0_f64..90.0).prop_map(|exponent| 10.0_f64.powf(exponent)) +} + +fn prop_assert_relative_close(actual: f64, expected: f64) -> Result<(), TestCaseError> { + let tolerance = 1e-10 * expected.abs().max(1.0e-300); + prop_assert!( + (actual - expected).abs() <= tolerance, + "expected {expected:e}, got {actual:e}" + ); + Ok(()) +} + // ============================================================================= // DIMENSIONAL TEST GENERATION MACROS // ============================================================================= @@ -177,3 +190,47 @@ test_geometry_properties!(2, 3); test_geometry_properties!(3, 4); test_geometry_properties!(4, 5); test_geometry_properties!(5, 6); + +proptest! { + /// Property: Low-dimensional simplex volume remains scale-aware across many orders + /// of magnitude. This guards against fixed absolute degeneracy thresholds. + #[test] + fn prop_low_dimensional_simplex_volume_accepts_scaled_valid_simplices(scale in nonzero_scale()) { + let segment = vec![Point::new([0.0]), Point::new([scale])]; + let length = simplex_volume(&segment).unwrap(); + prop_assert_relative_close(length, scale)?; + + let triangle = vec![ + Point::new([0.0, 0.0]), + Point::new([scale, 0.0]), + Point::new([0.0, scale]), + ]; + let area = simplex_volume(&triangle).unwrap(); + prop_assert_relative_close(area, scale * scale / 2.0)?; + + let tetrahedron = vec![ + Point::new([0.0, 0.0, 0.0]), + Point::new([scale, 0.0, 0.0]), + Point::new([0.0, scale, 0.0]), + Point::new([0.0, 0.0, scale]), + ]; + let volume = simplex_volume(&tetrahedron).unwrap(); + prop_assert_relative_close(volume, scale * scale * scale / 6.0)?; + } + + /// Property: Low-dimensional facet measure remains scale-aware for valid facets. + #[test] + fn prop_low_dimensional_facet_measure_accepts_scaled_valid_facets(scale in nonzero_scale()) { + let segment = vec![Point::new([0.0, 0.0]), Point::new([scale, 0.0])]; + let length = facet_measure(&segment).unwrap(); + prop_assert_relative_close(length, scale)?; + + let triangle = vec![ + Point::new([0.0, 0.0, 0.0]), + Point::new([scale, 0.0, 0.0]), + Point::new([0.0, scale, 0.0]), + ]; + let area = facet_measure(&triangle).unwrap(); + prop_assert_relative_close(area, scale * scale / 2.0)?; + } +} diff --git a/tests/proptest_triangulation.rs b/tests/proptest_triangulation.rs index c0bd1d5b..e73dd2e2 100644 --- a/tests/proptest_triangulation.rs +++ b/tests/proptest_triangulation.rs @@ -713,11 +713,11 @@ macro_rules! test_facet_topology_invariant { ) ) { // Build triangulation - if let Ok(mut dt) = DelaunayTriangulation::new_with_topology_guarantee( + if let Ok(dt) = DelaunayTriangulation::new_with_topology_guarantee( &vertices, TopologyGuarantee::PLManifold, ) { - let tri = dt.as_triangulation_mut(); + let mut tri = dt.as_triangulation().clone(); // Get all cell keys let cell_keys: Vec<_> = tri.cells().map(|(k, _)| k).collect(); From 70c901a7dfa357411bed2667fdb7d7f0219b3011 Mon Sep 17 00:00:00 2001 From: Adam Getchell Date: Tue, 28 Apr 2026 22:10:53 -0700 Subject: [PATCH 2/4] fix: harden Delaunay repair diagnostics and removal rollback - Make vertex removal transactional across repair and orientation canonicalization failures. - Preserve typed repair sources with operation-specific Delaunay diagnostics. - Deprecate direct mutable triangulation access ahead of planned v0.8.0 removal. - Clarify delaunayize fallback repair statistics and documentation. - Add regression, coverage, doctest, and prelude coverage for repair diagnostics and invariant behavior. --- README.md | 4 +- docs/api_design.md | 19 +- docs/dev/rust.md | 26 ++ docs/workflows.md | 20 +- src/core/algorithms/flips.rs | 73 +++- src/geometry/util/measures.rs | 31 ++ src/triangulation/delaunay.rs | 557 ++++++++++++++++++++------ src/triangulation/delaunayize.rs | 55 ++- tests/delaunay_public_api_coverage.rs | 153 +++++++ tests/prelude_exports.rs | 64 ++- 10 files changed, 868 insertions(+), 134 deletions(-) create mode 100644 tests/delaunay_public_api_coverage.rs diff --git a/README.md b/README.md index 2e08a344..8e416e34 100644 --- a/README.md +++ b/README.md @@ -64,7 +64,7 @@ combinatorial and geometric checks. well-conditioned exploratory work; not accepted by explicit repair APIs) - [x] Bulk insertion ordering (`InsertionOrderStrategy`): [Hilbert curve] (default) or input order - [x] Batch construction options (`ConstructionOptions`): optional deduplication and deterministic retries -- [x] Incremental construction APIs: insertion plus vertex removal (`remove_vertex`) +- [x] Incremental construction APIs: insertion plus transactional vertex removal (`remove_vertex`) - [x] 4-level validation hierarchy (element validity → TDS structural validity → manifold topology → Delaunay property), including full diagnostics via `validation_report` - [x] Local topology validation ([PL-manifold] default, [Pseudomanifold] opt-out) - [x] Coherent combinatorial orientation validation/normalization for cells, maintaining oriented simplicial complexes @@ -144,6 +144,8 @@ For the full periodic image-point method (Phase 2), see the [`DelaunayTriangulat see [`docs/workflows.md`](docs/workflows.md) for a minimal example and [`docs/api_design.md`](docs/api_design.md) for details. - **Flip-based Delaunay repair**, including the heuristic rebuild fallback (`repair_delaunay_with_flips*`): see [`docs/workflows.md`](docs/workflows.md). +- **Repair diagnostics and mutating-operation rollback**: + `remove_vertex` rolls back if post-removal repair or orientation canonicalization fails, and repair failures preserve typed source errors for debugging. - **Insertion outcomes and statistics** (`insert_with_statistics`, `InsertionOutcome`, `InsertionStatistics`): see [`docs/workflows.md`](docs/workflows.md) and [`docs/numerical_robustness_guide.md`](docs/numerical_robustness_guide.md). - **Topology guarantees** (`TopologyGuarantee`) and **automatic topology validation** (`ValidationPolicy`): diff --git a/docs/api_design.md b/docs/api_design.md index 357e7011..c8982c91 100644 --- a/docs/api_design.md +++ b/docs/api_design.md @@ -76,7 +76,7 @@ let mut dt: DelaunayTriangulation<_, (), (), 3> = let new_vertex = vertex!([0.5, 0.5, 0.5]); dt.insert(new_vertex).unwrap(); -// Vertex removal (maintains Delaunay property via fan retriangulation) +// Vertex removal (topology-preserving, with automatic repair when enabled) let vertex_key = dt.vertices().next().unwrap().0; dt.remove_vertex(vertex_key).unwrap(); ``` @@ -125,14 +125,21 @@ for topology guarantee and validation policy details. ### Key Characteristics -- **Automatic property preservation**: The Delaunay empty-circumsphere property is maintained automatically +- **Automatic property preservation**: Insertion maintains the Delaunay + empty-circumsphere property; removal runs flip-based repair when the active + `DelaunayRepairPolicy` permits it - **Cavity-based insertion**: New vertices are inserted by identifying conflicting cells, removing them, and filling the cavity -- **Fan retriangulation**: Vertex removal uses fan-based retriangulation of the vertex star +- **Transactional vertex removal**: Vertex removal uses an inverse k=1 fast path + when possible and fan-based retriangulation otherwise. If post-removal + Delaunay repair or orientation canonicalization fails, the triangulation and + internal caches are restored to their pre-removal state. - **Auxiliary data**: Vertices and cells carry optional user data (`U` / `V`). Read via `vertex.data()` / `cell.data()`, write via `dt.set_vertex_data(key, data)` / `dt.set_cell_data(key, data)` (O(1), invariant-preserving). See [`workflows.md`](workflows.md) for examples. - **Error handling**: Operations fail gracefully if they would violate invariants (see - [`invariants.md`](invariants.md)). + [`invariants.md`](invariants.md)). Mutating operations that invoke repair use + typed repair diagnostics where available, for example + `RepairOperationFailed { operation, source }`. - **Validation**: The active `ValidationPolicy` (set with `dt.set_validation_policy(...)`) governs automatic topology validation for subsequent construction/modification operations @@ -370,6 +377,10 @@ assert!(outcome.topology_repair.succeeded); empty-circumsphere property. 3. **Optional fallback rebuild** — rebuilds from the vertex set when both repair passes fail (`DelaunayizeConfig { fallback_rebuild: true, .. }`). + If a failed topology repair is recovered by fallback rebuild, + `outcome.topology_repair.succeeded` remains `false`; use + `outcome.used_fallback_rebuild` to distinguish successful rebuild recovery + from direct repair success. ### Configuration diff --git a/docs/dev/rust.md b/docs/dev/rust.md index 78c0fcf6..7e185de5 100644 --- a/docs/dev/rust.md +++ b/docs/dev/rust.md @@ -11,6 +11,7 @@ Agents must follow these rules when modifying or adding Rust code. - [Core Principles](#core-principles) - [Safety](#safety) - [Dimension Generic Architecture](#dimension-generic-architecture) +- [Numeric Conversions](#numeric-conversions) - [Borrowing and Ownership](#borrowing-and-ownership) - [Error Handling](#error-handling) - [Panic Policy](#panic-policy) @@ -100,6 +101,31 @@ Algorithms should operate generically over `D` whenever practical. --- +## Numeric Conversions + +Avoid unchecked numeric casts in geometry, topology, tests, and benchmarks when +precision or range can matter. + +Prefer repository helpers from `crate::geometry::util`, for example: + +- `safe_usize_to_scalar::(value)` +- `safe_scalar_to_f64(value)` +- `safe_scalar_from_f64::(value)` +- `safe_coords_to_f64(coords)` +- `safe_coords_from_f64::(coords)` + +Do not silence `clippy::cast_precision_loss` with `#[expect(...)]` simply +because the current values are small. Use a safe conversion helper and handle +or justify the `Result` at the call site. A lint expectation is appropriate only +when no safe conversion applies and the invariant is documented in the code. + +Avoid fallback conversions such as `unwrap_or(f64::NAN)`, +`unwrap_or(f64::INFINITY)`, or silently clamping failed conversions. These hide +the numerical state that geometric predicates and validation layers need in +order to fail explicitly. + +--- + ## Borrowing and Ownership Prefer **borrowing APIs** whenever possible. diff --git a/docs/workflows.md b/docs/workflows.md index 2f4489a4..b0e55347 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -305,9 +305,10 @@ For guidance on retry/skip behavior and choosing `RobustKernel`, see ## Builder API: removing a vertex -Vertex removal is supported and preserves Levels 1–3, but it may not preserve the Delaunay -property in all cases. If you need the Delaunay property after removals, run a repair pass and/or -validate explicitly. +Vertex removal is supported and preserves Levels 1–3. It uses an inverse k=1 fast path when +possible and fan retriangulation otherwise, then runs flip-based Delaunay repair when the active +`DelaunayRepairPolicy` allows it. If post-removal repair or orientation canonicalization fails, +the operation rolls back to the pre-removal triangulation. ```rust use delaunay::prelude::triangulation::*; @@ -328,11 +329,18 @@ let _cells_removed = dt.remove_vertex(vertex_key).unwrap(); // Topology should still be valid: assert!(dt.as_triangulation().validate().is_ok()); -// If you need Delaunay after edits (requires K: ExactPredicates): -// dt.repair_delaunay_with_flips().unwrap(); -// dt.is_valid().unwrap(); +// If automatic repair is enabled, successful removal has already attempted to +// restore the Delaunay property. +dt.is_valid().unwrap(); ``` +When automatic repair fails after the mutation, `remove_vertex` reports +`InvariantError::Delaunay(DelaunayTriangulationValidationError::RepairOperationFailed { operation: +DelaunayRepairOperation::VertexRemoval, source })`, preserving the underlying +`DelaunayRepairError` for callers that need to inspect the exact repair failure. +Successful removals invalidate internal locate hints and the spatial index so subsequent queries do +not observe stale topology-dependent cache entries. + ## Edit API: minimal flip example The Edit API exposes explicit bistellar flips. These operations do **not** automatically restore diff --git a/src/core/algorithms/flips.rs b/src/core/algorithms/flips.rs index da700bfb..51e9a7cc 100644 --- a/src/core/algorithms/flips.rs +++ b/src/core/algorithms/flips.rs @@ -6818,7 +6818,7 @@ mod tests { use approx::assert_relative_eq; use rand::{RngExt, SeedableRng, rngs::StdRng}; use slotmap::KeyData; - use std::sync::Once; + use std::{iter::once, sync::Once}; fn init_tracing() { static INIT: Once = Once::new(); @@ -8238,6 +8238,77 @@ mod tests { test_bistellar_roundtrip_dimension!(4, k3); test_bistellar_roundtrip_dimension!(5, k3); + #[test] + fn dynamic_flip_rejects_bad_context() { + init_tracing(); + let mut tds: Tds = Tds::empty(); + let v0 = VertexKey::from(KeyData::from_ffi(1)); + let v1 = VertexKey::from(KeyData::from_ffi(2)); + let v2 = VertexKey::from(KeyData::from_ffi(3)); + let v3 = VertexKey::from(KeyData::from_ffi(4)); + let v4 = VertexKey::from(KeyData::from_ffi(5)); + let c0 = CellKey::from(KeyData::from_ffi(11)); + let c1 = CellKey::from(KeyData::from_ffi(12)); + + let valid_shape = FlipContextDyn { + removed_face_vertices: [v0, v1, v2].into_iter().collect(), + inserted_face_vertices: [v3, v4].into_iter().collect(), + removed_cells: [c0, c1].into_iter().collect(), + direction: FlipDirection::Forward, + }; + + assert!(matches!( + apply_bistellar_flip_dynamic(&mut tds, 0, &valid_shape), + Err(FlipError::InvalidFlipContext { ref message }) if message.contains("k must be") + )); + assert!(matches!( + apply_bistellar_flip_dynamic(&mut tds, 5, &valid_shape), + Err(FlipError::InvalidFlipContext { ref message }) if message.contains("k must be") + )); + + let wrong_removed_face = FlipContextDyn { + removed_face_vertices: [v0, v1].into_iter().collect(), + ..valid_shape.clone() + }; + assert!(matches!( + apply_bistellar_flip_dynamic(&mut tds, 2, &wrong_removed_face), + Err(FlipError::InvalidFlipContext { ref message }) + if message.contains("removed-face must have") + )); + + let wrong_inserted_face = FlipContextDyn { + inserted_face_vertices: once(v3).collect(), + ..valid_shape.clone() + }; + assert!(matches!( + apply_bistellar_flip_dynamic(&mut tds, 2, &wrong_inserted_face), + Err(FlipError::InvalidFlipContext { ref message }) + if message.contains("inserted-face must have") + )); + + let wrong_removed_cells = FlipContextDyn { + removed_cells: once(c0).collect(), + ..valid_shape.clone() + }; + assert!(matches!( + apply_bistellar_flip_dynamic(&mut tds, 2, &wrong_removed_cells), + Err(FlipError::InvalidFlipContext { ref message }) + if message.contains("removed_cells must have") + )); + + let overlapping_faces = FlipContextDyn { + inserted_face_vertices: [v2, v3].into_iter().collect(), + ..valid_shape + }; + assert!(matches!( + apply_bistellar_flip_dynamic(&mut tds, 2, &overlapping_faces), + Err(FlipError::InvalidFlipContext { ref message }) + if message.contains("must be disjoint") + )); + assert_eq!(tds.number_of_vertices(), 0); + assert_eq!(tds.number_of_cells(), 0); + } + #[test] fn test_flip_k2_2d_edge_flip() { init_tracing(); diff --git a/src/geometry/util/measures.rs b/src/geometry/util/measures.rs index aee3c090..b5c3b238 100644 --- a/src/geometry/util/measures.rs +++ b/src/geometry/util/measures.rs @@ -769,6 +769,13 @@ mod tests { assert_relative_eq!(volume_neg, 5.0, epsilon = 1e-10); } + #[test] + fn simplex_volume_degenerate_segment_errors() { + let line = vec![Point::new([2.0]), Point::new([2.0])]; + let result = simplex_volume(&line); + assert!(result.is_err(), "coincident 1D points should be degenerate"); + } + #[test] fn test_simplex_volume_2d_triangle() { // 2D: Right triangle with legs 3 and 4 @@ -831,6 +838,18 @@ mod tests { assert!(result.is_err(), "Degenerate simplex should return an error"); } + #[test] + fn simplex_volume_coplanar_tetrahedron_errors() { + let coplanar = vec![ + Point::new([0.0, 0.0, 0.0]), + Point::new([1.0, 0.0, 0.0]), + Point::new([0.0, 1.0, 0.0]), + Point::new([0.25, 0.25, 0.0]), + ]; + let result = simplex_volume(&coplanar); + assert!(result.is_err(), "coplanar tetrahedron should be degenerate"); + } + #[test] fn test_simplex_volume_very_small_valid_simplices() { let small_val = 1e-14; @@ -1006,6 +1025,18 @@ mod tests { assert!(gram_det_from_edges(&edges).is_err()); } + #[test] + fn degeneracy_tolerance_zero_scale() { + assert_relative_eq!(degeneracy_tolerance::(0.0).unwrap(), 0.0); + assert_relative_eq!(degeneracy_tolerance::(-1.0).unwrap(), 0.0); + } + + #[test] + fn gram_determinant_nonfinite_errors() { + assert!(validate_gram_determinant(f64::NAN).is_err()); + assert!(validate_gram_determinant(f64::INFINITY).is_err()); + } + #[test] fn test_validate_gram_determinant_tiny_negative_errors() { assert!(validate_gram_determinant(-1e-13).is_err()); diff --git a/src/triangulation/delaunay.rs b/src/triangulation/delaunay.rs index 18b73e17..fa73ccb0 100644 --- a/src/triangulation/delaunay.rs +++ b/src/triangulation/delaunay.rs @@ -42,7 +42,7 @@ use crate::geometry::util::safe_usize_to_scalar; use crate::topology::manifold::validate_ridge_links_for_cells; use crate::topology::traits::topological_space::{GlobalTopology, TopologyKind}; use crate::triangulation::builder::DelaunayTriangulationBuilder; -use core::cmp::Ordering; +use core::{cmp::Ordering, fmt}; use num_traits::{NumCast, ToPrimitive, Zero}; use rand::SeedableRng; use rand::rngs::StdRng; @@ -322,6 +322,30 @@ pub enum DelaunayTriangulationConstructionError { ExplicitConstruction(#[from] crate::triangulation::builder::ExplicitConstructionError), } +/// Mutating Delaunay operation that can invoke flip-based repair internally. +/// +/// # Examples +/// +/// ```rust +/// use delaunay::prelude::triangulation::DelaunayRepairOperation; +/// +/// assert_eq!(DelaunayRepairOperation::VertexRemoval.to_string(), "vertex removal"); +/// ``` +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[non_exhaustive] +pub enum DelaunayRepairOperation { + /// Repair after removing a vertex. + VertexRemoval, +} + +impl fmt::Display for DelaunayRepairOperation { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::VertexRemoval => f.write_str("vertex removal"), + } + } +} + /// Errors that can occur during Delaunay triangulation validation and repair. /// /// The first three variants are returned by [`DelaunayTriangulation::validate`] @@ -333,10 +357,9 @@ pub enum DelaunayTriangulationConstructionError { /// [`DelaunayTriangulation::is_valid`] returns only the Level 4 /// [`VerificationFailed`](Self::VerificationFailed) variant. /// -/// The [`RepairFailed`](Self::RepairFailed) variant is **not** returned by -/// `validate()` or `is_valid()`. It is produced by mutating operations that -/// invoke flip-based repair internally (e.g. -/// [`DelaunayTriangulation::remove_vertex`]). +/// The repair-failure variants are **not** returned by `validate()` or +/// `is_valid()`. They are produced by mutating operations that invoke +/// flip-based repair internally (e.g. [`DelaunayTriangulation::remove_vertex`]). /// /// # Examples /// @@ -377,12 +400,11 @@ pub enum DelaunayTriangulationValidationError { message: String, }, - /// Flip-based Delaunay repair failed. + /// Flip-based Delaunay repair failed with string-only context. /// - /// This is returned by mutating operations that invoke flip-based repair - /// internally (e.g. [`DelaunayTriangulation::remove_vertex`]) when repair - /// encounters any error (budget exhaustion, topology violation, predicate - /// failure, etc.). + /// This variant is retained for compatibility with existing callers. New + /// mutating operations that can preserve the repair source should prefer + /// [`RepairOperationFailed`](Self::RepairOperationFailed). /// /// **Not** returned by `validate()` or `is_valid()` — those use /// [`VerificationFailed`](Self::VerificationFailed) for passive checks. @@ -391,6 +413,26 @@ pub enum DelaunayTriangulationValidationError { /// Description of the repair failure. message: String, }, + + /// Flip-based Delaunay repair failed during a specific mutating operation. + /// + /// This preserves the underlying [`DelaunayRepairError`] so callers can + /// inspect budget exhaustion, topology errors, predicate failures, and other + /// repair causes without parsing display text. Operations that report this + /// variant are responsible for documenting whether failure is transactional; + /// [`remove_vertex`](DelaunayTriangulation::remove_vertex) restores the + /// pre-removal triangulation when post-removal repair fails. + /// + /// **Not** returned by `validate()` or `is_valid()` — those use + /// [`VerificationFailed`](Self::VerificationFailed) for passive checks. + #[error("Delaunay repair failed during {operation}: {source}")] + RepairOperationFailed { + /// Mutating operation that invoked repair. + operation: DelaunayRepairOperation, + /// Underlying flip-repair failure. + #[source] + source: DelaunayRepairError, + }, } // ============================================================================= @@ -4705,6 +4747,23 @@ where /// [`remove_vertex()`](Self::remove_vertex), [`set_vertex_data`](Self::set_vertex_data), /// [`set_cell_data`](Self::set_cell_data), and read-only /// [`as_triangulation()`](Self::as_triangulation) access. + /// + /// # Examples + /// + /// ```rust + /// #![allow(deprecated)] + /// use delaunay::prelude::triangulation::*; + /// + /// let vertices = vec![ + /// vertex!([0.0, 0.0]), + /// vertex!([1.0, 0.0]), + /// vertex!([0.0, 1.0]), + /// ]; + /// let mut dt: DelaunayTriangulation<_, (), (), 2> = + /// DelaunayTriangulation::new(&vertices).unwrap(); + /// + /// assert!(dt.as_triangulation_mut().is_valid().is_ok()); + /// ``` #[must_use] #[deprecated( since = "0.7.7", @@ -6234,6 +6293,12 @@ where /// property in some cases. If the [`DelaunayRepairPolicy`] allows it, a flip-based /// repair pass is run automatically after removal. /// + /// The post-removal repair and orientation canonicalization steps are + /// transactional: if either step fails, this method restores the triangulation, + /// insertion state, and spatial index to their pre-removal state before + /// returning the error. On successful removal, topology-dependent caches + /// (`last_inserted_cell` and the spatial index) are invalidated. + /// /// **Future Enhancement**: Delaunay-aware cavity retriangulation will be added for /// removals. For now, occasional Delaunay violations after removal are expected and /// can be addressed by running flip-based repair (e.g., [`repair_delaunay_with_flips`](Self::repair_delaunay_with_flips)) @@ -6255,7 +6320,8 @@ where /// Returns [`InvariantError`] if: /// - The inverse k=1 flip encounters a neighbor-wiring failure (`InvariantError::Tds`). /// - Fan retriangulation fails (`InvariantError::Tds`). - /// - Delaunay flip-based repair fails after removal (`InvariantError::Delaunay`). + /// - Delaunay flip-based repair fails after removal + /// (`InvariantError::Delaunay(DelaunayTriangulationValidationError::RepairOperationFailed { .. })`). /// - Orientation canonicalization fails after repair (`InvariantError::Tds`). /// /// # Examples @@ -6285,7 +6351,7 @@ where /// let cells_removed = dt.remove_vertex(vertex_key).unwrap(); /// println!("Removed {} cells along with the vertex", cells_removed); /// - /// // Vertex removal preserves Levels 1–3 but may not preserve the Delaunay property. + /// // Vertex removal preserves topology; automatic repair is attempted when enabled. /// assert!(dt.as_triangulation().validate().is_ok()); /// ``` pub fn remove_vertex(&mut self, vertex_key: VertexKey) -> Result { @@ -6332,10 +6398,13 @@ where repair_result }; - repair_result.map_err(|e| { - InvariantError::Delaunay(DelaunayTriangulationValidationError::RepairFailed { - message: format!("Delaunay repair failed after vertex removal: {e}"), - }) + repair_result.map_err(|source| { + InvariantError::Delaunay( + DelaunayTriangulationValidationError::RepairOperationFailed { + operation: DelaunayRepairOperation::VertexRemoval, + source, + }, + ) })?; // Re-canonicalize geometric orientation (#258): flip repair may leave @@ -6354,7 +6423,11 @@ where })(); match result { - Ok(cells_removed) => Ok(cells_removed), + Ok(cells_removed) => { + self.insertion_state.last_inserted_cell = None; + self.spatial_index = None; + Ok(cells_removed) + } Err(err) => { let (tds, insertion_state, spatial_index) = snapshot; self.tri.tds = tds; @@ -6956,11 +7029,10 @@ mod tests { use crate::topology::traits::topological_space::ToroidalConstructionMode; use crate::triangulation::flips::BistellarFlips; use crate::vertex; - use rand::{RngExt, SeedableRng}; use slotmap::KeyData; - use std::sync::Once; + use std::{error::Error as StdError, sync::Once}; - type TestDelaunay4 = DelaunayTriangulation, (), (), 4>; + type TestDelaunay = DelaunayTriangulation, (), (), D>; fn init_tracing() { static INIT: Once = Once::new(); @@ -7362,6 +7434,240 @@ mod tests { assert_eq!(vertex_coords_f64(&infinite_vertex), None); } + fn coord_sequence_2d(vertices: &[Vertex]) -> Vec<[f64; 2]> { + vertices.iter().map(|v| *v.point().coords()).collect() + } + + #[test] + fn order_vertices_input_preserves_order() { + init_tracing(); + let vertices = vec![ + vertex!([2.0, 0.0]), + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + ]; + let expected = coord_sequence_2d(&vertices); + + let ordered = order_vertices_by_strategy(vertices, InsertionOrderStrategy::Input); + + assert_eq!(coord_sequence_2d(&ordered), expected); + } + + #[test] + fn dedup_exact_sorted_without_grid() { + init_tracing(); + let vertices = vec![ + vertex!([1.0, 0.0]), + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + vertex!([0.0, 1.0]), + ]; + + let unique = dedup_vertices_exact_sorted(vertices); + + assert_eq!( + coord_sequence_2d(&unique), + vec![[0.0, 0.0], [0.0, 1.0], [1.0, 0.0]] + ); + } + + #[test] + fn dedup_exact_grid_fallback() { + init_tracing(); + let vertices = vec![ + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + vertex!([0.0, 0.0]), + ]; + let mut grid = HashGridIndex::::new(1.0e-10); + + let unique = dedup_vertices_exact_hash_grid(vertices, &mut grid); + + assert_eq!(coord_sequence_2d(&unique), vec![[0.0, 0.0], [1.0, 0.0]]); + + let vertices_6d = vec![ + vertex!([1.0, 0.0, 0.0, 0.0, 0.0, 0.0]), + vertex!([1.0, 0.0, 0.0, 0.0, 0.0, 0.0]), + ]; + let mut unusable_grid = HashGridIndex::::new(1.0e-10); + + let fallback_unique = dedup_vertices_exact_hash_grid(vertices_6d, &mut unusable_grid); + + assert_eq!(fallback_unique.len(), 1); + } + + #[test] + fn epsilon_dedup_quantized_paths() { + init_tracing(); + let vertices = vec![ + vertex!([0.0, 0.0]), + vertex!([0.09, 0.0]), + vertex!([0.25, 0.0]), + ]; + + let unique = dedup_vertices_epsilon_quantized(vertices, 0.1); + + assert_eq!(coord_sequence_2d(&unique), vec![[0.0, 0.0], [0.25, 0.0]]); + + let zero_epsilon_vertices = vec![vertex!([0.0, 0.0]), vertex!([0.0, 0.0])]; + let zero_epsilon_unique = dedup_vertices_epsilon_quantized(zero_epsilon_vertices, 0.0); + assert_eq!(zero_epsilon_unique.len(), 2); + + let nonfinite_vertices = vec![ + vertex!([0.0, 0.0]), + Vertex::new_with_uuid(Point::new([f64::NAN, 0.0]), Uuid::new_v4(), None), + ]; + let nonfinite_unique = dedup_vertices_epsilon_quantized(nonfinite_vertices, 0.1); + assert_eq!(nonfinite_unique.len(), 2); + + let vertices_6d = vec![ + vertex!([0.0, 0.0, 0.0, 0.0, 0.0, 0.0]), + vertex!([0.01, 0.0, 0.0, 0.0, 0.0, 0.0]), + ]; + let fallback_unique = dedup_vertices_epsilon_quantized(vertices_6d, 0.1); + assert_eq!(fallback_unique.len(), 1); + } + + #[test] + fn dedup_epsilon_grid_fallback() { + init_tracing(); + let vertices = vec![ + vertex!([0.0, 0.0]), + vertex!([0.05, 0.0]), + vertex!([0.25, 0.0]), + ]; + let mut grid = HashGridIndex::::new(0.1); + + let unique = dedup_vertices_epsilon_hash_grid(vertices, 0.1, &mut grid); + + assert_eq!(coord_sequence_2d(&unique), vec![[0.0, 0.0], [0.25, 0.0]]); + + let fallback_vertices = vec![vertex!([0.0, 0.0]), vertex!([0.05, 0.0])]; + let mut unusable_grid = HashGridIndex::::new(0.0); + + let fallback_unique = + dedup_vertices_epsilon_hash_grid(fallback_vertices, 0.1, &mut unusable_grid); + + assert_eq!(fallback_unique.len(), 1); + } + + #[test] + fn preprocess_falls_back_when_grid_unusable() { + init_tracing(); + let exact_vertices = vec![ + vertex!([1.0, 0.0, 0.0, 0.0, 0.0, 0.0]), + vertex!([0.0, 0.0, 0.0, 0.0, 0.0, 0.0]), + vertex!([1.0, 0.0, 0.0, 0.0, 0.0, 0.0]), + ]; + + let exact = TestDelaunay::<6>::preprocess_vertices_for_construction( + &exact_vertices, + DedupPolicy::Exact, + InsertionOrderStrategy::Input, + InitialSimplexStrategy::First, + ) + .unwrap(); + + assert_eq!(exact.primary_slice(&exact_vertices).len(), 2); + assert!(exact.grid_cell_size().is_none()); + + let epsilon_vertices = vec![ + vertex!([0.0, 0.0, 0.0, 0.0, 0.0, 0.0]), + vertex!([0.01, 0.0, 0.0, 0.0, 0.0, 0.0]), + vertex!([0.5, 0.0, 0.0, 0.0, 0.0, 0.0]), + ]; + + let epsilon = TestDelaunay::<6>::preprocess_vertices_for_construction( + &epsilon_vertices, + DedupPolicy::Epsilon { tolerance: 0.1 }, + InsertionOrderStrategy::Input, + InitialSimplexStrategy::First, + ) + .unwrap(); + + assert_eq!(epsilon.primary_slice(&epsilon_vertices).len(), 2); + assert!(epsilon.grid_cell_size().is_none()); + } + + #[test] + fn preprocess_zero_epsilon_keeps_base() { + init_tracing(); + let vertices = vec![ + vertex!([0.0, 0.0, 0.0]), + vertex!([0.0, 0.0, 0.0]), + vertex!([1.0, 0.0, 0.0]), + ]; + + let preprocess = TestDelaunay::<3>::preprocess_vertices_for_construction( + &vertices, + DedupPolicy::Epsilon { tolerance: 0.0 }, + InsertionOrderStrategy::Input, + InitialSimplexStrategy::Balanced, + ) + .unwrap(); + + assert!(preprocess.grid_cell_size().is_some()); + assert_eq!(preprocess.primary_slice(&vertices).len(), vertices.len()); + assert!(preprocess.fallback_slice().is_none()); + } + + #[test] + fn quantize_and_neighbor_edges() { + init_tracing(); + assert_eq!(quantize_coords(&[0.25, -0.25], 10.0), Some([2, -3])); + assert_eq!(quantize_coords(&[f64::NAN, 0.0], 10.0), None); + assert_eq!(quantize_coords(&[1.0e308, 0.0], 1.0e308), None); + + let mut visited = Vec::new(); + let mut current = [0_i64, 0_i64]; + let completed = visit_quantized_neighbors(0, &[4, 7], &mut current, &mut |neighbor| { + visited.push(neighbor); + visited.len() < 4 + }); + + assert!(!completed); + assert_eq!(visited.len(), 4); + } + + #[test] + fn hilbert_fallback_for_nonfinite_coords() { + init_tracing(); + let vertices = vec![ + vertex!([1.0, 0.0]), + Vertex::new_with_uuid(Point::new([f64::NAN, 0.0]), Uuid::new_v4(), None), + vertex!([0.0, 0.0]), + ]; + + let ordered = order_vertices_hilbert(vertices, true); + + assert_eq!(ordered.len(), 3); + assert!( + ordered.iter().any(|v| v.point().coords()[0].is_nan()), + "fallback ordering should preserve the non-finite vertex" + ); + assert!( + ordered + .iter() + .any(|v| coords_equal_exact(v.point().coords(), &[0.0, 0.0])) + ); + assert!( + ordered + .iter() + .any(|v| coords_equal_exact(v.point().coords(), &[1.0, 0.0])) + ); + } + + #[test] + fn hilbert_fallback_for_unsupported_dim() { + init_tracing(); + let vertices = vec![vertex!([1.0; 17]), vertex!([0.0; 17])]; + + let ordered = order_vertices_hilbert(vertices, true); + + assert!(coords_equal_exact(ordered[0].point().coords(), &[0.0; 17])); + assert!(coords_equal_exact(ordered[1].point().coords(), &[1.0; 17])); + } + #[test] fn test_construction_statistics_record_insertion_tracks_inserted_common_fields() { init_tracing(); @@ -7671,13 +7977,10 @@ mod tests { } /// Build simplex vertices plus an interior point (all distinct). - #[expect( - clippy::cast_precision_loss, - reason = "D ≤ 5 in practice; no precision loss" - )] fn simplex_with_interior() -> Vec> { let mut verts = simplex_vertices::(); - let interior = [0.1_f64 / (D as f64); D]; + let dimension = safe_usize_to_scalar::(D).expect("test dimensions fit in f64"); + let interior = [0.1_f64 / dimension; D]; verts.push(vertex!(interior)); verts } @@ -8056,76 +8359,6 @@ mod tests { assert_eq!(dt.topology_guarantee(), TopologyGuarantee::PLManifold); } - /// Slow search helper to find a natural stale-key repro case. - /// - /// This remains ignored by default because it is nondeterministic and expensive. - /// For deterministic coverage, see `test_insert_remaps_vertex_key_after_forced_heuristic_rebuild`. - #[test] - #[ignore = "manual search helper; run explicitly to discover natural repro cases"] - fn find_stale_vertex_key_after_heuristic_rebuild_repro_case() { - const DIM: usize = 4; - const INITIAL_COUNT: usize = 12; - const CASES: usize = 2_000; - - init_tracing(); - - // This probes for a configuration that triggers a heuristic rebuild during automatic - // flip-repair after insertion, which historically could invalidate the returned VertexKey. - let mut rng = rand::rngs::StdRng::seed_from_u64(0xD3_1A_7A_1C_0A_17_u64); - - for case in 0..CASES { - let mut vertices: Vec> = Vec::with_capacity(INITIAL_COUNT); - for _ in 0..INITIAL_COUNT { - // Use a coarse lattice + tiny noise to encourage near-degenerate configurations. - let mut coords = [0.0_f64; DIM]; - for c in &mut coords { - let base: i32 = rng.random_range(-3..=3); - let noise: f64 = rng.random_range(-1.0e-6..=1.0e-6); - *c = >::from(base) + noise; - } - vertices.push(vertex!(coords)); - } - - let Ok(mut dt) = - DelaunayTriangulation::, (), (), DIM>::new(&vertices) - else { - continue; - }; - - let mut inserted_coords = [0.0_f64; DIM]; - for c in &mut inserted_coords { - let base: i32 = rng.random_range(-3..=3); - let noise: f64 = rng.random_range(-1.0e-6..=1.0e-6); - *c = >::from(base) + noise; - } - let inserted = vertex!(inserted_coords); - let inserted_uuid = inserted.uuid(); - - let Ok(vertex_key) = dt.insert(inserted) else { - continue; - }; - - let found = dt - .tri - .tds - .get_vertex_by_key(vertex_key) - .is_some_and(|v| v.uuid() == inserted_uuid); - - if !found { - tracing::debug!(case, "FOUND stale key after insertion"); - tracing::debug!(vertex_key = ?vertex_key, inserted_uuid = %inserted_uuid); - tracing::debug!("initial vertices:"); - for v in &vertices { - tracing::debug!(coords = ?v.point().coords(), " vertex"); - } - tracing::debug!("inserted vertex coords: {inserted_coords:?}"); - panic!("stale VertexKey returned from insert() after heuristic rebuild"); - } - } - - panic!("no stale-key case found after {CASES} attempts"); - } - #[test] fn test_remove_vertex_fast_path_inverse_k1() { init_tracing(); @@ -8167,21 +8400,46 @@ mod tests { } #[test] - fn test_remove_vertex_rolls_back_on_repair_failure() { + fn remove_vertex_invalidates_caches() { init_tracing(); - let vertices: Vec> = vec![ - vertex!([0.0, 0.0, 0.0]), - vertex!([1.0, 0.0, 0.0]), - vertex!([0.0, 1.0, 0.0]), - vertex!([0.0, 0.0, 1.0]), + let vertices: Vec> = vec![ + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + vertex!([0.0, 1.0]), ]; + let mut dt: DelaunayTriangulation<_, (), (), 2> = + DelaunayTriangulation::new(&vertices).unwrap(); - let mut dt: DelaunayTriangulation, (), (), 3> = + let vertex_key = dt.insert(vertex!([0.25, 0.25])).unwrap(); + assert!(dt.insertion_state.last_inserted_cell.is_some()); + assert!(dt.spatial_index.is_some()); + + dt.set_delaunay_repair_policy(DelaunayRepairPolicy::Never); + let removed_cells = dt.remove_vertex(vertex_key).unwrap(); + + assert!(removed_cells > 0); + assert!(dt.insertion_state.last_inserted_cell.is_none()); + assert!(dt.spatial_index.is_none()); + assert!(dt.as_triangulation().validate().is_ok()); + } + + fn interior_vertex_for_k1_insert() -> Vertex { + let denominator = safe_usize_to_scalar::(D + 2) + .expect("D + 2 should convert exactly for rollback test dimensions"); + let coord = 1.0 / denominator; + vertex!([coord; D]) + } + + fn assert_remove_vertex_rollback() { + init_tracing(); + let vertices = simplex_vertices::(); + + let mut dt: DelaunayTriangulation, (), (), D> = DelaunayTriangulation::new(&vertices).unwrap(); dt.set_topology_guarantee(TopologyGuarantee::PLManifold); let cell_key = dt.cells().next().unwrap().0; - let inserted_vertex = vertex!([0.2, 0.2, 0.2]); + let inserted_vertex = interior_vertex_for_k1_insert::(); let inserted_uuid = inserted_vertex.uuid(); dt.flip_k1_insert(cell_key, inserted_vertex).unwrap(); @@ -8195,10 +8453,20 @@ mod tests { let _guard = ForceRepairNonconvergentGuard::enable(); let result = dt.remove_vertex(vertex_key); - assert!( - result.is_err(), - "forced repair failure should make removal fail" - ); + let err = result.expect_err("forced repair failure should make removal fail"); + match err { + InvariantError::Delaunay( + DelaunayTriangulationValidationError::RepairOperationFailed { + operation: DelaunayRepairOperation::VertexRemoval, + source: DelaunayRepairError::NonConvergent { max_flips: 0, .. }, + }, + ) => { + // Expected forced path. + } + other => panic!( + "expected vertex-removal RepairOperationFailed from forced repair path, got {other:?}" + ), + } assert_eq!(dt.number_of_vertices(), vertex_count_before); assert_eq!(dt.number_of_cells(), cell_count_before); @@ -8206,6 +8474,22 @@ mod tests { assert!(dt.as_triangulation().validate().is_ok()); } + macro_rules! gen_remove_vertex_rollback_tests { + ($dim:literal) => { + pastey::paste! { + #[test] + fn []() { + assert_remove_vertex_rollback::<$dim>(); + } + } + }; + } + + gen_remove_vertex_rollback_tests!(2); + gen_remove_vertex_rollback_tests!(3); + gen_remove_vertex_rollback_tests!(4); + gen_remove_vertex_rollback_tests!(5); + #[test] fn test_repair_delaunay_with_flips_allows_pl_manifold() { init_tracing(); @@ -9529,23 +9813,23 @@ mod tests { #[test] fn test_repair_soft_fail_classification() { let nonconvergent = test_hooks::synthetic_nonconvergent_error(); - assert!(TestDelaunay4::can_soft_fail(&nonconvergent)); + assert!(TestDelaunay::<4>::can_soft_fail(&nonconvergent)); let postcondition = DelaunayRepairError::PostconditionFailed { message: "unresolved facet".to_string(), }; - assert!(TestDelaunay4::can_soft_fail(&postcondition)); + assert!(TestDelaunay::<4>::can_soft_fail(&postcondition)); let flip_error = DelaunayRepairError::Flip(FlipError::UnsupportedDimension { dimension: 1 }); - assert!(!TestDelaunay4::can_soft_fail(&flip_error)); + assert!(!TestDelaunay::<4>::can_soft_fail(&flip_error)); let topology_error = DelaunayRepairError::InvalidTopology { required: TopologyGuarantee::PLManifold, found: TopologyGuarantee::Pseudomanifold, message: "local repair requires manifold topology", }; - assert!(!TestDelaunay4::can_soft_fail(&topology_error)); + assert!(!TestDelaunay::<4>::can_soft_fail(&topology_error)); let verification_error = DelaunayRepairError::VerificationFailed { context: "local k=3 postcondition verification", @@ -9553,14 +9837,14 @@ mod tests { message: "bad ridge frame".to_string(), }, }; - assert!(!TestDelaunay4::can_soft_fail(&verification_error)); + assert!(!TestDelaunay::<4>::can_soft_fail(&verification_error)); let canonicalization_error = DelaunayRepairError::OrientationCanonicalizationFailed { message: "after flip repair: broken orientation".to_string(), }; - assert!(!TestDelaunay4::can_soft_fail(&canonicalization_error)); + assert!(!TestDelaunay::<4>::can_soft_fail(&canonicalization_error)); - let mapped_hard = TestDelaunay4::map_hard_repair_error(23, &flip_error); + let mapped_hard = TestDelaunay::<4>::map_hard_repair_error(23, &flip_error); assert!( matches!( mapped_hard, @@ -9573,7 +9857,7 @@ mod tests { ); let geometric_error = DelaunayRepairError::Flip(FlipError::DegenerateCell); - let mapped_geometric = TestDelaunay4::map_hard_repair_error(24, &geometric_error); + let mapped_geometric = TestDelaunay::<4>::map_hard_repair_error(24, &geometric_error); assert!( matches!( mapped_geometric, @@ -9585,7 +9869,7 @@ mod tests { "geometric hard D>=4 repair failures should remain retryable degeneracies: {mapped_geometric:?}" ); - let mapped_verification = TestDelaunay4::map_hard_repair_error(25, &verification_error); + let mapped_verification = TestDelaunay::<4>::map_hard_repair_error(25, &verification_error); assert!( matches!( mapped_verification, @@ -9603,7 +9887,8 @@ mod tests { message: "in_sphere failed".to_string(), }, }; - let mapped_predicate = TestDelaunay4::map_hard_repair_error(26, &predicate_verification); + let mapped_predicate = + TestDelaunay::<4>::map_hard_repair_error(26, &predicate_verification); assert!( matches!( mapped_predicate, @@ -10238,6 +10523,44 @@ mod tests { ); } + #[test] + fn repair_operation_failed_preserves_source() { + let source = DelaunayRepairError::NonConvergent { + max_flips: 7, + diagnostics: Box::new(DelaunayRepairDiagnostics { + facets_checked: 3, + flips_performed: 7, + max_queue_len: 5, + ambiguous_predicates: 0, + ambiguous_predicate_samples: Vec::new(), + predicate_failures: 0, + cycle_detections: 0, + cycle_signature_samples: Vec::new(), + attempt: 1, + queue_order: RepairQueueOrder::Fifo, + }), + }; + let err = DelaunayTriangulationValidationError::RepairOperationFailed { + operation: DelaunayRepairOperation::VertexRemoval, + source, + }; + + let msg = err.to_string(); + assert!(msg.contains("vertex removal")); + match &err { + DelaunayTriangulationValidationError::RepairOperationFailed { + operation: DelaunayRepairOperation::VertexRemoval, + source: DelaunayRepairError::NonConvergent { max_flips: 7, .. }, + } => {} + other => panic!("expected typed vertex-removal repair source, got {other:?}"), + } + let chained = err + .source() + .expect("typed repair failure should expose source error") + .to_string(); + assert!(chained.contains("failed to converge after 7 flips")); + } + #[test] fn test_delaunay_validation_error_tds_variant_display() { let inner = TdsError::InconsistentDataStructure { diff --git a/src/triangulation/delaunayize.rs b/src/triangulation/delaunayize.rs index d2724352..0e9e5536 100644 --- a/src/triangulation/delaunayize.rs +++ b/src/triangulation/delaunayize.rs @@ -147,6 +147,11 @@ impl Default for DelaunayizeConfig { #[non_exhaustive] pub struct DelaunayizeOutcome { /// Statistics from the PL-manifold topology repair pass. + /// + /// If topology repair fails but fallback rebuild succeeds, these remain the + /// failed/default repair stats for the repair attempt. Use + /// [`used_fallback_rebuild`](Self::used_fallback_rebuild) to distinguish + /// successful rebuild recovery from direct topology repair success. pub topology_repair: PlManifoldRepairStats, /// Statistics from the flip-based Delaunay repair pass. pub delaunay_repair: DelaunayRepairStats, @@ -500,6 +505,11 @@ where /// fails /// ([`DelaunayRepairFailedWithRebuild`](DelaunayizeError::DelaunayRepairFailedWithRebuild)). /// +/// When topology repair fails and fallback rebuild succeeds, this function +/// returns `Ok` with `used_fallback_rebuild = true` and +/// `topology_repair.succeeded = false`; the topology pass is not reported as +/// successful merely because rebuild recovered the workflow. +/// /// The `*WithRebuild` variants preserve both errors as typed fields so /// consumers can inspect both typed errors; /// [`Error::source`](std::error::Error::source) exposes the primary repair error. @@ -556,10 +566,7 @@ where Ok(rebuilt) => { *dt = rebuilt; return Ok(DelaunayizeOutcome { - topology_repair: PlManifoldRepairStats { - succeeded: true, - ..PlManifoldRepairStats::default() - }, + topology_repair: PlManifoldRepairStats::default(), delaunay_repair: DelaunayRepairStats::default(), used_fallback_rebuild: true, }); @@ -984,6 +991,46 @@ mod tests { assert!(!outcome.used_fallback_rebuild); } + #[test] + fn topology_repair_fallback_stats_failed() { + init_tracing(); + let vertices = [ + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + vertex!([0.0, 1.0]), + ]; + let mut dt: DelaunayTriangulation<_, (), (), 2> = + DelaunayTriangulation::new(&vertices).unwrap(); + + let (_, existing_cell) = dt.cells().next().unwrap(); + let duplicate_vertices = existing_cell.vertices().to_vec(); + dt.tri + .tds + .insert_cell_with_mapping(Cell::new(duplicate_vertices.clone(), None).unwrap()) + .unwrap(); + dt.tri + .tds + .insert_cell_with_mapping(Cell::new(duplicate_vertices, None).unwrap()) + .unwrap(); + + let outcome = delaunayize_by_flips( + &mut dt, + DelaunayizeConfig { + topology_max_cells_removed: 0, + fallback_rebuild: true, + ..DelaunayizeConfig::default() + }, + ) + .unwrap(); + + assert!(outcome.used_fallback_rebuild); + assert!( + !outcome.topology_repair.succeeded, + "fallback rebuild should not mark the failed topology repair as succeeded" + ); + assert!(dt.validate().is_ok()); + } + #[test] fn test_rebuild_restores_cell_data() { init_tracing(); diff --git a/tests/delaunay_public_api_coverage.rs b/tests/delaunay_public_api_coverage.rs new file mode 100644 index 00000000..ad700bec --- /dev/null +++ b/tests/delaunay_public_api_coverage.rs @@ -0,0 +1,153 @@ +//! Public API integration coverage for `DelaunayTriangulation`. + +#![forbid(unsafe_code)] + +use delaunay::prelude::geometry::AdaptiveKernel; +use delaunay::prelude::triangulation::{ + ConstructionOptions, DedupPolicy, DelaunayTriangulation, + DelaunayTriangulationConstructionError, InsertionOrderStrategy, RetryPolicy, TopologyGuarantee, +}; +use delaunay::vertex; +use rand::{RngExt, SeedableRng, rngs::StdRng}; + +type Dt = DelaunayTriangulation, (), (), D>; + +#[test] +fn topology_options_smoke_3d() { + let vertices = vec![ + vertex!([0.0, 0.0, 0.0]), + vertex!([1.0, 0.0, 0.0]), + vertex!([0.0, 1.0, 0.0]), + vertex!([0.0, 0.0, 1.0]), + ]; + let options = ConstructionOptions::default() + .with_dedup_policy(DedupPolicy::Exact) + .with_insertion_order(InsertionOrderStrategy::Input) + .with_retry_policy(RetryPolicy::Disabled); + + let dt = Dt::<3>::with_topology_guarantee_and_options( + &AdaptiveKernel::new(), + &vertices, + TopologyGuarantee::PLManifold, + options, + ) + .expect("3D construction with explicit options should succeed"); + + assert_eq!(dt.number_of_vertices(), 4); + assert_eq!(dt.number_of_cells(), 1); + assert!(dt.validate().is_ok()); +} + +#[test] +fn statistics_default_on_preprocess_error() { + let vertices = vec![ + vertex!([0.0, 0.0, 0.0]), + vertex!([1.0, 0.0, 0.0]), + vertex!([0.0, 1.0, 0.0]), + vertex!([0.0, 0.0, 1.0]), + ]; + let options = ConstructionOptions::default().with_dedup_policy(DedupPolicy::Epsilon { + tolerance: f64::NAN, + }); + + let result = Dt::<3>::with_topology_guarantee_and_options_with_construction_statistics( + &AdaptiveKernel::new(), + &vertices, + TopologyGuarantee::PLManifold, + options, + ); + + let err = result.expect_err("NaN epsilon should fail during preprocessing"); + assert_eq!(err.statistics.inserted, 0); + assert_eq!(err.statistics.total_skipped(), 0); + assert_eq!(err.statistics.total_attempts, 0); + assert!(err.statistics.skip_samples.is_empty()); + assert!(matches!( + err.error, + DelaunayTriangulationConstructionError::Triangulation(_) + )); +} + +#[test] +#[allow(deprecated)] +fn as_triangulation_mut_valid_view() { + let vertices = vec![ + vertex!([0.0, 0.0]), + vertex!([1.0, 0.0]), + vertex!([0.0, 1.0]), + ]; + let mut dt: DelaunayTriangulation<_, (), (), 2> = + DelaunayTriangulation::new(&vertices).expect("2D construction should succeed"); + + dt.insert(vertex!([0.2, 0.2])) + .expect("interior insertion should succeed"); + + let tri = dt.as_triangulation_mut(); + assert!(tri.is_valid().is_ok()); +} + +/// Slow search helper to find a natural stale-key repro case. +/// +/// This remains ignored by default because it is nondeterministic and expensive. +/// For deterministic coverage, see the forced heuristic rebuild tests in +/// `src/triangulation/delaunay.rs`. +#[test] +#[ignore = "manual search helper; run explicitly to discover natural repro cases"] +fn find_stale_key_after_rebuild() { + const DIM: usize = 4; + const INITIAL_COUNT: usize = 12; + const CASES: usize = 2_000; + + // This probes for a configuration that triggers a heuristic rebuild during automatic + // flip-repair after insertion, which historically could invalidate the returned VertexKey. + let mut rng = StdRng::seed_from_u64(0xD3_1A_7A_1C_0A_17_u64); + + for case in 0..CASES { + let mut vertices = Vec::with_capacity(INITIAL_COUNT); + for _ in 0..INITIAL_COUNT { + // Use a coarse lattice + tiny noise to encourage near-degenerate configurations. + let mut coords = [0.0_f64; DIM]; + for c in &mut coords { + let base: i32 = rng.random_range(-3..=3); + let noise: f64 = rng.random_range(-1.0e-6..=1.0e-6); + *c = f64::from(base) + noise; + } + vertices.push(vertex!(coords)); + } + + let Ok(mut dt) = Dt::<4>::new(&vertices) else { + continue; + }; + + let mut inserted_coords = [0.0_f64; DIM]; + for c in &mut inserted_coords { + let base: i32 = rng.random_range(-3..=3); + let noise: f64 = rng.random_range(-1.0e-6..=1.0e-6); + *c = f64::from(base) + noise; + } + let inserted = vertex!(inserted_coords); + let inserted_uuid = inserted.uuid(); + + let Ok(vertex_key) = dt.insert(inserted) else { + continue; + }; + + let found = dt + .tds() + .get_vertex_by_key(vertex_key) + .is_some_and(|v| v.uuid() == inserted_uuid); + + if !found { + tracing::debug!(case, "FOUND stale key after insertion"); + tracing::debug!(vertex_key = ?vertex_key, inserted_uuid = %inserted_uuid); + tracing::debug!("initial vertices:"); + for v in &vertices { + tracing::debug!(coords = ?v.point().coords(), " vertex"); + } + tracing::debug!("inserted vertex coords: {inserted_coords:?}"); + panic!("stale VertexKey returned from insert() after heuristic rebuild"); + } + } + + panic!("no stale-key case found after {CASES} attempts"); +} diff --git a/tests/prelude_exports.rs b/tests/prelude_exports.rs index 824e7c42..45a25bfa 100644 --- a/tests/prelude_exports.rs +++ b/tests/prelude_exports.rs @@ -7,9 +7,18 @@ use delaunay::prelude::generators::generate_random_points_seeded; use delaunay::prelude::geometry::{AdaptiveKernel, Point}; use delaunay::prelude::query::ConvexHull; +use delaunay::prelude::triangulation::delaunayize::{ + DelaunayizeConfig, DelaunayizeError, DelaunayizeOutcome, delaunayize_by_flips, +}; use delaunay::prelude::triangulation::flips::{BistellarFlips, TopologyGuarantee}; +use delaunay::prelude::triangulation::repair::{ + DelaunayCheckPolicy, DelaunayRepairDiagnostics, DelaunayRepairError, DelaunayRepairOutcome, + DelaunayRepairPolicy, DelaunayRepairStats, FlipError, RepairQueueOrder, + verify_delaunay_for_triangulation, +}; use delaunay::prelude::triangulation::{ - ConstructionOptions, DelaunayTriangulation, InsertionOrderStrategy, Vertex, + ConstructionOptions, DelaunayRepairOperation, DelaunayTriangulation, + DelaunayTriangulationValidationError, InsertionOrderStrategy, Vertex, }; use delaunay::vertex; @@ -36,3 +45,56 @@ fn preludes_cover_bench_apis() { assert!(dt.validate().is_ok()); assert_bistellar_flips(&dt); } + +#[test] +fn diagnostic_preludes_cover_repair_apis() { + let vertices: Vec> = vec![ + vertex!([0.0, 0.0, 0.0]), + vertex!([1.0, 0.0, 0.0]), + vertex!([0.0, 1.0, 0.0]), + vertex!([0.0, 0.0, 1.0]), + ]; + let mut dt = DelaunayTriangulation::new(&vertices).unwrap(); + + let repair_stats = DelaunayRepairStats::default(); + let repair_outcome = DelaunayRepairOutcome { + stats: repair_stats, + heuristic: None, + }; + assert!(!repair_outcome.used_heuristic()); + assert_eq!( + DelaunayRepairPolicy::default(), + DelaunayRepairPolicy::EveryInsertion + ); + assert!(!DelaunayCheckPolicy::default().should_check(1)); + assert_eq!(RepairQueueOrder::Fifo, RepairQueueOrder::Fifo); + let diagnostics = DelaunayRepairDiagnostics { + facets_checked: 0, + flips_performed: 0, + max_queue_len: 0, + ambiguous_predicates: 0, + ambiguous_predicate_samples: Vec::new(), + predicate_failures: 0, + cycle_detections: 0, + cycle_signature_samples: Vec::new(), + attempt: 1, + queue_order: RepairQueueOrder::Fifo, + }; + assert!(diagnostics.to_string().contains("checked")); + assert!(matches!( + DelaunayRepairError::Flip(FlipError::DegenerateCell), + DelaunayRepairError::Flip(_) + )); + let validation_error = DelaunayTriangulationValidationError::RepairOperationFailed { + operation: DelaunayRepairOperation::VertexRemoval, + source: DelaunayRepairError::Flip(FlipError::DegenerateCell), + }; + assert!(validation_error.to_string().contains("vertex removal")); + + verify_delaunay_for_triangulation(dt.as_triangulation()).unwrap(); + + let outcome = delaunayize_by_flips(&mut dt, DelaunayizeConfig::default()).unwrap(); + assert!(!outcome.used_fallback_rebuild); + let _typed_outcome: DelaunayizeOutcome = outcome; + let _typed_error: Option = None; +} From a460305ddbfada8124b582f9642ef39d7a9bddb5 Mon Sep 17 00:00:00 2001 From: Adam Getchell Date: Wed, 29 Apr 2026 06:45:15 -0700 Subject: [PATCH 3/4] fix: harden repair diagnostics and cache rollback - Preserve legacy repair-summary sample formatting while keeping typed diagnostic samples. - Exercise dynamic flip context guards across D=2 through D=5. - Invalidate insertion caches consistently before repair-driven TDS mutations. - Assert remove-vertex rollback restores insertion hints and spatial-index state. - Gate public API coverage debug tracing behind the test-debug feature. --- src/core/algorithms/flips.rs | 186 +++++++++++++++++++--- src/core/collections/spatial_hash_grid.rs | 27 ++++ src/triangulation/delaunay.rs | 52 +++++- tests/delaunay_public_api_coverage.rs | 12 +- 4 files changed, 252 insertions(+), 25 deletions(-) diff --git a/src/core/algorithms/flips.rs b/src/core/algorithms/flips.rs index 51e9a7cc..b6cee27a 100644 --- a/src/core/algorithms/flips.rs +++ b/src/core/algorithms/flips.rs @@ -4355,26 +4355,26 @@ struct RepairDiagnostics { postcondition_facet_debug_emitted: usize, } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] struct InsertedSimplexSkipSample { location: RepairSkipLocation, removed_face: VertexKeyList, inserted_face: VertexKeyList, } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Clone, Copy, PartialEq, Eq)] struct RidgeMultiplicitySkipSample { ridge: RidgeHandle, multiplicity: usize, } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Clone, Copy, PartialEq, Eq)] struct MissingCellSkipSample { location: RepairSkipLocation, cell_key: CellKey, } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Clone, Copy, PartialEq, Eq)] enum RepairSkipLocation { Edge(EdgeKey), Facet(FacetHandle), @@ -4382,6 +4382,75 @@ enum RepairSkipLocation { Triangle(TriangleHandle), } +impl RepairSkipLocation { + fn fmt_label(self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Edge(edge) => write!(f, "edge={edge:?}"), + Self::Facet(facet) => write!(f, "facet={facet:?}"), + Self::Ridge(ridge) => write!(f, "ridge={ridge:?}"), + Self::Triangle(triangle) => write!(f, "triangle={triangle:?}"), + } + } +} + +impl fmt::Display for RepairSkipLocation { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.fmt_label(f) + } +} + +impl fmt::Debug for RepairSkipLocation { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(self, f) + } +} + +impl fmt::Display for InsertedSimplexSkipSample { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.location.fmt_label(f)?; + write!( + f, + " removed_face={:?} inserted_face={:?}", + self.removed_face, self.inserted_face + ) + } +} + +impl fmt::Debug for InsertedSimplexSkipSample { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&self.to_string(), f) + } +} + +impl fmt::Display for RidgeMultiplicitySkipSample { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "ridge={:?} multiplicity={}", + self.ridge, self.multiplicity + ) + } +} + +impl fmt::Debug for RidgeMultiplicitySkipSample { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&self.to_string(), f) + } +} + +impl fmt::Display for MissingCellSkipSample { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.location.fmt_label(f)?; + write!(f, " missing_cell={:?}", self.cell_key) + } +} + +impl fmt::Debug for MissingCellSkipSample { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&self.to_string(), f) + } +} + fn vertex_key_list(vertices: &[VertexKey]) -> VertexKeyList { vertices.iter().copied().collect() } @@ -7979,6 +8048,58 @@ mod tests { assert_eq!(diagnostics.missing_cell_sample, Some(first_missing_sample)); } + #[test] + fn test_repair_skip_samples_keep_legacy_debug_shape() { + let cell = CellKey::from(KeyData::from_ffi(91)); + let missing_cell = CellKey::from(KeyData::from_ffi(92)); + let v0 = VertexKey::from(KeyData::from_ffi(101)); + let v1 = VertexKey::from(KeyData::from_ffi(102)); + let v2 = VertexKey::from(KeyData::from_ffi(103)); + let facet = FacetHandle::new(cell, 0); + let ridge = RidgeHandle::new(cell, 0, 1); + let triangle = TriangleHandle::new(v0, v1, v2); + + let removed_face: VertexKeyList = [v0, v1].into_iter().collect(); + let inserted_face: VertexKeyList = std::iter::once(v2).collect(); + let inserted_sample = InsertedSimplexSkipSample { + location: RepairSkipLocation::Facet(facet), + removed_face: removed_face.clone(), + inserted_face: inserted_face.clone(), + }; + assert_eq!( + format!("{:?}", Some(inserted_sample)), + format!( + "{:?}", + Some(format!( + "facet={facet:?} removed_face={removed_face:?} inserted_face={inserted_face:?}" + )) + ) + ); + + let ridge_sample = RidgeMultiplicitySkipSample { + ridge, + multiplicity: 3, + }; + assert_eq!( + format!("{:?}", Some(ridge_sample)), + format!("{:?}", Some(format!("ridge={ridge:?} multiplicity=3"))) + ); + + let missing_sample = MissingCellSkipSample { + location: RepairSkipLocation::Triangle(triangle), + cell_key: missing_cell, + }; + assert_eq!( + format!("{:?}", Some(missing_sample)), + format!( + "{:?}", + Some(format!( + "triangle={triangle:?} missing_cell={missing_cell:?}" + )) + ) + ); + } + #[derive(Debug, Clone, PartialEq, Eq)] struct TopologySnapshot { vertex_uuids: Vec, @@ -8238,21 +8359,30 @@ mod tests { test_bistellar_roundtrip_dimension!(4, k3); test_bistellar_roundtrip_dimension!(5, k3); - #[test] - fn dynamic_flip_rejects_bad_context() { + fn synthetic_vertex_key(index: u64) -> VertexKey { + VertexKey::from(KeyData::from_ffi(index)) + } + + fn synthetic_cell_key(index: u64) -> CellKey { + CellKey::from(KeyData::from_ffi(index)) + } + + fn dynamic_flip_rejects_bad_context_for_dimension() { init_tracing(); - let mut tds: Tds = Tds::empty(); - let v0 = VertexKey::from(KeyData::from_ffi(1)); - let v1 = VertexKey::from(KeyData::from_ffi(2)); - let v2 = VertexKey::from(KeyData::from_ffi(3)); - let v3 = VertexKey::from(KeyData::from_ffi(4)); - let v4 = VertexKey::from(KeyData::from_ffi(5)); - let c0 = CellKey::from(KeyData::from_ffi(11)); - let c1 = CellKey::from(KeyData::from_ffi(12)); + let mut tds: Tds = Tds::empty(); + let vertices = (1..=D + 2) + .map(|index| { + synthetic_vertex_key( + u64::try_from(index).expect("test vertex key index should fit in u64"), + ) + }) + .collect::>(); + let c0 = synthetic_cell_key(11); + let c1 = synthetic_cell_key(12); let valid_shape = FlipContextDyn { - removed_face_vertices: [v0, v1, v2].into_iter().collect(), - inserted_face_vertices: [v3, v4].into_iter().collect(), + removed_face_vertices: vertices[..D].iter().copied().collect(), + inserted_face_vertices: vertices[D..D + 2].iter().copied().collect(), removed_cells: [c0, c1].into_iter().collect(), direction: FlipDirection::Forward, }; @@ -8262,12 +8392,12 @@ mod tests { Err(FlipError::InvalidFlipContext { ref message }) if message.contains("k must be") )); assert!(matches!( - apply_bistellar_flip_dynamic(&mut tds, 5, &valid_shape), + apply_bistellar_flip_dynamic(&mut tds, D + 2, &valid_shape), Err(FlipError::InvalidFlipContext { ref message }) if message.contains("k must be") )); let wrong_removed_face = FlipContextDyn { - removed_face_vertices: [v0, v1].into_iter().collect(), + removed_face_vertices: vertices[..D - 1].iter().copied().collect(), ..valid_shape.clone() }; assert!(matches!( @@ -8277,7 +8407,7 @@ mod tests { )); let wrong_inserted_face = FlipContextDyn { - inserted_face_vertices: once(v3).collect(), + inserted_face_vertices: once(vertices[D]).collect(), ..valid_shape.clone() }; assert!(matches!( @@ -8297,7 +8427,7 @@ mod tests { )); let overlapping_faces = FlipContextDyn { - inserted_face_vertices: [v2, v3].into_iter().collect(), + inserted_face_vertices: [vertices[D - 1], vertices[D]].into_iter().collect(), ..valid_shape }; assert!(matches!( @@ -8309,6 +8439,22 @@ mod tests { assert_eq!(tds.number_of_cells(), 0); } + macro_rules! gen_dynamic_flip_bad_context_tests { + ($dim:literal) => { + pastey::paste! { + #[test] + fn []() { + dynamic_flip_rejects_bad_context_for_dimension::<$dim>(); + } + } + }; + } + + gen_dynamic_flip_bad_context_tests!(2); + gen_dynamic_flip_bad_context_tests!(3); + gen_dynamic_flip_bad_context_tests!(4); + gen_dynamic_flip_bad_context_tests!(5); + #[test] fn test_flip_k2_2d_edge_flip() { init_tracing(); diff --git a/src/core/collections/spatial_hash_grid.rs b/src/core/collections/spatial_hash_grid.rs index 4004d57f..51cae89f 100644 --- a/src/core/collections/spatial_hash_grid.rs +++ b/src/core/collections/spatial_hash_grid.rs @@ -72,6 +72,14 @@ pub struct HashGridIndex { cells: FastHashMap, SmallBuffer>, } +#[cfg(test)] +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct HashGridIndexSnapshot { + cell_size: String, + usable: bool, + cells: Vec, +} + impl HashGridIndex where T: CoordinateScalar, @@ -93,6 +101,25 @@ where self.cell_size } + #[cfg(test)] + pub(crate) fn debug_snapshot(&self) -> HashGridIndexSnapshot + where + T: std::fmt::Debug, + K: std::fmt::Debug, + { + let mut cells = self + .cells + .iter() + .map(|(key, bucket)| format!("{key:?}={bucket:?}")) + .collect::>(); + cells.sort(); + HashGridIndexSnapshot { + cell_size: format!("{:?}", self.cell_size), + usable: self.usable, + cells, + } + } + pub fn clear(&mut self) { self.cells.clear(); } diff --git a/src/triangulation/delaunay.rs b/src/triangulation/delaunay.rs index fa73ccb0..4be0a0ee 100644 --- a/src/triangulation/delaunay.rs +++ b/src/triangulation/delaunay.rs @@ -3503,6 +3503,7 @@ where ); let escalation_result = { + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); repair_delaunay_local_single_pass(tds, kernel, &full_seeds, escalated_budget) }; @@ -3700,6 +3701,7 @@ where if !seed_cells.is_empty() { let max_flips = local_repair_flip_budget::(seed_cells.len()); let repair_result = { + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); repair_delaunay_local_single_pass( tds, @@ -3723,6 +3725,7 @@ where } Err(repair_err) => { if D < 4 { + self.invalidate_repair_caches(); Self::try_d_lt4_global_repair_fallback( &mut self.tri.tds, &self.tri.kernel, @@ -3966,6 +3969,7 @@ where if !seed_cells.is_empty() { let max_flips = local_repair_flip_budget::(seed_cells.len()); let repair_result = { + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); repair_delaunay_local_single_pass( tds, @@ -3989,6 +3993,7 @@ where } Err(repair_err) => { if D < 4 { + self.invalidate_repair_caches(); Self::try_d_lt4_global_repair_fallback( &mut self.tri.tds, &self.tri.kernel, @@ -4209,6 +4214,7 @@ where ); let repair_started = Instant::now(); let repair_result = { + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); repair_delaunay_local_single_pass(tds, kernel, &all_cells, 512).map(|_| ()) }; @@ -4228,6 +4234,7 @@ where let repair_started = Instant::now(); let max_flips = (soft_fail_seeds.len() * (D + 1) * 16).max(512); let repair_result = { + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); repair_delaunay_local_single_pass(tds, kernel, soft_fail_seeds, max_flips) .map(|_| ()) @@ -4694,9 +4701,13 @@ where #[cfg(test)] pub(crate) fn tds_mut(&mut self) -> &mut Tds { // Direct mutable access can invalidate performance caches. + self.invalidate_repair_caches(); + &mut self.tri.tds + } + + fn invalidate_repair_caches(&mut self) { self.insertion_state.last_inserted_cell = None; self.spatial_index = None; - &mut self.tri.tds } /// Returns mutable TDS access for crate-internal repair algorithms. @@ -4704,8 +4715,7 @@ where /// Repair passes may rewrite topology and invalidate locate hints, so this /// deliberately clears the ephemeral caches before handing out the borrow. pub(crate) fn tds_mut_for_repair(&mut self) -> &mut Tds { - self.insertion_state.last_inserted_cell = None; - self.spatial_index = None; + self.invalidate_repair_caches(); &mut self.tri.tds } @@ -4926,6 +4936,7 @@ where message: "Bistellar flips require a PL-manifold (vertex-link validation)", }); } + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); let stats = repair_delaunay_with_flips_k2_k3(tds, kernel, None, topology, max_flips)?; @@ -4965,6 +4976,7 @@ where ) -> Result { let topology = self.tri.topology_guarantee(); let kernel = RobustKernel::::new(); + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &kernel); repair_delaunay_with_flips_k2_k3_run(tds, kernel, seed_cells, topology, max_flips) } @@ -5274,6 +5286,7 @@ where let _ = (rebuild_repair_policy, rebuild_check_policy); let topology = candidate.tri.topology_guarantee(); + candidate.invalidate_repair_caches(); let (tds, kernel) = (&mut candidate.tri.tds, &candidate.tri.kernel); let stats = repair_delaunay_with_flips_k2_k3( tds, @@ -6143,6 +6156,7 @@ where }; let repair_result = { + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); repair_delaunay_with_flips_k2_k3_run(tds, kernel, seed_ref, topology, max_flips) }; @@ -6387,6 +6401,7 @@ where if self.should_run_delaunay_repair_for(topology, 0) { let seed_ref = seed_cells.as_deref(); let repair_result = { + self.invalidate_repair_caches(); let (tds, kernel) = (&mut self.tri.tds, &self.tri.kernel); repair_delaunay_with_flips_k2_k3(tds, kernel, seed_ref, topology, None) }; @@ -8411,6 +8426,13 @@ mod tests { DelaunayTriangulation::new(&vertices).unwrap(); let vertex_key = dt.insert(vertex!([0.25, 0.25])).unwrap(); + let hint_cell = dt.cells().next().map(|(key, _)| key); + dt.insertion_state.last_inserted_cell = hint_cell; + let mut spatial_index = HashGridIndex::::new(1.0); + for (vertex_key, vertex) in dt.vertices() { + spatial_index.insert_vertex(vertex_key, vertex.point().coords()); + } + dt.spatial_index = Some(spatial_index); assert!(dt.insertion_state.last_inserted_cell.is_some()); assert!(dt.spatial_index.is_some()); @@ -8450,6 +8472,18 @@ mod tests { .expect("Inserted vertex not found"); let vertex_count_before = dt.number_of_vertices(); let cell_count_before = dt.number_of_cells(); + let hint_cell_before = dt.cells().next().map(|(key, _)| key); + dt.insertion_state.last_inserted_cell = hint_cell_before; + let mut spatial_index = HashGridIndex::::new(1.0); + for (vertex_key, vertex) in dt.vertices() { + spatial_index.insert_vertex(vertex_key, vertex.point().coords()); + } + dt.spatial_index = Some(spatial_index); + let last_inserted_cell_before = dt.insertion_state.last_inserted_cell; + let spatial_index_before = dt + .spatial_index + .as_ref() + .map(HashGridIndex::::debug_snapshot); let _guard = ForceRepairNonconvergentGuard::enable(); let result = dt.remove_vertex(vertex_key); @@ -8470,6 +8504,17 @@ mod tests { assert_eq!(dt.number_of_vertices(), vertex_count_before); assert_eq!(dt.number_of_cells(), cell_count_before); + assert_eq!( + dt.insertion_state.last_inserted_cell, last_inserted_cell_before, + "remove_vertex rollback should restore last_inserted_cell" + ); + assert_eq!( + dt.spatial_index + .as_ref() + .map(HashGridIndex::::debug_snapshot), + spatial_index_before, + "remove_vertex rollback should restore spatial_index" + ); assert!(dt.vertices().any(|(_, v)| v.uuid() == inserted_uuid)); assert!(dt.as_triangulation().validate().is_ok()); } @@ -9324,6 +9369,7 @@ mod tests { let mut dt: DelaunayTriangulation<_, (), (), 2> = DelaunayTriangulation::new(&vertices).unwrap(); + dt.set_delaunay_repair_policy(DelaunayRepairPolicy::Never); // Initially no last_inserted_cell assert!(dt.insertion_state.last_inserted_cell.is_none()); diff --git a/tests/delaunay_public_api_coverage.rs b/tests/delaunay_public_api_coverage.rs index ad700bec..cace8a02 100644 --- a/tests/delaunay_public_api_coverage.rs +++ b/tests/delaunay_public_api_coverage.rs @@ -103,6 +103,9 @@ fn find_stale_key_after_rebuild() { let mut rng = StdRng::seed_from_u64(0xD3_1A_7A_1C_0A_17_u64); for case in 0..CASES { + #[cfg(not(feature = "test-debug"))] + let _ = case; + let mut vertices = Vec::with_capacity(INITIAL_COUNT); for _ in 0..INITIAL_COUNT { // Use a coarse lattice + tiny noise to encourage near-degenerate configurations. @@ -137,7 +140,12 @@ fn find_stale_key_after_rebuild() { .get_vertex_by_key(vertex_key) .is_some_and(|v| v.uuid() == inserted_uuid); - if !found { + if found { + continue; + } + + #[cfg(feature = "test-debug")] + { tracing::debug!(case, "FOUND stale key after insertion"); tracing::debug!(vertex_key = ?vertex_key, inserted_uuid = %inserted_uuid); tracing::debug!("initial vertices:"); @@ -145,8 +153,8 @@ fn find_stale_key_after_rebuild() { tracing::debug!(coords = ?v.point().coords(), " vertex"); } tracing::debug!("inserted vertex coords: {inserted_coords:?}"); - panic!("stale VertexKey returned from insert() after heuristic rebuild"); } + panic!("stale VertexKey returned from insert() after heuristic rebuild"); } panic!("no stale-key case found after {CASES} attempts"); From a95c61056f5bb5e1f25a3c020f47c10b85cb5423 Mon Sep 17 00:00:00 2001 From: Adam Getchell Date: Wed, 29 Apr 2026 08:30:44 -0700 Subject: [PATCH 4/4] test: cover remove-vertex fallback rollback - Add D=2..=5 rollback coverage for the non-k1 remove_vertex fallback path. - Reuse cache and topology rollback assertions across fast-path and fallback removal tests. - Compile the stale-key investigation helper only with the test-debug feature. --- src/triangulation/delaunay.rs | 102 +++++++++++++++++++++----- tests/delaunay_public_api_coverage.rs | 5 +- 2 files changed, 87 insertions(+), 20 deletions(-) diff --git a/src/triangulation/delaunay.rs b/src/triangulation/delaunay.rs index 4be0a0ee..db10fb2a 100644 --- a/src/triangulation/delaunay.rs +++ b/src/triangulation/delaunay.rs @@ -8452,24 +8452,31 @@ mod tests { vertex!([coord; D]) } - fn assert_remove_vertex_rollback() { - init_tracing(); - let vertices = simplex_vertices::(); - - let mut dt: DelaunayTriangulation, (), (), D> = - DelaunayTriangulation::new(&vertices).unwrap(); - dt.set_topology_guarantee(TopologyGuarantee::PLManifold); - - let cell_key = dt.cells().next().unwrap().0; - let inserted_vertex = interior_vertex_for_k1_insert::(); - let inserted_uuid = inserted_vertex.uuid(); - dt.flip_k1_insert(cell_key, inserted_vertex).unwrap(); + fn rollback_probe_vertex(point_index: usize) -> Vertex { + let dimension = + safe_usize_to_scalar::(D).expect("test dimensions should convert exactly"); + let point_index_scalar = + safe_usize_to_scalar::(point_index).expect("point index should convert exactly"); + let mut coords = [0.2 / dimension; D]; + let axis = point_index % D; + coords[axis] += point_index_scalar.mul_add(0.005, 0.02); + vertex!(coords) + } + + fn incident_cell_count( + dt: &DelaunayTriangulation, (), (), D>, + vertex_key: VertexKey, + ) -> usize { + dt.cells() + .filter(|(_, cell)| cell.vertices().contains(&vertex_key)) + .count() + } - let vertex_key = dt - .vertices() - .find(|(_, v)| v.uuid() == inserted_uuid) - .map(|(k, _)| k) - .expect("Inserted vertex not found"); + fn assert_forced_remove_vertex_rolls_back( + dt: &mut DelaunayTriangulation, (), (), D>, + vertex_key: VertexKey, + inserted_uuid: Uuid, + ) { let vertex_count_before = dt.number_of_vertices(); let cell_count_before = dt.number_of_cells(); let hint_cell_before = dt.cells().next().map(|(key, _)| key); @@ -8519,6 +8526,62 @@ mod tests { assert!(dt.as_triangulation().validate().is_ok()); } + fn assert_remove_vertex_rollback() { + init_tracing(); + let vertices = simplex_vertices::(); + + let mut dt: DelaunayTriangulation, (), (), D> = + DelaunayTriangulation::new(&vertices).unwrap(); + dt.set_topology_guarantee(TopologyGuarantee::PLManifold); + + let cell_key = dt.cells().next().unwrap().0; + let inserted_vertex = interior_vertex_for_k1_insert::(); + let inserted_uuid = inserted_vertex.uuid(); + dt.flip_k1_insert(cell_key, inserted_vertex).unwrap(); + + let vertex_key = dt + .vertices() + .find(|(_, v)| v.uuid() == inserted_uuid) + .map(|(k, _)| k) + .expect("Inserted vertex not found"); + + assert_forced_remove_vertex_rolls_back(&mut dt, vertex_key, inserted_uuid); + } + + fn assert_remove_vertex_fallback_rollback() { + init_tracing(); + let vertices = simplex_vertices::(); + + let mut dt: DelaunayTriangulation, (), (), D> = + DelaunayTriangulation::new(&vertices).unwrap(); + dt.set_topology_guarantee(TopologyGuarantee::PLManifold); + + let mut inserted_vertices = Vec::new(); + for point_index in 0..(D + 3) { + let inserted_vertex = rollback_probe_vertex::(point_index); + let inserted_uuid = inserted_vertex.uuid(); + let vertex_key = dt + .insert(inserted_vertex) + .expect("rollback fallback fixture insertion should succeed"); + inserted_vertices.push((vertex_key, inserted_uuid)); + } + + let (vertex_key, inserted_uuid, incident_cells) = inserted_vertices + .iter() + .find_map(|&(vertex_key, inserted_uuid)| { + let incident_cells = incident_cell_count(&dt, vertex_key); + (incident_cells != D + 1).then_some((vertex_key, inserted_uuid, incident_cells)) + }) + .expect("expected at least one inserted vertex with a non-simplex star"); + assert_ne!( + incident_cells, + D + 1, + "fallback rollback fixture must avoid the inverse-k=1 simplex-star path" + ); + + assert_forced_remove_vertex_rolls_back(&mut dt, vertex_key, inserted_uuid); + } + macro_rules! gen_remove_vertex_rollback_tests { ($dim:literal) => { pastey::paste! { @@ -8526,6 +8589,11 @@ mod tests { fn []() { assert_remove_vertex_rollback::<$dim>(); } + + #[test] + fn []() { + assert_remove_vertex_fallback_rollback::<$dim>(); + } } }; } diff --git a/tests/delaunay_public_api_coverage.rs b/tests/delaunay_public_api_coverage.rs index cace8a02..711a4596 100644 --- a/tests/delaunay_public_api_coverage.rs +++ b/tests/delaunay_public_api_coverage.rs @@ -8,6 +8,7 @@ use delaunay::prelude::triangulation::{ DelaunayTriangulationConstructionError, InsertionOrderStrategy, RetryPolicy, TopologyGuarantee, }; use delaunay::vertex; +#[cfg(feature = "test-debug")] use rand::{RngExt, SeedableRng, rngs::StdRng}; type Dt = DelaunayTriangulation, (), (), D>; @@ -91,6 +92,7 @@ fn as_triangulation_mut_valid_view() { /// This remains ignored by default because it is nondeterministic and expensive. /// For deterministic coverage, see the forced heuristic rebuild tests in /// `src/triangulation/delaunay.rs`. +#[cfg(feature = "test-debug")] #[test] #[ignore = "manual search helper; run explicitly to discover natural repro cases"] fn find_stale_key_after_rebuild() { @@ -103,9 +105,6 @@ fn find_stale_key_after_rebuild() { let mut rng = StdRng::seed_from_u64(0xD3_1A_7A_1C_0A_17_u64); for case in 0..CASES { - #[cfg(not(feature = "test-debug"))] - let _ = case; - let mut vertices = Vec::with_capacity(INITIAL_COUNT); for _ in 0..INITIAL_COUNT { // Use a coarse lattice + tiny noise to encourage near-degenerate configurations.