From 4d7efecc39846b45cf1193b010e206c4eb32a571 Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Wed, 15 Mar 2023 21:36:42 +0200 Subject: [PATCH 1/7] Basic cycle validation --- crates/fj-kernel/src/objects/full/edge.rs | 10 ++ crates/fj-kernel/src/validate/cycle.rs | 121 +++++++++++++++++++++- crates/fj-kernel/src/validate/mod.rs | 5 + 3 files changed, 134 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/objects/full/edge.rs b/crates/fj-kernel/src/objects/full/edge.rs index 65216649b..646ea688b 100644 --- a/crates/fj-kernel/src/objects/full/edge.rs +++ b/crates/fj-kernel/src/objects/full/edge.rs @@ -86,6 +86,16 @@ impl HalfEdge { self.curve.point_from_path_coords(start) } + /// Compute the surface position where the half-edge ends + pub fn end_position(&self) -> Point<2> { + // Computing the surface position from the curve position is fine. + // `HalfEdge` "owns" its end position. There is no competing code that + // could compute the surface position from slightly different data. + + let [_, end] = self.boundary; + self.curve.point_from_path_coords(end) + } + /// Access the vertex from where this half-edge starts pub fn start_vertex(&self) -> &Handle { &self.start_vertex diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 16cc826f5..c3e8c4396 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -1,12 +1,129 @@ use crate::objects::Cycle; +use crate::objects::HalfEdge; +use fj_math::Point; +use fj_math::Scalar; +use itertools::Itertools; use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Cycle { fn validate_with_config( &self, - _: &ValidationConfig, - _: &mut Vec, + config: &ValidationConfig, + errors: &mut Vec, ) { + CycleValidationError::check_half_edges_disconnected( + self, config, errors, + ) + } +} + +/// [`Cycle`] validation failed +#[derive(Clone, Debug, thiserror::Error)] +pub enum CycleValidationError { + /// [`Cycle`]'s half-edges are not connected + #[error( + "Adjacent `HalfEdge`s are distinct\n\ + - End position of first `HalfEdge`: {end_of_first:?}\n\ + - Start position of second `HalfEdge`: {start_of_second:?}\n\ + - `HalfEdge`s: {half_edges:#?}" + )] + HalfEdgesDisconnected { + /// The end position of the first [`HalfEdge`] + end_of_first: Point<2>, + + /// The start position of the second [`HalfEdge`] + start_of_second: Point<2>, + + /// The distance between the two vertices + distance: Scalar, + + /// The half-edge + half_edges: (HalfEdge, HalfEdge), + }, + #[error("Expected at least one `HalfEdge`\n")] + NotEnoughHalfEdges, +} + +impl CycleValidationError { + fn check_half_edges_disconnected( + cycle: &Cycle, + config: &ValidationConfig, + errors: &mut Vec, + ) { + for (first, second) in cycle + .half_edges() + .chain(std::iter::once(cycle.half_edges().next().unwrap())) + .tuple_windows() + { + let end_of_first = first.end_position(); + let start_of_second = second.start_position(); + + let distance = (end_of_first - start_of_second).magnitude(); + + if distance > config.identical_max_distance { + errors.push( + Self::HalfEdgesDisconnected { + end_of_first, + start_of_second, + distance, + half_edges: ( + first.clone_object(), + second.clone_object(), + ), + } + .into(), + ); + } + } + } +} + +#[cfg(test)] +mod tests { + use fj_math::Point; + + use crate::{ + builder::{CycleBuilder, HalfEdgeBuilder}, + objects::{Cycle, HalfEdge}, + services::Services, + validate::{ + cycle::CycleValidationError, HalfEdgeValidationError, Validate, + ValidationError, + }, + }; + + #[test] + fn half_edges_connected() -> anyhow::Result<()> { + let mut services = Services::new(); + + let valid = Cycle::new([]) + .update_as_polygon_from_points( + [[0.0, 0.0], [1.0, 0.0], [1.0, 1.0]], + &mut services.objects, + ) + .0; + + valid.validate_and_return_first_error()?; + + let first = HalfEdgeBuilder::line_segment([[0., 0.], [1., 0.]], None) + .build(&mut services.objects); + let second = HalfEdgeBuilder::line_segment([[0., 0.], [1., 0.]], None) + .build(&mut services.objects); + + let invalid = Cycle::new([]) + .add_half_edge(first, &mut services.objects) + .0 + .add_half_edge(second, &mut services.objects) + .0; + + assert!(matches!( + invalid.validate_and_return_first_error(), + Err(ValidationError::Cycle( + CycleValidationError::HalfEdgesDisconnected { .. } + )) + )); + + Ok(()) } } diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index f45bfbd5b..5a0dcfc84 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -9,6 +9,7 @@ mod solid; mod surface; mod vertex; +use self::cycle::CycleValidationError; pub use self::{edge::HalfEdgeValidationError, face::FaceValidationError}; use std::convert::Infallible; @@ -87,6 +88,10 @@ pub enum ValidationError { /// `HalfEdge` validation error #[error("`HalfEdge` validation error:\n{0}")] HalfEdge(#[from] HalfEdgeValidationError), + + /// `Cycle` validation error + #[error("`Cycle` validation error:\n{0}")] + Cycle(#[from] CycleValidationError), } impl From for ValidationError { From 7ab3e71535bb7fd87886ff8781888767951af032 Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Wed, 15 Mar 2023 21:40:46 +0200 Subject: [PATCH 2/7] Make clippy happy --- crates/fj-kernel/src/validate/cycle.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index c3e8c4396..a59106302 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -39,7 +39,7 @@ pub enum CycleValidationError { distance: Scalar, /// The half-edge - half_edges: (HalfEdge, HalfEdge), + half_edges: Box<(HalfEdge, HalfEdge)>, }, #[error("Expected at least one `HalfEdge`\n")] NotEnoughHalfEdges, @@ -67,10 +67,10 @@ impl CycleValidationError { end_of_first, start_of_second, distance, - half_edges: ( + half_edges: Box::new(( first.clone_object(), second.clone_object(), - ), + )), } .into(), ); @@ -81,16 +81,12 @@ impl CycleValidationError { #[cfg(test)] mod tests { - use fj_math::Point; use crate::{ builder::{CycleBuilder, HalfEdgeBuilder}, - objects::{Cycle, HalfEdge}, + objects::Cycle, services::Services, - validate::{ - cycle::CycleValidationError, HalfEdgeValidationError, Validate, - ValidationError, - }, + validate::{cycle::CycleValidationError, Validate, ValidationError}, }; #[test] From 20bc74b8484f5ca479ee7360ba888abf129342db Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Wed, 15 Mar 2023 21:51:19 +0200 Subject: [PATCH 3/7] Cleanup unwrap --- crates/fj-kernel/src/validate/cycle.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index a59106302..60f206d26 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -51,8 +51,15 @@ impl CycleValidationError { config: &ValidationConfig, errors: &mut Vec, ) { + // If there are less than two half edges + if cycle.half_edges().nth(1).is_none() { + errors.push(Self::NotEnoughHalfEdges.into()); + return; + } for (first, second) in cycle .half_edges() + // Chain the first half_edge so that we make sure that the last connects to the first. + // This unwrap will never fail because we checked before that there are enough half_edges. .chain(std::iter::once(cycle.half_edges().next().unwrap())) .tuple_windows() { From 7d1be64c590ed88994f10d7ecc90b2bddf899479 Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Thu, 16 Mar 2023 08:42:00 +0200 Subject: [PATCH 4/7] Change minimum half_edge count to 1 Forgot about the fact that a single circle counts as a cycle --- crates/fj-kernel/src/validate/cycle.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 60f206d26..a7259c956 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -51,8 +51,8 @@ impl CycleValidationError { config: &ValidationConfig, errors: &mut Vec, ) { - // If there are less than two half edges - if cycle.half_edges().nth(1).is_none() { + // If there are no half edges + if cycle.half_edges().next().is_none() { errors.push(Self::NotEnoughHalfEdges.into()); return; } From ae2c0f224b5ccbb5860862262bfba1985dd7486d Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Thu, 16 Mar 2023 13:01:01 +0200 Subject: [PATCH 5/7] Make requested changes - Inlined the `end_position` function to not expose potentially error- prone behaviour. - Seperate empty check to new method. - Switch to `circular_tuple_windows` - Add test for empty cycle --- crates/fj-kernel/src/validate/cycle.rs | 62 ++++++++++++++++---------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index a7259c956..c5a8c3728 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -14,7 +14,8 @@ impl Validate for Cycle { ) { CycleValidationError::check_half_edges_disconnected( self, config, errors, - ) + ); + CycleValidationError::check_enough_half_edges(self, config, errors); } } @@ -46,9 +47,9 @@ pub enum CycleValidationError { } impl CycleValidationError { - fn check_half_edges_disconnected( + fn check_enough_half_edges( cycle: &Cycle, - config: &ValidationConfig, + _config: &ValidationConfig, errors: &mut Vec, ) { // If there are no half edges @@ -56,14 +57,18 @@ impl CycleValidationError { errors.push(Self::NotEnoughHalfEdges.into()); return; } - for (first, second) in cycle - .half_edges() - // Chain the first half_edge so that we make sure that the last connects to the first. - // This unwrap will never fail because we checked before that there are enough half_edges. - .chain(std::iter::once(cycle.half_edges().next().unwrap())) - .tuple_windows() - { - let end_of_first = first.end_position(); + } + + fn check_half_edges_disconnected( + cycle: &Cycle, + config: &ValidationConfig, + errors: &mut Vec, + ) { + for (first, second) in cycle.half_edges().circular_tuple_windows() { + let end_of_first = { + let [_, end] = first.boundary(); + first.curve().point_from_path_coords(end) + }; let start_of_second = second.start_position(); let distance = (end_of_first - start_of_second).magnitude(); @@ -109,24 +114,35 @@ mod tests { valid.validate_and_return_first_error()?; - let first = HalfEdgeBuilder::line_segment([[0., 0.], [1., 0.]], None) - .build(&mut services.objects); - let second = HalfEdgeBuilder::line_segment([[0., 0.], [1., 0.]], None) - .build(&mut services.objects); - - let invalid = Cycle::new([]) - .add_half_edge(first, &mut services.objects) - .0 - .add_half_edge(second, &mut services.objects) - .0; - + let disconnected = { + let first = + HalfEdgeBuilder::line_segment([[0., 0.], [1., 0.]], None) + .build(&mut services.objects); + let second = + HalfEdgeBuilder::line_segment([[0., 0.], [1., 0.]], None) + .build(&mut services.objects); + + Cycle::new([]) + .add_half_edge(first, &mut services.objects) + .0 + .add_half_edge(second, &mut services.objects) + .0 + }; assert!(matches!( - invalid.validate_and_return_first_error(), + disconnected.validate_and_return_first_error(), Err(ValidationError::Cycle( CycleValidationError::HalfEdgesDisconnected { .. } )) )); + let empty = Cycle::new([]); + assert!(matches!( + empty.validate_and_return_first_error(), + Err(ValidationError::Cycle( + CycleValidationError::NotEnoughHalfEdges + )) + )); + Ok(()) } } From 77bb3f521b16d6e8a7e59c5daebd677d41fad0bf Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Thu, 16 Mar 2023 13:08:54 +0200 Subject: [PATCH 6/7] Make clippy happy --- crates/fj-kernel/src/validate/cycle.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index c5a8c3728..0c0442038 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -55,7 +55,6 @@ impl CycleValidationError { // If there are no half edges if cycle.half_edges().next().is_none() { errors.push(Self::NotEnoughHalfEdges.into()); - return; } } From f0423c66e294be6a9f0445879e9f681123f33db6 Mon Sep 17 00:00:00 2001 From: A-Walrus Date: Thu, 16 Mar 2023 14:09:47 +0200 Subject: [PATCH 7/7] Remove obselete `end_position` function --- crates/fj-kernel/src/objects/full/edge.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/crates/fj-kernel/src/objects/full/edge.rs b/crates/fj-kernel/src/objects/full/edge.rs index 646ea688b..65216649b 100644 --- a/crates/fj-kernel/src/objects/full/edge.rs +++ b/crates/fj-kernel/src/objects/full/edge.rs @@ -86,16 +86,6 @@ impl HalfEdge { self.curve.point_from_path_coords(start) } - /// Compute the surface position where the half-edge ends - pub fn end_position(&self) -> Point<2> { - // Computing the surface position from the curve position is fine. - // `HalfEdge` "owns" its end position. There is no competing code that - // could compute the surface position from slightly different data. - - let [_, end] = self.boundary; - self.curve.point_from_path_coords(end) - } - /// Access the vertex from where this half-edge starts pub fn start_vertex(&self) -> &Handle { &self.start_vertex