From c6b1eca78ae715ae695d3bf59610f2f8ac67814f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 1 Mar 2023 14:40:56 +0100 Subject: [PATCH 1/7] Remove redundant test The validation check it was testing had already been removed. That the test didn't fail was in fact accidental. --- crates/fj-kernel/src/validate/face.rs | 49 +-------------------------- 1 file changed, 1 insertion(+), 48 deletions(-) diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index b8b38002f..169ee65f3 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -153,58 +153,11 @@ mod tests { builder::{CycleBuilder, FaceBuilder, HalfEdgeBuilder}, insert::Insert, objects::{Cycle, Face, HalfEdge, SurfaceVertex}, - partial::{Partial, PartialCycle, PartialFace, PartialObject}, + partial::{Partial, PartialFace, PartialObject}, services::Services, validate::Validate, }; - #[test] - fn face_surface_mismatch() -> anyhow::Result<()> { - let mut services = Services::new(); - - let valid = { - let mut face = PartialFace { - surface: Partial::from(services.objects.surfaces.xy_plane()), - ..Default::default() - }; - face.exterior.write().update_as_polygon_from_points([ - [0., 0.], - [3., 0.], - [0., 3.], - ]); - face.add_interior().write().update_as_polygon_from_points([ - [1., 1.], - [1., 2.], - [2., 1.], - ]); - - face.build(&mut services.objects) - }; - let invalid = { - let surface = services.objects.surfaces.xz_plane(); - - let mut cycle = PartialCycle::default(); - cycle.update_as_polygon_from_points([[1., 1.], [1., 2.], [2., 1.]]); - cycle.infer_vertex_positions_if_necessary(&surface.geometry()); - let cycle = cycle - .build(&mut services.objects) - .insert(&mut services.objects); - - let interiors = [cycle]; - Face::new( - valid.surface().clone(), - valid.exterior().clone(), - interiors, - valid.color(), - ) - }; - - valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); - - Ok(()) - } - #[test] fn face_invalid_interior_winding() -> anyhow::Result<()> { let mut services = Services::new(); From 0be422ca77ed37c610db168379bd40e5aaf09cd7 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 1 Mar 2023 14:43:13 +0100 Subject: [PATCH 2/7] Make validation test assertions more specific --- crates/fj-kernel/src/validate/cycle.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 5996824fc..c1e045dde 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -135,7 +135,7 @@ mod tests { objects::Cycle, partial::{Partial, PartialCycle, PartialObject}, services::Services, - validate::Validate, + validate::{CycleValidationError, Validate, ValidationError}, }; #[test] @@ -174,7 +174,12 @@ mod tests { }; valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); + assert!(matches!( + invalid.validate_and_return_first_error(), + Err(ValidationError::Cycle( + CycleValidationError::HalfEdgeConnection { .. } + )) + )); Ok(()) } @@ -210,7 +215,12 @@ mod tests { }; valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); + assert!(matches!( + invalid.validate_and_return_first_error(), + Err(ValidationError::Cycle( + CycleValidationError::HalfEdgeBoundaryMismatch { .. } + )) + )); Ok(()) } From 33dd1facf1c2cfe05970861d0cb37cf71ab5a589 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 1 Mar 2023 14:47:43 +0100 Subject: [PATCH 3/7] Remove unused code --- crates/fj-kernel/src/validate/edge.rs | 16 +--------------- crates/fj-kernel/src/validate/face.rs | 20 +------------------- 2 files changed, 2 insertions(+), 34 deletions(-) diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index d473182e5..e14058603 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -1,7 +1,7 @@ use fj_math::{Point, Scalar}; use crate::{ - objects::{GlobalEdge, GlobalVertex, HalfEdge, Surface}, + objects::{GlobalEdge, GlobalVertex, HalfEdge}, storage::Handle, }; @@ -50,20 +50,6 @@ pub enum HalfEdgeValidationError { half_edge: HalfEdge, }, - /// Mismatch between the surface's of the curve and surface form - #[error( - "Surface form of vertex must be defined on same surface as curve\n\ - - `Surface` of curve: {curve_surface:#?}\n\ - - `Surface` of surface form: {surface_form_surface:#?}" - )] - SurfaceMismatch { - /// The surface of the vertex' curve - curve_surface: Handle, - - /// The surface of the vertex' surface form - surface_form_surface: Handle, - }, - /// [`HalfEdge`]'s vertices are coincident #[error( "Vertices of `HalfEdge` on curve are coincident\n\ diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 169ee65f3..d014037d7 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -1,7 +1,7 @@ use fj_math::{Point, Scalar, Winding}; use crate::{ - objects::{Cycle, Face, Surface, SurfaceVertex}, + objects::{Face, SurfaceVertex}, storage::Handle, }; @@ -21,24 +21,6 @@ impl Validate for Face { /// [`Face`] validation error #[derive(Clone, 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, - - /// The invalid interior cycle of the [`Face`] - interior: Handle, - - /// 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\ From fedd3d553d7fe8927b01fd64c1b4a6f22c1e425a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 1 Mar 2023 14:52:41 +0100 Subject: [PATCH 4/7] Prepare code for follow-on change This prevents a warning popping up due to the next change I'm working on. --- crates/fj-operations/src/shape_processor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-operations/src/shape_processor.rs b/crates/fj-operations/src/shape_processor.rs index 40f4bcce0..28fc2e4c9 100644 --- a/crates/fj-operations/src/shape_processor.rs +++ b/crates/fj-operations/src/shape_processor.rs @@ -60,7 +60,7 @@ impl ShapeProcessor { pub enum Error { /// Error converting to shape #[error("Error converting to shape")] - ToShape(#[from] ValidationError), + ToShape(#[from] Box), /// Model has zero size #[error("Model has zero size")] From 321774e4d693cfe41d19827a5b5b460d196ae6e7 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 1 Mar 2023 14:53:09 +0100 Subject: [PATCH 5/7] Don't box variants of `ValidationError` This makes them easier to pattern-match on, which is important for some improvements to validation unit tests I'm working on. --- crates/fj-kernel/src/validate/edge.rs | 8 ++++---- crates/fj-kernel/src/validate/face.rs | 8 ++++---- crates/fj-kernel/src/validate/mod.rs | 5 +++-- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index e14058603..7750b88ea 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -92,11 +92,11 @@ impl HalfEdgeValidationError { if matching_global_vertex.is_none() { errors.push( - Box::new(Self::GlobalVertexMismatch { + Self::GlobalVertexMismatch { global_vertex_from_half_edge, global_vertices_from_global_form, half_edge: half_edge.clone(), - }) + } .into(), ); } @@ -112,12 +112,12 @@ impl HalfEdgeValidationError { if distance < config.distinct_min_distance { errors.push( - Box::new(Self::VerticesAreCoincident { + Self::VerticesAreCoincident { back_position, front_position, distance, half_edge: half_edge.clone(), - }) + } .into(), ); } diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index d014037d7..18e7617b3 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -76,11 +76,11 @@ impl FaceValidationError { if exterior_winding == interior_winding { errors.push( - Box::new(Self::InvalidInteriorWinding { + Self::InvalidInteriorWinding { exterior_winding, interior_winding, face: face.clone(), - }) + } .into(), ); } @@ -109,13 +109,13 @@ impl FaceValidationError { if distance > config.identical_max_distance { errors.push( - Box::new(Self::VertexPositionMismatch { + Self::VertexPositionMismatch { surface_position: surface_vertex.position(), surface_position_as_global, global_position, distance, surface_vertex: surface_vertex.clone(), - }) + } .into(), ); } diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 082a0b527..f46b45c56 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -23,6 +23,7 @@ use fj_math::Scalar; /// This trait is used automatically when inserting an object into a store. pub trait Validate: Sized { /// Validate the object using default config and return on first error + #[allow(clippy::result_large_err)] fn validate_and_return_first_error(&self) -> Result<(), ValidationError> { let mut errors = Vec::new(); self.validate(&mut errors); @@ -88,11 +89,11 @@ pub enum ValidationError { /// `Face` validation error #[error("`Face` validation error:\n{0}")] - Face(#[from] Box), + Face(#[from] FaceValidationError), /// `HalfEdge` validation error #[error("`HalfEdge` validation error:\n{0}")] - HalfEdge(#[from] Box), + HalfEdge(#[from] HalfEdgeValidationError), } impl From for ValidationError { From 64e71cc5548e55355c1fd7b7edea9b8e008b13c4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 1 Mar 2023 14:54:29 +0100 Subject: [PATCH 6/7] Make validation test assertions more explicit --- crates/fj-kernel/src/validate/edge.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index 7750b88ea..7dff46b04 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -133,7 +133,7 @@ mod tests { objects::HalfEdge, partial::{Partial, PartialCycle}, services::Services, - validate::Validate, + validate::{HalfEdgeValidationError, Validate, ValidationError}, }; #[test] @@ -181,7 +181,12 @@ mod tests { }; valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); + assert!(matches!( + invalid.validate_and_return_first_error(), + Err(ValidationError::HalfEdge( + HalfEdgeValidationError::GlobalVertexMismatch { .. } + )) + )); Ok(()) } @@ -217,7 +222,12 @@ mod tests { }; valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); + assert!(matches!( + invalid.validate_and_return_first_error(), + Err(ValidationError::HalfEdge( + HalfEdgeValidationError::VerticesAreCoincident { .. } + )) + )); Ok(()) } From 6bc79bd56ac5964334af385fa9d57b5c24f87215 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 1 Mar 2023 14:54:29 +0100 Subject: [PATCH 7/7] Make validation test assertions more explicit --- crates/fj-kernel/src/validate/face.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 18e7617b3..80a5fbb4e 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -137,7 +137,7 @@ mod tests { objects::{Cycle, Face, HalfEdge, SurfaceVertex}, partial::{Partial, PartialFace, PartialObject}, services::Services, - validate::Validate, + validate::{FaceValidationError, Validate, ValidationError}, }; #[test] @@ -177,7 +177,12 @@ mod tests { }; valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); + assert!(matches!( + invalid.validate_and_return_first_error(), + Err(ValidationError::Face( + FaceValidationError::InvalidInteriorWinding { .. } + )) + )); Ok(()) } @@ -252,7 +257,12 @@ mod tests { }; valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); + assert!(matches!( + invalid.validate_and_return_first_error(), + Err(ValidationError::Face( + FaceValidationError::VertexPositionMismatch { .. } + )) + )); Ok(()) }