Merge pull request #1017 from hannobraun/reverse2

Clean up and expand `algorithms::reverse`

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

@@ -3,12 +3,12 @@
 //! Algorithmic code is collected in this module, to keep other modules focused
 //! on their respective purpose.
 
-mod reverse;
 mod triangulate;
 
 pub mod approx;
 pub mod intersect;
+pub mod reverse;
 pub mod sweep;
 pub mod transform;
 
-pub use self::{reverse::reverse_face, triangulate::triangulate};
+pub use self::triangulate::triangulate;

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

@@ -0,0 +1,41 @@
+use crate::objects::{Curve, GlobalCurve};
+
+use super::Reverse;
+
+impl Reverse for Curve {
+    /// Reverse the curve
+    ///
+    /// # Implementation Note
+    ///
+    /// Right now, the orientation of a face is defined by the orientation of
+    /// its surface. By extension, the orientation of a surface is defined by
+    /// the orientation of the curve it was swept from. This leads to some
+    /// complications. See this issue for context:
+    /// <https://github.com/hannobraun/Fornjot/issues/695>
+    ///
+    /// It would probably be much better, if `Surface`s were without
+    /// orientation, which would then make it possible for curves to be without
+    /// direction. Then this implementation would not exist.
+    fn reverse(self) -> Self {
+        Curve::new(self.kind().reverse(), self.global().reverse())
+    }
+}
+
+impl Reverse for GlobalCurve {
+    /// Reverse the curve
+    ///
+    /// # Implementation Note
+    ///
+    /// Right now, the orientation of a face is defined by the orientation of
+    /// its surface. By extension, the orientation of a surface is defined by
+    /// the orientation of the curve it was swept from. This leads to some
+    /// complications. See this issue for context:
+    /// <https://github.com/hannobraun/Fornjot/issues/695>
+    ///
+    /// It would probably be much better, if `Surface`s were without
+    /// orientation, which would then make it possible for curves to be without
+    /// direction. Then this implementation would not exist.
+    fn reverse(self) -> Self {
+        Self::from_kind(self.kind().reverse())
+    }
+}

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

@@ -0,0 +1,18 @@
+use crate::objects::Cycle;
+
+use super::Reverse;
+
+impl Reverse for Cycle {
+    fn reverse(self) -> Self {
+        let surface = *self.surface();
+
+        let mut edges = self
+            .into_edges()
+            .map(|edge| edge.reverse())
+            .collect::<Vec<_>>();
+
+        edges.reverse();
+
+        Cycle::new(surface).with_edges(edges)
+    }
+}

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

@@ -0,0 +1,12 @@
+use crate::objects::Edge;
+
+use super::Reverse;
+
+impl Reverse for Edge {
+    fn reverse(self) -> Self {
+        Edge::from_curve_and_vertices(
+            self.curve().reverse(),
+            self.vertices().reverse(),
+        )
+    }
+}

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

@@ -2,23 +2,39 @@ use fj_math::{Circle, Line, Point, Vector};
 
 use crate::objects::{Curve, CurveKind, Cycle, Edge, Face};
 
