diff --git a/crates/fj-kernel/src/algorithms/mod.rs b/crates/fj-kernel/src/algorithms/mod.rs index 631839911..1ec28c787 100644 --- a/crates/fj-kernel/src/algorithms/mod.rs +++ b/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; diff --git a/crates/fj-kernel/src/algorithms/reverse/curve.rs b/crates/fj-kernel/src/algorithms/reverse/curve.rs new file mode 100644 index 000000000..87048df73 --- /dev/null +++ b/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: + /// + /// + /// 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: + /// + /// + /// 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()) + } +} diff --git a/crates/fj-kernel/src/algorithms/reverse/cycle.rs b/crates/fj-kernel/src/algorithms/reverse/cycle.rs new file mode 100644 index 000000000..db73de4bb --- /dev/null +++ b/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::>(); + + edges.reverse(); + + Cycle::new(surface).with_edges(edges) + } +} diff --git a/crates/fj-kernel/src/algorithms/reverse/edge.rs b/crates/fj-kernel/src/algorithms/reverse/edge.rs new file mode 100644 index 000000000..623a67c12 --- /dev/null +++ b/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(), + ) + } +} diff --git a/crates/fj-kernel/src/algorithms/reverse.rs b/crates/fj-kernel/src/algorithms/reverse/face.rs similarity index 57% rename from crates/fj-kernel/src/algorithms/reverse.rs rename to crates/fj-kernel/src/algorithms/reverse/face.rs index d1d70acd4..970041ee1 100644 --- a/crates/fj-kernel/src/algorithms/reverse.rs +++ b/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: +/// fn reverse_local_coordinates_in_cycle<'r>( cycles: impl IntoIterator + 'r, ) -> impl Iterator + '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) diff --git a/crates/fj-kernel/src/algorithms/reverse/mod.rs b/crates/fj-kernel/src/algorithms/reverse/mod.rs new file mode 100644 index 000000000..2b736706f --- /dev/null +++ b/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; +} diff --git a/crates/fj-kernel/src/algorithms/reverse/surface.rs b/crates/fj-kernel/src/algorithms/reverse/surface.rs new file mode 100644 index 000000000..82652defd --- /dev/null +++ b/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: + /// + /// + /// 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: + /// + /// + /// 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()), + } + } +} diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index a84826c13..f34f47fd1 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/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,7 +48,7 @@ fn create_bottom_face( if is_sweep_along_negative_direction { face.clone() } else { - reverse_face(face) + face.clone().reverse() } } @@ -54,7 +56,7 @@ 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 diff --git a/crates/fj-kernel/src/objects/surface.rs b/crates/fj-kernel/src/objects/surface.rs index 9f0169f2c..76f1e42de 100644 --- a/crates/fj-kernel/src/objects/surface.rs +++ b/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, diff --git a/crates/fj-operations/src/difference_2d.rs b/crates/fj-operations/src/difference_2d.rs index d00c0af77..d187a062f 100644 --- a/crates/fj-operations/src/difference_2d.rs +++ b/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) -}