Merge pull request #1328 from hannobraun/validate

Move face validation to new infrastructure

crates/fj-kernel/src/objects/face.rs

@@ -51,35 +51,12 @@ impl Face {
     }
 
     /// Construct a new instance of `Face`
-    ///
-    /// # Panics
-    ///
-    /// Panics, if the provided cycles are not defined in the same surface.
-    ///
-    /// Panics, if the winding of the interior cycles is not opposite that of
-    /// the exterior cycle.
     pub fn new(
         exterior: Handle<Cycle>,
-        the_interiors: impl IntoIterator<Item = Handle<Cycle>>,
+        interiors: impl IntoIterator<Item = Handle<Cycle>>,
         color: Color,
     ) -> Self {
-        let surface = exterior.surface();
-        let mut interiors = Vec::new();
-
-        for interior in the_interiors.into_iter() {
-            assert_eq!(
-                surface.id(),
-                interior.surface().id(),
-                "Cycles that bound a face must be in face's surface"
-            );
-            assert_ne!(
-                exterior.winding(),
-                interior.winding(),
-                "Interior cycles must have opposite winding of exterior cycle"
-            );
-
-            interiors.push(interior);
-        }
+        let interiors = interiors.into_iter().collect();
 
         Self {
             exterior,

crates/fj-kernel/src/objects/mod.rs

@@ -103,7 +103,7 @@ use crate::{
     path::GlobalPath,
     storage::{Handle, Store},
     validate::{
-        CycleValidationError, HalfEdgeValidationError,
+        CycleValidationError, FaceValidationError, HalfEdgeValidationError,
         SurfaceVertexValidationError, Validate2, VertexValidationError,
     },
 };
@@ -204,7 +204,10 @@ pub struct Faces {
 
 impl Faces {
     /// Insert a [`Face`] into the store
-    pub fn insert(&self, face: Face) -> Result<Handle<Face>, Infallible> {
+    pub fn insert(
+        &self,
+        face: Face,
+    ) -> Result<Handle<Face>, FaceValidationError> {
         face.validate()?;
         Ok(self.store.insert(face))
     }

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

@@ -1,16 +1,165 @@
-use std::convert::Infallible;
+use fj_math::Winding;
 
-use crate::objects::Face;
+use crate::{
+    objects::{Cycle, Face, Surface},
+    storage::Handle,
+};
 
 use super::{Validate2, ValidationConfig};
 
 impl Validate2 for Face {
-    type Error = Infallible;
+    type Error = FaceValidationError;
 
     fn validate_with_config(
         &self,
         _: &ValidationConfig,
     ) -> Result<(), Self::Error> {
+        FaceValidationError::check_surface_identity(self)?;
+        FaceValidationError::check_interior_winding(self)?;
+        Ok(())
+    }
+}
+
+/// [`Face`] validation error
+#[derive(Debug, thiserror::Error)]
+pub enum FaceValidationError {
+    /// [`Surface`] of an interior [`Cycle`] doesn't match [`Face`]'s `Surface`
+    #[error(
+        "`Surface` of an interior `Cycle` doesn't match `Face`'s `Surface`\n\
+        - `Surface` of the `Face`: {surface:#?}\n\
+        - Invalid interior `Cycle`: {interior:#?}\n\
+        - `Face`: {face:#?}"
+    )]
+    SurfaceMismatch {
+        /// The surface of the [`Face`]
+        surface: Handle<Surface>,
+
+        /// The invalid interior cycle of the [`Face`]
+        interior: Handle<Cycle>,
+
+        /// The face
+        face: Face,
+    },
+
+    /// 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_surface_identity(face: &Face) -> Result<(), Self> {
+        let surface = face.surface();
+
+        for interior in face.interiors() {
+            if surface.id() != interior.surface().id() {
+                return Err(Self::SurfaceMismatch {
+                    surface: surface.clone(),
+                    interior: interior.clone(),
+                    face: face.clone(),
+                });
+            }
+        }
+
+        Ok(())
+    }
+
+    fn check_interior_winding(face: &Face) -> Result<(), Self> {
+        let exterior_winding = face.exterior().winding();
+
+        for interior in face.interiors() {
+            let interior_winding = interior.winding();
+
+            if exterior_winding == interior_winding {
+                return Err(Self::InvalidInteriorWinding {
+                    exterior_winding,
+                    interior_winding,
+                    face: face.clone(),
+                });
+            }
+            assert_ne!(
+                exterior_winding,
+                interior.winding(),
+                "Interior cycles must have opposite winding of exterior cycle"
+            );
+        }
+
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::{
+        algorithms::reverse::Reverse,
+        builder::CycleBuilder,
+        objects::{Cycle, Face, Objects},
+        partial::HasPartial,
+        validate::Validate2,
+    };
+
+    #[test]
+    fn face_surface_mismatch() -> anyhow::Result<()> {
+        let objects = Objects::new();
+
+        let valid = Face::builder(&objects)
+            .with_surface(objects.surfaces.xy_plane())
+            .with_exterior_polygon_from_points([[0., 0.], [3., 0.], [0., 3.]])
+            .with_interior_polygon_from_points([[1., 1.], [1., 2.], [2., 1.]])
+            .build();
+        let invalid = {
+            let interiors = [Cycle::partial()
+                .with_poly_chain_from_points(
+                    objects.surfaces.xz_plane(),
+                    [[1., 1.], [1., 2.], [2., 1.]],
+                )
+                .close_with_line_segment()
+                .build(&objects)?];
+
+            Face::new(valid.exterior().clone(), interiors, valid.color())
+        };
+
+        assert!(valid.validate().is_ok());
+        assert!(invalid.validate().is_err());
+
+        Ok(())
+    }
+
+    #[test]
+    fn face_invalid_interior_winding() -> anyhow::Result<()> {
+        let objects = Objects::new();
+
+        let valid = Face::builder(&objects)
+            .with_surface(objects.surfaces.xy_plane())
+            .with_exterior_polygon_from_points([[0., 0.], [3., 0.], [0., 3.]])
+            .with_interior_polygon_from_points([[1., 1.], [1., 2.], [2., 1.]])
+            .build();
+        let invalid = {
+            let interiors = valid
+                .interiors()
+                .cloned()
+                .map(|cycle| cycle.reverse(&objects))
+                .collect::<Result<Vec<_>, _>>()?;
+
+            Face::new(valid.exterior().clone(), interiors, valid.color())
+        };
+
+        assert!(valid.validate().is_ok());
+        assert!(invalid.validate().is_err());
+
         Ok(())
     }
 }

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

@@ -28,6 +28,7 @@ mod vertex;
 pub use self::{
     cycle::CycleValidationError,
     edge::HalfEdgeValidationError,
+    face::FaceValidationError,
     uniqueness::UniquenessIssues,
     vertex::{SurfaceVertexValidationError, VertexValidationError},
 };
@@ -182,6 +183,10 @@ pub enum ValidationError {
     #[error(transparent)]
     Cycle(#[from] CycleValidationError),
 
+    /// `Face` validation error
+    #[error(transparent)]
+    Face(#[from] FaceValidationError),
+
     /// `HalfEdge` validation error
     #[error(transparent)]
     HalfEdge(#[from] HalfEdgeValidationError),