-/// Reverse the direction of a face
-pub fn reverse_face(face: &Face) -> Face {
-    if face.triangles().is_some() {
-        panic!("Reversing tri-rep faces is not supported");
-    }
+use super::Reverse;
+
+impl Reverse for Face {
+    fn reverse(self) -> Self {
+        if self.triangles().is_some() {
+            panic!("Reversing tri-rep faces is not supported");
+        }
 
-    let surface = face.surface().reverse();
+        let surface = self.surface().reverse();
 
-    let exteriors = reverse_local_coordinates_in_cycle(face.exteriors());
-    let interiors = reverse_local_coordinates_in_cycle(face.interiors());
+        let exteriors = reverse_local_coordinates_in_cycle(self.exteriors());
+        let interiors = reverse_local_coordinates_in_cycle(self.interiors());
 
-    Face::new(surface)
-        .with_exteriors(exteriors)
-        .with_interiors(interiors)
-        .with_color(face.color())
+        Face::new(surface)
+            .with_exteriors(exteriors)
+            .with_interiors(interiors)
+            .with_color(self.color())
+    }
 }
 
+/// Reverse local coordinates within the cycle, leaving global ones as-is
+///
+/// # Implementation Note
+///
+/// This is probably overly complicated. If the orientation of a face were
+/// defined by the direction of the half-edges that bound it, we could reverse
+/// the whole cycle with no weird distinction. The `Reverse` implementation of
+/// `Face` could just use the `Reverse` implementation of `Cycle` then.
+///
+/// Please note that, as of this writing, half-edges don't really exist as a
+/// 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 {
@@ -67,18 +83,19 @@ fn reverse_local_coordinates_in_cycle<'r>(
 mod tests {
     use pretty_assertions::assert_eq;
 
-    use crate::objects::{Face, Surface};
+    use crate::{
+        algorithms::reverse::Reverse,
+        objects::{Face, Surface},
+    };
 
     #[test]
     fn reverse_face() {
         let surface = Surface::xy_plane();
-        let original = Face::build(surface).polygon_from_points([
-            [0., 0.],
-            [1., 0.],
-            [0., 1.],
-        ]);
+        let original = Face::build(surface)
+            .polygon_from_points([[0., 0.], [1., 0.], [0., 1.]])
+            .into_face();
 
-        let reversed = super::reverse_face(&original);
+        let reversed = original.reverse();
 
         let surface = Surface::xy_plane().reverse();
         let expected = Face::build(surface)

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

@@ -0,0 +1,14 @@
+//! Reverse the direction/orientation of objects
+
+mod curve;
+mod cycle;
+mod edge;
+mod face;
+mod surface;
+
+/// Reverse the direction/orientation of an object
+pub trait Reverse {
+    /// Reverse the direction/orientation of the object
+    #[must_use]
+    fn reverse(self) -> Self;
+}

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

@@ -0,0 +1,32 @@
+use crate::objects::Surface;
+
+use super::Reverse;
+
+impl Reverse for Surface {
+    /// Reverse the surface
+    ///
+    /// # Implementation Note
+    ///
+    /// Right now, the orientation of a face is defined by the orientation of
+    /// its surface. This leads to some complications. See this issue for
+    /// context:
+    /// <https://github.com/hannobraun/Fornjot/issues/695>
+    ///
+    /// It would probably be much better, if `Surface`s were without
+    /// orientation, and the orientation of a face were defined by the direction
+    /// of the half-edges that bound it.
+    ///
+    /// Please note that, as of this writing, half-edges don't really exist as a
+    /// 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>
+    ///
+    /// So, in conclusion, in a perfect world, this implementation would not
+    /// exist.
+    fn reverse(self) -> Self {
+        match self {
+            Self::SweptCurve(surface) => Self::SweptCurve(surface.reverse()),
+        }
+    }
+}

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

@@ -1,7 +1,9 @@
 use fj_interop::mesh::Color;
 
 use crate::{
-    algorithms::{approx::Tolerance, reverse_face, transform::TransformObject},
+    algorithms::{
+        approx::Tolerance, reverse::Reverse, transform::TransformObject,
+    },
     objects::{Face, Shell},
 };
 
@@ -46,15 +48,15 @@ fn create_bottom_face(
     if is_sweep_along_negative_direction {
         face.clone()
     } else {
-        reverse_face(face)
+        face.clone().reverse()
     }
 }
 
 fn create_top_face(face: Face, path: Path) -> Face {
     let mut face = face.translate(path.inner());
 
     if path.is_negative_direction() {
-        face = reverse_face(&face);
+        face = face.reverse();
     };
 
     face

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

@@ -44,14 +44,6 @@ impl Surface {
         Self::SweptCurve(SweptCurve { curve, path })
     }
 
-    /// Create a new instance that is reversed
-    #[must_use]
-    pub fn reverse(self) -> Self {
-        match self {
-            Self::SweptCurve(surface) => Self::SweptCurve(surface.reverse()),
-        }
-    }
-
     /// Convert a point in surface coordinates to model coordinates
     pub fn point_from_surface_coords(
         &self,

crates/fj-operations/src/difference_2d.rs

@@ -1,8 +1,8 @@
 use fj_interop::{debug::DebugInfo, mesh::Color};
 use fj_kernel::{
-    algorithms::approx::Tolerance,
+    algorithms::{approx::Tolerance, reverse::Reverse},
     iter::ObjectIters,
-    objects::{Curve, Cycle, Edge, Face, GlobalCurve, Sketch},
+    objects::{Face, Sketch},
     validation::{validate, Validated, ValidationConfig, ValidationError},
 };
 use fj_math::Aabb;
@@ -47,12 +47,10 @@ impl Shape for fj::Difference2d {
                 );
 
                 for cycle in face.exteriors() {
-                    let cycle = add_cycle(cycle.clone(), false);
-                    exteriors.push(cycle);
+                    exteriors.push(cycle.clone());
                 }
                 for cycle in face.interiors() {
-                    let cycle = add_cycle(cycle.clone(), true);
-                    interiors.push(cycle);
+                    interiors.push(cycle.clone().reverse());
                 }
             }
 
@@ -64,8 +62,7 @@ impl Shape for fj::Difference2d {
                 );
 
                 for cycle in face.exteriors() {
-                    let cycle = add_cycle(cycle.clone(), true);
-                    interiors.push(cycle);
+                    interiors.push(cycle.clone().reverse());
                 }
             }
 
@@ -88,39 +85,3 @@ impl Shape for fj::Difference2d {
         self.shapes()[0].bounding_volume()
     }
 }
-
-fn add_cycle(cycle: Cycle, reverse: bool) -> Cycle {
-    let mut edges = Vec::new();
-    for edge in cycle.edges() {
-        let curve_local = if reverse {
-            edge.curve().kind().reverse()
-        } else {
-            *edge.curve().kind()
-        };
-
-        let curve_global = GlobalCurve::from_kind(if reverse {
-            edge.curve().global().kind().reverse()
-        } else {
-            *edge.curve().global().kind()
-        });
-
-        let vertices = if reverse {
-            edge.vertices().reverse()
-        } else {
-            *edge.vertices()
-        };
-
-        let edge = Edge::from_curve_and_vertices(
-            Curve::new(curve_local, curve_global),
-            vertices,
-        );
-
-        edges.push(edge);
-    }
-
-    if reverse {
-        edges.reverse();
-    }
-
-    Cycle::new(*cycle.surface()).with_edges(edges)
-}