Continue cleanup of HalfEdge

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<Face>, &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<Face>, &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)
                         );

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<ValidationError>,
     ) {
         CycleValidationError::check_half_edge_connections(self, 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
@@ -37,6 +40,28 @@ pub enum CycleValidationError {
         /// The back vertex of the next half-edge
         next: Handle<SurfaceVertex>,
     },
+
+    /// 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\
+        - Surface vertex: {surface_vertex:#?}\n\
+        - Curve position converted to surface: {curve_position_on_surface:?}\n\
+        - Distance between the positions: {distance}"
+    )]
+    HalfEdgeBoundaryMismatch {
+        /// The position on the curve
+        position_on_curve: Point<1>,
+
+        /// The surface vertex
+        surface_vertex: Handle<SurfaceVertex>,
+
+        /// 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,48 @@ impl CycleValidationError {
             }
         }
     }
+
+    fn check_half_edge_boundaries(
+        cycle: &Cycle,
+        config: &ValidationConfig,
+        errors: &mut Vec<ValidationError>,
+    ) {
+        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()
+                    .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::HalfEdgeBoundaryMismatch {
+                            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 +174,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::<Vec<_>>();
+
+            // 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(())
+    }
 }

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,
-        SurfaceVertex, VerticesInNormalizedOrder,
-    },
+    objects::{GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Surface},
     storage::Handle,
 };
 
@@ -21,7 +17,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,
@@ -64,17 +59,16 @@ 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<GlobalVertex>; 2],
+        /// The [`GlobalVertex`] from the [`HalfEdge`]'s start vertex
+        global_vertex_from_half_edge: Handle<GlobalVertex>,
 
-        /// The [`GlobalCurve`] from the [`HalfEdge`]'s global form
+        /// The [`GlobalVertex`] instances from the [`HalfEdge`]'s global form
         global_vertices_from_global_form: [Handle<GlobalVertex>; 2],
 
         /// The half-edge
@@ -115,28 +109,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<SurfaceVertex>,
-
-        /// The curve position converted into a surface position
-        curve_position_on_surface: Point<2>,
-
-        /// The distance between the positions
-        distance: Scalar,
-    },
 }
 
 impl HalfEdgeValidationError {
@@ -164,32 +136,23 @@ impl HalfEdgeValidationError {
         half_edge: &HalfEdge,
         errors: &mut Vec<ValidationError>,
     ) {
-        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(),
                 })
@@ -203,19 +166,16 @@ impl HalfEdgeValidationError {
         errors: &mut Vec<ValidationError>,
     ) {
         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(),
+            );
         }
     }
 
@@ -239,37 +199,6 @@ impl HalfEdgeValidationError {
             );
         }
     }
-
-    fn check_position(
-        half_edge: &HalfEdge,
-        config: &ValidationConfig,
-        errors: &mut Vec<ValidationError>,
-    ) {
-        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 +360,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(())
-    }
 }