Merge pull request #1094 from hannobraun/path

Dumb down `RangeOnPath`

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

@@ -23,7 +23,8 @@ impl Approx for &HalfEdge {
         cache: &mut Self::Cache,
     ) -> Self::Approximation {
         let &[a, b] = self.vertices();
-        let range = RangeOnPath::new([a, b].map(|vertex| vertex.position()));
+        let boundary = [a, b].map(|vertex| vertex.position());
+        let range = RangeOnPath { boundary };
 
         let first = ApproxPoint::new(
             a.surface_form().position(),

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

@@ -28,7 +28,9 @@
 //! fit together in a valid mesh, no matter which ranges of a path are being
 //! approximated, and how many times.
 
-use fj_math::{Circle, Point, Scalar};
+use std::iter;
+
+use fj_math::{Circle, Point, Scalar, Sign};
 
 use crate::path::GlobalPath;
 
@@ -57,57 +59,17 @@ impl Approx for (GlobalPath, RangeOnPath) {
 /// The range on which a path should be approximated
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)]
 pub struct RangeOnPath {
-    boundary: [Point<1>; 2],
-    is_reversed: bool,
-}
-
-impl RangeOnPath {
-    /// Construct an instance of `RangeOnCurve`
-    ///
-    /// Ranges are normalized on construction, meaning that the order of
-    /// vertices passed to this constructor does not influence the range that is
-    /// constructed.
-    ///
-    /// This is done to prevent bugs during mesh construction: The curve
-    /// approximation code is regularly faced with ranges that are reversed
-    /// versions of each other. This can lead to slightly different
-    /// approximations, which in turn leads to the aforementioned invalid
-    /// meshes.
-    ///
-    /// The caller can use `is_reversed` to determine, if the range was reversed
-    /// during normalization, to adjust the approximation accordingly.
-    pub fn new(boundary: [impl Into<Point<1>>; 2]) -> Self {
-        let [a, b] = boundary.map(Into::into);
-
-        let (boundary, is_reversed) = if a < b {
-            ([a, b], false)
-        } else {
-            ([b, a], true)
-        };
-
-        Self {
-            boundary,
-            is_reversed,
-        }
-    }
-
-    /// Indicate whether the range was reversed during normalization
-    pub fn is_reversed(&self) -> bool {
-        self.is_reversed
-    }
-
-    /// Access the boundary of the range
-    pub fn boundary(&self) -> [Point<1>; 2] {
-        self.boundary
-    }
+    /// The boundary of the range
+    pub boundary: [Point<1>; 2],
 }
 
 impl<T> From<[T; 2]> for RangeOnPath
 where
     T: Into<Point<1>>,
 {
     fn from(boundary: [T; 2]) -> Self {
-        Self::new(boundary)
+        let boundary = boundary.map(Into::into);
+        Self { boundary }
     }
 }
 
@@ -130,10 +92,6 @@ fn approx_circle<const D: usize>(
         points.push((point_curve, point_global));
     }
 
-    if range.is_reversed() {
-        points.reverse();
-    }
-
     points
 }
 
@@ -168,29 +126,37 @@ impl PathApproxParams {
         &self,
         range: impl Into<RangeOnPath>,
     ) -> impl Iterator<Item = Point<1>> + '_ {
-        use std::iter;
-
         let range = range.into();
 
         let [a, b] = range.boundary.map(|point| point.t / self.increment());
+        let direction = (b - a).sign();
+        let [min, max] = if a < b { [a, b] } else { [b, a] };
 
-        // We can't generate a point exactly at the end of the range as part of
-        // the approximation. Make sure we stop one step before that.
-        let b = if b.ceil() == b { b - 1. } else { b };
+        // We can't generate a point exactly at the boundaries of the range as
+        // part of the approximation. Make sure we stay inside the range.
+        let min = min.floor() + 1.;
+        let max = max.ceil() - 1.;
 
-        let start = a.floor() + 1.;
-        let end = b - 1.;
+        let [start, end] = match direction {
+            Sign::Negative => [max, min],
+            Sign::Positive | Sign::Zero => [min, max],
+        };
 
         let mut i = start;
         iter::from_fn(move || {
-            if i <= end {
-                let t = self.increment() * i;
-                i += Scalar::ONE;
+            let is_finished = match direction {
+                Sign::Negative => i < end,
+                Sign::Positive | Sign::Zero => i > end,
+            };
 
-                Some(Point::from([t]))
-            } else {
-                None
+            if is_finished {
+                return None;
             }
+
+            let t = self.increment() * i;
+            i += direction.to_scalar();
+
+            Some(Point::from([t]))
         })
     }
 }
@@ -226,26 +192,39 @@ mod tests {
 
     #[test]
     fn points_for_circle() {
-        // Needed to support type inference.
+        // At the chosen values for radius and tolerance (see below), the
+        // increment is `PI / 4`, so ~1.57.
+
+        // Empty range
         let empty: [Scalar; 0] = [];
+        test_path([[0.], [0.]], empty);
+
+        // Ranges contain all generated points. Start is before the first
+        // increment and after the last one in each case.
+        test_path([[0.], [TAU]], [1., 2., 3.]);
+        test_path([[1.], [TAU]], [1., 2., 3.]);
+        test_path([[0.], [TAU - 1.]], [1., 2., 3.]);
+
+        // Here the range is restricted to cut of the first or last increment.
+        test_path([[2.], [TAU]], [2., 3.]);
+        test_path([[0.], [TAU - 2.]], [1., 2.]);
 
-        // At the chosen values, the increment is ~1.25.
-        test_path([[0.], [0.]], empty); // empty range
-        test_path([[0.], [TAU]], [1., 2., 3.]); // start before first increment
-        test_path([[1.], [TAU]], [1., 2., 3.]); // start before first increment
-        test_path([[0.], [TAU - 1.]], [1., 2., 3.]); // end after last increment
-        test_path([[2.], [TAU]], [2., 3.]); // start after first increment
-        test_path([[0.], [TAU - 2.]], [1., 2.]); // end before last increment
+        // And everything again, but in reverse.
+        test_path([[TAU], [0.]], [3., 2., 1.]);
+        test_path([[TAU], [1.]], [3., 2., 1.]);
+        test_path([[TAU - 1.], [0.]], [3., 2., 1.]);
+        test_path([[TAU], [2.]], [3., 2.]);
+        test_path([[TAU - 2.], [0.]], [2., 1.]);
 
         fn test_path(
             range: impl Into<RangeOnPath>,
             expected_coords: impl IntoIterator<Item = impl Into<Scalar>>,
         ) {
-            // Choose radius and tolerance such, that we need 5 vertices to
+            // Choose radius and tolerance such, that we need 4 vertices to
             // approximate a full circle. This is the lowest number that we can
             // still cover all the edge cases with
             let radius = 1.;
-            let tolerance = 0.25;
+            let tolerance = 0.375;
 
             let circle = Circle::from_center_and_radius([0., 0.], radius);
             let params = PathApproxParams::for_circle(&circle, tolerance);