Merge pull request #1084 from hannobraun/approx

Clean up approximation code

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

@@ -9,23 +9,19 @@
 //! done, to give the caller (who knows the boundary anyway) more options on how
 //! to further process the approximation.
 
-use std::cmp::max;
-
-use fj_math::{Circle, Point, Scalar};
-
 use crate::{
-    objects::{Curve, GlobalCurve, Vertex},
-    path::GlobalPath,
+    objects::{Curve, GlobalCurve},
+    path::RangeOnPath,
 };
 
 use super::{Approx, ApproxCache, ApproxPoint, Tolerance};
 
-impl Approx for (&Curve, RangeOnCurve) {
+impl Approx for (&Curve, RangeOnPath) {
     type Approximation = CurveApprox;
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
         let (curve, range) = self;
@@ -39,19 +35,18 @@ impl Approx for (&Curve, RangeOnCurve) {
                     curve.path().point_from_path_coords(point.local_form);
                 ApproxPoint::new(point_surface, point.global_form)
                     .with_source((*curve, point.local_form))
-            })
-            .collect();
+            });
 
-        CurveApprox { points }
+        CurveApprox::empty().with_points(points)
     }
 }
 
-impl Approx for (&GlobalCurve, RangeOnCurve) {
+impl Approx for (&GlobalCurve, RangeOnPath) {
     type Approximation = GlobalCurveApprox;
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
         let (curve, range) = self;
@@ -60,190 +55,37 @@ impl Approx for (&GlobalCurve, RangeOnCurve) {
             return approx;
         }
 
-        let points = match curve.path() {
-            GlobalPath::Circle(circle) => {
-                approx_circle(&circle, range, tolerance)
-            }
-            GlobalPath::Line(_) => vec![],
-        };
-
+        let points = curve.path().approx(range, tolerance);
         cache.insert_global_curve(curve, GlobalCurveApprox { points })
     }
 }
 
-/// Approximate a circle
-///
-/// `tolerance` specifies how much the approximation is allowed to deviate
-/// from the circle.
-fn approx_circle(
-    circle: &Circle<3>,
-    range: impl Into<RangeOnCurve>,
-    tolerance: Tolerance,
-) -> Vec<ApproxPoint<1>> {
-    let mut points = Vec::new();
-
-    let radius = circle.a().magnitude();
-    let range = range.into();
-
-    // To approximate the circle, we use a regular polygon for which
-    // the circle is the circumscribed circle. The `tolerance`
-    // parameter is the maximum allowed distance between the polygon
-    // and the circle. This is the same as the difference between
-    // the circumscribed circle and the incircle.
-
-    let n = number_of_vertices_for_circle(tolerance, radius, range.length());
-
-    for i in 1..n {
-        let angle = range.start().position().t
-            + (Scalar::TAU / n as f64 * i as f64) * range.direction();
-
-        let point_curve = Point::from([angle]);
-        let point_global = circle.point_from_circle_coords(point_curve);
-
-        points.push(ApproxPoint::new(point_curve, point_global));
-    }
-
-    if range.is_reversed() {
-        points.reverse();
-    }
-
-    points
-}
-
-fn number_of_vertices_for_circle(
-    tolerance: Tolerance,
-    radius: Scalar,
-    range: Scalar,
-) -> u64 {
-    let n = (range / (Scalar::ONE - (tolerance.inner() / radius)).acos() / 2.)
-        .ceil()
-        .into_u64();
-
-    max(n, 3)
-}
-
-/// The range on which a curve should be approximated
-#[derive(Clone, Copy, Debug)]
-pub struct RangeOnCurve {
-    boundary: [Vertex; 2],
-    is_reversed: bool,
-}
-
-impl RangeOnCurve {
-    /// 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([a, b]: [Vertex; 2]) -> Self {
-        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 start of the range
-    pub fn start(&self) -> Vertex {
-        self.boundary[0]
-    }
-
-    /// Access the end of the range
-    pub fn end(&self) -> Vertex {
-        self.boundary[1]
-    }
-
-    /// Compute the signed length of the range
-    pub fn signed_length(&self) -> Scalar {
-        (self.end().position() - self.start().position()).t
-    }
-
-    /// Compute the absolute length of the range
-    pub fn length(&self) -> Scalar {
-        self.signed_length().abs()
-    }
-
-    /// Compute the direction of the range
-    ///
-    /// Returns a [`Scalar`] that is zero or +/- one.
-    pub fn direction(&self) -> Scalar {
-        self.signed_length().sign()
-    }
-}
-
 /// An approximation of a [`Curve`]
 #[derive(Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct CurveApprox {
     /// The points that approximate the curve
     pub points: Vec<ApproxPoint<2>>,
 }
 
+impl CurveApprox {
+    /// Create an empty instance of `CurveApprox`
+    pub fn empty() -> Self {
+        Self { points: Vec::new() }
+    }
+
+    /// Add points to the approximation
+    pub fn with_points(
+        mut self,
+        points: impl IntoIterator<Item = ApproxPoint<2>>,
+    ) -> Self {
+        self.points.extend(points);
+        self
+    }
+}
+
 /// An approximation of a [`GlobalCurve`]
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct GlobalCurveApprox {
     /// The points that approximate the curve
     pub points: Vec<ApproxPoint<1>>,
 }
