Merge pull request #1059 from hannobraun/face

Limit faces to one exterior

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

@@ -75,30 +75,14 @@ impl Approx for &Face {
         // would need to provide its own approximation, as the edges that bound
         // it have nothing to do with its curvature.
 
-        let mut exteriors = Vec::new();
-        let mut interiors = BTreeSet::new();
+        let exterior = self.exterior().approx(tolerance);
 
-        for cycle in self.exteriors() {
-            let cycle = cycle.approx(tolerance);
-            exteriors.push(cycle);
-        }
+        let mut interiors = BTreeSet::new();
         for cycle in self.interiors() {
             let cycle = cycle.approx(tolerance);
             interiors.insert(cycle);
         }
 
-        // Only polygons with exactly one exterior cycle are supported.
-        //
-        // See this issue for some background:
-        // https://github.com/hannobraun/Fornjot/issues/250
-        let exterior = exteriors
-            .pop()
-            .expect("Can't approximate face without exterior cycle");
-        assert!(
-            exteriors.is_empty(),
-            "Approximation only supports faces with one exterior cycle",
-        );
-
         FaceApprox {
             exterior,
             interiors,

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

@@ -10,16 +10,21 @@ impl Reverse for Face {
     fn reverse(self) -> Self {
         let surface = self.surface().reverse();
 
-        let exteriors = reverse_local_coordinates_in_cycle(self.exteriors());
-        let interiors = reverse_local_coordinates_in_cycle(self.interiors());
+        let exterior = reverse_local_coordinates_in_cycle(self.exterior());
+        let interiors = reverse_local_coordinates_in_cycles(self.interiors());
 
-        Face::new(surface)
-            .with_exteriors(exteriors)
+        Face::new(surface, exterior)
             .with_interiors(interiors)
             .with_color(self.color())
     }
 }
 
+fn reverse_local_coordinates_in_cycles<'r>(
+    cycles: impl IntoIterator<Item = &'r Cycle> + 'r,
+) -> impl Iterator<Item = Cycle> + 'r {
+    cycles.into_iter().map(reverse_local_coordinates_in_cycle)
+}
+
 /// Reverse local coordinates within the cycle, leaving global ones as-is
 ///
 /// # Implementation Note
