Merge pull request #1593 from hannobraun/sweep

Rewrite parts of sweep code

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

@@ -1,14 +1,10 @@
 use fj_interop::{ext::ArrayExt, mesh::Color};
-use fj_math::{Line, Scalar, Vector};
+use fj_math::{Point, Scalar, Vector};
-use iter_fixed::IntoIteratorFixed;
 
 use crate::{
-    algorithms::{reverse::Reverse, transform::TransformObject},
+    builder::{CycleBuilder, HalfEdgeBuilder},
-    geometry::path::SurfacePath,
     insert::Insert,
-    objects::{
+    objects::{Face, HalfEdge, Objects},
-        Curve, Cycle, Face, GlobalEdge, HalfEdge, Objects, SurfaceVertex,
-    },
     partial::{Partial, PartialFace, PartialObject},
     services::Service,
     storage::Handle,
@@ -17,7 +13,7 @@ use crate::{
 use super::{Sweep, SweepCache};
 
 impl Sweep for (Handle<HalfEdge>, Color) {
-    type Swept = Handle<Face>;
+    type Swept = (Handle<Face>, Handle<HalfEdge>);
 
     fn sweep_with_cache(
         self,
@@ -28,156 +24,135 @@ impl Sweep for (Handle<HalfEdge>, Color) {
         let (edge, color) = self;
         let path = path.into();
 
-        let surface =
+        // The result of sweeping an edge is a face. Let's create that.
-            edge.curve().clone().sweep_with_cache(path, cache, objects);
+        let mut face = PartialFace {
-
-        // We can't use the edge we're sweeping from as the bottom edge, as that
-        // is not defined in the right surface. Let's create a new bottom edge,
-        // by swapping the surface of the original.
-        let bottom_edge = {
-            let points_curve_and_surface = edge
-                .boundary()
-                .map(|point| (point, [point.t, Scalar::ZERO]));
-
-            let curve = {
-                // Please note that creating a line here is correct, even if the
-                // global curve is a circle. Projected into the side surface, it
-                // is going to be a line either way.
-                let path =
-                    SurfacePath::Line(Line::from_points_with_line_coords(
-                        points_curve_and_surface,
-                    ));
-
-                Curve::new(
-                    surface.clone(),
-                    path,
-                    edge.curve().global_form().clone(),
-                )
-                .insert(objects)
-            };
-
-            let boundary = {
-                let points_surface = points_curve_and_surface
-                    .map(|(_, point_surface)| point_surface);
-
-                edge.boundary()
-                    .zip_ext(edge.surface_vertices())
-                    .into_iter_fixed()
-                    .zip(points_surface)
-                    .collect::<[_; 2]>()
-                    .map(|((point, surface_vertex), point_surface)| {
-                        let surface_vertex = SurfaceVertex::new(
-                            point_surface,
-                            surface.clone(),
-                            surface_vertex.global_form().clone(),
-                        )
-                        .insert(objects);
-
-                        (point, surface_vertex)
-                    })
-            };
-
-            HalfEdge::new(curve, boundary, edge.global_form().clone())
-                .insert(objects)
-        };
-
-        let side_edges = bottom_edge
-            .boundary()
-            .zip_ext(bottom_edge.surface_vertices())
-            .map(|(point, surface_vertex)| {
-                (point, surface_vertex.clone(), surface.clone())
-                    .sweep_with_cache(path, cache, objects)
-            });
-
-        let top_edge = {
-            let surface_vertices = side_edges.clone().map(|edge| {
-                let [_, surface_vertex] = edge.surface_vertices();
-                surface_vertex.clone()
-            });
-
-            let points_curve_and_surface = bottom_edge
-                .boundary()
-                .map(|point| (point, [point.t, Scalar::ONE]));
-
-            let curve = {
-                let global = bottom_edge
-                    .curve()
-                    .global_form()
-                    .clone()
-                    .translate(path, objects);
-
-                // Please note that creating a line here is correct, even if the
-                // global curve is a circle. Projected into the side surface, it
-                // is going to be a line either way.
-                let path =
-                    SurfacePath::Line(Line::from_points_with_line_coords(
-                        points_curve_and_surface,
-                    ));
-
-                Curve::new(surface, path, global).insert(objects)
-            };
-
-            let global = GlobalEdge::new(
-                curve.global_form().clone(),
-                surface_vertices
-                    .clone()
-                    .map(|surface_vertex| surface_vertex.global_form().clone()),
-            )
-            .insert(objects);
-
-            let boundary = bottom_edge
-                .boundary()
-                .into_iter_fixed()
-                .zip(surface_vertices)
-                .collect::<[_; 2]>();
-
-            HalfEdge::new(curve, boundary, global).insert(objects)
-        };
-
-        let cycle = {
-            let a = bottom_edge;
-            let [d, b] = side_edges;
-            let c = top_edge;
-
-            let mut edges = [a, b, c, d];
-
-            // Make sure that edges are oriented correctly.
-            let mut i = 0;
-            while i < edges.len() {
-                let j = (i + 1) % edges.len();
-
-                let [_, prev_last] = edges[i].surface_vertices();
-                let [next_first, _] = edges[j].surface_vertices();
-
-                // Need to compare surface forms here, as the global forms might
-                // be coincident when sweeping circles, despite the vertices
-                // being different!
-                if prev_last.id() != next_first.id() {
-                    edges[j] = edges[j].clone().reverse(objects);
-                }
-
-                i += 1;
-            }
-
-            Cycle::new(edges).insert(objects)
-        };
-
-        let face = PartialFace {
-            exterior: Partial::from(cycle),
             color: Some(color),
             ..Default::default()
         };
-        face.build(objects).insert(objects)
+
+        // A face (and everything in it) is defined on a surface. A surface can
+        // be created by sweeping a curve, so let's sweep the curve of the edge
+        // we're sweeping.
+        face.exterior.write().surface = Partial::from(
+            edge.curve().clone().sweep_with_cache(path, cache, objects),
+        );
+
+        // Now we're ready to create the edges.
+        let mut edge_bottom = face.exterior.write().add_half_edge();
+        let mut edge_up = face.exterior.write().add_half_edge();
+        let mut edge_top = face.exterior.write().add_half_edge();
+        let mut edge_down = face.exterior.write().add_half_edge();
+
+        // Those edges aren't fully defined yet. We'll do that shortly, but
+        // first we have to figure a few things out.
+        //
+        // Let's start with the global vertices and edges.
+        let (global_vertices, global_edges) = {
+            let [a, b] = edge
+                .surface_vertices()
+                .map(|surface_vertex| surface_vertex.global_form().clone());
+            let (edge_right, [_, c]) =
+                b.clone().sweep_with_cache(path, cache, objects);
+            let (edge_left, [_, d]) =
+                a.clone().sweep_with_cache(path, cache, objects);
+
+            (
+                [a, b, c, d],
+                [edge.global_form().clone(), edge_right, edge_left],
+            )
+        };
+
+        // Next, let's figure out the surface coordinates of the edge vertices.
+        let surface_points = {
+            let [a, b] = edge.boundary();
+
+            [
+                [a.t, Scalar::ZERO],
+                [b.t, Scalar::ZERO],
+                [b.t, Scalar::ONE],
+                [a.t, Scalar::ONE],
+            ]
+            .map(Point::from)
+        };
+
+        // Now, the boundaries of each edge.
+        let boundaries = {
+            let [a, b] = edge.boundary();
+            let [c, d] = [0., 1.].map(|coord| Point::from([coord]));
+
+            [[a, b], [c, d], [b, a], [d, c]]
+        };
+
+        // Armed with all of that, we can set the edge's vertices.
+        [
+            edge_bottom.write(),
+            edge_up.write(),
+            edge_top.write(),
+            edge_down.write(),
+        ]
+        .zip_ext(boundaries)
+        .zip_ext(surface_points)
+        .zip_ext(global_vertices)
+        .map(
+            |(((mut half_edge, boundary), surface_point), global_vertex)| {
+                for ((a, _), b) in
+                    half_edge.vertices.each_mut_ext().zip_ext(boundary)
+                {
+                    *a = Some(b);
+                }
+
+                // Writing to the start vertices is enough. Neighboring half-
+                // edges share surface vertices, so writing the start vertex of
+                // each half-edge writes to all vertices.
+                let mut vertex = half_edge.vertices[0].1.write();
+                vertex.position = Some(surface_point);
+                vertex.global_form = Partial::from(global_vertex);
+            },
+        );
+
+        // With the vertices set, we can now update the curves.
+        //
+        // Those are all line segments. For the bottom and top curve, because
+        // even if the original edge was a circle, it's still going to be a line
+        // when projected into the new surface. For the side edges, because
+        // we're sweeping along a straight path.
+        for mut edge in [
+            edge_bottom.write(),
+            edge_up.write(),
+            edge_top.write(),
+            edge_down.write(),
+        ] {
+            edge.update_as_line_segment();
+        }
+
+        // Finally, we can make sure that all edges refer to the correct global
+        // edges.
+        [edge_bottom.write(), edge_up.write(), edge_down.write()]
+            .zip_ext(global_edges)
+            .map(|(mut half_edge, global_edge)| {
+                let global_edge = Partial::from(global_edge);
+
+                half_edge.curve.write().global_form =
+                    global_edge.read().curve.clone();
+                half_edge.global_form = global_edge;
+            });
+
+        // And we're done creating the face! All that's left to do is build our
+        // return values.
+        let face = face.build(objects).insert(objects);
+        let edge_top = edge_top.build(objects);
+        (face, edge_top)
     }
 }
 
 #[cfg(test)]
 mod tests {
     use fj_interop::{ext::ArrayExt, mesh::Color};
+    use fj_math::Point;
     use pretty_assertions::assert_eq;
 
     use crate::{
-        algorithms::{reverse::Reverse, sweep::Sweep},
+        algorithms::sweep::Sweep,
         builder::HalfEdgeBuilder,
         insert::Insert,
         partial::{
@@ -202,7 +177,7 @@ mod tests {
                 .insert(&mut services.objects)
         };
 
-        let face = (half_edge, Color::default())
+        let (face, _) = (half_edge, Color::default())
             .sweep([0., 0., 1.], &mut services.objects);
 
         let expected_face = {
@@ -244,16 +219,16 @@ mod tests {
                 top.curve.write().surface = surface.clone();
 
                 {
-                    let [back, front] = top
+                    let [(back, back_surface), (front, front_surface)] =
-                        .vertices
+                        top.vertices.each_mut_ext();
-                        .each_mut_ext()
-                        .map(|(_, surface_vertex)| surface_vertex);
 
-                    let mut back = back.write();
+                    *back = Some(Point::from([1.]));
-                    back.position = Some([0., 1.].into());
+                    *back_surface = side_up.vertices[1].1.clone();
-                    back.surface = surface.clone();
 
-                    *front = side_up.vertices[1].1.clone();
+                    *front = Some(Point::from([0.]));
+                    let mut front_surface = front_surface.write();
+                    front_surface.position = Some([0., 1.].into());
+                    front_surface.surface = surface.clone();
                 }
 
                 top.infer_global_form();
@@ -261,8 +236,7 @@ mod tests {
 
                 Partial::from(
                     top.build(&mut services.objects)
-                        .insert(&mut services.objects)
+                        .insert(&mut services.objects),
-                        .reverse(&mut services.objects),
                 )
                 .read()
                 .clone()
@@ -271,13 +245,14 @@ mod tests {
                 let mut side_down = PartialHalfEdge::default();
                 side_down.curve.write().surface = surface;
 
-                let [back, front] = side_down
+                let [(back, back_surface), (front, front_surface)] =
-                    .vertices
+                    side_down.vertices.each_mut_ext();
-                    .each_mut_ext()
-                    .map(|(_, surface_vertex)| surface_vertex);
 
-                *back = bottom.vertices[0].1.clone();
+                *back = Some(Point::from([1.]));
-                *front = top.vertices[1].1.clone();
+                *front = Some(Point::from([0.]));
+
+                *back_surface = top.vertices[1].1.clone();
+                *front_surface = bottom.vertices[0].1.clone();
 
                 side_down.infer_global_form();
                 side_down.update_as_line_segment();
@@ -285,8 +260,7 @@ mod tests {
                 Partial::from(
                     side_down
                         .build(&mut services.objects)
-                        .insert(&mut services.objects)
+                        .insert(&mut services.objects),
-                        .reverse(&mut services.objects),
                 )
                 .read()
                 .clone()

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

@@ -1,11 +1,13 @@
+use fj_interop::ext::ArrayExt;
 use fj_math::{Scalar, Vector};
 
 use crate::{
     algorithms::{reverse::Reverse, transform::TransformObject},
+    builder::{CycleBuilder, FaceBuilder},
     geometry::path::GlobalPath,
     insert::Insert,
     objects::{Face, Objects, Shell},
-    partial::{Partial, PartialObject, PartialShell},
+    partial::{Partial, PartialFace, PartialObject, PartialShell},
     services::Service,
     storage::Handle,
 };
@@ -47,35 +49,78 @@ impl Sweep for Handle<Face> {
                 self.clone().reverse(objects)
             }
         };
-        faces.push(bottom_face);
+        faces.push(bottom_face.clone());
 
-        let top_face = {
+        let mut top_face = PartialFace {
-            let mut face = self.clone().translate(path, objects);
+            color: Some(self.color()),
+            ..PartialFace::default()
+        };
+        let top_surface =
+            bottom_face.surface().clone().translate(path, objects);
 
-            if is_negative_sweep {
+        for (i, cycle) in bottom_face.all_cycles().cloned().enumerate() {
-                face = face.reverse(objects);
+            let cycle = cycle.reverse(objects);
+
+            let mut top_cycle = if i == 0 {
+                top_face.exterior.clone()
+            } else {
+                top_face.add_interior()
             };
 
-            face
+            let mut original_edges = Vec::new();
-        };
+            let mut top_edges = Vec::new();
-        faces.push(top_face);
+            for half_edge in cycle.half_edges().cloned() {
-
+                let (face, top_edge) = (half_edge.clone(), self.color())
-        // Generate side faces
-        for cycle in self.all_cycles() {
-            for half_edge in cycle.half_edges() {
-                let half_edge = if is_negative_sweep {
-                    half_edge.clone().reverse(objects)
-                } else {
-                    half_edge.clone()
-                };
-
-                let face = (half_edge, self.color())
                     .sweep_with_cache(path, cache, objects);
 
                 faces.push(face);
+
+                original_edges.push(half_edge);
+                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);
+
+            for half_edge in &mut top_cycle.write().half_edges {
+                for (_, surface_vertex) in &mut half_edge.write().vertices {
+                    let mut surface_vertex = surface_vertex.write();
+                    let global_point =
+                        surface_vertex.global_form.read().position;
+
+                    if surface_vertex.position.is_none() {
+                        if let Some(global_point) = global_point {
+                            surface_vertex.position = Some(
+                                top_surface
+                                    .geometry()
+                                    .project_global_point(global_point),
+                            );
+                        }
+                    }
+                }
+            }
+
+            for (bottom, top) in original_edges
+                .into_iter()
+                .zip(top_cycle.write().half_edges.iter_mut())
+            {
+                top.write().curve.write().path =
+                    Some(bottom.curve().path().into());
+
+                let boundary = bottom.boundary();
+
+                for ((top, _), bottom) in
+                    top.write().vertices.each_mut_ext().zip_ext(boundary)
+                {
+                    *top = Some(bottom);
+                }
             }
         }
 
+        let top_face = top_face.build(objects).insert(objects);
+        faces.push(top_face);
+
         let faces = faces.into_iter().map(Partial::from).collect();
         PartialShell { faces }.build(objects).insert(objects)
     }
@@ -159,7 +204,9 @@ mod tests {
                     .build(&mut services.objects)
                     .insert(&mut services.objects)
             };
-            (half_edge, Color::default()).sweep(UP, &mut services.objects)
+            let (face, _) =
+                (half_edge, Color::default()).sweep(UP, &mut services.objects);
+            face
         });
 
         assert!(side_faces
@@ -226,7 +273,9 @@ mod tests {
                     .insert(&mut services.objects)
                     .reverse(&mut services.objects)
             };
-            (half_edge, Color::default()).sweep(DOWN, &mut services.objects)
+            let (face, _) = (half_edge, Color::default())
+                .sweep(DOWN, &mut services.objects);
+            face
         });
 
         assert!(side_faces

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

@@ -1,120 +1,14 @@
-use fj_math::{Point, Scalar, Vector};
+use fj_math::Vector;
 
 use crate::{
-    geometry::path::SurfacePath,
     insert::Insert,
-    objects::{
+    objects::{GlobalCurve, GlobalEdge, GlobalVertex, Objects},
-        Curve, GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Objects,
-        Surface, SurfaceVertex,
-    },
     services::Service,
     storage::Handle,
 };
 
 use super::{Sweep, SweepCache};
 
-impl Sweep for (Point<1>, Handle<SurfaceVertex>, Handle<Surface>) {
-    type Swept = Handle<HalfEdge>;
-
-    fn sweep_with_cache(
-        self,
-        path: impl Into<Vector<3>>,
-        cache: &mut SweepCache,
-        objects: &mut Service<Objects>,
-    ) -> Self::Swept {
-        let (point, surface_vertex, surface) = self;
-        let path = path.into();
-
-        // The result of sweeping a `Vertex` is an `Edge`. Seems
-        // straight-forward at first, but there are some subtleties we need to
-        // understand:
-        //
-        // 1. To create an `Edge`, we need the `Curve` that defines it. A
-        //    `Curve` is defined in a `Surface`, and we're going to need that to
-        //    create the `Curve`. Which is why this `Sweep` implementation is
-        //    for `(Vertex, Surface)`, and not just for `Vertex`.
-        // 2. Please note that, while the output `Edge` has two vertices, our
-        //    input `Vertex` is not one of them! It can't be, unless the `Curve`
-        //    of the output `Edge` happens to be the same `Curve` that the input
-        //    `Vertex` is defined on. That would be an edge case that probably
-        //    can't result in anything valid, and we're going to ignore it for
-        //    now.
-        // 3. This means, we have to compute everything that defines the
-        //    output `Edge`: The `Curve`, the vertices, and the `GlobalCurve`.
-        //
-        // Before we get to that though, let's make sure that whoever called
-        // this didn't give us bad input.
-
-        // So, we're supposed to create the `Edge` by sweeping a `Vertex` using
-        // `path`. Unless `path` is identical to the path that created the
-        // `Surface`, this doesn't make any sense. Let's make sure this
-        // requirement is met.
-        //
-        // Further, the `Curve` that was swept to create the `Surface` needs to
-        // be the same `Curve` that the input `Vertex` is defined on. If it's
-        // not, we have no way of knowing the surface coordinates of the input
-        // `Vertex` on the `Surface`, and we're going to need to do that further
-        // down. There's no way to check for that, unfortunately.
-        assert_eq!(path, surface.geometry().v);
-
-        // With that out of the way, let's start by creating the `GlobalEdge`,
-        // as that is the most straight-forward part of this operations, and
-        // we're going to need it soon anyway.
-        let (edge_global, vertices_global) = surface_vertex
-            .global_form()
-            .clone()
-            .sweep_with_cache(path, cache, objects);
-
-        // Next, let's compute the surface coordinates of the two vertices of
-        // the output `Edge`, as we're going to need these for the rest of this
-        // operation.
-        //
-        // They both share a u-coordinate, which is the t-coordinate of our
-        // input `Vertex`. Remember, we validated above, that the `Curve` of the
-        // `Surface` and the curve of the input `Vertex` are the same, so we can
-        // do that.
-        //
-        // Now remember what we also validated above: That `path`, which we're
-        // using to create the output `Edge`, also created the `Surface`, and
-        // thereby defined its coordinate system. That makes the v-coordinates
-        // straight-forward: The start of the edge is at zero, the end is at
-        // one.
-        let points_surface = [
-            Point::from([point.t, Scalar::ZERO]),
-            Point::from([point.t, Scalar::ONE]),
-        ];
-
-        // Armed with those coordinates, creating the `Curve` of the output
-        // `Edge` is straight-forward.
-        let curve = {
-            let (path, _) = SurfacePath::line_from_points(points_surface);
-
-            Curve::new(surface.clone(), path, edge_global.curve().clone())
-                .insert(objects)
-        };
-
-        let vertices_surface = {
-            let [_, position] = points_surface;
-            let [_, global_form] = vertices_global;
-
-            [
-                surface_vertex,
-                SurfaceVertex::new(position, surface, global_form)
-                    .insert(objects),
-            ]
-        };
-
-        // And now the vertices. Again, nothing wild here.
-        let boundary = vertices_surface.map(|surface_vertex| {
-            (Point::from([surface_vertex.position().v]), surface_vertex)
-        });
-
-        // And finally, creating the output `Edge` is just a matter of
-        // assembling the pieces we've already created.
-        HalfEdge::new(curve, boundary, edge_global).insert(objects)
-    }
-}
-
 impl Sweep for Handle<GlobalVertex> {
     type Swept = (Handle<GlobalEdge>, [Self; 2]);
 
@@ -145,61 +39,3 @@ impl Sweep for Handle<GlobalVertex> {
         (global_edge, vertices)
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use fj_math::Point;
-    use pretty_assertions::assert_eq;
-
-    use crate::{
-        algorithms::sweep::Sweep,
-        builder::{CurveBuilder, HalfEdgeBuilder},
-        insert::Insert,
-        partial::{
-            Partial, PartialCurve, PartialGlobalVertex, PartialHalfEdge,
-            PartialObject, PartialSurfaceVertex,
-        },
-        services::Services,
-    };
-
-    #[test]
-    fn vertex_surface() {
-        let mut services = Services::new();
-
-        let surface = services.objects.surfaces.xz_plane();
-        let (position, surface_vertex) = {
-            let mut curve = PartialCurve {
-                surface: Partial::from(surface.clone()),
-                ..Default::default()
-            };
-            curve.update_as_u_axis();
-
-            let surface_form = Partial::from_partial(PartialSurfaceVertex {
-                position: Some(Point::from([0., 0.])),
-                surface: Partial::from(surface.clone()),
-                global_form: Partial::from_partial(PartialGlobalVertex {
-                    position: Some(Point::from([0., 0., 0.])),
-                }),
-            })
-            .build(&mut services.objects);
-
-            (Point::from([0.]), surface_form)
-        };
-
-        let half_edge = (position, surface_vertex, surface.clone())
-            .sweep([0., 0., 1.], &mut services.objects);
-
-        let expected_half_edge = {
-            let mut half_edge = PartialHalfEdge::default();
-            half_edge.update_as_line_segment_from_points(
-                surface,
-                [[0., 0.], [0., 1.]],
-            );
-
-            half_edge
-                .build(&mut services.objects)
-                .insert(&mut services.objects)
-        };
-        assert_eq!(half_edge, expected_half_edge);
-    }
-}

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

@@ -100,7 +100,7 @@ impl HalfEdgeBuilder for PartialHalfEdge {
         if angle_rad <= -Scalar::TAU || angle_rad >= Scalar::TAU {
             panic!("arc angle must be in the range (-2pi, 2pi) radians");
         }
-        let points_surface = self.vertices.each_ref_ext().map(|vertex| {
+        let [start, end] = self.vertices.each_ref_ext().map(|vertex| {
             vertex
                 .1
                 .read()
@@ -108,11 +108,7 @@ impl HalfEdgeBuilder for PartialHalfEdge {
                 .expect("Can't infer arc without surface position")
         });
 
-        let arc = fj_math::Arc::from_endpoints_and_angle(
+        let arc = fj_math::Arc::from_endpoints_and_angle(start, end, angle_rad);
-            points_surface[0],
-            points_surface[1],
-            angle_rad,
-        );
 
         let path = self
             .curve