From 0aa266e6ae754170ddbabe49c4f43a0f31417793 Mon Sep 17 00:00:00 2001 From: Mitja Kleider Date: Mon, 11 Mar 2024 21:38:41 +0100 Subject: [PATCH 1/3] Refactor multiple_references tests Use BuildRegion to create cycles with valid windings. --- crates/fj-core/src/validate/sketch.rs | 53 +++++++++++---------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/crates/fj-core/src/validate/sketch.rs b/crates/fj-core/src/validate/sketch.rs index 7495eb9f5..d7fe7c4a3 100644 --- a/crates/fj-core/src/validate/sketch.rs +++ b/crates/fj-core/src/validate/sketch.rs @@ -54,7 +54,9 @@ mod tests { use crate::{ assert_contains_err, objects::{Cycle, HalfEdge, Region, Sketch, Vertex}, - operations::{build::BuildHalfEdge, insert::Insert}, + operations::{ + build::BuildHalfEdge, build::BuildRegion, insert::Insert, + }, validate::{ references::ReferenceCountError, SketchValidationError, Validate, ValidationError, @@ -66,15 +68,15 @@ mod tests { fn should_find_cycle_multiple_references() -> anyhow::Result<()> { let mut core = Core::new(); - let shared_cycle = Cycle::new(vec![]).insert(&mut core); + let region = ::circle([0., 0.], 1., &mut core) + .insert(&mut core); + let valid_sketch = Sketch::new(vec![region.clone()]).insert(&mut core); + valid_sketch.validate_and_return_first_error()?; + let shared_cycle = region.exterior(); let invalid_sketch = Sketch::new(vec![ - Region::new( - Cycle::new(vec![]).insert(&mut core), - vec![shared_cycle.clone()], - ) - .insert(&mut core), - Region::new(shared_cycle, vec![]).insert(&mut core), + Region::new(shared_cycle.clone(), vec![]).insert(&mut core), + Region::new(shared_cycle.clone(), vec![]).insert(&mut core), ]); assert_contains_err!( invalid_sketch, @@ -83,13 +85,6 @@ mod tests { )) ); - let valid_sketch = Sketch::new(vec![Region::new( - Cycle::new(vec![]).insert(&mut core), - vec![], - ) - .insert(&mut core)]); - valid_sketch.validate_and_return_first_error()?; - Ok(()) } @@ -97,20 +92,18 @@ mod tests { fn should_find_half_edge_multiple_references() -> anyhow::Result<()> { let mut core = Core::new(); - let half_edge = - HalfEdge::line_segment([[0., 0.], [1., 0.]], None, &mut core) - .insert(&mut core); - let sibling_edge = - HalfEdge::from_sibling(&half_edge, Vertex::new().insert(&mut core)) - .insert(&mut core); - - let exterior = - Cycle::new(vec![half_edge.clone(), sibling_edge.clone()]) - .insert(&mut core); + let region = ::polygon( + [[0., 0.], [1., 1.], [0., 1.]], + &mut core, + ) + .insert(&mut core); + let valid_sketch = Sketch::new(vec![region.clone()]).insert(&mut core); + valid_sketch.validate_and_return_first_error()?; - let interior = - Cycle::new(vec![half_edge.clone(), sibling_edge.clone()]) - .insert(&mut core); + let exterior = region.exterior(); + let cloned_edges: Vec<_> = + exterior.half_edges().iter().map(|e| e.clone()).collect(); + let interior = Cycle::new(cloned_edges).insert(&mut core); let invalid_sketch = Sketch::new(vec![ @@ -123,10 +116,6 @@ mod tests { )) ); - let valid_sketch = - Sketch::new(vec![Region::new(exterior, vec![]).insert(&mut core)]); - valid_sketch.validate_and_return_first_error()?; - Ok(()) } } From 9794df8eadd8770660eb0d683df6bff0bd0f1a93 Mon Sep 17 00:00:00 2001 From: Mitja Kleider Date: Sun, 10 Mar 2024 12:25:56 +0100 Subject: [PATCH 2/3] Add winding validation for sketches (#2158) --- crates/fj-core/src/validate/sketch.rs | 118 ++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/crates/fj-core/src/validate/sketch.rs b/crates/fj-core/src/validate/sketch.rs index d7fe7c4a3..1724d7ae9 100644 --- a/crates/fj-core/src/validate/sketch.rs +++ b/crates/fj-core/src/validate/sketch.rs @@ -1,4 +1,5 @@ use crate::{objects::Sketch, validate_references}; +use fj_math::Winding; use super::{ references::{ReferenceCountError, ReferenceCounter}, @@ -12,6 +13,8 @@ impl Validate for Sketch { errors: &mut Vec, ) { SketchValidationError::check_object_references(self, config, errors); + SketchValidationError::check_exterior_cycle(self, config, errors); + SketchValidationError::check_interior_cycles(self, config, errors); } } @@ -21,6 +24,14 @@ pub enum SketchValidationError { /// Object within sketch referenced by more than one other object #[error("Object within sketch referenced by more than one other Object")] MultipleReferences(#[from] ReferenceCountError), + /// Region within sketch has exterior cycle with clockwise winding + #[error("Exterior cycle within sketch region has clockwise winding")] + ClockwiseExteriorCycle(), + /// Region within sketch has interior cycle with counter-clockwise winding + #[error( + "Interior cycle within sketch region has counter-clockwise winding" + )] + CounterClockwiseInteriorCycle(), } impl SketchValidationError { @@ -47,6 +58,38 @@ impl SketchValidationError { referenced_cycles, Cycle; ); } + + fn check_exterior_cycle( + sketch: &Sketch, + _config: &ValidationConfig, + errors: &mut Vec, + ) { + sketch.regions().iter().for_each(|region| { + if region.exterior().winding() == Winding::Cw { + errors.push(ValidationError::Sketch( + SketchValidationError::ClockwiseExteriorCycle(), + )) + } + }); + } + + fn check_interior_cycles( + sketch: &Sketch, + _config: &ValidationConfig, + errors: &mut Vec, + ) { + sketch.regions().iter().for_each(|region| { + region + .interiors() + .iter() + .filter(|interior| interior.winding() == Winding::Ccw) + .for_each(|_interior| { + errors.push(ValidationError::Sketch( + SketchValidationError::CounterClockwiseInteriorCycle(), + )); + }) + }); + } } #[cfg(test)] @@ -118,4 +161,79 @@ mod tests { Ok(()) } + + #[test] + fn should_find_clockwise_exterior_cycle() -> anyhow::Result<()> { + let mut core = Core::new(); + + let valid_outer_circle = + HalfEdge::circle([0., 0.], 1., &mut core).insert(&mut core); + let valid_exterior = + Cycle::new(vec![valid_outer_circle.clone()]).insert(&mut core); + let valid_sketch = + Sketch::new(vec![ + Region::new(valid_exterior.clone(), vec![]).insert(&mut core) + ]); + valid_sketch.validate_and_return_first_error()?; + + let invalid_outer_circle = HalfEdge::from_sibling( + &valid_outer_circle, + Vertex::new().insert(&mut core), + ) + .insert(&mut core); + let invalid_exterior = + Cycle::new(vec![invalid_outer_circle.clone()]).insert(&mut core); + let invalid_sketch = + Sketch::new(vec![ + Region::new(invalid_exterior.clone(), vec![]).insert(&mut core) + ]); + assert_contains_err!( + invalid_sketch, + ValidationError::Sketch( + SketchValidationError::ClockwiseExteriorCycle() + ) + ); + + Ok(()) + } + + #[test] + fn should_find_counterclockwise_interior_cycle() -> anyhow::Result<()> { + let mut core = Core::new(); + + let outer_circle = + HalfEdge::circle([0., 0.], 2., &mut core).insert(&mut core); + let inner_circle = + HalfEdge::circle([0., 0.], 1., &mut core).insert(&mut core); + let cw_inner_circle = HalfEdge::from_sibling( + &inner_circle, + Vertex::new().insert(&mut core), + ) + .insert(&mut core); + let exterior = Cycle::new(vec![outer_circle.clone()]).insert(&mut core); + + let valid_interior = + Cycle::new(vec![cw_inner_circle.clone()]).insert(&mut core); + let valid_sketch = Sketch::new(vec![Region::new( + exterior.clone(), + vec![valid_interior], + ) + .insert(&mut core)]); + valid_sketch.validate_and_return_first_error()?; + + let invalid_interior = + Cycle::new(vec![inner_circle.clone()]).insert(&mut core); + let invalid_sketch = Sketch::new(vec![Region::new( + exterior.clone(), + vec![invalid_interior], + ) + .insert(&mut core)]); + assert_contains_err!( + invalid_sketch, + ValidationError::Sketch( + SketchValidationError::CounterClockwiseInteriorCycle() + ) + ); + Ok(()) + } } From 9f6bb58dd0e5ecbdfae7e0651d9e0934e0af7b13 Mon Sep 17 00:00:00 2001 From: Mitja Kleider Date: Mon, 11 Mar 2024 21:41:53 +0100 Subject: [PATCH 3/3] Refactor SketchValidationErrors Add info to help determine which cycles are wrong --- crates/fj-core/src/validate/sketch.rs | 42 +++++++++++++++++++-------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/crates/fj-core/src/validate/sketch.rs b/crates/fj-core/src/validate/sketch.rs index 1724d7ae9..d17a86284 100644 --- a/crates/fj-core/src/validate/sketch.rs +++ b/crates/fj-core/src/validate/sketch.rs @@ -1,3 +1,4 @@ +use crate::{objects::Cycle, storage::Handle}; use crate::{objects::Sketch, validate_references}; use fj_math::Winding; @@ -13,7 +14,7 @@ impl Validate for Sketch { errors: &mut Vec, ) { SketchValidationError::check_object_references(self, config, errors); - SketchValidationError::check_exterior_cycle(self, config, errors); + SketchValidationError::check_exterior_cycles(self, config, errors); SketchValidationError::check_interior_cycles(self, config, errors); } } @@ -25,13 +26,23 @@ pub enum SketchValidationError { #[error("Object within sketch referenced by more than one other Object")] MultipleReferences(#[from] ReferenceCountError), /// Region within sketch has exterior cycle with clockwise winding - #[error("Exterior cycle within sketch region has clockwise winding")] - ClockwiseExteriorCycle(), + #[error( + "Exterior cycle within sketch region has clockwise winding\n + Cycle: {cycle:#?}" + )] + ClockwiseExteriorCycle { + /// Cycle with clockwise winding + cycle: Handle, + }, /// Region within sketch has interior cycle with counter-clockwise winding #[error( - "Interior cycle within sketch region has counter-clockwise winding" + "Interior cycle within sketch region has counter-clockwise winding\n + Cycle: {cycle:#?}" )] - CounterClockwiseInteriorCycle(), + CounterClockwiseInteriorCycle { + /// Cycle with counter-clockwise winding + cycle: Handle, + }, } impl SketchValidationError { @@ -59,15 +70,18 @@ impl SketchValidationError { ); } - fn check_exterior_cycle( + fn check_exterior_cycles( sketch: &Sketch, _config: &ValidationConfig, errors: &mut Vec, ) { sketch.regions().iter().for_each(|region| { - if region.exterior().winding() == Winding::Cw { + let cycle = region.exterior(); + if cycle.winding() == Winding::Cw { errors.push(ValidationError::Sketch( - SketchValidationError::ClockwiseExteriorCycle(), + SketchValidationError::ClockwiseExteriorCycle { + cycle: cycle.clone(), + }, )) } }); @@ -83,9 +97,11 @@ impl SketchValidationError { .interiors() .iter() .filter(|interior| interior.winding() == Winding::Ccw) - .for_each(|_interior| { + .for_each(|cycle| { errors.push(ValidationError::Sketch( - SketchValidationError::CounterClockwiseInteriorCycle(), + SketchValidationError::CounterClockwiseInteriorCycle { + cycle: cycle.clone(), + }, )); }) }); @@ -190,7 +206,7 @@ mod tests { assert_contains_err!( invalid_sketch, ValidationError::Sketch( - SketchValidationError::ClockwiseExteriorCycle() + SketchValidationError::ClockwiseExteriorCycle { cycle: _ } ) ); @@ -231,7 +247,9 @@ mod tests { assert_contains_err!( invalid_sketch, ValidationError::Sketch( - SketchValidationError::CounterClockwiseInteriorCycle() + SketchValidationError::CounterClockwiseInteriorCycle { + cycle: _ + } ) ); Ok(())