Merge pull request #2367 from hannobraun/validation

Port reference counting validation checks for `Sketch` to new infrastructure

crates/fj-core/src/validate/mod.rs

@@ -65,7 +65,6 @@ mod curve;
 mod cycle;
 mod face;
 mod half_edge;
-mod references;
 mod region;
 mod shell;
 mod sketch;
@@ -78,10 +77,7 @@ use crate::{
     validation::{ValidationConfig, ValidationError},
 };
 
-pub use self::{
-    references::ObjectNotExclusivelyOwned, sketch::SketchValidationError,
-    solid::SolidValidationError,
-};
+pub use self::{sketch::SketchValidationError, solid::SolidValidationError};
 
 /// Assert that some object has a validation error which matches a specific
 /// pattern. This is preferred to matching on [`Validate::validate_and_return_first_error`], since usually we don't care about the order.

crates/fj-core/src/validate/references.rs

@@ -1,68 +0,0 @@
-use std::{any::type_name_of_val, collections::HashMap, fmt};
-
-use crate::storage::Handle;
-
-/// Object that should be exclusively owned by another, is not
-///
-/// Some objects are expected to be "owned" by a single other object. This means
-/// that only one reference to these objects must exist within the topological
-/// object graph.
-#[derive(Clone, Debug, thiserror::Error)]
-pub struct ObjectNotExclusivelyOwned<T, U> {
-    object: Handle<T>,
-    referenced_by: Vec<Handle<U>>,
-}
-
-impl<T, U> fmt::Display for ObjectNotExclusivelyOwned<T, U>
-where
-    T: fmt::Debug,
-    U: fmt::Debug,
-{
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
-            f,
-            "`{}` ({:?}) referenced by multiple `{}` objects ({:?})",
-            type_name_of_val(&self.object),
-            self.object,
-            type_name_of_val(&self.referenced_by),
-            self.referenced_by
-        )
-    }
-}
-
-#[derive(Default)]
-pub struct ReferenceCounter<T, U>(HashMap<Handle<T>, Vec<Handle<U>>>);
-
-impl<T, U> ReferenceCounter<T, U> {
-    pub fn new() -> Self {
-        Self(HashMap::new())
-    }
-
-    pub fn count_reference(&mut self, to: Handle<T>, from: Handle<U>) {
-        self.0.entry(to).or_default().push(from);
-    }
-
-    pub fn find_multiples(&self) -> Vec<ObjectNotExclusivelyOwned<T, U>> {
-        self.0
-            .iter()
-            .filter(|(_, referenced_by)| referenced_by.len() > 1)
-            .map(|(object, referenced_by)| ObjectNotExclusivelyOwned {
-                object: object.clone(),
-                referenced_by: referenced_by.to_vec(),
-            })
-            .collect()
-    }
-}
-
-/// Find errors and convert to [`crate::validate::ValidationError`]
-#[macro_export]
-macro_rules! validate_references {
-    ($errors:ident;$($counter:ident, $err:ident;)*) => {
-        $(
-            $counter.find_multiples().iter().for_each(|multiple| {
-                let reference_error = ValidationError::$err(multiple.clone());
-                $errors.push(reference_error.into());
-            });
-        )*
-    };
-}

crates/fj-core/src/validate/sketch.rs

@@ -3,14 +3,14 @@ use fj_math::Winding;
 use crate::{
     geometry::Geometry,
     storage::Handle,
-    topology::{Cycle, Sketch},
-    validate_references,
-    validation::{checks::AdjacentHalfEdgesNotConnected, ValidationCheck},
+    topology::{Cycle, HalfEdge, Region, Sketch},
+    validation::{
+        checks::{AdjacentHalfEdgesNotConnected, MultipleReferencesToObject},
+        ValidationCheck,
+    },
 };
 
