From 606c50121bae6656f7619680be2f6f8f9bc95936 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 11 Jan 2023 16:37:36 +0100 Subject: [PATCH 01/10] Prepare for not stopping on first validation error --- crates/fj-kernel/src/objects/object.rs | 10 ++++++---- crates/fj-kernel/src/services/validation.rs | 7 +++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/objects/object.rs b/crates/fj-kernel/src/objects/object.rs index 4bbb24fa8..8c81f6bee 100644 --- a/crates/fj-kernel/src/objects/object.rs +++ b/crates/fj-kernel/src/objects/object.rs @@ -66,14 +66,16 @@ macro_rules! object { } /// Validate the object - pub fn validate(&self) -> Result<(), ValidationError> { + pub fn validate(&self, errors: &mut Vec) { match self { $( - Self::$ty((_, object)) => object.validate()?, + Self::$ty((_, object)) => { + if let Err(err) = object.validate() { + errors.push(err.into()); + } + } )* } - - Ok(()) } } diff --git a/crates/fj-kernel/src/services/validation.rs b/crates/fj-kernel/src/services/validation.rs index c3f8f9d4d..bd7369201 100644 --- a/crates/fj-kernel/src/services/validation.rs +++ b/crates/fj-kernel/src/services/validation.rs @@ -37,9 +37,12 @@ impl State for Validation { type Event = ValidationFailed; fn decide(&self, command: Self::Command, events: &mut Vec) { - if let Err(err) = command.object.validate() { + let mut errors = Vec::new(); + command.object.validate(&mut errors); + + for err in errors { events.push(ValidationFailed { - object: command.object.into(), + object: command.object.clone().into(), err, }); } From c93fdade736298257f152536e06dda0bfdcac446 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 12 Jan 2023 14:26:50 +0100 Subject: [PATCH 02/10] Remove `Validate::Error` --- crates/fj-kernel/src/validate/curve.rs | 12 +++--------- crates/fj-kernel/src/validate/cycle.rs | 6 ++---- crates/fj-kernel/src/validate/edge.rs | 12 +++--------- crates/fj-kernel/src/validate/face.rs | 6 ++---- crates/fj-kernel/src/validate/mod.rs | 7 ++----- crates/fj-kernel/src/validate/shell.rs | 8 ++------ crates/fj-kernel/src/validate/sketch.rs | 8 ++------ crates/fj-kernel/src/validate/solid.rs | 8 ++------ crates/fj-kernel/src/validate/surface.rs | 8 ++------ crates/fj-kernel/src/validate/vertex.rs | 16 ++++------------ 10 files changed, 24 insertions(+), 67 deletions(-) diff --git a/crates/fj-kernel/src/validate/curve.rs b/crates/fj-kernel/src/validate/curve.rs index d7a8e6e87..64ea96417 100644 --- a/crates/fj-kernel/src/validate/curve.rs +++ b/crates/fj-kernel/src/validate/curve.rs @@ -1,27 +1,21 @@ -use std::convert::Infallible; - use crate::objects::{Curve, GlobalCurve}; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Curve { - type Error = Infallible; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { Ok(()) } } impl Validate for GlobalCurve { - type Error = Infallible; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { Ok(()) } } diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 6b5cbd1fd..b13176769 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -5,15 +5,13 @@ use crate::{ storage::Handle, }; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Cycle { - type Error = CycleValidationError; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { CycleValidationError::check_half_edge_connections(self)?; // We don't need to check that all half-edges are defined in the same diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index 474522283..13992b2d5 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -1,5 +1,3 @@ -use std::convert::Infallible; - use fj_interop::ext::ArrayExt; use fj_math::{Point, Scalar}; @@ -11,15 +9,13 @@ use crate::{ storage::Handle, }; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for HalfEdge { - type Error = HalfEdgeValidationError; - fn validate_with_config( &self, config: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { HalfEdgeValidationError::check_curve_identity(self)?; HalfEdgeValidationError::check_global_curve_identity(self)?; HalfEdgeValidationError::check_global_vertex_identity(self)?; @@ -34,12 +30,10 @@ impl Validate for HalfEdge { } impl Validate for GlobalEdge { - type Error = Infallible; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { Ok(()) } } diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 2aabcda3e..08dd6a2dd 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -5,15 +5,13 @@ use crate::{ storage::Handle, }; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Face { - type Error = FaceValidationError; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { FaceValidationError::check_surface_identity(self)?; FaceValidationError::check_interior_winding(self)?; Ok(()) diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 4cb607f72..e5867a6ca 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -25,11 +25,8 @@ use fj_math::Scalar; /// /// This trait is used automatically when inserting an object into a store. pub trait Validate: Sized { - /// The error that validation of the implementing type can result in - type Error: Into; - /// Validate the object using default configuration - fn validate(&self) -> Result<(), Self::Error> { + fn validate(&self) -> Result<(), ValidationError> { self.validate_with_config(&ValidationConfig::default()) } @@ -37,7 +34,7 @@ pub trait Validate: Sized { fn validate_with_config( &self, config: &ValidationConfig, - ) -> Result<(), Self::Error>; + ) -> Result<(), ValidationError>; } /// Configuration required for the validation process diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index e71626fad..3bb225598 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -1,16 +1,12 @@ -use std::convert::Infallible; - use crate::objects::Shell; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Shell { - type Error = Infallible; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { Ok(()) } } diff --git a/crates/fj-kernel/src/validate/sketch.rs b/crates/fj-kernel/src/validate/sketch.rs index 1a1896658..8fabbd2e2 100644 --- a/crates/fj-kernel/src/validate/sketch.rs +++ b/crates/fj-kernel/src/validate/sketch.rs @@ -1,16 +1,12 @@ -use std::convert::Infallible; - use crate::objects::Sketch; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Sketch { - type Error = Infallible; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { Ok(()) } } diff --git a/crates/fj-kernel/src/validate/solid.rs b/crates/fj-kernel/src/validate/solid.rs index 9e484cb4b..e10fefd19 100644 --- a/crates/fj-kernel/src/validate/solid.rs +++ b/crates/fj-kernel/src/validate/solid.rs @@ -1,16 +1,12 @@ -use std::convert::Infallible; - use crate::objects::Solid; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Solid { - type Error = Infallible; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { Ok(()) } } diff --git a/crates/fj-kernel/src/validate/surface.rs b/crates/fj-kernel/src/validate/surface.rs index 8c1998787..4b888c63f 100644 --- a/crates/fj-kernel/src/validate/surface.rs +++ b/crates/fj-kernel/src/validate/surface.rs @@ -1,16 +1,12 @@ -use std::convert::Infallible; - use crate::objects::Surface; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Surface { - type Error = Infallible; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { Ok(()) } } diff --git a/crates/fj-kernel/src/validate/vertex.rs b/crates/fj-kernel/src/validate/vertex.rs index 6fa016363..80e8142bd 100644 --- a/crates/fj-kernel/src/validate/vertex.rs +++ b/crates/fj-kernel/src/validate/vertex.rs @@ -1,5 +1,3 @@ -use std::convert::Infallible; - use fj_math::{Point, Scalar}; use crate::{ @@ -7,15 +5,13 @@ use crate::{ storage::Handle, }; -use super::{Validate, ValidationConfig}; +use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Vertex { - type Error = VertexValidationError; - fn validate_with_config( &self, config: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { VertexValidationError::check_surface_identity(self)?; VertexValidationError::check_position(self, config)?; Ok(()) @@ -23,24 +19,20 @@ impl Validate for Vertex { } impl Validate for SurfaceVertex { - type Error = SurfaceVertexValidationError; - fn validate_with_config( &self, config: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { SurfaceVertexValidationError::check_position(self, config)?; Ok(()) } } impl Validate for GlobalVertex { - type Error = Infallible; - fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), Self::Error> { + ) -> Result<(), ValidationError> { Ok(()) } } From b4066b316f39d6bc2fe647ef7fd70112bba8a4e4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 11 Jan 2023 16:37:36 +0100 Subject: [PATCH 03/10] Prepare for not stopping on first validation error --- crates/fj-kernel/src/validate/cycle.rs | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index b13176769..cae6cddd6 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -12,12 +12,18 @@ impl Validate for Cycle { &self, _: &ValidationConfig, ) -> Result<(), ValidationError> { - CycleValidationError::check_half_edge_connections(self)?; + let mut errors = Vec::new(); + + CycleValidationError::check_half_edge_connections(self, &mut errors); // We don't need to check that all half-edges are defined in the same // surface. We already check that they are connected by identical // surface vertices, so that would be redundant. + if let Some(error) = errors.into_iter().next() { + return Err(error); + } + Ok(()) } } @@ -41,7 +47,10 @@ pub enum CycleValidationError { } impl CycleValidationError { - fn check_half_edge_connections(cycle: &Cycle) -> Result<(), Self> { + fn check_half_edge_connections( + cycle: &Cycle, + errors: &mut Vec, + ) { for (a, b) in cycle.half_edges().circular_tuple_windows() { let [_, prev] = a.vertices(); let [next, _] = b.vertices(); @@ -50,14 +59,15 @@ impl CycleValidationError { let next = next.surface_form(); if prev.id() != next.id() { - return Err(Self::HalfEdgeConnection { - prev: prev.clone(), - next: next.clone(), - }); + errors.push( + Self::HalfEdgeConnection { + prev: prev.clone(), + next: next.clone(), + } + .into(), + ); } } - - Ok(()) } } From 12586f34aef5d70cd694a7b936e3520c1e6b39c0 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 11 Jan 2023 16:37:36 +0100 Subject: [PATCH 04/10] Prepare for not stopping on first validation error --- crates/fj-kernel/src/validate/edge.rs | 103 ++++++++++++++++---------- 1 file changed, 65 insertions(+), 38 deletions(-) diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index 13992b2d5..f28eb7291 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -16,15 +16,28 @@ impl Validate for HalfEdge { &self, config: &ValidationConfig, ) -> Result<(), ValidationError> { - HalfEdgeValidationError::check_curve_identity(self)?; - HalfEdgeValidationError::check_global_curve_identity(self)?; - HalfEdgeValidationError::check_global_vertex_identity(self)?; - HalfEdgeValidationError::check_vertex_positions(self, config)?; + let mut errors = Vec::new(); + + HalfEdgeValidationError::check_curve_identity(self, &mut errors); + HalfEdgeValidationError::check_global_curve_identity(self, &mut errors); + HalfEdgeValidationError::check_global_vertex_identity( + self, + &mut errors, + ); + HalfEdgeValidationError::check_vertex_positions( + self, + config, + &mut errors, + ); // We don't need to check anything about surfaces here. We already check // curves, which makes sure the vertices are consistent with each other, // and the validation of those vertices checks the surfaces. + if let Some(err) = errors.into_iter().next() { + return Err(err); + } + Ok(()) } } @@ -122,38 +135,49 @@ pub enum HalfEdgeValidationError { } impl HalfEdgeValidationError { - fn check_curve_identity(half_edge: &HalfEdge) -> Result<(), Self> { + fn check_curve_identity( + half_edge: &HalfEdge, + errors: &mut Vec, + ) { let back_curve = half_edge.back().curve(); let front_curve = half_edge.front().curve(); if back_curve.id() != front_curve.id() { - return Err(Self::CurveMismatch { - back_curve: back_curve.clone(), - front_curve: front_curve.clone(), - half_edge: half_edge.clone(), - }); + errors.push( + Self::CurveMismatch { + back_curve: back_curve.clone(), + front_curve: front_curve.clone(), + half_edge: half_edge.clone(), + } + .into(), + ); } - - Ok(()) } - fn check_global_curve_identity(half_edge: &HalfEdge) -> Result<(), Self> { + fn check_global_curve_identity( + half_edge: &HalfEdge, + errors: &mut Vec, + ) { let global_curve_from_curve = half_edge.curve().global_form(); let global_curve_from_global_form = half_edge.global_form().curve(); if global_curve_from_curve.id() != global_curve_from_global_form.id() { - return Err(Self::GlobalCurveMismatch { - global_curve_from_curve: global_curve_from_curve.clone(), - global_curve_from_global_form: global_curve_from_global_form - .clone(), - half_edge: half_edge.clone(), - }); + errors.push( + Self::GlobalCurveMismatch { + global_curve_from_curve: global_curve_from_curve.clone(), + global_curve_from_global_form: + global_curve_from_global_form.clone(), + half_edge: half_edge.clone(), + } + .into(), + ); } - - Ok(()) } - fn check_global_vertex_identity(half_edge: &HalfEdge) -> Result<(), Self> { + fn check_global_vertex_identity( + half_edge: &HalfEdge, + errors: &mut Vec, + ) { let global_vertices_from_vertices = { let (global_vertices_from_vertices, _) = VerticesInNormalizedOrder::new( @@ -177,35 +201,38 @@ impl HalfEdgeValidationError { .map(|global_vertex| global_vertex.id()); if ids_from_vertices != ids_from_global_form { - return Err(Self::GlobalVertexMismatch { - global_vertices_from_vertices, - global_vertices_from_global_form, - half_edge: half_edge.clone(), - }); + errors.push( + Self::GlobalVertexMismatch { + global_vertices_from_vertices, + global_vertices_from_global_form, + half_edge: half_edge.clone(), + } + .into(), + ); } - - Ok(()) } fn check_vertex_positions( half_edge: &HalfEdge, config: &ValidationConfig, - ) -> Result<(), Self> { + errors: &mut Vec, + ) { let back_position = half_edge.back().position(); let front_position = half_edge.front().position(); let distance = (back_position - front_position).magnitude(); if distance < config.distinct_min_distance { - return Err(Self::VerticesAreCoincident { - back_position, - front_position, - distance, - half_edge: half_edge.clone(), - }); + errors.push( + Self::VerticesAreCoincident { + back_position, + front_position, + distance, + half_edge: half_edge.clone(), + } + .into(), + ); } - - Ok(()) } } From 7fb7be280ca3cd4771fe0d6a248eb78ca8ef3bc1 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 12 Jan 2023 14:52:07 +0100 Subject: [PATCH 05/10] Remove redundant assertion --- crates/fj-kernel/src/validate/face.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 08dd6a2dd..e3fb35d46 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -88,11 +88,6 @@ impl FaceValidationError { face: face.clone(), }); } - assert_ne!( - exterior_winding, - interior.winding(), - "Interior cycles must have opposite winding of exterior cycle" - ); } Ok(()) From 2fabcb4428db164329dc80598d7ddcc949e54800 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 11 Jan 2023 16:37:36 +0100 Subject: [PATCH 06/10] Prepare for not stopping on first validation error --- crates/fj-kernel/src/validate/face.rs | 45 ++++++++++++++++----------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index e3fb35d46..55405c71c 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -12,8 +12,15 @@ impl Validate for Face { &self, _: &ValidationConfig, ) -> Result<(), ValidationError> { - FaceValidationError::check_surface_identity(self)?; - FaceValidationError::check_interior_winding(self)?; + let mut errors = Vec::new(); + + FaceValidationError::check_surface_identity(self, &mut errors); + FaceValidationError::check_interior_winding(self, &mut errors); + + if let Some(err) = errors.into_iter().next() { + return Err(err); + } + Ok(()) } } @@ -59,38 +66,40 @@ pub enum FaceValidationError { } impl FaceValidationError { - fn check_surface_identity(face: &Face) -> Result<(), Self> { + fn check_surface_identity(face: &Face, errors: &mut Vec) { 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(), - }); + errors.push( + Self::SurfaceMismatch { + surface: surface.clone(), + interior: interior.clone(), + face: face.clone(), + } + .into(), + ); } } - - Ok(()) } - fn check_interior_winding(face: &Face) -> Result<(), Self> { + fn check_interior_winding(face: &Face, errors: &mut Vec) { 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(), - }); + errors.push( + Self::InvalidInteriorWinding { + exterior_winding, + interior_winding, + face: face.clone(), + } + .into(), + ); } } - - Ok(()) } } From ae1023662783ac2d3d8f00aa808488bd4e144d56 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 11 Jan 2023 16:37:36 +0100 Subject: [PATCH 07/10] Prepare for not stopping on first validation error --- crates/fj-kernel/src/validate/vertex.rs | 49 ++++++++++++++++--------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/crates/fj-kernel/src/validate/vertex.rs b/crates/fj-kernel/src/validate/vertex.rs index 80e8142bd..67e702f67 100644 --- a/crates/fj-kernel/src/validate/vertex.rs +++ b/crates/fj-kernel/src/validate/vertex.rs @@ -12,8 +12,15 @@ impl Validate for Vertex { &self, config: &ValidationConfig, ) -> Result<(), ValidationError> { - VertexValidationError::check_surface_identity(self)?; - VertexValidationError::check_position(self, config)?; + let mut errors = Vec::new(); + + VertexValidationError::check_surface_identity(self, &mut errors); + VertexValidationError::check_position(self, config, &mut errors); + + if let Some(err) = errors.into_iter().next() { + return Err(err); + } + Ok(()) } } @@ -78,24 +85,29 @@ pub enum VertexValidationError { } impl VertexValidationError { - fn check_surface_identity(vertex: &Vertex) -> Result<(), Self> { + fn check_surface_identity( + vertex: &Vertex, + errors: &mut Vec, + ) { let curve_surface = vertex.curve().surface(); let surface_form_surface = vertex.surface_form().surface(); if curve_surface.id() != surface_form_surface.id() { - return Err(Self::SurfaceMismatch { - curve_surface: curve_surface.clone(), - surface_form_surface: surface_form_surface.clone(), - }); + errors.push( + Self::SurfaceMismatch { + curve_surface: curve_surface.clone(), + surface_form_surface: surface_form_surface.clone(), + } + .into(), + ); } - - Ok(()) } fn check_position( vertex: &Vertex, config: &ValidationConfig, - ) -> Result<(), Self> { + errors: &mut Vec, + ) { let curve_position_as_surface = vertex .curve() .path() @@ -105,15 +117,16 @@ impl VertexValidationError { let distance = curve_position_as_surface.distance_to(&surface_position); if distance > config.identical_max_distance { - return Err(Self::PositionMismatch { - vertex: vertex.clone(), - surface_vertex: vertex.surface_form().clone_object(), - curve_position_as_surface, - distance, - }); + errors.push( + Self::PositionMismatch { + vertex: vertex.clone(), + surface_vertex: vertex.surface_form().clone_object(), + curve_position_as_surface, + distance, + } + .into(), + ); } - - Ok(()) } } From 84a9beddca35f82dc3b7112ddf6862cf95544f23 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 11 Jan 2023 16:37:36 +0100 Subject: [PATCH 08/10] Prepare for not stopping on first validation error --- crates/fj-kernel/src/validate/vertex.rs | 29 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/crates/fj-kernel/src/validate/vertex.rs b/crates/fj-kernel/src/validate/vertex.rs index 67e702f67..985599e90 100644 --- a/crates/fj-kernel/src/validate/vertex.rs +++ b/crates/fj-kernel/src/validate/vertex.rs @@ -30,7 +30,14 @@ impl Validate for SurfaceVertex { &self, config: &ValidationConfig, ) -> Result<(), ValidationError> { - SurfaceVertexValidationError::check_position(self, config)?; + let mut errors = Vec::new(); + + SurfaceVertexValidationError::check_position(self, config, &mut errors); + + if let Some(err) = errors.into_iter().next() { + return Err(err); + } + Ok(()) } } @@ -160,7 +167,8 @@ impl SurfaceVertexValidationError { fn check_position( surface_vertex: &SurfaceVertex, config: &ValidationConfig, - ) -> Result<(), Self> { + errors: &mut Vec, + ) { let surface_position_as_global = surface_vertex .surface() .geometry() @@ -170,15 +178,16 @@ impl SurfaceVertexValidationError { let distance = surface_position_as_global.distance_to(&global_position); if distance > config.identical_max_distance { - return Err(Self::PositionMismatch { - surface_vertex: surface_vertex.clone(), - global_vertex: surface_vertex.global_form().clone_object(), - surface_position_as_global, - distance, - }); + errors.push( + Self::PositionMismatch { + surface_vertex: surface_vertex.clone(), + global_vertex: surface_vertex.global_form().clone_object(), + surface_position_as_global, + distance, + } + .into(), + ); } - - Ok(()) } } From 6a1748048c2291ab9043c7877bd921ddf8c34dbf Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 12 Jan 2023 15:00:25 +0100 Subject: [PATCH 09/10] Add `Validate::validate_and_return_fiurst_error` This method is completely redundant with `Validate::validate` right now, but it will become useful in a moment, when I change `validate` to emit an arbitrary number of errors. --- crates/fj-kernel/src/validate/cycle.rs | 4 ++-- crates/fj-kernel/src/validate/edge.rs | 16 ++++++++-------- crates/fj-kernel/src/validate/face.rs | 8 ++++---- crates/fj-kernel/src/validate/mod.rs | 5 +++++ crates/fj-kernel/src/validate/vertex.rs | 12 ++++++------ 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index cae6cddd6..0f6240dfc 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -117,8 +117,8 @@ mod tests { Cycle::new(half_edges) }; - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index f28eb7291..1a7576f7d 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -275,8 +275,8 @@ mod tests { HalfEdge::new(vertices, valid.global_form().clone()) }; - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } @@ -301,8 +301,8 @@ mod tests { tmp.build(&mut services.objects) }); - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } @@ -333,8 +333,8 @@ mod tests { tmp.build(&mut services.objects) }); - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } @@ -370,8 +370,8 @@ mod tests { valid.global_form().clone(), ); - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 55405c71c..8f9a3f983 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -151,8 +151,8 @@ mod tests { Face::new(valid.exterior().clone(), interiors, valid.color()) }; - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } @@ -188,8 +188,8 @@ mod tests { Face::new(valid.exterior().clone(), interiors, valid.color()) }; - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index e5867a6ca..8f5a7e394 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -25,6 +25,11 @@ 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 + fn validate_and_return_first_error(&self) -> Result<(), ValidationError> { + self.validate() + } + /// Validate the object using default configuration fn validate(&self) -> Result<(), ValidationError> { self.validate_with_config(&ValidationConfig::default()) diff --git a/crates/fj-kernel/src/validate/vertex.rs b/crates/fj-kernel/src/validate/vertex.rs index 985599e90..1e682dc23 100644 --- a/crates/fj-kernel/src/validate/vertex.rs +++ b/crates/fj-kernel/src/validate/vertex.rs @@ -234,8 +234,8 @@ mod tests { Vertex::new(valid.position(), valid.curve().clone(), surface_form) }; - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } @@ -271,8 +271,8 @@ mod tests { Vertex::new(valid.position(), valid.curve().clone(), surface_form) }; - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } @@ -293,8 +293,8 @@ mod tests { GlobalVertex::new([1., 0., 0.]).insert(&mut services.objects), ); - valid.validate()?; - assert!(invalid.validate().is_err()); + valid.validate_and_return_first_error()?; + assert!(invalid.validate_and_return_first_error().is_err()); Ok(()) } From f9e76b147324250b371de13b2ce795642d16f5b1 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 12 Jan 2023 15:02:06 +0100 Subject: [PATCH 10/10] Don't stop on first validation error Instead, emit all validation error, to form a more complete picture. --- crates/fj-kernel/src/objects/object.rs | 6 +---- crates/fj-kernel/src/validate/curve.rs | 8 +++--- crates/fj-kernel/src/validate/cycle.rs | 13 +++------- crates/fj-kernel/src/validate/edge.rs | 30 ++++++---------------- crates/fj-kernel/src/validate/face.rs | 15 +++-------- crates/fj-kernel/src/validate/mod.rs | 16 +++++++++--- crates/fj-kernel/src/validate/shell.rs | 4 +-- crates/fj-kernel/src/validate/sketch.rs | 4 +-- crates/fj-kernel/src/validate/solid.rs | 4 +-- crates/fj-kernel/src/validate/surface.rs | 4 +-- crates/fj-kernel/src/validate/vertex.rs | 32 +++++++----------------- 11 files changed, 49 insertions(+), 87 deletions(-) diff --git a/crates/fj-kernel/src/objects/object.rs b/crates/fj-kernel/src/objects/object.rs index 8c81f6bee..0c0e98133 100644 --- a/crates/fj-kernel/src/objects/object.rs +++ b/crates/fj-kernel/src/objects/object.rs @@ -69,11 +69,7 @@ macro_rules! object { pub fn validate(&self, errors: &mut Vec) { match self { $( - Self::$ty((_, object)) => { - if let Err(err) = object.validate() { - errors.push(err.into()); - } - } + Self::$ty((_, object)) => object.validate(errors), )* } } diff --git a/crates/fj-kernel/src/validate/curve.rs b/crates/fj-kernel/src/validate/curve.rs index 64ea96417..250eaea47 100644 --- a/crates/fj-kernel/src/validate/curve.rs +++ b/crates/fj-kernel/src/validate/curve.rs @@ -6,8 +6,8 @@ impl Validate for Curve { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - Ok(()) + _: &mut Vec, + ) { } } @@ -15,7 +15,7 @@ impl Validate for GlobalCurve { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - Ok(()) + _: &mut Vec, + ) { } } diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 0f6240dfc..89a9b030c 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -11,20 +11,13 @@ impl Validate for Cycle { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - let mut errors = Vec::new(); - - CycleValidationError::check_half_edge_connections(self, &mut errors); + errors: &mut Vec, + ) { + CycleValidationError::check_half_edge_connections(self, errors); // We don't need to check that all half-edges are defined in the same // surface. We already check that they are connected by identical // surface vertices, so that would be redundant. - - if let Some(error) = errors.into_iter().next() { - return Err(error); - } - - Ok(()) } } diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index 1a7576f7d..c79b1f72c 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -15,30 +15,16 @@ impl Validate for HalfEdge { fn validate_with_config( &self, config: &ValidationConfig, - ) -> Result<(), ValidationError> { - let mut errors = Vec::new(); - - HalfEdgeValidationError::check_curve_identity(self, &mut errors); - HalfEdgeValidationError::check_global_curve_identity(self, &mut errors); - HalfEdgeValidationError::check_global_vertex_identity( - self, - &mut errors, - ); - HalfEdgeValidationError::check_vertex_positions( - self, - config, - &mut errors, - ); + errors: &mut Vec, + ) { + HalfEdgeValidationError::check_curve_identity(self, errors); + HalfEdgeValidationError::check_global_curve_identity(self, errors); + HalfEdgeValidationError::check_global_vertex_identity(self, errors); + HalfEdgeValidationError::check_vertex_positions(self, config, errors); // We don't need to check anything about surfaces here. We already check // curves, which makes sure the vertices are consistent with each other, // and the validation of those vertices checks the surfaces. - - if let Some(err) = errors.into_iter().next() { - return Err(err); - } - - Ok(()) } } @@ -46,8 +32,8 @@ impl Validate for GlobalEdge { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - Ok(()) + _: &mut Vec, + ) { } } diff --git a/crates/fj-kernel/src/validate/face.rs b/crates/fj-kernel/src/validate/face.rs index 8f9a3f983..b3f3dc5c6 100644 --- a/crates/fj-kernel/src/validate/face.rs +++ b/crates/fj-kernel/src/validate/face.rs @@ -11,17 +11,10 @@ impl Validate for Face { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - let mut errors = Vec::new(); - - FaceValidationError::check_surface_identity(self, &mut errors); - FaceValidationError::check_interior_winding(self, &mut errors); - - if let Some(err) = errors.into_iter().next() { - return Err(err); - } - - Ok(()) + errors: &mut Vec, + ) { + FaceValidationError::check_surface_identity(self, errors); + FaceValidationError::check_interior_winding(self, errors); } } diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 8f5a7e394..3539962af 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -27,19 +27,27 @@ use fj_math::Scalar; pub trait Validate: Sized { /// Validate the object using default config and return on first error fn validate_and_return_first_error(&self) -> Result<(), ValidationError> { - self.validate() + let mut errors = Vec::new(); + self.validate(&mut errors); + + if let Some(err) = errors.into_iter().next() { + return Err(err); + } + + Ok(()) } /// Validate the object using default configuration - fn validate(&self) -> Result<(), ValidationError> { - self.validate_with_config(&ValidationConfig::default()) + fn validate(&self, errors: &mut Vec) { + self.validate_with_config(&ValidationConfig::default(), errors) } /// Validate the object fn validate_with_config( &self, config: &ValidationConfig, - ) -> Result<(), ValidationError>; + errors: &mut Vec, + ); } /// Configuration required for the validation process diff --git a/crates/fj-kernel/src/validate/shell.rs b/crates/fj-kernel/src/validate/shell.rs index 3bb225598..f6603ad55 100644 --- a/crates/fj-kernel/src/validate/shell.rs +++ b/crates/fj-kernel/src/validate/shell.rs @@ -6,7 +6,7 @@ impl Validate for Shell { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - Ok(()) + _: &mut Vec, + ) { } } diff --git a/crates/fj-kernel/src/validate/sketch.rs b/crates/fj-kernel/src/validate/sketch.rs index 8fabbd2e2..d3eefbebe 100644 --- a/crates/fj-kernel/src/validate/sketch.rs +++ b/crates/fj-kernel/src/validate/sketch.rs @@ -6,7 +6,7 @@ impl Validate for Sketch { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - Ok(()) + _: &mut Vec, + ) { } } diff --git a/crates/fj-kernel/src/validate/solid.rs b/crates/fj-kernel/src/validate/solid.rs index e10fefd19..e2982bd6a 100644 --- a/crates/fj-kernel/src/validate/solid.rs +++ b/crates/fj-kernel/src/validate/solid.rs @@ -6,7 +6,7 @@ impl Validate for Solid { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - Ok(()) + _: &mut Vec, + ) { } } diff --git a/crates/fj-kernel/src/validate/surface.rs b/crates/fj-kernel/src/validate/surface.rs index 4b888c63f..4462f9147 100644 --- a/crates/fj-kernel/src/validate/surface.rs +++ b/crates/fj-kernel/src/validate/surface.rs @@ -6,7 +6,7 @@ impl Validate for Surface { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - Ok(()) + _: &mut Vec, + ) { } } diff --git a/crates/fj-kernel/src/validate/vertex.rs b/crates/fj-kernel/src/validate/vertex.rs index 1e682dc23..97617b087 100644 --- a/crates/fj-kernel/src/validate/vertex.rs +++ b/crates/fj-kernel/src/validate/vertex.rs @@ -11,17 +11,10 @@ impl Validate for Vertex { fn validate_with_config( &self, config: &ValidationConfig, - ) -> Result<(), ValidationError> { - let mut errors = Vec::new(); - - VertexValidationError::check_surface_identity(self, &mut errors); - VertexValidationError::check_position(self, config, &mut errors); - - if let Some(err) = errors.into_iter().next() { - return Err(err); - } - - Ok(()) + errors: &mut Vec, + ) { + VertexValidationError::check_surface_identity(self, errors); + VertexValidationError::check_position(self, config, errors); } } @@ -29,16 +22,9 @@ impl Validate for SurfaceVertex { fn validate_with_config( &self, config: &ValidationConfig, - ) -> Result<(), ValidationError> { - let mut errors = Vec::new(); - - SurfaceVertexValidationError::check_position(self, config, &mut errors); - - if let Some(err) = errors.into_iter().next() { - return Err(err); - } - - Ok(()) + errors: &mut Vec, + ) { + SurfaceVertexValidationError::check_position(self, config, errors); } } @@ -46,8 +32,8 @@ impl Validate for GlobalVertex { fn validate_with_config( &self, _: &ValidationConfig, - ) -> Result<(), ValidationError> { - Ok(()) + _: &mut Vec, + ) { } }