-
-#[cfg(test)]
-mod tests {
-    use fj_math::Scalar;
-
-    use crate::algorithms::approx::Tolerance;
-
-    #[test]
-    fn number_of_vertices_for_circle() {
-        verify_result(50., 100., Scalar::TAU, 3);
-        verify_result(50., 100., Scalar::PI, 3);
-        verify_result(10., 100., Scalar::TAU, 7);
-        verify_result(10., 100., Scalar::PI, 4);
-        verify_result(1., 100., Scalar::TAU, 23);
-        verify_result(1., 100., Scalar::PI, 12);
-
-        fn verify_result(
-            tolerance: impl Into<Tolerance>,
-            radius: impl Into<Scalar>,
-            range: impl Into<Scalar>,
-            n: u64,
-        ) {
-            let tolerance = tolerance.into();
-            let radius = radius.into();
-            let range = range.into();
-
-            assert_eq!(
-                n,
-                super::number_of_vertices_for_circle(tolerance, radius, range)
-            );
-
-            assert!(calculate_error(radius, range, n) <= tolerance.inner());
-            if n > 3 {
-                assert!(
-                    calculate_error(radius, range, n - 1) >= tolerance.inner()
-                );
-            }
-        }
-
-        fn calculate_error(radius: Scalar, range: Scalar, n: u64) -> Scalar {
-            radius - radius * (range / Scalar::from_u64(n) / 2.).cos()
-        }
-    }
-}

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

@@ -15,13 +15,16 @@ impl Approx for &Cycle {
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
+        let tolerance = tolerance.into();
+
         let half_edges = self
             .half_edges()
             .map(|half_edge| half_edge.approx_with_cache(tolerance, cache))
             .collect();
+
         CycleApprox { half_edges }
     }
 }

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

@@ -5,23 +5,20 @@
 //! approximations are usually used to build cycle approximations, and this way,
 //! the caller doesn't have to call with duplicate vertices.
 
-use crate::objects::HalfEdge;
+use crate::{objects::HalfEdge, path::RangeOnPath};
 
-use super::{
-    curve::{CurveApprox, RangeOnCurve},
-    Approx, ApproxCache, ApproxPoint, Tolerance,
-};
+use super::{curve::CurveApprox, Approx, ApproxCache, ApproxPoint, Tolerance};
 
 impl Approx for &HalfEdge {
     type Approximation = HalfEdgeApprox;
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
         let &[a, b] = self.vertices();
-        let range = RangeOnCurve::new([a, b]);
+        let range = RangeOnPath::new([a, b].map(|vertex| vertex.position()));
 
         let first = ApproxPoint::new(
             a.surface_form().position(),

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

@@ -18,9 +18,11 @@ impl Approx for &Faces {
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
+        let tolerance = tolerance.into();
+
         let approx = self
             .into_iter()
             .map(|face| face.approx_with_cache(tolerance, cache))
@@ -67,9 +69,11 @@ impl Approx for &Face {
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
+        let tolerance = tolerance.into();
+
         // Curved faces whose curvature is not fully defined by their edges
         // are not supported yet. For that reason, we can fully ignore `face`'s
         // `surface` field and just pass the edges to `Self::for_edges`.

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

@@ -34,15 +34,15 @@ pub trait Approx: Sized {
     ///
     /// `tolerance` defines how far the approximation is allowed to deviate from
     /// the actual object.
-    fn approx(self, tolerance: Tolerance) -> Self::Approximation {
+    fn approx(self, tolerance: impl Into<Tolerance>) -> Self::Approximation {
         let mut cache = ApproxCache::new();
         self.approx_with_cache(tolerance, &mut cache)
     }
 
     /// Approximate the object, using the provided cache
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation;
 }

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

@@ -11,7 +11,7 @@ impl Approx for &Shell {
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
         self.faces().approx_with_cache(tolerance, cache)

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

@@ -11,7 +11,7 @@ impl Approx for &Sketch {
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
         self.faces().approx_with_cache(tolerance, cache)

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

@@ -11,9 +11,11 @@ impl Approx for &Solid {
 
     fn approx_with_cache(
         self,
-        tolerance: Tolerance,
+        tolerance: impl Into<Tolerance>,
         cache: &mut ApproxCache,
     ) -> Self::Approximation {
+        let tolerance = tolerance.into();
+
         self.shells()
             .flat_map(|shell| shell.approx_with_cache(tolerance, cache))
             .collect()

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

@@ -11,7 +11,8 @@ pub struct Surface {
 
 impl Surface {
     /// Construct a `Surface` from two paths that define its coordinate system
-    pub fn new(u: GlobalPath, v: Vector<3>) -> Self {
+    pub fn new(u: GlobalPath, v: impl Into<Vector<3>>) -> Self {
+        let v = v.into();
         Self { u, v }
     }