Merge pull request #1604 from hannobraun/surface

Consolidate redundant references to `Surface`

crates/fj-kernel/src/algorithms/intersect/curve_face.rs

@@ -161,8 +161,6 @@ mod tests {
     fn compute() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut curve = PartialCurve::default();
         curve.update_as_line_from_points([[-3., 0.], [-2., 0.]]);
         let curve = curve.build(&mut services.objects);
@@ -184,10 +182,9 @@ mod tests {
 
         let face = {
             let mut face = PartialFace {
-                surface: surface.clone(),
+                surface: Partial::from(services.objects.surfaces.xy_plane()),
                 ..Default::default()
             };
-            face.exterior.write().surface = surface;
             face.exterior
                 .write()
                 .update_as_polygon_from_points(exterior);

crates/fj-kernel/src/algorithms/intersect/face_face.rs

@@ -94,10 +94,9 @@ mod tests {
         ]
         .map(|surface| {
             let mut face = PartialFace {
-                surface: Partial::from(surface.clone()),
+                surface: Partial::from(surface),
                 ..Default::default()
             };
-            face.exterior.write().surface = Partial::from(surface);
             face.exterior.write().update_as_polygon_from_points(points);
 
             face.build(&mut services.objects)
@@ -126,10 +125,9 @@ mod tests {
         ];
         let [a, b] = surfaces.clone().map(|surface| {
             let mut face = PartialFace {
-                surface: Partial::from(surface.clone()),
+                surface: Partial::from(surface),
                 ..Default::default()
             };
-            face.exterior.write().surface = Partial::from(surface);
             face.exterior.write().update_as_polygon_from_points(points);
 
             face.build(&mut services.objects)

crates/fj-kernel/src/algorithms/intersect/face_point.rs

@@ -148,13 +148,10 @@ mod tests {
     fn point_is_outside_face() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [0., 0.],
             [1., 1.],
@@ -173,13 +170,10 @@ mod tests {
     fn ray_hits_vertex_while_passing_outside() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [0., 0.],
             [2., 1.],
@@ -201,13 +195,10 @@ mod tests {
     fn ray_hits_vertex_at_cycle_seam() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [4., 2.],
             [0., 4.],
@@ -229,13 +220,10 @@ mod tests {
     fn ray_hits_vertex_while_staying_inside() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [0., 0.],
             [2., 1.],
@@ -258,13 +246,10 @@ mod tests {
     fn ray_hits_parallel_edge_and_leaves_face_at_vertex() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [0., 0.],
             [2., 1.],
@@ -287,13 +272,10 @@ mod tests {
     fn ray_hits_parallel_edge_and_does_not_leave_face_there() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [0., 0.],
             [2., 1.],
@@ -317,13 +299,10 @@ mod tests {
     fn point_is_coincident_with_edge() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [0., 0.],
             [2., 0.],
@@ -354,13 +333,10 @@ mod tests {
     fn point_is_coincident_with_vertex() {
         let mut services = Services::new();
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [0., 0.],
             [1., 0.],

crates/fj-kernel/src/algorithms/intersect/ray_face.rs

@@ -163,13 +163,10 @@ mod tests {
 
         let ray = HorizontalRayToTheRight::from([0., 0., 0.]);
 
-        let surface = Partial::from(services.objects.surfaces.yz_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.yz_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [-1., -1.],
             [1., -1.],
@@ -190,13 +187,10 @@ mod tests {
 
         let ray = HorizontalRayToTheRight::from([0., 0., 0.]);
 
-        let surface = Partial::from(services.objects.surfaces.yz_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.yz_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [-1., -1.],
             [1., -1.],
@@ -220,13 +214,10 @@ mod tests {
 
         let ray = HorizontalRayToTheRight::from([0., 0., 0.]);
 
-        let surface = Partial::from(services.objects.surfaces.yz_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.yz_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [-1., -1.],
             [1., -1.],
@@ -247,13 +238,10 @@ mod tests {
 
         let ray = HorizontalRayToTheRight::from([0., 0., 0.]);
 
-        let surface = Partial::from(services.objects.surfaces.yz_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.yz_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [-1., -1.],
             [1., -1.],
@@ -285,13 +273,10 @@ mod tests {
 
         let ray = HorizontalRayToTheRight::from([0., 0., 0.]);
 
-        let surface = Partial::from(services.objects.surfaces.yz_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.yz_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [-1., -1.],
             [1., -1.],
@@ -323,13 +308,10 @@ mod tests {
 
         let ray = HorizontalRayToTheRight::from([0., 0., 0.]);
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [-1., -1.],
             [1., -1.],
@@ -352,13 +334,10 @@ mod tests {
 
         let ray = HorizontalRayToTheRight::from([0., 0., 0.]);
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior.write().update_as_polygon_from_points([
             [-1., -1.],
             [1., -1.],

crates/fj-kernel/src/algorithms/reverse/cycle.rs

@@ -17,6 +17,6 @@ impl Reverse for Handle<Cycle> {
 
         edges.reverse();
 
-        Cycle::new(self.surface().clone(), edges).insert(objects)
+        Cycle::new(edges).insert(objects)
     }
 }

crates/fj-kernel/src/algorithms/sweep/edge.rs

@@ -38,8 +38,7 @@ impl Sweep for (Handle<HalfEdge>, &Surface, Color) {
                 (edge.curve().clone(), surface)
                     .sweep_with_cache(path, cache, objects),
             );
-            face.surface = surface.clone();
+            face.surface = surface;
-            face.exterior.write().surface = surface;
         }
 
         // Now we're ready to create the edges.
@@ -269,10 +268,7 @@ mod tests {
                 .clone()
             };
 
-            let mut cycle = PartialCycle {
+            let mut cycle = PartialCycle::default();
-                surface: Partial::from(surface.clone()),
-                ..Default::default()
-            };
             cycle.half_edges.extend(
                 [bottom, side_up, top, side_down].map(Partial::from_partial),
             );

crates/fj-kernel/src/algorithms/sweep/face.rs

@@ -83,8 +83,6 @@ impl Sweep for Handle<Face> {
                 top_edges.push(Partial::from(top_edge));
             }
 
-            top_cycle.write().surface = Partial::from(top_surface.clone());
-
             top_cycle
                 .write()
                 .connect_to_closed_edges(top_edges, &top_surface.geometry());
@@ -165,8 +163,6 @@ mod tests {
 
             let mut face = sketch.add_face();
             face.write().surface = Partial::from(surface.clone());
-            face.write().exterior.write().surface =
-                Partial::from(surface.clone());
             face.write()
                 .exterior
                 .write()
@@ -185,7 +181,6 @@ mod tests {
                 ..Default::default()
             };
 
-            bottom.exterior.write().surface = Partial::from(surface.clone());
             bottom
                 .exterior
                 .write()
@@ -200,11 +195,10 @@ mod tests {
             let surface = surface.clone().translate(UP, &mut services.objects);
 
             let mut top = PartialFace {
-                surface: Partial::from(surface.clone()),
+                surface: Partial::from(surface),
                 ..Default::default()
             };
 
-            top.exterior.write().surface = Partial::from(surface);
             top.exterior.write().update_as_polygon_from_points(TRIANGLE);
 
             top.build(&mut services.objects)
@@ -246,8 +240,6 @@ mod tests {
 
             let mut face = sketch.add_face();
             face.write().surface = Partial::from(surface.clone());
-            face.write().exterior.write().surface =
-                Partial::from(surface.clone());
             face.write()
                 .exterior
                 .write()
@@ -265,11 +257,10 @@ mod tests {
                 surface.clone().translate(DOWN, &mut services.objects);
 
             let mut bottom = PartialFace {
-                surface: Partial::from(surface.clone()),
+                surface: Partial::from(surface),
                 ..Default::default()
             };
 
-            bottom.exterior.write().surface = Partial::from(surface);
             bottom
                 .exterior
                 .write()
@@ -286,7 +277,6 @@ mod tests {
                 ..Default::default()
             };
 
-            top.exterior.write().surface = Partial::from(surface.clone());
             top.exterior.write().update_as_polygon_from_points(TRIANGLE);
 
             top.build(&mut services.objects)

crates/fj-kernel/src/algorithms/transform/cycle.rs

@@ -14,16 +14,12 @@ impl TransformObject for Cycle {
         objects: &mut Service<Objects>,
         cache: &mut TransformCache,
     ) -> Self {
-        let surface = self
-            .surface()
-            .clone()
-            .transform_with_cache(transform, objects, cache);
         let half_edges = self.half_edges().map(|half_edge| {
             half_edge
                 .clone()
                 .transform_with_cache(transform, objects, cache)
         });
 
-        Self::new(surface, half_edges)
+        Self::new(half_edges)
     }
 }

crates/fj-kernel/src/algorithms/triangulate/mod.rs

@@ -96,13 +96,10 @@ mod tests {
         let c = [2., 2.];
         let d = [0., 1.];
 
-        let surface = Partial::from(services.objects.surfaces.xy_plane());
-
         let mut face = PartialFace {
-            surface: surface.clone(),
+            surface: Partial::from(services.objects.surfaces.xy_plane()),
             ..Default::default()
         };
-        face.exterior.write().surface = surface;
         face.exterior
             .write()
             .update_as_polygon_from_points([a, b, c, d]);
@@ -144,7 +141,6 @@ mod tests {
             surface: Partial::from(surface.clone()),
             ..Default::default()
         };
-        face.exterior.write().surface = Partial::from(surface.clone());
         face.exterior
             .write()
             .update_as_polygon_from_points([a, b, c, d]);
@@ -211,7 +207,6 @@ mod tests {
             surface: Partial::from(surface.clone()),
             ..Default::default()
         };
-        face.exterior.write().surface = Partial::from(surface.clone());
         face.exterior
             .write()
             .update_as_polygon_from_points([a, b, c, d, e]);

crates/fj-kernel/src/builder/cycle.rs

@@ -74,6 +74,12 @@ pub trait CycleBuilder {
     ) -> O::SameSize<Partial<HalfEdge>>
     where
         O: ObjectArgument<Partial<HalfEdge>>;
+
+    /// Infer the positions of all vertices, if necessary
+    fn infer_vertex_positions_if_necessary(
+        &mut self,
+        surface: &SurfaceGeometry,
+    );
 }
 
 impl CycleBuilder for PartialCycle {
@@ -187,4 +193,15 @@ impl CycleBuilder for PartialCycle {
             this
         })
     }
+
+    fn infer_vertex_positions_if_necessary(
+        &mut self,
+        surface: &SurfaceGeometry,
+    ) {
+        for half_edge in &mut self.half_edges {
+            half_edge
+                .write()
+                .infer_vertex_positions_if_necessary(surface);
+        }
+    }
 }

crates/fj-kernel/src/builder/face.rs

@@ -5,7 +5,7 @@ use fj_interop::ext::ArrayExt;
 use crate::{
     geometry::path::SurfacePath,
     objects::{Cycle, Surface},
-    partial::{MaybeSurfacePath, Partial, PartialCycle, PartialFace},
+    partial::{MaybeSurfacePath, Partial, PartialFace},
 };
 
 use super::SurfaceBuilder;
@@ -33,16 +33,13 @@ pub trait FaceBuilder {
 
 impl FaceBuilder for PartialFace {
     fn add_interior(&mut self) -> Partial<Cycle> {
-        let cycle = Partial::from_partial(PartialCycle {
+        let cycle = Partial::new();
-            surface: self.exterior.read().surface.clone(),
-            ..Default::default()
-        });
         self.interiors.push(cycle.clone());
         cycle
     }
 
     fn update_surface_as_plane(&mut self) -> Partial<Surface> {
-        let mut exterior = self.exterior.write();
+        let exterior = self.exterior.read();
         let mut vertices = exterior
             .half_edges
             .iter()
@@ -69,7 +66,7 @@ impl FaceBuilder for PartialFace {
             let first_three_points_global =
                 first_three_vertices.each_ref_ext().map(|(_, point)| *point);
 
-            let (first_three_points_surface, surface) = exterior
+            let (first_three_points_surface, surface) = self
                 .surface
                 .write()
                 .update_as_plane_from_points(first_three_points_global);
@@ -92,7 +89,7 @@ impl FaceBuilder for PartialFace {
             surface_vertex.write().position = Some(point);
         }
 
-        exterior.surface.clone()
+        self.surface.clone()
     }
 
     fn infer_curves(&mut self) {

crates/fj-kernel/src/objects/full/cycle.rs

@@ -3,16 +3,11 @@ use std::slice;
 use fj_interop::ext::SliceExt;
 use fj_math::{Scalar, Winding};
 
-use crate::{
+use crate::{geometry::path::SurfacePath, objects::HalfEdge, storage::Handle};
-    geometry::path::SurfacePath,
-    objects::{HalfEdge, Surface},
-    storage::Handle,
-};
 
 /// A cycle of connected half-edges
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct Cycle {
-    surface: Handle<Surface>,
     half_edges: Vec<Handle<HalfEdge>>,
 }
 
@@ -22,10 +17,7 @@ impl Cycle {
     /// # Panics
     ///
     /// Panics, if `half_edges` does not yield at least one half-edge.
-    pub fn new(
+    pub fn new(half_edges: impl IntoIterator<Item = Handle<HalfEdge>>) -> Self {
-        surface: Handle<Surface>,
-        half_edges: impl IntoIterator<Item = Handle<HalfEdge>>,
-    ) -> Self {
         let half_edges = half_edges.into_iter().collect::<Vec<_>>();
 
         // This is not a validation check, and thus not part of the validation
@@ -37,15 +29,7 @@ impl Cycle {
             "Cycle must contain at least one half-edge"
         );
 
-        Self {
+        Self { half_edges }
-            surface,
-            half_edges,
-        }
-    }
-
-    /// Access the surface that the cycle is in
-    pub fn surface(&self) -> &Handle<Surface> {
-        &self.surface
     }
 
     /// Access the half-edges that make up the cycle

crates/fj-kernel/src/partial/objects/cycle.rs

@@ -1,6 +1,5 @@
 use crate::{
-    builder::HalfEdgeBuilder,
+    objects::{Cycle, HalfEdge, Objects},
-    objects::{Cycle, HalfEdge, Objects, Surface},
     partial::{FullToPartialCache, Partial, PartialObject},
     services::Service,
 };
@@ -8,9 +7,6 @@ use crate::{
 /// A partial [`Cycle`]
 #[derive(Clone, Debug, Default)]
 pub struct PartialCycle {
-    /// The surface that the cycle is defined in
-    pub surface: Partial<Surface>,
-
     /// The half-edges that make up the cycle
     pub half_edges: Vec<Partial<HalfEdge>>,
 }
@@ -20,7 +16,6 @@ impl PartialObject for PartialCycle {
 
     fn from_full(cycle: &Self::Full, cache: &mut FullToPartialCache) -> Self {
         Self {
-            surface: Partial::from_full(cycle.surface().clone(), cache),
             half_edges: cycle
                 .half_edges()
                 .cloned()
@@ -30,15 +25,11 @@ impl PartialObject for PartialCycle {
     }
 
     fn build(self, objects: &mut Service<Objects>) -> Self::Full {
-        let surface = self.surface.build(objects);
+        let half_edges = self
-        let surface_geometry = surface.geometry();
+            .half_edges
-        let half_edges = self.half_edges.into_iter().map(|mut half_edge| {
+            .into_iter()
-            half_edge
+            .map(|half_edge| half_edge.build(objects));
-                .write()
-                .infer_vertex_positions_if_necessary(&surface_geometry);
-            half_edge.build(objects)
-        });
 
-        Cycle::new(surface, half_edges)
+        Cycle::new(half_edges)
     }
 }

crates/fj-kernel/src/partial/objects/face.rs

@@ -1,6 +1,7 @@
 use fj_interop::mesh::Color;
 
 use crate::{
+    builder::CycleBuilder,
     objects::{Cycle, Face, Objects, Surface},
     partial::{FullToPartialCache, Partial, PartialObject},
     services::Service,
@@ -39,8 +40,18 @@ impl PartialObject for PartialFace {
         }
     }
 
-    fn build(self, objects: &mut Service<Objects>) -> Self::Full {
+    fn build(mut self, objects: &mut Service<Objects>) -> Self::Full {
         let surface = self.surface.build(objects);
+
+        self.exterior
+            .write()
+            .infer_vertex_positions_if_necessary(&surface.geometry());
+        for interior in &mut self.interiors {
+            interior
+                .write()
+                .infer_vertex_positions_if_necessary(&surface.geometry());
+        }
+
         let exterior = self.exterior.build(objects);
         let interiors =
             self.interiors.into_iter().map(|cycle| cycle.build(objects));

crates/fj-kernel/src/validate/cycle.rs

@@ -17,11 +17,6 @@ impl Validate for Cycle {
     ) {
         CycleValidationError::check_half_edge_connections(self, errors);
         CycleValidationError::check_half_edge_boundaries(self, config, errors);
-        CycleValidationError::check_vertex_positions(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
-        // surface vertices, so that would be redundant.
     }
 }
 
@@ -71,33 +66,6 @@ pub enum CycleValidationError {
         /// The half-edge
         half_edge: Handle<HalfEdge>,
     },
-
-    /// Mismatch between [`SurfaceVertex`] and `GlobalVertex` positions
-    #[error(
-        "`SurfaceVertex` position doesn't match position of its global form\n\
-        - Surface position: {surface_position:?}\n\
-        - Surface position converted to global position: \
-            {surface_position_as_global:?}\n\
-        - Global position: {global_position:?}\n\
-        - Distance between the positions: {distance}\n\
-        - `SurfaceVertex`: {surface_vertex:#?}"
-    )]
-    VertexSurfacePositionMismatch {
-        /// The position of the surface vertex
-        surface_position: Point<2>,
-
-        /// The surface position converted into a global position
-        surface_position_as_global: Point<3>,
-
-        /// The position of the global vertex
-        global_position: Point<3>,
-
-        /// The distance between the positions
-        distance: Scalar,
-
-        /// The surface vertex
-        surface_vertex: Handle<SurfaceVertex>,
-    },
 }
 
 impl CycleValidationError {
@@ -158,49 +126,15 @@ impl CycleValidationError {
             }
         }
     }
-
-    fn check_vertex_positions(
-        cycle: &Cycle,
-        config: &ValidationConfig,
-        errors: &mut Vec<ValidationError>,
-    ) {
-        for half_edge in cycle.half_edges() {
-            for surface_vertex in half_edge.surface_vertices() {
-                let surface_position_as_global = cycle
-                    .surface()
-                    .geometry()
-                    .point_from_surface_coords(surface_vertex.position());
-                let global_position = surface_vertex.global_form().position();
-
-                let distance =
-                    surface_position_as_global.distance_to(&global_position);
-
-                if distance > config.identical_max_distance {
-                    errors.push(
-                        Self::VertexSurfacePositionMismatch {
-                            surface_position: surface_vertex.position(),
-                            surface_position_as_global,
-                            global_position,
-                            distance,
-                            surface_vertex: surface_vertex.clone(),
-                        }
-                        .into(),
-                    );
-                }
-            }
-        }
-    }
 }
 
 #[cfg(test)]
 mod tests {
-    use fj_interop::ext::ArrayExt;
+    use fj_math::Point;
-    use fj_math::{Point, Scalar, Vector};
 
     use crate::{
-        builder::{CycleBuilder, HalfEdgeBuilder},
+        builder::CycleBuilder,
-        insert::Insert,
+        objects::Cycle,
-        objects::{Cycle, HalfEdge, SurfaceVertex},
         partial::{Partial, PartialCycle, PartialObject},
         services::Services,
         validate::Validate,
@@ -211,11 +145,11 @@ mod tests {
         let mut services = Services::new();
 
         let valid = {
-            let mut cycle = PartialCycle {
+            let surface = services.objects.surfaces.xy_plane();
-                surface: Partial::from(services.objects.surfaces.xy_plane()),
+
-                ..Default::default()
+            let mut cycle = PartialCycle::default();
-            };
             cycle.update_as_polygon_from_points([[0., 0.], [1., 0.], [0., 1.]]);
+            cycle.infer_vertex_positions_if_necessary(&surface.geometry());
             cycle.build(&mut services.objects)
         };
         let invalid = {
@@ -238,7 +172,7 @@ mod tests {
                 .into_iter()
                 .map(|half_edge| half_edge.build(&mut services.objects));
 
-            Cycle::new(valid.surface().clone(), half_edges)
+            Cycle::new(half_edges)
         };
 
         valid.validate_and_return_first_error()?;
@@ -252,11 +186,11 @@ mod tests {
         let mut services = Services::new();
 
         let valid = {
-            let mut cycle = PartialCycle {
+            let surface = services.objects.surfaces.xy_plane();
-                surface: Partial::from(services.objects.surfaces.xy_plane()),
+
-                ..Default::default()
+            let mut cycle = PartialCycle::default();
-            };
             cycle.update_as_polygon_from_points([[0., 0.], [1., 0.], [0., 1.]]);
+            cycle.infer_vertex_positions_if_necessary(&surface.geometry());
             cycle.build(&mut services.objects)
         };
         let invalid = {
@@ -274,69 +208,7 @@ mod tests {
                 .into_iter()
                 .map(|half_edge| half_edge.build(&mut services.objects));
 
-            Cycle::new(valid.surface().clone(), half_edges)
+            Cycle::new(half_edges)
-        };
-
-        valid.validate_and_return_first_error()?;
-        assert!(invalid.validate_and_return_first_error().is_err());
-
-        Ok(())
-    }
-
-    #[test]
-    fn surface_vertex_position_mismatch() -> anyhow::Result<()> {
-        let mut services = Services::new();
-
-        let valid = {
-            let surface = services.objects.surfaces.xy_plane();
-
-            let mut cycle = PartialCycle {
-                surface: Partial::from(surface.clone()),
-                ..Default::default()
-            };
-
-            let mut half_edge = cycle.add_half_edge();
-            half_edge.write().update_as_circle_from_radius(1.);
-            half_edge
-                .write()
-                .infer_vertex_positions_if_necessary(&surface.geometry());
-
-            cycle.build(&mut services.objects)
-        };
-        let invalid = {
-            let half_edge = {
-                let half_edge = valid.half_edges().next().unwrap();
-
-                let boundary = half_edge
-                    .boundary()
-                    .map(|point| point + Vector::from([Scalar::PI / 2.]));
-
-                let mut surface_vertices =
-                    half_edge.surface_vertices().map(Clone::clone);
-
-                let mut invalid = None;
-                for surface_vertex in surface_vertices.each_mut_ext() {
-                    let invalid = invalid.get_or_insert_with(|| {
-                        SurfaceVertex::new(
-                            [0., 1.],
-                            surface_vertex.global_form().clone(),
-                        )
-                        .insert(&mut services.objects)
-                    });
-                    *surface_vertex = invalid.clone();
-                }
-
-                let boundary = boundary.zip_ext(surface_vertices);
-
-                HalfEdge::new(
-                    half_edge.curve().clone(),
-                    boundary,
-                    half_edge.global_form().clone(),
-                )
-                .insert(&mut services.objects)
-            };
-
-            Cycle::new(valid.surface().clone(), [half_edge])
         };
 
         valid.validate_and_return_first_error()?;

crates/fj-kernel/src/validate/face.rs

@@ -1,7 +1,7 @@
-use fj_math::Winding;
+use fj_math::{Point, Scalar, Winding};
 
 use crate::{
-    objects::{Cycle, Face, Surface},
+    objects::{Cycle, Face, Surface, SurfaceVertex},
     storage::Handle,
 };
 
@@ -10,11 +10,11 @@ use super::{Validate, ValidationConfig, ValidationError};
 impl Validate for Face {
     fn validate_with_config(
         &self,
-        _: &ValidationConfig,
+        config: &ValidationConfig,
         errors: &mut Vec<ValidationError>,
     ) {
-        FaceValidationError::check_surface_identity(self, errors);
         FaceValidationError::check_interior_winding(self, errors);
+        FaceValidationError::check_vertex_positions(self, config, errors);
     }
 }
 
@@ -56,26 +56,36 @@ pub enum FaceValidationError {
         /// The face
         face: Face,
     },
+
+    /// Mismatch between [`SurfaceVertex`] and `GlobalVertex` positions
+    #[error(
+        "`SurfaceVertex` position doesn't match position of its global form\n\
+        - Surface position: {surface_position:?}\n\
+        - Surface position converted to global position: \
+            {surface_position_as_global:?}\n\
+        - Global position: {global_position:?}\n\
+        - Distance between the positions: {distance}\n\
+        - `SurfaceVertex`: {surface_vertex:#?}"
+    )]
+    VertexPositionMismatch {
+        /// The position of the surface vertex
+        surface_position: Point<2>,
+
+        /// The surface position converted into a global position
+        surface_position_as_global: Point<3>,
+
+        /// The position of the global vertex
+        global_position: Point<3>,
+
+        /// The distance between the positions
+        distance: Scalar,
+
+        /// The surface vertex
+        surface_vertex: Handle<SurfaceVertex>,
+    },
 }
 
 impl FaceValidationError {
-    fn check_surface_identity(face: &Face, errors: &mut Vec<ValidationError>) {
-        let surface = face.surface();
-
-        for interior in face.interiors() {
-            if surface.id() != interior.surface().id() {
-                errors.push(
-                    Box::new(Self::SurfaceMismatch {
-                        surface: surface.clone(),
-                        interior: interior.clone(),
-                        face: face.clone(),
-                    })
-                    .into(),
-                );
-            }
-        }
-    }
-
     fn check_interior_winding(face: &Face, errors: &mut Vec<ValidationError>) {
         let exterior_winding = face.exterior().winding();
 
@@ -94,15 +104,53 @@ impl FaceValidationError {
             }
         }
     }
+
+    fn check_vertex_positions(
+        face: &Face,
+        config: &ValidationConfig,
+        errors: &mut Vec<ValidationError>,
+    ) {
+        for cycle in face.all_cycles() {
+            for half_edge in cycle.half_edges() {
+                for surface_vertex in half_edge.surface_vertices() {
+                    let surface_position_as_global = face
+                        .surface()
+                        .geometry()
+                        .point_from_surface_coords(surface_vertex.position());
+                    let global_position =
+                        surface_vertex.global_form().position();
+
+                    let distance = surface_position_as_global
+                        .distance_to(&global_position);
+
+                    if distance > config.identical_max_distance {
+                        errors.push(
+                            Box::new(Self::VertexPositionMismatch {
+                                surface_position: surface_vertex.position(),
+                                surface_position_as_global,
+                                global_position,
+                                distance,
+                                surface_vertex: surface_vertex.clone(),
+                            })
+                            .into(),
+                        );
+                    }
+                }
+            }
+        }
+    }
 }
 
 #[cfg(test)]
 mod tests {
+    use fj_interop::ext::ArrayExt;
+    use fj_math::{Scalar, Vector};
+
     use crate::{
         algorithms::reverse::Reverse,
-        builder::{CycleBuilder, FaceBuilder},
+        builder::{CycleBuilder, FaceBuilder, HalfEdgeBuilder},
         insert::Insert,
-        objects::Face,
+        objects::{Cycle, Face, HalfEdge, SurfaceVertex},
         partial::{Partial, PartialCycle, PartialFace, PartialObject},
         services::Services,
         validate::Validate,
@@ -112,14 +160,11 @@ mod tests {
     fn face_surface_mismatch() -> anyhow::Result<()> {
         let mut services = Services::new();
 
-        let surface = services.objects.surfaces.xy_plane();
-
         let valid = {
             let mut face = PartialFace {
-                surface: Partial::from(surface.clone()),
+                surface: Partial::from(services.objects.surfaces.xy_plane()),
                 ..Default::default()
             };
-            face.exterior.write().surface = Partial::from(surface);
             face.exterior.write().update_as_polygon_from_points([
                 [0., 0.],
                 [3., 0.],
@@ -134,11 +179,11 @@ mod tests {
             face.build(&mut services.objects)
         };
         let invalid = {
-            let mut cycle = PartialCycle {
+            let surface = services.objects.surfaces.xz_plane();
-                surface: Partial::from(services.objects.surfaces.xz_plane()),
+
-                ..Default::default()
+            let mut cycle = PartialCycle::default();
-            };
             cycle.update_as_polygon_from_points([[1., 1.], [1., 2.], [2., 1.]]);
+            cycle.infer_vertex_positions_if_necessary(&surface.geometry());
             let cycle = cycle
                 .build(&mut services.objects)
                 .insert(&mut services.objects);
@@ -162,14 +207,11 @@ mod tests {
     fn face_invalid_interior_winding() -> anyhow::Result<()> {
         let mut services = Services::new();
 
-        let surface = services.objects.surfaces.xy_plane();
-
         let valid = {
             let mut face = PartialFace {
-                surface: Partial::from(surface.clone()),
+                surface: Partial::from(services.objects.surfaces.xy_plane()),
                 ..Default::default()
             };
-            face.exterior.write().surface = Partial::from(surface);
             face.exterior.write().update_as_polygon_from_points([
                 [0., 0.],
                 [3., 0.],
@@ -202,4 +244,74 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn surface_vertex_position_mismatch() -> anyhow::Result<()> {
+        let mut services = Services::new();
+
+        let valid = {
+            let surface = services.objects.surfaces.xy_plane();
+
+            let mut face = PartialFace {
+                surface: Partial::from(surface.clone()),
+                ..Default::default()
+            };
+
+            let mut half_edge = face.exterior.write().add_half_edge();
+            half_edge.write().update_as_circle_from_radius(1.);
+            half_edge
+                .write()
+                .infer_vertex_positions_if_necessary(&surface.geometry());
+
+            face.build(&mut services.objects)
+        };
+        let invalid = {
+            let half_edge = {
+                let half_edge = valid.exterior().half_edges().next().unwrap();
+
+                let boundary = half_edge
+                    .boundary()
+                    .map(|point| point + Vector::from([Scalar::PI / 2.]));
+
+                let mut surface_vertices =
+                    half_edge.surface_vertices().map(Clone::clone);
+
+                let mut invalid = None;
+                for surface_vertex in surface_vertices.each_mut_ext() {
+                    let invalid = invalid.get_or_insert_with(|| {
+                        SurfaceVertex::new(
+                            [0., 1.],
+                            surface_vertex.global_form().clone(),
+                        )
+                        .insert(&mut services.objects)
+                    });
+                    *surface_vertex = invalid.clone();
+                }
+
+                let boundary = boundary.zip_ext(surface_vertices);
+
+                HalfEdge::new(
+                    half_edge.curve().clone(),
+                    boundary,
+                    half_edge.global_form().clone(),
+                )
+                .insert(&mut services.objects)
+            };
+
+            let exterior =
+                Cycle::new([half_edge]).insert(&mut services.objects);
+
+            Face::new(
+                valid.surface().clone(),
+                exterior,
+                valid.interiors().cloned(),
+                valid.color(),
+            )
+        };
+
+        valid.validate_and_return_first_error()?;
+        assert!(invalid.validate_and_return_first_error().is_err());
+
+        Ok(())
+    }
 }

crates/fj-operations/src/sketch.rs

@@ -36,10 +36,7 @@ impl Shape for fj::Sketch {
                     Partial::from_partial(half_edge)
                 };
                 let exterior = {
-                    let mut cycle = PartialCycle {
+                    let mut cycle = PartialCycle::default();
-                        surface: surface.clone(),
-                        ..Default::default()
-                    };
                     cycle.half_edges.push(half_edge);
                     Partial::from_partial(cycle)
                 };
@@ -59,10 +56,7 @@ impl Shape for fj::Sketch {
                 );
 
                 let exterior = {
-                    let mut cycle = PartialCycle {
+                    let mut cycle = PartialCycle::default();
-                        surface: Partial::from(surface.clone()),
-                        ..Default::default()
-                    };
                     let mut line_segments = vec![];
                     let mut arcs = vec![];
                     poly_chain.to_segments().into_iter().for_each(