Merge pull request #2446 from hannobraun/join

Improve documentation around `add_joined_half_edges`

crates/fj-core/src/operations/join/cycle.rs

@@ -22,13 +22,17 @@ pub trait JoinCycle {
     /// joins the new half-edge to the provided one, and adds the new half-edge
     /// to the cycle.
     ///
-    /// The geometry for each new half-edge needs to be provided as well.
+    /// Expects the provided half-edges to be in an unnatural order:
     ///
-    /// Also requires the surface that the cycle is defined in.
+    /// - This is the opposite order that they would appear in, in a cycle.
+    /// - Meaning each half-edge ends where the _previous_ one starts.
+    ///
+    /// The geometry for each new half-edge needs to be provided as well. Also
+    /// requires the surface that the cycle is defined in.
     #[must_use]
-    fn add_joined_edges<Es>(
+    fn add_joined_half_edges<Es>(
         &self,
-        edges: Es,
+        half_edges: Es,
         surface: Handle<Surface>,
         core: &mut Core,
     ) -> Self
@@ -89,24 +93,28 @@ pub trait JoinCycle {
 }
 
 impl JoinCycle for Cycle {
-    fn add_joined_edges<Es>(
+    fn add_joined_half_edges<Es>(
         &self,
-        edges: Es,
+        half_edges: Es,
         surface: Handle<Surface>,
         core: &mut Core,
     ) -> Self
     where
         Es: IntoIterator<Item = (Handle<HalfEdge>, LocalCurveGeom)>,
         Es::IntoIter: Clone + ExactSizeIterator,
     {
-        let half_edges = edges
+        let half_edges = half_edges
             .into_iter()
             .circular_tuple_windows()
-            .map(|((prev_half_edge, _), (half_edge, curve_geom))| {
+            // If you're confused about the naming of `next_half_edge` and/or
+            // the relative order of `half_edge`/`next_half_edge`, please refer
+            // to the documentation of this method. This matches the order it
+            // expects half-edges in.
+            .map(|((next_half_edge, _), (half_edge, curve_geom))| {
                 let half_edge = HalfEdge::unjoined(core)
                     .update_curve(|_, _| half_edge.curve().clone(), core)
                     .update_start_vertex(
-                        |_, _| prev_half_edge.start_vertex().clone(),
+                        |_, _| next_half_edge.start_vertex().clone(),
                         core,
                     )
                     .insert(core);

crates/fj-core/src/operations/sweep/cycle.rs

@@ -79,6 +79,24 @@ impl SweepCycle for Cycle {
 
             faces.push(swept_half_edge.face);
 
+            // The order of these top half-edges is going to be important later,
+            // so let's make sure we understand what's going on:
+            //
+            // - We are iterating through the bottom half-edges here. That means
+            //   the order of those bottom half-edges is natural, as we'd expect
+            //   it:
+            //   - We see them in the order that they appear in the cycle.
+            //   - Each half-edge we see ends where the next one starts.
+            // - By sweeping the bottom half-edges, we are creating a top half-
+            //   edges that have opposite orientation.
+            // - And yet we're adding them to a list, in the same order that we
+            //   iterate over the bottom half-edges.
+            // - As a result, the order of the list is unnatural, going against
+            //   expectations:
+            //   - This is the opposite order than the one in which they'll
+            //     appear within a cycle eventually.
+            //   - Each half-edge ends where the _previous_ one (in the list)
+            //     starts.
             top_half_edges.push((
                 swept_half_edge.top_half_edge,
                 swept_half_edge.top_boundary,
@@ -118,8 +136,15 @@ impl SweepCycle for Cycle {
             )
             .collect::<Vec<_>>();
 
-        let top_cycle =
-            Cycle::empty().add_joined_edges(top_half_edges, top_surface, core);
+        // The half-edges within `top_half_edges` which we're passing into
+        // `add_joined_edges` are in unnatural order, as per the comment above.
+        // This happens to be exactly the order that `add_joined_edges` wants
+        // them to be in, so it works out.
+        let top_cycle = Cycle::empty().add_joined_half_edges(
+            top_half_edges,
+            top_surface,
+            core,
+        );
 
         SweptCycle { faces, top_cycle }
     }