Merge pull request #2280 from hannobraun/validation

Migrate validation check for interior cycle winding to new infrastructure

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

@@ -1,11 +1,9 @@
-use fj_math::Winding;
-
 use crate::{
     geometry::Geometry,
     objects::Face,
     validation::{
-        checks::FaceHasNoBoundary, ValidationCheck, ValidationConfig,
+        checks::{FaceHasNoBoundary, InteriorCycleHasInvalidWinding},
-        ValidationError,
+        ValidationCheck, ValidationConfig, ValidationError,
     },
 };
 
@@ -21,143 +19,9 @@ impl Validate for Face {
         errors.extend(
             FaceHasNoBoundary::check(self, geometry, config).map(Into::into),
         );
-        FaceValidationError::check_interior_winding(self, geometry, errors);
+        errors.extend(
-    }
+            InteriorCycleHasInvalidWinding::check(self, geometry, config)
-}
+                .map(Into::into),
-
-/// [`Face`] validation error
-#[derive(Clone, Debug, thiserror::Error)]
-pub enum FaceValidationError {
-    /// Interior of [`Face`] has invalid winding; must be opposite of exterior
-    #[error(
-        "Interior of `Face` has invalid winding; must be opposite of exterior\n\
-        - Winding of exterior cycle: {exterior_winding:#?}\n\
-        - Winding of interior cycle: {interior_winding:#?}\n\
-        - `Face`: {face:#?}"
-    )]
-    InvalidInteriorWinding {
-        /// The winding of the [`Face`]'s exterior cycle
-        exterior_winding: Winding,
-
-        /// The winding of the invalid interior cycle
-        interior_winding: Winding,
-
-        /// The face
-        face: Face,
-    },
-}
-
-impl FaceValidationError {
-    fn check_interior_winding(
-        face: &Face,
-        geometry: &Geometry,
-        errors: &mut Vec<ValidationError>,
-    ) {
-        if face.region().exterior().half_edges().is_empty() {
-            // Can't determine winding, if the cycle has no edges. Sounds like a
-            // job for a different validation check.
-            return;
-        }
-
-        let exterior_winding = face.region().exterior().winding(geometry);
-
-        for interior in face.region().interiors() {
-            if interior.half_edges().is_empty() {
-                // Can't determine winding, if the cycle has no edges. Sounds
-                // like a job for a different validation check.
-                continue;
-            }
-            let interior_winding = interior.winding(geometry);
-
-            if exterior_winding == interior_winding {
-                errors.push(
-                    Self::InvalidInteriorWinding {
-                        exterior_winding,
-                        interior_winding,
-                        face: face.clone(),
-                    }
-                    .into(),
-                );
-            }
-        }
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use crate::{
-        assert_contains_err,
-        objects::{Cycle, Face, Region},
-        operations::{
-            build::{BuildCycle, BuildFace},
-            derive::DeriveFrom,
-            insert::Insert,
-            reverse::Reverse,
-            update::{UpdateFace, UpdateRegion},
-        },
-        validate::{FaceValidationError, Validate},
-        validation::ValidationError,
-        Core,
-    };
-
-    #[test]
-    fn interior_winding() -> anyhow::Result<()> {
-        let mut core = Core::new();
-
-        let valid =
-            Face::unbound(core.layers.objects.surfaces.xy_plane(), &mut core)
-                .update_region(
-                    |region, core| {
-                        region
-                            .update_exterior(
-                                |_, core| {
-                                    Cycle::polygon(
-                                        [[0., 0.], [3., 0.], [0., 3.]],
-                                        core,
-                                    )
-                                },
-                                core,
-                            )
-                            .add_interiors(
-                                [Cycle::polygon(
-                                    [[1., 1.], [1., 2.], [2., 1.]],
-                                    core,
-                                )],
-                                core,
-                            )
-                    },
-                    &mut core,
-                );
-        let invalid = {
-            let interiors = valid
-                .region()
-                .interiors()
-                .iter()
-                .cloned()
-                .map(|cycle| {
-                    cycle
-                        .reverse(&mut core)
-                        .insert(&mut core)
-                        .derive_from(&cycle, &mut core)
-                })
-                .collect::<Vec<_>>();
-
-            let region =
-                Region::new(valid.region().exterior().clone(), interiors)
-                    .insert(&mut core);
-
-            Face::new(valid.surface().clone(), region)
-        };
-
-        valid.validate_and_return_first_error(&core.layers.geometry)?;
-        assert_contains_err!(
-            core,
-            invalid,
-            ValidationError::Face(
-                FaceValidationError::InvalidInteriorWinding { .. }
-            )
         );
-
-        Ok(())
     }
 }

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

@@ -79,9 +79,8 @@ use crate::{
 };
 
 pub use self::{
-    edge::EdgeValidationError, face::FaceValidationError,
+    edge::EdgeValidationError, shell::ShellValidationError,
-    shell::ShellValidationError, sketch::SketchValidationError,
+    sketch::SketchValidationError, solid::SolidValidationError,
-    solid::SolidValidationError,
 };
 
 /// Assert that some object has a validation error which matches a specific

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

@@ -0,0 +1,136 @@
+use fj_math::Winding;
+
+use crate::{
+    objects::{Cycle, Face},
+    storage::Handle,
+    validation::ValidationCheck,
+};
+
+/// Interior [`Cycle`] of [`Face`] has invalid winding
+///
+/// The winding of a face's exterior cycle is part of what defines the
+/// orientation of that face. The winding of the interior cycle has no such
+/// meaning attached to it, but it can't be arbitrary either. Triangulation, for
+/// example, might need to assume that it is the opposite of the exterior
+/// winding.
+///
+/// This validation check ensures just that: that the winding of the interior
+/// cycles of a face is the opposite of the winding of that face's exterior
+/// cycle.
+#[derive(Clone, Debug, thiserror::Error)]
+#[error(
+    "Interior of `Face` has invalid winding; must be opposite of exterior\n\
+    - Winding of exterior cycle: {exterior_winding:#?}\n\
+    - Interior cycle with invalid winding: {interior_cycle:#?}\n\
+    - Winding of invalid interior cycle: {interior_winding:#?}"
+)]
+pub struct InteriorCycleHasInvalidWinding {
+    /// The winding of the [`Face`]'s exterior cycle
+    pub exterior_winding: Winding,
+
+    /// The interior cycle with invalid winding
+    pub interior_cycle: Handle<Cycle>,
+
+    /// The winding of the invalid interior cycle
+    pub interior_winding: Winding,
+}
+
+impl ValidationCheck<Face> for InteriorCycleHasInvalidWinding {
+    fn check(
+        object: &Face,
+        geometry: &crate::geometry::Geometry,
+        _: &crate::validation::ValidationConfig,
+    ) -> impl Iterator<Item = Self> {
+        object.region().interiors().iter().filter_map(|interior| {
+            let exterior = object.region().exterior();
+
+            if exterior.half_edges().is_empty()
+                || interior.half_edges().is_empty()
+            {
+                // Can't determine winding, if the cycle has no edges. Sounds
+                // like a job for a different validation check.
+                return None;
+            }
+
+            let exterior_winding = exterior.winding(geometry);
+            let interior_winding = interior.winding(geometry);
+
+            if exterior_winding == interior_winding {
+                return Some(InteriorCycleHasInvalidWinding {
+                    exterior_winding,
+                    interior_cycle: interior.clone(),
+                    interior_winding,
+                });
+            }
+
+            None
+        })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::{
+        objects::{Cycle, Face, Region},
+        operations::{
+            build::{BuildCycle, BuildFace},
+            derive::DeriveFrom,
+            insert::Insert,
+            reverse::Reverse,
+            update::{UpdateFace, UpdateRegion},
+        },
+        validation::{checks::InteriorCycleHasInvalidWinding, ValidationCheck},
+        Core,
+    };
+
+    #[test]
+    fn interior_winding() -> anyhow::Result<()> {
+        let mut core = Core::new();
+
+        let valid = Face::polygon(
+            core.layers.objects.surfaces.xy_plane(),
+            [[0., 0.], [3., 0.], [0., 3.]],
+            &mut core,
+        )
+        .update_region(
+            |region, core| {
+                region.add_interiors(
+                    [Cycle::polygon([[1., 1.], [1., 2.], [2., 1.]], core)],
+                    core,
+                )
+            },
+            &mut core,
+        );
+        InteriorCycleHasInvalidWinding::check_and_return_first_error(
+            &valid,
+            &core.layers.geometry,
+        )?;
+
+        let invalid = {
+            let interiors = valid
+                .region()
+                .interiors()
+                .iter()
+                .cloned()
+                .map(|cycle| {
+                    cycle
+                        .reverse(&mut core)
+                        .insert(&mut core)
+                        .derive_from(&cycle, &mut core)
+                })
+                .collect::<Vec<_>>();
+
+            let region =
+                Region::new(valid.region().exterior().clone(), interiors)
+                    .insert(&mut core);
+
+            Face::new(valid.surface().clone(), region)
+        };
+        InteriorCycleHasInvalidWinding::check_and_expect_one_error(
+            &invalid,
+            &core.layers.geometry,
+        );
+
+        Ok(())
+    }
+}

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

@@ -3,9 +3,11 @@
 //! See documentation of [parent module](super) for more information.
 
 mod face_boundary;
+mod face_winding;
 mod half_edge_connection;
 
 pub use self::{
     face_boundary::FaceHasNoBoundary,
+    face_winding::InteriorCycleHasInvalidWinding,
     half_edge_connection::AdjacentHalfEdgesNotConnected,
 };

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

@@ -1,11 +1,14 @@
 use std::{convert::Infallible, fmt};
 
 use crate::validate::{
-    EdgeValidationError, FaceValidationError, ShellValidationError,
+    EdgeValidationError, ShellValidationError, SketchValidationError,
-    SketchValidationError, SolidValidationError,
+    SolidValidationError,
 };
 
-use super::checks::{AdjacentHalfEdgesNotConnected, FaceHasNoBoundary};
+use super::checks::{
+    AdjacentHalfEdgesNotConnected, FaceHasNoBoundary,
+    InteriorCycleHasInvalidWinding,
+};
 
 /// An error that can occur during a validation
 #[derive(Clone, Debug, thiserror::Error)]
@@ -18,14 +21,14 @@ pub enum ValidationError {
     #[error(transparent)]
     FaceHasNoBoundary(#[from] FaceHasNoBoundary),
 
+    /// Interior cycle has invalid winding
+    #[error(transparent)]
+    InteriorCycleHasInvalidWinding(#[from] InteriorCycleHasInvalidWinding),
+
     /// `Edge` validation error
     #[error("`Edge` validation error")]
     Edge(#[from] EdgeValidationError),
 
-    /// `Face` validation error
-    #[error("`Face` validation error")]
-    Face(#[from] FaceValidationError),
-
     /// `Shell` validation error
     #[error("`Shell` validation error")]
     Shell(#[from] ShellValidationError),