From 92438c42c5aca246d6b27ad9f9fe13c1db0719bf Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 24 Jan 2023 16:45:37 +0100 Subject: [PATCH 1/8] Make more use of `HalfEdge::start_vertex` --- .../fj-kernel/src/algorithms/intersect/face_point.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/intersect/face_point.rs b/crates/fj-kernel/src/algorithms/intersect/face_point.rs index 1e018e9df2..5fcdcb72a3 100644 --- a/crates/fj-kernel/src/algorithms/intersect/face_point.rs +++ b/crates/fj-kernel/src/algorithms/intersect/face_point.rs @@ -1,6 +1,7 @@ //! Intersection between faces and points in 2D use fj_math::Point; +use itertools::Itertools; use crate::{ objects::{Face, HalfEdge, SurfaceVertex}, @@ -33,7 +34,9 @@ impl Intersect for (&Handle, &Point<2>) { .cloned() .and_then(|edge| (&ray, &edge).intersect()); - for half_edge in cycle.half_edges() { + for (half_edge, next_half_edge) in + cycle.half_edges().circular_tuple_windows::<(_, _)>() + { let hit = (&ray, half_edge).intersect(); let count_hit = match (hit, previous_hit) { @@ -48,15 +51,13 @@ impl Intersect for (&Handle, &Point<2>) { )); } (Some(RaySegmentIntersection::RayStartsOnOnFirstVertex), _) => { - let [vertex, _] = - half_edge.surface_vertices().map(Clone::clone); + let vertex = half_edge.start_vertex().clone(); return Some( FacePointIntersection::PointIsOnVertex(vertex) ); } (Some(RaySegmentIntersection::RayStartsOnSecondVertex), _) => { - let [_, vertex] = - half_edge.surface_vertices().map(Clone::clone); + let vertex = next_half_edge.start_vertex().clone(); return Some( FacePointIntersection::PointIsOnVertex(vertex) ); From fdd1c9539b8b56df1ad073dc5a77b1197bd0b39c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 25 Jan 2023 12:39:51 +0100 Subject: [PATCH 2/8] Move validation check from `HalfEdge` to `Cycle` `HalfEdge` will soon only reference a single `SurfaceVertex`. Since this validation check relies on having access to both, it needs to move to the level of `Cycle`. --- crates/fj-kernel/src/validate/cycle.rs | 98 +++++++++++++++++++++++++- crates/fj-kernel/src/validate/edge.rs | 87 +---------------------- 2 files changed, 98 insertions(+), 87 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 3e253c0094..86ac24cec4 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -1,3 +1,5 @@ +use fj_interop::ext::ArrayExt; +use fj_math::{Point, Scalar}; use itertools::Itertools; use crate::{ @@ -10,10 +12,11 @@ use super::{Validate, ValidationConfig, ValidationError}; impl Validate for Cycle { fn validate_with_config( &self, - _: &ValidationConfig, + config: &ValidationConfig, errors: &mut Vec, ) { CycleValidationError::check_half_edge_connections(self, errors); + CycleValidationError::check_position(self, config, 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 @@ -37,6 +40,28 @@ pub enum CycleValidationError { /// The back vertex of the next half-edge next: Handle, }, + + /// Mismatch between position of the vertex and position of its surface form + #[error( + "Half-edge boundary on curve doesn't match surface vertex position\n\ + - Position on curve: {position_on_curve:#?}\n\ + - Surface vertex: {surface_vertex:#?}\n\ + - Curve position converted to surface: {curve_position_on_surface:?}\n\ + - Distance between the positions: {distance}" + )] + VertexPositionMismatch { + /// The position on the curve + position_on_curve: Point<1>, + + /// The surface vertex + surface_vertex: Handle, + + /// The curve position converted into a surface position + curve_position_on_surface: Point<2>, + + /// The distance between the positions + distance: Scalar, + }, } impl CycleValidationError { @@ -59,10 +84,45 @@ impl CycleValidationError { } } } + + fn check_position( + cycle: &Cycle, + config: &ValidationConfig, + errors: &mut Vec, + ) { + for half_edge in cycle.half_edges() { + for (position_on_curve, surface_vertex) in + half_edge.boundary().zip_ext(half_edge.surface_vertices()) + { + let curve_position_on_surface = half_edge + .curve() + .path() + .point_from_path_coords(position_on_curve); + let surface_position = surface_vertex.position(); + + let distance = + curve_position_on_surface.distance_to(&surface_position); + + if distance > config.identical_max_distance { + errors.push( + Self::VertexPositionMismatch { + position_on_curve, + surface_vertex: surface_vertex.clone(), + curve_position_on_surface, + distance, + } + .into(), + ); + } + } + } + } } #[cfg(test)] mod tests { + use fj_math::Point; + use crate::{ builder::CycleBuilder, objects::Cycle, @@ -111,4 +171,40 @@ mod tests { Ok(()) } + + #[test] + fn vertex_position_mismatch() -> anyhow::Result<()> { + let mut services = Services::new(); + + let valid = { + let mut cycle = PartialCycle { + surface: Partial::from(services.objects.surfaces.xy_plane()), + ..Default::default() + }; + cycle.update_as_polygon_from_points([[0., 0.], [1., 0.], [0., 1.]]); + cycle.build(&mut services.objects) + }; + let invalid = { + let mut half_edges = valid + .half_edges() + .map(|half_edge| Partial::from(half_edge.clone())) + .collect::>(); + + // Update a single boundary position so it becomes wrong. + if let Some(half_edge) = half_edges.first_mut() { + half_edge.write().vertices[0].0.replace(Point::from([-1.])); + } + + let half_edges = half_edges + .into_iter() + .map(|half_edge| half_edge.build(&mut services.objects)); + + Cycle::new(half_edges) + }; + + 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 bbcb2765a3..7a4a5c2650 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -4,7 +4,7 @@ use fj_math::{Point, Scalar}; use crate::{ objects::{ GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Surface, - SurfaceVertex, VerticesInNormalizedOrder, + VerticesInNormalizedOrder, }, storage::Handle, }; @@ -21,7 +21,6 @@ impl Validate for HalfEdge { HalfEdgeValidationError::check_global_vertex_identity(self, errors); HalfEdgeValidationError::check_surface_identity(self, errors); HalfEdgeValidationError::check_vertex_positions(self, config, errors); - HalfEdgeValidationError::check_position(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, @@ -115,28 +114,6 @@ pub enum HalfEdgeValidationError { /// The half-edge half_edge: HalfEdge, }, - - /// Mismatch between position of the vertex and position of its surface form - #[error( - "Position on curve doesn't match surface vertex position\n\ - - Position on curve: {position_on_curve:#?}\n\ - - Surface vertex: {surface_vertex:#?}\n\ - - Curve position converted to surface: {curve_position_on_surface:?}\n\ - - Distance between the positions: {distance}" - )] - VertexPositionMismatch { - /// The position on the curve - position_on_curve: Point<1>, - - /// The surface vertex - surface_vertex: Handle, - - /// The curve position converted into a surface position - curve_position_on_surface: Point<2>, - - /// The distance between the positions - distance: Scalar, - }, } impl HalfEdgeValidationError { @@ -239,37 +216,6 @@ impl HalfEdgeValidationError { ); } } - - fn check_position( - half_edge: &HalfEdge, - config: &ValidationConfig, - errors: &mut Vec, - ) { - for (position_on_curve, surface_vertex) in - half_edge.boundary().zip_ext(half_edge.surface_vertices()) - { - let curve_position_on_surface = half_edge - .curve() - .path() - .point_from_path_coords(position_on_curve); - let surface_position = surface_vertex.position(); - - let distance = - curve_position_on_surface.distance_to(&surface_position); - - if distance > config.identical_max_distance { - errors.push( - Box::new(Self::VertexPositionMismatch { - position_on_curve, - surface_vertex: surface_vertex.clone(), - curve_position_on_surface, - distance, - }) - .into(), - ); - } - } - } } #[cfg(test)] @@ -431,35 +377,4 @@ mod tests { Ok(()) } - - #[test] - fn vertex_position_mismatch() -> anyhow::Result<()> { - let mut services = Services::new(); - - let valid = { - let mut half_edge = PartialHalfEdge::default(); - half_edge.update_as_line_segment_from_points( - services.objects.surfaces.xy_plane(), - [[0., 0.], [1., 0.]], - ); - - half_edge.build(&mut services.objects) - }; - let invalid = { - let vertices = valid.surface_vertices().map(|surface_vertex| { - (Point::from([2.]), surface_vertex.clone()) - }); - - HalfEdge::new( - valid.curve().clone(), - vertices, - valid.global_form().clone(), - ) - }; - - valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); - - Ok(()) - } } From 9d6feccd2f63d74f3ed8496ce8173873fef14c14 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 25 Jan 2023 12:41:29 +0100 Subject: [PATCH 3/8] Update name of enum variant --- 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 86ac24cec4..01b1dd2af4 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -49,7 +49,7 @@ pub enum CycleValidationError { - Curve position converted to surface: {curve_position_on_surface:?}\n\ - Distance between the positions: {distance}" )] - VertexPositionMismatch { + HalfEdgeBoundaryMismatch { /// The position on the curve position_on_curve: Point<1>, @@ -105,7 +105,7 @@ impl CycleValidationError { if distance > config.identical_max_distance { errors.push( - Self::VertexPositionMismatch { + Self::HalfEdgeBoundaryMismatch { position_on_curve, surface_vertex: surface_vertex.clone(), curve_position_on_surface, From 9d0090c5c9842a0a134a4968302c301a7efc3248 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 25 Jan 2023 12:42:06 +0100 Subject: [PATCH 4/8] Update method name --- 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 01b1dd2af4..41564eb2b4 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -16,7 +16,7 @@ impl Validate for Cycle { errors: &mut Vec, ) { CycleValidationError::check_half_edge_connections(self, errors); - CycleValidationError::check_position(self, config, errors); + CycleValidationError::check_half_edge_boundaries(self, config, 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 @@ -85,7 +85,7 @@ impl CycleValidationError { } } - fn check_position( + fn check_half_edge_boundaries( cycle: &Cycle, config: &ValidationConfig, errors: &mut Vec, From 4bc3ec17d4181bffce4cc462f94b7bae9d6a9f05 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 25 Jan 2023 12:42:23 +0100 Subject: [PATCH 5/8] Update doc comment --- crates/fj-kernel/src/validate/cycle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 41564eb2b4..725ac7ad40 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -41,7 +41,7 @@ pub enum CycleValidationError { next: Handle, }, - /// Mismatch between position of the vertex and position of its surface form + /// Mismatch between half-edge boundary and surface vertex position #[error( "Half-edge boundary on curve doesn't match surface vertex position\n\ - Position on curve: {position_on_curve:#?}\n\ From 917e21af591a8d35c4a290fb88087bd26f6efe92 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 25 Jan 2023 12:45:37 +0100 Subject: [PATCH 6/8] Avoid using `HalfEdge::surface_vertices` --- crates/fj-kernel/src/validate/cycle.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 725ac7ad40..b186e34298 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -90,10 +90,13 @@ impl CycleValidationError { config: &ValidationConfig, errors: &mut Vec, ) { - for half_edge in cycle.half_edges() { - for (position_on_curve, surface_vertex) in - half_edge.boundary().zip_ext(half_edge.surface_vertices()) - { + for (half_edge, next) in + cycle.half_edges().circular_tuple_windows::<(_, _)>() + { + let boundary_and_vertices = half_edge + .boundary() + .zip_ext([half_edge.start_vertex(), next.start_vertex()]); + for (position_on_curve, surface_vertex) in boundary_and_vertices { let curve_position_on_surface = half_edge .curve() .path() From 0091cff219f718e4b27daaf9b18f4df6620d47d5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 25 Jan 2023 12:55:20 +0100 Subject: [PATCH 7/8] Simplify validation checks Since surface vertices are shared between neighboring half-edges, it's overkill to check both surface vertices of each half-edge. --- crates/fj-kernel/src/validate/edge.rs | 61 ++++++++++----------------- 1 file changed, 22 insertions(+), 39 deletions(-) diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index 7a4a5c2650..d2c585363e 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -1,11 +1,7 @@ -use fj_interop::ext::ArrayExt; use fj_math::{Point, Scalar}; use crate::{ - objects::{ - GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Surface, - VerticesInNormalizedOrder, - }, + objects::{GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Surface}, storage::Handle, }; @@ -63,15 +59,14 @@ pub enum HalfEdgeValidationError { #[error( "Global forms of `HalfEdge` vertices do not match vertices of \n\ `HalfEdge`'s global form\n\ - - `GlobalVertex` objects from `Vertex` objects: \ - {global_vertices_from_vertices:#?}\n\ + - `GlobalVertex` from start vertex: {global_vertex_from_half_edge:#?}\n\ - `GlobalVertex` objects from `GlobalEdge`: \ {global_vertices_from_global_form:#?}\n\ - `HalfEdge`: {half_edge:#?}" )] GlobalVertexMismatch { - /// The [`GlobalVertex`] from the [`HalfEdge`]'s vertices - global_vertices_from_vertices: [Handle; 2], + /// The [`GlobalVertex`] from the [`HalfEdge`]'s start vertex + global_vertex_from_half_edge: Handle, /// The [`GlobalCurve`] from the [`HalfEdge`]'s global form global_vertices_from_global_form: [Handle; 2], @@ -141,32 +136,23 @@ impl HalfEdgeValidationError { half_edge: &HalfEdge, errors: &mut Vec, ) { - let global_vertices_from_vertices = { - let (global_vertices_from_vertices, _) = - VerticesInNormalizedOrder::new( - half_edge.surface_vertices().each_ref_ext().map( - |surface_vertex| surface_vertex.global_form().clone(), - ), - ); - - global_vertices_from_vertices.access_in_normalized_order() - }; + let global_vertex_from_half_edge = + half_edge.start_vertex().global_form().clone(); let global_vertices_from_global_form = half_edge .global_form() .vertices() .access_in_normalized_order(); - let ids_from_vertices = global_vertices_from_vertices - .each_ref_ext() - .map(|global_vertex| global_vertex.id()); - let ids_from_global_form = global_vertices_from_global_form - .each_ref_ext() - .map(|global_vertex| global_vertex.id()); + let matching_global_vertex = global_vertices_from_global_form + .iter() + .find(|global_vertex| { + global_vertex.id() == global_vertex_from_half_edge.id() + }); - if ids_from_vertices != ids_from_global_form { + if matching_global_vertex.is_none() { errors.push( Box::new(Self::GlobalVertexMismatch { - global_vertices_from_vertices, + global_vertex_from_half_edge, global_vertices_from_global_form, half_edge: half_edge.clone(), }) @@ -180,19 +166,16 @@ impl HalfEdgeValidationError { errors: &mut Vec, ) { let curve_surface = half_edge.curve().surface(); + let surface_form_surface = half_edge.start_vertex().surface(); - for surface_vertex in half_edge.surface_vertices() { - let surface_form_surface = surface_vertex.surface(); - - if curve_surface.id() != surface_form_surface.id() { - errors.push( - Box::new(Self::SurfaceMismatch { - curve_surface: curve_surface.clone(), - surface_form_surface: surface_form_surface.clone(), - }) - .into(), - ); - } + if curve_surface.id() != surface_form_surface.id() { + errors.push( + Box::new(Self::SurfaceMismatch { + curve_surface: curve_surface.clone(), + surface_form_surface: surface_form_surface.clone(), + }) + .into(), + ); } } From d5227b7512d9c1240f788d8f15b5e52acef59704 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 25 Jan 2023 13:11:51 +0100 Subject: [PATCH 8/8] Fix doc comment --- crates/fj-kernel/src/validate/edge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index d2c585363e..75445fefdf 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -68,7 +68,7 @@ pub enum HalfEdgeValidationError { /// The [`GlobalVertex`] from the [`HalfEdge`]'s start vertex global_vertex_from_half_edge: Handle, - /// The [`GlobalCurve`] from the [`HalfEdge`]'s global form + /// The [`GlobalVertex`] instances from the [`HalfEdge`]'s global form global_vertices_from_global_form: [Handle; 2], /// The half-edge