From 8eb974b225029e4c4cc825ba51210273ee79d119 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 14:50:34 +0100 Subject: [PATCH 1/4] Add `FaceValidationError` --- crates/fj-kernel/src/objects/mod.rs | 7 +++++-- crates/fj-kernel/src/validate/face.rs | 8 +++++--- crates/fj-kernel/src/validate/mod.rs | 5 +++++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/crates/fj-kernel/src/objects/mod.rs b/crates/fj-kernel/src/objects/mod.rs index 07a37bd53..c35a28972 100644 --- a/crates/fj-kernel/src/objects/mod.rs +++ b/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, Infallible> { + pub fn insert( + &self, + face: Face, + ) -> Result, FaceValidationError> { face.validate()?; Ok(self.store.insert(face)) } diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 934695e19..a740f4e18 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -1,11 +1,9 @@ -use std::convert::Infallible; - use crate::objects::Face; use super::{Validate2, ValidationConfig}; impl Validate2 for Face { - type Error = Infallible; + type Error = FaceValidationError; fn validate_with_config( &self, @@ -14,3 +12,7 @@ impl Validate2 for Face { Ok(()) } } + +/// [`Face`] validation error +#[derive(Debug, thiserror::Error)] +pub enum FaceValidationError {} diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 5d73d839f..f1a290243 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/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), From af69b58fa8f65fa6f0c0d16f3ec2abc09f8ebab9 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 9 Nov 2022 11:18:29 +0100 Subject: [PATCH 2/4] Move face validation check to new infrastructure --- crates/fj-kernel/src/objects/face.rs | 6 -- crates/fj-kernel/src/validate/face.rs | 81 ++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 8 deletions(-) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index 82803390b..0296392e3 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -63,15 +63,9 @@ impl Face { the_interiors: impl IntoIterator>, 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(), diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index a740f4e18..29a6c0da3 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -1,4 +1,7 @@ -use crate::objects::Face; +use crate::{ + objects::{Cycle, Face, Surface}, + storage::Handle, +}; use super::{Validate2, ValidationConfig}; @@ -9,10 +12,84 @@ impl Validate2 for Face { &self, _: &ValidationConfig, ) -> Result<(), Self::Error> { + FaceValidationError::check_surface_identity(self)?; Ok(()) } } /// [`Face`] validation error #[derive(Debug, thiserror::Error)] -pub enum FaceValidationError {} +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, + + /// The invalid interior cycle of the [`Face`] + interior: Handle, + + /// 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(()) + } +} + +#[cfg(test)] +mod tests { + use crate::{ + 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(()) + } +} From ff74d958c44bcdd5e34cfb07dc67531ede1a9cff Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 9 Nov 2022 11:25:54 +0100 Subject: [PATCH 3/4] Move face validation check to new infrastructure --- crates/fj-kernel/src/objects/face.rs | 14 +----- crates/fj-kernel/src/validate/face.rs | 70 +++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 12 deletions(-) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index 0296392e3..39470cebd 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -60,20 +60,10 @@ impl Face { /// the exterior cycle. pub fn new( exterior: Handle, - the_interiors: impl IntoIterator>, + interiors: impl IntoIterator>, color: Color, ) -> Self { - let mut interiors = Vec::new(); - - for interior in the_interiors.into_iter() { - 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, diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 29a6c0da3..e4881e784 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -1,3 +1,5 @@ +use fj_math::Winding; + use crate::{ objects::{Cycle, Face, Surface}, storage::Handle, @@ -13,6 +15,7 @@ impl Validate2 for Face { _: &ValidationConfig, ) -> Result<(), Self::Error> { FaceValidationError::check_surface_identity(self)?; + FaceValidationError::check_interior_winding(self)?; Ok(()) } } @@ -37,6 +40,24 @@ pub enum FaceValidationError { /// 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 { @@ -55,11 +76,35 @@ impl FaceValidationError { 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, @@ -92,4 +137,29 @@ mod tests { 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::, _>>()?; + + Face::new(valid.exterior().clone(), interiors, valid.color()) + }; + + assert!(valid.validate().is_ok()); + assert!(invalid.validate().is_err()); + + Ok(()) + } } From ac17c12bec6563351ab7693a9a712826f6f8bae1 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 9 Nov 2022 11:27:51 +0100 Subject: [PATCH 4/4] Update doc comment --- crates/fj-kernel/src/objects/face.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index 39470cebd..649c3d571 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -51,13 +51,6 @@ 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, interiors: impl IntoIterator>,