@@ -33,76 +38,66 @@ impl Reverse for Face {
 /// concept in the kernel. We kind of treat `Edge` as a half-edge, but in an
 /// inconsistent way that causes problems. This issue has some context on that:
 /// <https://github.com/hannobraun/Fornjot/issues/993>
-fn reverse_local_coordinates_in_cycle<'r>(
-    cycles: impl IntoIterator<Item = &'r Cycle> + 'r,
-) -> impl Iterator<Item = Cycle> + 'r {
-    cycles.into_iter().map(|cycle| {
-        let surface = cycle.surface().reverse();
-
-        let edges = cycle.edges().map(|edge| {
-            let curve = {
-                let local = match edge.curve().kind() {
-                    CurveKind::Circle(circle) => {
-                        let center = Point::from([
-                            circle.center().u,
-                            -circle.center().v,
-                        ]);
-
-                        let a = Vector::from([circle.a().u, -circle.a().v]);
-                        let b = Vector::from([circle.b().u, -circle.b().v]);
-
-                        CurveKind::Circle(Circle::new(center, a, b))
-                    }
-                    CurveKind::Line(line) => {
-                        let origin =
-                            Point::from([line.origin().u, -line.origin().v]);
-                        let direction = Vector::from([
-                            line.direction().u,
-                            -line.direction().v,
-                        ]);
-
-                        CurveKind::Line(Line::from_origin_and_direction(
-                            origin, direction,
-                        ))
-                    }
-                };
-
-                Curve::new(
-                    edge.curve().surface().reverse(),
-                    local,
-                    *edge.curve().global_form(),
-                )
+fn reverse_local_coordinates_in_cycle(cycle: &Cycle) -> Cycle {
+    let surface = cycle.surface().reverse();
+
+    let edges = cycle.edges().map(|edge| {
+        let curve = {
+            let local = match edge.curve().kind() {
+                CurveKind::Circle(circle) => {
+                    let center =
+                        Point::from([circle.center().u, -circle.center().v]);
+
+                    let a = Vector::from([circle.a().u, -circle.a().v]);
+                    let b = Vector::from([circle.b().u, -circle.b().v]);
+
+                    CurveKind::Circle(Circle::new(center, a, b))
+                }
+                CurveKind::Line(line) => {
+                    let origin =
+                        Point::from([line.origin().u, -line.origin().v]);
+                    let direction =
+                        Vector::from([line.direction().u, -line.direction().v]);
+
+                    CurveKind::Line(Line::from_origin_and_direction(
+                        origin, direction,
+                    ))
+                }
             };
 
-            let vertices = edge.vertices().map(|vertex| {
-                let surface_vertex = {
-                    let vertex = vertex.surface_form();
-
-                    let position = Point::from([
-                        vertex.position().u,
-                        -vertex.position().v,
-                    ]);
-
-                    SurfaceVertex::new(
-                        position,
-                        vertex.surface().reverse(),
-                        *vertex.global_form(),
-                    )
-                };
-
-                Vertex::new(
-                    vertex.position(),
-                    curve,
-                    surface_vertex,
+            Curve::new(
+                edge.curve().surface().reverse(),
+                local,
+                *edge.curve().global_form(),
+            )
+        };
+
+        let vertices = edge.vertices().map(|vertex| {
+            let surface_vertex = {
+                let vertex = vertex.surface_form();
+
+                let position =
+                    Point::from([vertex.position().u, -vertex.position().v]);
+
+                SurfaceVertex::new(
+                    position,
+                    vertex.surface().reverse(),
                     *vertex.global_form(),
                 )
-            });
+            };
 
-            Edge::from_curve_and_vertices(curve, vertices)
+            Vertex::new(
+                vertex.position(),
+                curve,
+                surface_vertex,
+                *vertex.global_form(),
+            )
         });
 
-        Cycle::new(surface, edges)
-    })
+        Edge::from_curve_and_vertices(curve, vertices)
+    });
+
+    Cycle::new(surface, edges)
 }
 
 #[cfg(test)]

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

@@ -177,6 +177,6 @@ impl Sweep for Edge {
             Cycle::new(surface, edges)
         };
 
-        Face::new(surface).with_exteriors([cycle]).with_color(color)
+        Face::new(surface, cycle).with_color(color)
     }
 }

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

@@ -67,13 +67,14 @@ impl TransformObject for Face {
     fn transform(self, transform: &Transform) -> Self {
         let surface = self.surface().transform(transform);
 
-        let exteriors = transform_cycles(self.exteriors(), transform);
-        let interiors = transform_cycles(self.interiors(), transform);
+        let exterior = self.exterior().clone().transform(transform);
+        let interiors = self
+            .interiors()
+            .map(|cycle| cycle.clone().transform(transform));
 
         let color = self.color();
 
-        Face::new(surface)
-            .with_exteriors(exteriors)
+        Face::new(surface, exterior)
             .with_interiors(interiors)
             .with_color(color)
     }
@@ -158,12 +159,3 @@ impl TransformObject for Vertex {
         )
     }
 }
-
-fn transform_cycles<'a>(
-    cycles: impl IntoIterator<Item = &'a Cycle> + 'a,
-    transform: &'a Transform,
-) -> impl Iterator<Item = Cycle> + 'a {
-    cycles
-        .into_iter()
-        .map(|cycle| cycle.clone().transform(transform))
-}

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

@@ -22,10 +22,8 @@ impl FaceBuilder {
         &self,
         points: impl IntoIterator<Item = impl Into<Point<2>>>,
     ) -> FacePolygon {
-        let face = Face::new(self.surface)
-            .with_exteriors([
-                Cycle::build(self.surface).polygon_from_points(points)
-            ]);
+        let cycle = Cycle::build(self.surface).polygon_from_points(points);
+        let face = Face::new(self.surface, cycle);
 
         FacePolygon { face }
     }

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

