Merge pull request #1597 from hannobraun/surface

Remove reference to `Surface` from `SurfaceVertex`

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

@@ -196,8 +196,10 @@ mod tests {
                 half_edge
             };
             let side_up = {
-                let mut side_up = PartialHalfEdge::default();
+                let mut side_up = PartialHalfEdge {
-                side_up.replace_surface(surface.clone());
+                    surface: surface.clone(),
+                    ..Default::default()
+                };
 
                 {
                     let [back, front] = side_up
@@ -217,8 +219,10 @@ mod tests {
                 side_up
             };
             let top = {
-                let mut top = PartialHalfEdge::default();
+                let mut top = PartialHalfEdge {
-                top.replace_surface(surface.clone());
+                    surface: surface.clone(),
+                    ..Default::default()
+                };
 
                 {
                     let [(back, back_surface), (front, front_surface)] =
@@ -243,8 +247,10 @@ mod tests {
                 .clone()
             };
             let side_down = {
-                let mut side_down = PartialHalfEdge::default();
+                let mut side_down = PartialHalfEdge {
-                side_down.replace_surface(surface);
+                    surface,
+                    ..Default::default()
+                };
 
                 let [(back, back_surface), (front, front_surface)] =
                     side_down.vertices.each_mut_ext();

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

@@ -18,16 +18,12 @@ impl TransformObject for SurfaceVertex {
         // coordinates and thus transforming the surface takes care of it.
         let position = self.position();
 
-        let surface = self
-            .surface()
-            .clone()
-            .transform_with_cache(transform, objects, cache);
         let global_form = self
             .global_form()
             .clone()
             .transform_with_cache(transform, objects, cache);
 
-        Self::new(position, surface, global_form)
+        Self::new(position, global_form)
     }
 }
 

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

@@ -149,7 +149,7 @@ impl CycleBuilder for PartialCycle {
 
             let [_, vertex] = &mut new_half_edge.vertices;
             vertex.1 = shared_surface_vertex;
-            new_half_edge.replace_surface(self.surface.clone());
+            new_half_edge.surface = self.surface.clone();
             new_half_edge.infer_global_form();
         }
 

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

@@ -11,16 +11,6 @@ use super::CurveBuilder;
 
 /// Builder API for [`PartialHalfEdge`]
 pub trait HalfEdgeBuilder {
-    /// Completely replace the surface in this half-edge's object graph
-    ///
-    /// Please note that this operation will write to both vertices that the
-    /// half-edge references. If any of them were created from full objects,
-    /// this will break the connection to those, meaning that building the
-    /// partial objects won't result in those full objects again. This will be
-    /// the case, even if those full objects already referenced the provided
-    /// surface.
-    fn replace_surface(&mut self, surface: impl Into<Partial<Surface>>);
-
     /// Update partial half-edge to be a circle, from the given radius
     fn update_as_circle_from_radius(&mut self, radius: impl Into<Scalar>);
 
@@ -61,16 +51,6 @@ pub trait HalfEdgeBuilder {
 }
 
 impl HalfEdgeBuilder for PartialHalfEdge {
-    fn replace_surface(&mut self, surface: impl Into<Partial<Surface>>) {
-        let surface = surface.into();
-
-        self.surface = surface.clone();
-
-        for vertex in &mut self.vertices {
-            vertex.1.write().surface = surface.clone();
-        }
-    }
-
     fn update_as_circle_from_radius(&mut self, radius: impl Into<Scalar>) {
         let path = self.curve.write().update_as_circle_from_radius(radius);
 
@@ -134,7 +114,7 @@ impl HalfEdgeBuilder for PartialHalfEdge {
         surface: impl Into<Partial<Surface>>,
         points: [impl Into<Point<2>>; 2],
     ) {
-        self.replace_surface(surface.into());
+        self.surface = surface.into();
 
         for (vertex, point) in self.vertices.each_mut_ext().zip_ext(points) {
             let mut surface_form = vertex.1.write();

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

@@ -1,33 +1,11 @@
-use fj_math::Point;
-
 use crate::partial::{PartialGlobalVertex, PartialSurfaceVertex};
 
 /// Builder API for [`PartialSurfaceVertex`]
-pub trait SurfaceVertexBuilder {
+pub trait SurfaceVertexBuilder {}
-    /// Infer the position of the surface vertex' global form
-    ///
-    /// Updates the global vertex referenced by this surface vertex with the
-    /// inferred position, and also returns the position.
-    fn infer_global_position(&mut self) -> Point<3>;
-}
 
 impl SurfaceVertexBuilder for PartialSurfaceVertex {
-    fn infer_global_position(&mut self) -> Point<3> {
+    // No methods are currently defined. This trait serves as a placeholder, to
-        let position_surface = self
+    // make it clear where to add such methods, once necessary.
-            .position
-            .expect("Can't infer global position without surface position");
-        let surface = self
-            .surface
-            .read()
-            .geometry
-            .expect("Can't infer global position without surface geometry");
-
-        let position_global =
-            surface.point_from_surface_coords(position_surface);
-        self.global_form.write().position = Some(position_global);
-
-        position_global
-    }
 }
 
 /// Builder API for [`PartialGlobalVertex`]

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

@@ -1,12 +1,11 @@
 use fj_math::Point;
 
-use crate::{objects::Surface, storage::Handle};
+use crate::storage::Handle;
 
 /// A vertex, defined in surface (2D) coordinates
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct SurfaceVertex {
     position: Point<2>,
-    surface: Handle<Surface>,
     global_form: Handle<GlobalVertex>,
 }
 
@@ -14,13 +13,11 @@ impl SurfaceVertex {
     /// Construct a new instance of `SurfaceVertex`
     pub fn new(
         position: impl Into<Point<2>>,
-        surface: Handle<Surface>,
         global_form: Handle<GlobalVertex>,
     ) -> Self {
         let position = position.into();
         Self {
             position,
-            surface,
             global_form,
         }
     }
@@ -30,11 +27,6 @@ impl SurfaceVertex {
         self.position
     }
 
-    /// Access the surface that the vertex is defined in
-    pub fn surface(&self) -> &Handle<Surface> {
-        &self.surface
-    }
-
     /// Access the global form of the vertex
     pub fn global_form(&self) -> &Handle<GlobalVertex> {
         &self.global_form

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

@@ -8,9 +8,7 @@ use crate::{
         Curve, GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Objects,
         Surface, SurfaceVertex,
     },
-    partial::{
+    partial::{FullToPartialCache, Partial, PartialObject},
-        FullToPartialCache, Partial, PartialObject, PartialSurfaceVertex,
-    },
     services::Service,
 };
 
@@ -106,11 +104,7 @@ impl Default for PartialHalfEdge {
 
         let curve = Partial::<Curve>::new();
         let vertices = array::from_fn(|_| {
-            let surface_form = Partial::from_partial(PartialSurfaceVertex {
+            let surface_form = Partial::default();
-                surface: surface.clone(),
-                ..Default::default()
-            });
-
             (None, surface_form)
         });
 

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

@@ -1,7 +1,7 @@
 use fj_math::Point;
 
 use crate::{
-    objects::{GlobalVertex, Objects, Surface, SurfaceVertex},
+    objects::{GlobalVertex, Objects, SurfaceVertex},
     partial::{FullToPartialCache, Partial, PartialObject},
     services::Service,
 };
@@ -12,9 +12,6 @@ pub struct PartialSurfaceVertex {
     /// The position of the vertex on the surface
     pub position: Option<Point<2>>,
 
-    /// The surface that the vertex is defined in
-    pub surface: Partial<Surface>,
-
     /// The global form of the vertex
     pub global_form: Partial<GlobalVertex>,
 }
@@ -28,10 +25,6 @@ impl PartialObject for PartialSurfaceVertex {
     ) -> Self {
         Self {
             position: Some(surface_vertex.position()),
-            surface: Partial::from_full(
-                surface_vertex.surface().clone(),
-                cache,
-            ),
             global_form: Partial::from_full(
                 surface_vertex.global_form().clone(),
                 cache,
@@ -43,10 +36,9 @@ impl PartialObject for PartialSurfaceVertex {
         let position = self
             .position
             .expect("Can't build `SurfaceVertex` without position");
-        let surface = self.surface.build(objects);
         let global_form = self.global_form.build(objects);
 
-        SurfaceVertex::new(position, surface, global_form)
+        SurfaceVertex::new(position, global_form)
     }
 }
 

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

@@ -1,7 +1,9 @@
 use fj_math::{Point, Scalar};
 
 use crate::{
-    objects::{GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Surface},
+    objects::{
+        GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Surface, SurfaceVertex,
+    },
     storage::Handle,
 };
 
@@ -15,12 +17,8 @@ impl Validate for HalfEdge {
     ) {
         HalfEdgeValidationError::check_global_curve_identity(self, errors);
         HalfEdgeValidationError::check_global_vertex_identity(self, errors);
-        HalfEdgeValidationError::check_surface_identity(self, errors);
+        HalfEdgeValidationError::check_vertex_coincidence(self, config, errors);
         HalfEdgeValidationError::check_vertex_positions(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,
-        // and the validation of those vertices checks the surfaces.
     }
 }
 
@@ -109,6 +107,33 @@ pub enum HalfEdgeValidationError {
         /// The half-edge
         half_edge: 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 HalfEdgeValidationError {
@@ -161,25 +186,7 @@ impl HalfEdgeValidationError {
         }
     }
 
-    fn check_surface_identity(
+    fn check_vertex_coincidence(
-        half_edge: &HalfEdge,
-        errors: &mut Vec<ValidationError>,
-    ) {
-        let surface = half_edge.surface();
-        let surface_form_surface = half_edge.start_vertex().surface();
-
-        if surface.id() != surface_form_surface.id() {
-            errors.push(
-                Box::new(Self::SurfaceMismatch {
-                    curve_surface: surface.clone(),
-                    surface_form_surface: surface_form_surface.clone(),
-                })
-                .into(),
-            );
-        }
-    }
-
-    fn check_vertex_positions(
         half_edge: &HalfEdge,
         config: &ValidationConfig,
         errors: &mut Vec<ValidationError>,
@@ -199,6 +206,36 @@ impl HalfEdgeValidationError {
             );
         }
     }
+
+    fn check_vertex_positions(
+        half_edge: &HalfEdge,
+        config: &ValidationConfig,
+        errors: &mut Vec<ValidationError>,
+    ) {
+        for surface_vertex in half_edge.surface_vertices() {
+            let surface_position_as_global = half_edge
+                .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::VertexSurfacePositionMismatch {
+                        surface_position: surface_vertex.position(),
+                        surface_position_as_global,
+                        global_position,
+                        distance,
+                        surface_vertex: surface_vertex.clone(),
+                    })
+                    .into(),
+                );
+            }
+        }
+    }
 }
 
 #[cfg(test)]
@@ -209,7 +246,7 @@ mod tests {
     use crate::{
         builder::HalfEdgeBuilder,
         insert::Insert,
-        objects::{GlobalCurve, HalfEdge},
+        objects::{GlobalCurve, HalfEdge, SurfaceVertex},
         partial::{Partial, PartialHalfEdge, PartialObject},
         services::Services,
         validate::Validate,
@@ -301,46 +338,6 @@ mod tests {
         Ok(())
     }
 
-    #[test]
-    fn vertex_surface_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
-                .boundary()
-                .zip_ext(valid.surface_vertices())
-                .map(|(point, surface_vertex)| {
-                    let mut surface_vertex =
-                        Partial::from(surface_vertex.clone());
-                    surface_vertex.write().surface =
-                        Partial::from(services.objects.surfaces.xz_plane());
-
-                    (point, surface_vertex.build(&mut services.objects))
-                });
-
-            HalfEdge::new(
-                valid.surface().clone(),
-                valid.curve().clone(),
-                vertices,
-                valid.global_form().clone(),
-            )
-        };
-
-        valid.validate_and_return_first_error()?;
-        assert!(invalid.validate_and_return_first_error().is_err());
-
-        Ok(())
-    }
-
     #[test]
     fn half_edge_vertices_are_coincident() -> anyhow::Result<()> {
         let mut services = Services::new();
@@ -372,4 +369,44 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn surface_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 mut surface_vertices =
+                valid.surface_vertices().map(Clone::clone);
+
+            let [_, surface_vertex] = surface_vertices.each_mut_ext();
+            *surface_vertex = SurfaceVertex::new(
+                [2., 0.],
+                surface_vertex.global_form().clone(),
+            )
+            .insert(&mut services.objects);
+
+            let boundary = valid.boundary().zip_ext(surface_vertices);
+
+            HalfEdge::new(
+                valid.surface().clone(),
+                valid.curve().clone(),
+                boundary,
+                valid.global_form().clone(),
+            )
+        };
+
+        valid.validate_and_return_first_error()?;
+        assert!(invalid.validate_and_return_first_error().is_err());
+
+        Ok(())
+    }
 }

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

@@ -12,7 +12,7 @@ mod vertex;
 
 pub use self::{
     cycle::CycleValidationError, edge::HalfEdgeValidationError,
-    face::FaceValidationError, vertex::SurfaceVertexValidationError,
+    face::FaceValidationError,
 };
 
 use std::convert::Infallible;
@@ -94,10 +94,6 @@ pub enum ValidationError {
     /// `HalfEdge` validation error
     #[error("`HalfEdge` validation error:\n{0}")]
     HalfEdge(#[from] Box<HalfEdgeValidationError>),
-
-    /// `SurfaceVertex` validation error
-    #[error("`SurfaceVertex` validation error:\n{0}")]
-    SurfaceVertex(#[from] Box<SurfaceVertexValidationError>),
 }
 
 impl From<Infallible> for ValidationError {

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

@@ -1,5 +1,3 @@
-use fj_math::{Point, Scalar};
-
 use crate::objects::{GlobalVertex, SurfaceVertex};
 
 use super::{Validate, ValidationConfig, ValidationError};
@@ -7,10 +5,9 @@ use super::{Validate, ValidationConfig, ValidationError};
 impl Validate for SurfaceVertex {
     fn validate_with_config(
         &self,
-        config: &ValidationConfig,
+        _: &ValidationConfig,
-        errors: &mut Vec<ValidationError>,
+        _: &mut Vec<ValidationError>,
     ) {
-        SurfaceVertexValidationError::check_position(self, config, errors);
     }
 }
 
@@ -22,102 +19,3 @@ impl Validate for GlobalVertex {
     ) {
     }
 }
-
-/// [`SurfaceVertex`] validation error
-#[derive(Clone, Debug, thiserror::Error)]
-pub enum SurfaceVertexValidationError {
-    /// Mismatch between position and position of global form
-    #[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:#?}"
-    )]
-    PositionMismatch {
-        /// 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: SurfaceVertex,
-    },
-}
-
-impl SurfaceVertexValidationError {
-    fn check_position(
-        surface_vertex: &SurfaceVertex,
-        config: &ValidationConfig,
-        errors: &mut Vec<ValidationError>,
-    ) {
-        let surface_position_as_global = surface_vertex
-            .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::PositionMismatch {
-                    surface_position: surface_vertex.position(),
-                    surface_position_as_global,
-                    global_position,
-                    distance,
-                    surface_vertex: surface_vertex.clone(),
-                })
-                .into(),
-            );
-        }
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use fj_math::Point;
-
-    use crate::{
-        insert::Insert,
-        objects::{GlobalVertex, SurfaceVertex},
-        partial::{
-            Partial, PartialGlobalVertex, PartialObject, PartialSurfaceVertex,
-        },
-        services::Services,
-        validate::Validate,
-    };
-
-    #[test]
-    fn surface_vertex_position_mismatch() -> anyhow::Result<()> {
-        let mut services = Services::new();
-
-        let valid = PartialSurfaceVertex {
-            position: Some([0., 0.].into()),
-            surface: Partial::from(services.objects.surfaces.xy_plane()),
-            global_form: Partial::from_partial(PartialGlobalVertex {
-                position: Some(Point::from([0., 0., 0.])),
-            }),
-        }
-        .build(&mut services.objects);
-        let invalid = SurfaceVertex::new(
-            valid.position(),
-            valid.surface().clone(),
-            GlobalVertex::new([1., 0., 0.]).insert(&mut services.objects),
-        );
-
-        valid.validate_and_return_first_error()?;
-        assert!(invalid.validate_and_return_first_error().is_err());
-
-        Ok(())
-    }
-}

crates/fj-operations/src/sketch.rs

@@ -30,8 +30,10 @@ impl Shape for fj::Sketch {
                 let half_edge = {
                     let surface = Partial::from(surface);
 
-                    let mut half_edge = PartialHalfEdge::default();
+                    let mut half_edge = PartialHalfEdge {
-                    half_edge.replace_surface(surface);
+                        surface,
+                        ..Default::default()
+                    };
                     half_edge.update_as_circle_from_radius(circle.radius());
 
                     Partial::from_partial(half_edge)