-use super::{
-    references::ReferenceCounter, Validate, ValidationConfig, ValidationError,
-};
+use super::{Validate, ValidationConfig, ValidationError};
 
 impl Validate for Sketch {
     fn validate(
@@ -23,7 +23,18 @@ impl Validate for Sketch {
             AdjacentHalfEdgesNotConnected::check(self, geometry, config)
                 .map(Into::into),
         );
-        SketchValidationError::check_object_references(self, config, errors);
+        errors.extend(
+            MultipleReferencesToObject::<Cycle, Region>::check(
+                self, geometry, config,
+            )
+            .map(Into::into),
+        );
+        errors.extend(
+            MultipleReferencesToObject::<HalfEdge, Cycle>::check(
+                self, geometry, config,
+            )
+            .map(Into::into),
+        );
         SketchValidationError::check_exterior_cycles(
             self, geometry, config, errors,
         );
@@ -58,30 +69,6 @@ pub enum SketchValidationError {
 }
 
 impl SketchValidationError {
-    fn check_object_references(
-        sketch: &Sketch,
-        _config: &ValidationConfig,
-        errors: &mut Vec<ValidationError>,
-    ) {
-        let mut referenced_edges = ReferenceCounter::new();
-        let mut referenced_cycles = ReferenceCounter::new();
-
-        sketch.regions().iter().for_each(|r| {
-            r.all_cycles().for_each(|c| {
-                referenced_cycles.count_reference(c.clone(), r.clone());
-                c.half_edges().into_iter().for_each(|e| {
-                    referenced_edges.count_reference(e.clone(), c.clone());
-                })
-            })
-        });
-
-        validate_references!(
-            errors;
-            referenced_edges, MultipleReferencesToHalfEdge;
-            referenced_cycles, MultipleReferencesToCycle;
-        );
-    }
-
     fn check_exterior_cycles(
         sketch: &Sketch,
         geometry: &Geometry,
@@ -126,84 +113,12 @@ impl SketchValidationError {
 mod tests {
     use crate::{
         assert_contains_err,
-        operations::{
-            build::BuildHalfEdge, build::BuildRegion, insert::Insert,
-        },
+        operations::{build::BuildHalfEdge, insert::Insert},
         topology::{Cycle, HalfEdge, Region, Sketch, Vertex},
         validate::{SketchValidationError, Validate, ValidationError},
         Core,
     };
 
-    #[test]
-    fn should_find_cycle_multiple_references() -> anyhow::Result<()> {
-        let mut core = Core::new();
-
-        let surface = core.layers.topology.surfaces.space_2d();
-
-        let region = <Region as BuildRegion>::circle(
-            [0., 0.],
-            1.,
-            surface.clone(),
-            &mut core,
-        )
-        .insert(&mut core);
-        let valid_sketch = Sketch::new(surface.clone(), vec![region.clone()])
-            .insert(&mut core);
-        valid_sketch.validate_and_return_first_error(&core.layers.geometry)?;
-
-        let shared_cycle = region.exterior();
-        let invalid_sketch = Sketch::new(
-            surface,
-            vec![
-                Region::new(shared_cycle.clone(), vec![]).insert(&mut core),
-                Region::new(shared_cycle.clone(), vec![]).insert(&mut core),
-            ],
-        );
-        assert_contains_err!(
-            core,
-            invalid_sketch,
-            ValidationError::MultipleReferencesToCycle(_)
-        );
-
-        Ok(())
-    }
-
-    #[test]
-    fn should_find_half_edge_multiple_references() -> anyhow::Result<()> {
-        let mut core = Core::new();
-
-        let surface = core.layers.topology.surfaces.space_2d();
-
-        let region = <Region as BuildRegion>::polygon(
-            [[0., 0.], [1., 1.], [0., 1.]],
-            surface.clone(),
-            &mut core,
-        )
-        .insert(&mut core);
-        let valid_sketch = Sketch::new(surface.clone(), vec![region.clone()])
-            .insert(&mut core);
-        valid_sketch.validate_and_return_first_error(&core.layers.geometry)?;
-
-        let exterior = region.exterior();
-        let cloned_edges: Vec<_> =
-            exterior.half_edges().iter().cloned().collect();
-        let interior = Cycle::new(cloned_edges).insert(&mut core);
-
-        let invalid_sketch = Sketch::new(
-            surface,
-            vec![
-                Region::new(exterior.clone(), vec![interior]).insert(&mut core)
-            ],
-        );
-        assert_contains_err!(
-            core,
-            invalid_sketch,
-            ValidationError::MultipleReferencesToHalfEdge(_)
-        );
-
-        Ok(())
-    }
-
     #[test]
     fn should_find_clockwise_exterior_cycle() -> anyhow::Result<()> {
         let mut core = Core::new();

crates/fj-core/src/validate/solid.rs

@@ -4,13 +4,11 @@ use crate::{
     geometry::Geometry,
     storage::Handle,
     topology::{Solid, Vertex},
-    validate_references,
+    validation::checks::ReferenceCounter,
 };
 use fj_math::Point;
 
-use super::{
-    references::ReferenceCounter, Validate, ValidationConfig, ValidationError,
-};
+use super::{Validate, ValidationConfig, ValidationError};
 
 impl Validate for Solid {
     fn validate(
@@ -142,33 +140,28 @@ impl SolidValidationError {
         _config: &ValidationConfig,
         errors: &mut Vec<ValidationError>,
     ) {
-        let mut referenced_regions = ReferenceCounter::new();
-        let mut referenced_faces = ReferenceCounter::new();
-        let mut referenced_edges = ReferenceCounter::new();
-        let mut referenced_cycles = ReferenceCounter::new();
+        let mut faces = ReferenceCounter::new();
+        let mut regions = ReferenceCounter::new();
+        let mut cycles = ReferenceCounter::new();
+        let mut half_edges = ReferenceCounter::new();
 
         solid.shells().iter().for_each(|s| {
             s.faces().into_iter().for_each(|f| {
-                referenced_faces.count_reference(f.clone(), s.clone());
-                referenced_regions
-                    .count_reference(f.region().clone(), f.clone());
+                faces.count(f.clone(), s.clone());
+                regions.count(f.region().clone(), f.clone());
                 f.region().all_cycles().for_each(|c| {
-                    referenced_cycles
-                        .count_reference(c.clone(), f.region().clone());
+                    cycles.count(c.clone(), f.region().clone());
                     c.half_edges().into_iter().for_each(|e| {
-                        referenced_edges.count_reference(e.clone(), c.clone());
+                        half_edges.count(e.clone(), c.clone());
                     })
                 })
             })
         });
 
-        validate_references!(
-            errors;
-            referenced_regions, MultipleReferencesToRegion;
-            referenced_faces, MultipleReferencesToFace;
-            referenced_edges, MultipleReferencesToHalfEdge;
-            referenced_cycles, MultipleReferencesToCycle;
-        );
+        errors.extend(faces.multiples().map(Into::into));
+        errors.extend(regions.multiples().map(Into::into));
+        errors.extend(cycles.multiples().map(Into::into));
+        errors.extend(half_edges.multiples().map(Into::into));
     }
 }
 

crates/fj-core/src/validation/checks/mod.rs

@@ -8,6 +8,7 @@ mod face_boundary;
 mod face_winding;
 mod half_edge_connection;
 mod half_edge_has_no_sibling;
+mod multiple_references;
 
 pub use self::{
     coincident_half_edges_are_not_siblings::CoincidentHalfEdgesAreNotSiblings,
@@ -16,4 +17,5 @@ pub use self::{
     face_winding::InteriorCycleHasInvalidWinding,
     half_edge_connection::AdjacentHalfEdgesNotConnected,
     half_edge_has_no_sibling::HalfEdgeHasNoSibling,
+    multiple_references::{MultipleReferencesToObject, ReferenceCounter},
 };

crates/fj-core/src/validation/checks/multiple_references.rs

@@ -0,0 +1,186 @@
+use std::{any::type_name_of_val, collections::HashMap, fmt};
+
+use crate::{
+    storage::Handle,
+    topology::{Cycle, HalfEdge, Region, Sketch},
+    validation::ValidationCheck,
+};
+
+/// Object that should be exclusively owned by another, is not
+///
+/// Some objects are expected to be "owned" by a single other object. This means
+/// that only one reference to these objects must exist within the topological
+/// object graph.
+#[derive(Clone, Debug, thiserror::Error)]
+pub struct MultipleReferencesToObject<T, U> {
+    object: Handle<T>,
+    referenced_by: Vec<Handle<U>>,
+}
+
+impl<T, U> fmt::Display for MultipleReferencesToObject<T, U>
+where
+    T: fmt::Debug,
+    U: fmt::Debug,
+{
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(
+            f,
+            "`{}` ({:?}) referenced by multiple `{}` objects ({:?})",
+            type_name_of_val(&self.object),
+            self.object,
+            type_name_of_val(&self.referenced_by),
+            self.referenced_by
+        )
+    }
+}
+
+impl ValidationCheck<Sketch> for MultipleReferencesToObject<Cycle, Region> {
+    fn check<'r>(
+        object: &'r Sketch,
+        _: &'r crate::geometry::Geometry,
+        _: &'r crate::validation::ValidationConfig,
+    ) -> impl Iterator<Item = Self> + 'r {
+        let mut cycles = ReferenceCounter::new();
+
+        for region in object.regions() {
+            for cycle in region.all_cycles() {
+                cycles.count(cycle.clone(), region.clone());
+            }
+        }
+
+        cycles.multiples()
+    }
+}
+
+impl ValidationCheck<Sketch> for MultipleReferencesToObject<HalfEdge, Cycle> {
+    fn check<'r>(
+        object: &'r Sketch,
+        _: &'r crate::geometry::Geometry,
+        _: &'r crate::validation::ValidationConfig,
+    ) -> impl Iterator<Item = Self> + 'r {
+        let mut half_edges = ReferenceCounter::new();
+
+        for region in object.regions() {
+            for cycle in region.all_cycles() {
+                for half_edge in cycle.half_edges() {
+                    half_edges.count(half_edge.clone(), cycle.clone());
+                }
+            }
+        }
+
+        half_edges.multiples()
+    }
+}
+
+// Warnings are temporarily silenced, until this struct can be made private.
+// This can happen once this validation check has been fully ported from the old
+// infrastructure.
+#[allow(missing_docs)]
+#[derive(Default)]
+pub struct ReferenceCounter<T, U>(HashMap<Handle<T>, Vec<Handle<U>>>);
+
+// Warnings are temporarily silenced, until this struct can be made private.
+// This can happen once this validation check has been fully ported from the old
+// infrastructure.
+#[allow(missing_docs)]
+impl<T, U> ReferenceCounter<T, U> {
+    pub fn new() -> Self {
+        Self(HashMap::new())
+    }
+
+    pub fn count(&mut self, to: Handle<T>, from: Handle<U>) {
+        self.0.entry(to).or_default().push(from);
+    }
+
+    pub fn multiples(
+        self,
+    ) -> impl Iterator<Item = MultipleReferencesToObject<T, U>> {
+        self.0
+            .into_iter()
+            .filter(|(_, referenced_by)| referenced_by.len() > 1)
+            .map(|(object, referenced_by)| MultipleReferencesToObject {
+                object: object.clone(),
+                referenced_by: referenced_by.to_vec(),
+            })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::{
+        operations::{
+            build::BuildSketch,
+            update::{UpdateRegion, UpdateSketch},
+        },
+        topology::{Cycle, HalfEdge, Region, Sketch},
+        validation::{checks::MultipleReferencesToObject, ValidationCheck},
+        Core,
+    };
+
+    #[test]
+    fn multiple_references_to_cycle() -> anyhow::Result<()> {
+        let mut core = Core::new();
+
+        let valid = Sketch::circle([0., 0.], 1., &mut core);
+        MultipleReferencesToObject::<
+            Cycle,
+            Region
+        >::check_and_return_first_error(
+            &valid,
+            &core.layers.geometry,
+        )?;
+
+        let invalid = valid.add_regions(
+            [Region::new(
+                valid.regions().first().exterior().clone(),
+                vec![],
+            )],
+            &mut core,
+        );
+        MultipleReferencesToObject::<Cycle, Region>::check_and_expect_one_error(
+            &invalid,
+            &core.layers.geometry,
+        );
+
+        Ok(())
+    }
+
+    #[test]
+    fn multiple_references_to_half_edge() -> anyhow::Result<()> {
+        let mut core = Core::new();
+
+        let valid = Sketch::polygon([[0., 0.], [1., 1.], [0., 1.]], &mut core);
+        MultipleReferencesToObject::<
+            HalfEdge,
+            Cycle
+        >::check_and_return_first_error(
+            &valid,
+            &core.layers.geometry,
+        )?;
+
+        let invalid = valid.update_region(
+            valid.regions().first(),
+            |region, core| {
+                [region.add_interiors(
+                    [Cycle::new(
+                        region.exterior().half_edges().iter().cloned(),
+                    )],
+                    core,
+                )]
+            },
+            &mut core,
+        );
+        assert!(
+            MultipleReferencesToObject::<
+                HalfEdge,
+                Cycle
+            >::check_and_return_first_error(
+                &invalid,
+                &core.layers.geometry,
+            )
+            .is_err()
+        );
+
+        Ok(())
+    }
+}

crates/fj-core/src/validation/error.rs

@@ -2,15 +2,13 @@ use std::{convert::Infallible, fmt};
 
 use crate::{
     topology::{Cycle, Face, HalfEdge, Region, Shell},
-    validate::{
-        ObjectNotExclusivelyOwned, SketchValidationError, SolidValidationError,
-    },
+    validate::{SketchValidationError, SolidValidationError},
 };
 
 use super::checks::{
     AdjacentHalfEdgesNotConnected, CoincidentHalfEdgesAreNotSiblings,
     CurveGeometryMismatch, FaceHasNoBoundary, HalfEdgeHasNoSibling,
-    InteriorCycleHasInvalidWinding,
+    InteriorCycleHasInvalidWinding, MultipleReferencesToObject,
 };
 
 /// An error that can occur during a validation
@@ -44,19 +42,25 @@ pub enum ValidationError {
 
     /// Multiple references to [`Cycle`]
     #[error(transparent)]
-    MultipleReferencesToCycle(ObjectNotExclusivelyOwned<Cycle, Region>),
+    MultipleReferencesToCycle(
+        #[from] MultipleReferencesToObject<Cycle, Region>,
+    ),
 
     /// Multiple references to [`Face`]
     #[error(transparent)]
-    MultipleReferencesToFace(ObjectNotExclusivelyOwned<Face, Shell>),
+    MultipleReferencesToFace(#[from] MultipleReferencesToObject<Face, Shell>),
 
     /// Multiple references to [`HalfEdge`]
     #[error(transparent)]
-    MultipleReferencesToHalfEdge(ObjectNotExclusivelyOwned<HalfEdge, Cycle>),
+    MultipleReferencesToHalfEdge(
+        #[from] MultipleReferencesToObject<HalfEdge, Cycle>,
+    ),
 
     /// Multiple references to [`Region`]
     #[error(transparent)]
-    MultipleReferencesToRegion(ObjectNotExclusivelyOwned<Region, Face>),
+    MultipleReferencesToRegion(
+        #[from] MultipleReferencesToObject<Region, Face>,
+    ),
 
     /// `Solid` validation error
     #[error("`Solid` validation error")]