@@ -47,7 +47,7 @@ impl<'a> IntoIterator for &'a Faces {
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct Face {
     surface: Surface,
-    exteriors: Vec<Cycle>,
+    exterior: Cycle,
     interiors: Vec<Cycle>,
     color: Color,
 }
@@ -60,41 +60,17 @@ impl Face {
 
     /// Construct a new instance of `Face`
     ///
-    /// Creates the face with no exteriors, no interiors and the default color.
-    /// This can be overridden using the `with_` methods.
-    pub fn new(surface: Surface) -> Self {
+    /// Creates the face with no interiors and the default color. This can be
+    /// overridden using the `with_` methods.
+    pub fn new(surface: Surface, exterior: Cycle) -> Self {
         Self {
             surface,
-            exteriors: Vec::new(),
+            exterior,
             interiors: Vec::new(),
             color: Color::default(),
         }
     }
 
-    /// Add exterior cycles to the face
-    ///
-    /// Consumes the face and returns the updated instance.
-    ///
-    /// # Panics
-    ///
-    /// Panics, if the added cycles are not defined in the face's surface.
-    pub fn with_exteriors(
-        mut self,
-        exteriors: impl IntoIterator<Item = Cycle>,
-    ) -> Self {
-        for cycle in exteriors.into_iter() {
-            assert_eq!(
-                self.surface(),
-                cycle.surface(),
-                "Cycles that bound a face must be in face's surface"
-            );
-
-            self.exteriors.push(cycle);
-        }
-
-        self
-    }
-
     /// Add interior cycles to the face
     ///
     /// Consumes the face and returns the updated instance.
@@ -133,8 +109,8 @@ impl Face {
     }
 
     /// Access the cycles that bound the face on the outside
-    pub fn exteriors(&self) -> impl Iterator<Item = &Cycle> + '_ {
-        self.exteriors.iter()
+    pub fn exterior(&self) -> &Cycle {
+        &self.exterior
     }
 
     /// Access the cycles that bound the face on the inside
@@ -145,11 +121,8 @@ impl Face {
     }
 
     /// Access all cycles of this face
-    ///
-    /// This is equivalent to chaining the iterators returned by
-    /// [`Face::exteriors`] and [`Face::interiors`].
     pub fn all_cycles(&self) -> impl Iterator<Item = &Cycle> + '_ {
-        self.exteriors().chain(self.interiors())
+        [self.exterior()].into_iter().chain(self.interiors())
     }
 
     /// Access the color of the face

crates/fj-operations/src/difference_2d.rs

@@ -49,9 +49,7 @@ impl Shape for fj::Difference2d {
                     "Trying to subtract faces with different surfaces.",
                 );
 
-                for cycle in face.exteriors() {
-                    exteriors.push(cycle.clone());
-                }
+                exteriors.push(face.exterior().clone());
                 for cycle in face.interiors() {
                     interiors.push(cycle.clone().reverse());
                 }
@@ -64,14 +62,28 @@ impl Shape for fj::Difference2d {
                     "Trying to subtract faces with different surfaces.",
                 );
 
-                for cycle in face.exteriors() {
-                    interiors.push(cycle.clone().reverse());
-                }
+                interiors.push(face.exterior().clone().reverse());
             }
 
+            // Faces only support one exterior, while the code here comes from
+            // the time when a face could have multiple exteriors. This was only
+            // a special case, i.e. faces that connected to themselves, and I
+            // have my doubts that this code was ever correct in the first
+            // place.
+            //
+            // Anyway, the following should make sure that at least any problems
+            // this code causes become obvious. I don't know if this can ever
+            // trigger, but better safe than sorry.
+            let exterior = exteriors
+                .pop()
+                .expect("Can't construct face without an exterior");
+            assert!(
+                exteriors.is_empty(),
+                "Can't construct face with multiple exteriors"
+            );
+
             faces.push(
-                Face::new(*surface)
-                    .with_exteriors(exteriors)
+                Face::new(*surface, exterior)
                     .with_interiors(interiors)
                     .with_color(Color(self.color())),
             );

crates/fj-operations/src/sketch.rs

@@ -30,9 +30,7 @@ impl Shape for fj::Sketch {
                     .circle_from_radius(Scalar::from_f64(circle.radius()));
                 let cycle = Cycle::new(surface, [edge]);
 
-                Face::new(surface)
-                    .with_exteriors([cycle])
-                    .with_color(Color(self.color()))
+                Face::new(surface, cycle).with_color(Color(self.color()))
             }
             fj::Chain::PolyChain(poly_chain) => {
                 let points =