From cbceccdced5a44a31e3b7b4dce76bc6652dd8562 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 27 Mar 2024 14:16:14 +0100 Subject: [PATCH 1/5] Add explicit lifetime to `ValidationCheck::check` I'm running into a situation with an implementation, where the lifetime is no longer correctly infered. Making it explicit is the only way I could find to address that. --- crates/fj-core/src/validation/checks/face_boundary.rs | 10 +++++----- crates/fj-core/src/validation/checks/face_winding.rs | 10 +++++----- .../src/validation/checks/half_edge_connection.rs | 10 +++++----- crates/fj-core/src/validation/validation_check.rs | 10 +++++----- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/crates/fj-core/src/validation/checks/face_boundary.rs b/crates/fj-core/src/validation/checks/face_boundary.rs index 3f48cc731..737ebae1c 100644 --- a/crates/fj-core/src/validation/checks/face_boundary.rs +++ b/crates/fj-core/src/validation/checks/face_boundary.rs @@ -17,11 +17,11 @@ use crate::{ pub struct FaceHasNoBoundary {} impl ValidationCheck for FaceHasNoBoundary { - fn check( - object: &Face, - _: &Geometry, - _: &ValidationConfig, - ) -> impl Iterator { + fn check<'r>( + object: &'r Face, + _: &'r Geometry, + _: &'r ValidationConfig, + ) -> impl Iterator + 'r { let error = if object.region().exterior().half_edges().is_empty() { Some(FaceHasNoBoundary {}) } else { diff --git a/crates/fj-core/src/validation/checks/face_winding.rs b/crates/fj-core/src/validation/checks/face_winding.rs index af8baf7a9..fb2273bce 100644 --- a/crates/fj-core/src/validation/checks/face_winding.rs +++ b/crates/fj-core/src/validation/checks/face_winding.rs @@ -37,11 +37,11 @@ pub struct InteriorCycleHasInvalidWinding { } impl ValidationCheck for InteriorCycleHasInvalidWinding { - fn check( - object: &Face, - geometry: &Geometry, - _: &ValidationConfig, - ) -> impl Iterator { + fn check<'r>( + object: &'r Face, + geometry: &'r Geometry, + _: &'r ValidationConfig, + ) -> impl Iterator + 'r { object.region().interiors().iter().filter_map(|interior| { let exterior = object.region().exterior(); diff --git a/crates/fj-core/src/validation/checks/half_edge_connection.rs b/crates/fj-core/src/validation/checks/half_edge_connection.rs index 554799b23..f0a4bd43d 100644 --- a/crates/fj-core/src/validation/checks/half_edge_connection.rs +++ b/crates/fj-core/src/validation/checks/half_edge_connection.rs @@ -58,11 +58,11 @@ pub struct AdjacentHalfEdgesNotConnected { } impl ValidationCheck for AdjacentHalfEdgesNotConnected { - fn check( - object: &Cycle, - geometry: &Geometry, - config: &ValidationConfig, - ) -> impl Iterator { + fn check<'r>( + object: &'r Cycle, + geometry: &'r Geometry, + config: &'r ValidationConfig, + ) -> impl Iterator + 'r { object.half_edges().pairs().filter_map(|(first, second)| { let end_pos_of_first_half_edge = { let [_, end] = geometry.of_half_edge(first).boundary.inner; diff --git a/crates/fj-core/src/validation/validation_check.rs b/crates/fj-core/src/validation/validation_check.rs index 154536993..99922a7a1 100644 --- a/crates/fj-core/src/validation/validation_check.rs +++ b/crates/fj-core/src/validation/validation_check.rs @@ -10,11 +10,11 @@ use super::ValidationConfig; /// to. `Self` is the object, while `T` identifies the validation check. pub trait ValidationCheck: Sized { /// Run the validation check on the implementing object - fn check( - object: &T, - geometry: &Geometry, - config: &ValidationConfig, - ) -> impl Iterator; + fn check<'r>( + object: &'r T, + geometry: &'r Geometry, + config: &'r ValidationConfig, + ) -> impl Iterator + 'r; /// Convenience method to run the check return the first error /// From 8453a89ef311a78db751fbd0ffeece7389898f09 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 27 Mar 2024 14:17:29 +0100 Subject: [PATCH 2/5] Move check to separate function This prepares for implementing the validation check for multiple object types, with slightly different implementations for each. --- .../validation/checks/half_edge_connection.rs | 60 +++++++++++-------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/crates/fj-core/src/validation/checks/half_edge_connection.rs b/crates/fj-core/src/validation/checks/half_edge_connection.rs index f0a4bd43d..e2071fb0e 100644 --- a/crates/fj-core/src/validation/checks/half_edge_connection.rs +++ b/crates/fj-core/src/validation/checks/half_edge_connection.rs @@ -63,35 +63,43 @@ impl ValidationCheck for AdjacentHalfEdgesNotConnected { geometry: &'r Geometry, config: &'r ValidationConfig, ) -> impl Iterator + 'r { - object.half_edges().pairs().filter_map(|(first, second)| { - let end_pos_of_first_half_edge = { - let [_, end] = geometry.of_half_edge(first).boundary.inner; - geometry - .of_half_edge(first) - .path - .point_from_path_coords(end) - }; - let start_pos_of_second_half_edge = - geometry.of_half_edge(second).start_position(); - - let distance_between_positions = (end_pos_of_first_half_edge - - start_pos_of_second_half_edge) - .magnitude(); - - if distance_between_positions > config.identical_max_distance { - return Some(AdjacentHalfEdgesNotConnected { - end_pos_of_first_half_edge, - start_pos_of_second_half_edge, - distance_between_positions, - unconnected_half_edges: [first.clone(), second.clone()], - }); - } - - None - }) + check_cycle(object, geometry, config) } } +fn check_cycle<'r>( + object: &'r Cycle, + geometry: &'r Geometry, + config: &'r ValidationConfig, +) -> impl Iterator + 'r { + object.half_edges().pairs().filter_map(|(first, second)| { + let end_pos_of_first_half_edge = { + let [_, end] = geometry.of_half_edge(first).boundary.inner; + geometry + .of_half_edge(first) + .path + .point_from_path_coords(end) + }; + let start_pos_of_second_half_edge = + geometry.of_half_edge(second).start_position(); + + let distance_between_positions = (end_pos_of_first_half_edge + - start_pos_of_second_half_edge) + .magnitude(); + + if distance_between_positions > config.identical_max_distance { + return Some(AdjacentHalfEdgesNotConnected { + end_pos_of_first_half_edge, + start_pos_of_second_half_edge, + distance_between_positions, + unconnected_half_edges: [first.clone(), second.clone()], + }); + } + + None + }) +} + #[cfg(test)] mod tests { From 2cf29e6851438c402eae59dbe5287e6f6b0b06c6 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 23 Apr 2024 13:46:31 +0200 Subject: [PATCH 3/5] Update argument name --- crates/fj-core/src/validation/checks/half_edge_connection.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/validation/checks/half_edge_connection.rs b/crates/fj-core/src/validation/checks/half_edge_connection.rs index e2071fb0e..7e61cee87 100644 --- a/crates/fj-core/src/validation/checks/half_edge_connection.rs +++ b/crates/fj-core/src/validation/checks/half_edge_connection.rs @@ -68,11 +68,11 @@ impl ValidationCheck for AdjacentHalfEdgesNotConnected { } fn check_cycle<'r>( - object: &'r Cycle, + cycle: &'r Cycle, geometry: &'r Geometry, config: &'r ValidationConfig, ) -> impl Iterator + 'r { - object.half_edges().pairs().filter_map(|(first, second)| { + cycle.half_edges().pairs().filter_map(|(first, second)| { let end_pos_of_first_half_edge = { let [_, end] = geometry.of_half_edge(first).boundary.inner; geometry From 1a483918cf34480869a6917755d1c7424a9e0f18 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 23 Apr 2024 14:38:22 +0200 Subject: [PATCH 4/5] Clean up imports --- crates/fj-core/src/validate/sketch.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/fj-core/src/validate/sketch.rs b/crates/fj-core/src/validate/sketch.rs index 39170e10f..5445cc235 100644 --- a/crates/fj-core/src/validate/sketch.rs +++ b/crates/fj-core/src/validate/sketch.rs @@ -1,8 +1,12 @@ -use crate::geometry::Geometry; -use crate::{storage::Handle, topology::Cycle}; -use crate::{topology::Sketch, validate_references}; use fj_math::Winding; +use crate::{ + geometry::Geometry, + storage::Handle, + topology::{Cycle, Sketch}, + validate_references, +}; + use super::{ references::{ReferenceCountError, ReferenceCounter}, Validate, ValidationConfig, ValidationError, From 97bf88fbe8b5e9226c0ca0dd85becd5c8b404321 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 23 Apr 2024 14:38:58 +0200 Subject: [PATCH 5/5] Move `Cycle` validation check to `Face`/`Sketch` The code of this check is going to require access to a `Surface` soon, due to changes I'm working on. The reshuffling done in this commit is preparation for having this surface available. --- crates/fj-core/src/validate/cycle.rs | 15 ++-- crates/fj-core/src/validate/face.rs | 9 ++- crates/fj-core/src/validate/sketch.rs | 5 ++ .../validation/checks/half_edge_connection.rs | 68 +++++++++++++++---- 4 files changed, 73 insertions(+), 24 deletions(-) diff --git a/crates/fj-core/src/validate/cycle.rs b/crates/fj-core/src/validate/cycle.rs index bc9ed4ddf..854a9adba 100644 --- a/crates/fj-core/src/validate/cycle.rs +++ b/crates/fj-core/src/validate/cycle.rs @@ -1,10 +1,7 @@ use crate::{ geometry::Geometry, topology::Cycle, - validation::{ - checks::AdjacentHalfEdgesNotConnected, ValidationCheck, - ValidationConfig, ValidationError, - }, + validation::{ValidationConfig, ValidationError}, }; use super::Validate; @@ -12,13 +9,9 @@ use super::Validate; impl Validate for Cycle { fn validate( &self, - config: &ValidationConfig, - errors: &mut Vec, - geometry: &Geometry, + _: &ValidationConfig, + _: &mut Vec, + _: &Geometry, ) { - errors.extend( - AdjacentHalfEdgesNotConnected::check(self, geometry, config) - .map(Into::into), - ); } } diff --git a/crates/fj-core/src/validate/face.rs b/crates/fj-core/src/validate/face.rs index 4ffd9d9c0..a168dff8a 100644 --- a/crates/fj-core/src/validate/face.rs +++ b/crates/fj-core/src/validate/face.rs @@ -2,7 +2,10 @@ use crate::{ geometry::Geometry, topology::Face, validation::{ - checks::{FaceHasNoBoundary, InteriorCycleHasInvalidWinding}, + checks::{ + AdjacentHalfEdgesNotConnected, FaceHasNoBoundary, + InteriorCycleHasInvalidWinding, + }, ValidationCheck, ValidationConfig, ValidationError, }, }; @@ -16,6 +19,10 @@ impl Validate for Face { errors: &mut Vec, geometry: &Geometry, ) { + errors.extend( + AdjacentHalfEdgesNotConnected::check(self, geometry, config) + .map(Into::into), + ); errors.extend( FaceHasNoBoundary::check(self, geometry, config).map(Into::into), ); diff --git a/crates/fj-core/src/validate/sketch.rs b/crates/fj-core/src/validate/sketch.rs index 5445cc235..b8231bd09 100644 --- a/crates/fj-core/src/validate/sketch.rs +++ b/crates/fj-core/src/validate/sketch.rs @@ -5,6 +5,7 @@ use crate::{ storage::Handle, topology::{Cycle, Sketch}, validate_references, + validation::{checks::AdjacentHalfEdgesNotConnected, ValidationCheck}, }; use super::{ @@ -19,6 +20,10 @@ impl Validate for Sketch { errors: &mut Vec, geometry: &Geometry, ) { + errors.extend( + AdjacentHalfEdgesNotConnected::check(self, geometry, config) + .map(Into::into), + ); SketchValidationError::check_object_references(self, config, errors); SketchValidationError::check_exterior_cycles( self, geometry, config, errors, diff --git a/crates/fj-core/src/validation/checks/half_edge_connection.rs b/crates/fj-core/src/validation/checks/half_edge_connection.rs index 7e61cee87..489b42608 100644 --- a/crates/fj-core/src/validation/checks/half_edge_connection.rs +++ b/crates/fj-core/src/validation/checks/half_edge_connection.rs @@ -3,7 +3,7 @@ use fj_math::{Point, Scalar}; use crate::{ geometry::Geometry, storage::Handle, - topology::{Cycle, HalfEdge}, + topology::{Cycle, Face, HalfEdge, Region, Sketch}, validation::{validation_check::ValidationCheck, ValidationConfig}, }; @@ -57,16 +57,40 @@ pub struct AdjacentHalfEdgesNotConnected { pub unconnected_half_edges: [Handle; 2], } -impl ValidationCheck for AdjacentHalfEdgesNotConnected { +impl ValidationCheck for AdjacentHalfEdgesNotConnected { fn check<'r>( - object: &'r Cycle, + object: &'r Face, geometry: &'r Geometry, config: &'r ValidationConfig, ) -> impl Iterator + 'r { - check_cycle(object, geometry, config) + check_region(object.region(), geometry, config) } } +impl ValidationCheck for AdjacentHalfEdgesNotConnected { + fn check<'r>( + object: &'r Sketch, + geometry: &'r Geometry, + config: &'r ValidationConfig, + ) -> impl Iterator + 'r { + object + .regions() + .iter() + .flat_map(|region| check_region(region, geometry, config)) + } +} + +fn check_region<'r>( + region: &'r Region, + geometry: &'r Geometry, + config: &'r ValidationConfig, +) -> impl Iterator + 'r { + [region.exterior()] + .into_iter() + .chain(region.interiors()) + .flat_map(|cycle| check_cycle(cycle, geometry, config)) +} + fn check_cycle<'r>( cycle: &'r Cycle, geometry: &'r Geometry, @@ -105,10 +129,10 @@ mod tests { use crate::{ operations::{ - build::{BuildCycle, BuildHalfEdge}, - update::UpdateCycle, + build::{BuildFace, BuildHalfEdge}, + update::{UpdateCycle, UpdateFace, UpdateRegion}, }, - topology::{Cycle, HalfEdge}, + topology::{Face, HalfEdge}, validation::ValidationCheck, Core, }; @@ -119,16 +143,36 @@ mod tests { fn adjacent_half_edges_not_connected() -> anyhow::Result<()> { let mut core = Core::new(); - let valid = Cycle::polygon([[0., 0.], [1., 0.], [1., 1.]], &mut core); + // We're only testing for `Face` here, not `Sketch`. Should be fine, as + // most of the code is shared. + let valid = Face::polygon( + core.layers.topology.surfaces.space_2d(), + [[0., 0.], [1., 0.], [1., 1.]], + &mut core, + ); AdjacentHalfEdgesNotConnected::check_and_return_first_error( &valid, &core.layers.geometry, )?; - let invalid = valid.update_half_edge( - valid.half_edges().first(), - |_, core| { - [HalfEdge::line_segment([[0., 0.], [2., 0.]], None, core)] + let invalid = valid.update_region( + |region, core| { + region.update_exterior( + |cycle, core| { + cycle.update_half_edge( + cycle.half_edges().first(), + |_, core| { + [HalfEdge::line_segment( + [[0., 0.], [2., 0.]], + None, + core, + )] + }, + core, + ) + }, + core, + ) }, &mut core, );