diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 63bbca52e..4e8b1e52d 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/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(), diff --git a/crates/fj-kernel/src/algorithms/approx/path.rs b/crates/fj-kernel/src/algorithms/approx/path.rs index db2191364..ef704642a 100644 --- a/crates/fj-kernel/src/algorithms/approx/path.rs +++ b/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,49 +59,8 @@ 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>; 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 From<[T; 2]> for RangeOnPath @@ -107,7 +68,8 @@ where T: Into>, { fn from(boundary: [T; 2]) -> Self { - Self::new(boundary) + let boundary = boundary.map(Into::into); + Self { boundary } } } @@ -130,10 +92,6 @@ fn approx_circle( points.push((point_curve, point_global)); } - if range.is_reversed() { - points.reverse(); - } - points } @@ -168,29 +126,37 @@ impl PathApproxParams { &self, range: impl Into, ) -> impl Iterator> + '_ { - 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, expected_coords: impl IntoIterator>, ) { - // 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);