Merge pull request #2042 from hannobraun/approx

Keep boundary and points separately in `CurveApprox`

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

@@ -4,13 +4,13 @@ use fj_math::Point;
 
 use crate::geometry::CurveBoundary;
 
-use super::CurveApproxSegment;
+use super::{CurveApproxPoints, CurveApproxSegment};
 
 /// Partial approximation of a curve
 #[derive(Clone, Debug, Default, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct CurveApprox {
     /// The approximated segments that are part of this approximation
-    pub segments: Vec<CurveApproxSegment>,
+    pub segments: Vec<(CurveBoundary<Point<1>>, CurveApproxPoints)>,
 }
 
 impl CurveApprox {
@@ -18,7 +18,8 @@ impl CurveApprox {
     pub fn reverse(&mut self) -> &mut Self {
         self.segments.reverse();
 
-        for segment in &mut self.segments {
+        for (boundary, segment) in &mut self.segments {
+            *boundary = boundary.reverse();
             segment.reverse();
         }
 
@@ -27,11 +28,12 @@ impl CurveApprox {
 
     /// Reduce the approximation to the subset defined by the provided boundary
     pub fn make_subset(&mut self, boundary: CurveBoundary<Point<1>>) {
-        for segment in &mut self.segments {
+        for (b, segment) in &mut self.segments {
+            *b = b.subset(boundary);
             segment.make_subset(boundary.normalize());
         }
 
-        self.segments.retain(|segment| !segment.is_empty());
+        self.segments.retain(|(_, segment)| !segment.is_empty());
     }
 
     /// Merge the provided segment into the approximation
@@ -43,11 +45,11 @@ impl CurveApprox {
 
         let mut i = 0;
         loop {
-            let Some(segment) = self.segments.get(i) else {
+            let Some((boundary, _)) = self.segments.get(i) else {
                 break;
             };
 
-            if segment.overlaps(&new_segment) {
+            if boundary.overlaps(&new_segment.boundary) {
                 let segment = self.segments.swap_remove(i);
                 overlapping_segments.push_back(segment);
                 continue;
@@ -56,21 +58,37 @@ impl CurveApprox {
             i += 1;
         }
 
-        let mut merged_segment = new_segment;
-        for segment in overlapping_segments {
-            merged_segment.merge(&segment);
+        let mut merged_boundary = new_segment.boundary;
+        let mut merged_segment = new_segment.points;
+
+        for (boundary, segment) in overlapping_segments {
+            assert!(
+                merged_boundary.overlaps(&boundary),
+                "Shouldn't merge segments that don't overlap."
+            );
+
+            merged_boundary = merged_boundary.union(boundary);
+            merged_segment.merge(&segment, boundary);
         }
 
-        self.segments.push(merged_segment.clone());
+        self.segments
+            .push((merged_boundary, merged_segment.clone()));
         self.segments.sort();
-        merged_segment
+
+        CurveApproxSegment {
+            boundary: merged_boundary,
+            points: merged_segment,
+        }
     }
 }
 
 impl<const N: usize> From<[CurveApproxSegment; N]> for CurveApprox {
     fn from(segments: [CurveApproxSegment; N]) -> Self {
         Self {
-            segments: segments.into_iter().collect(),
+            segments: segments
+                .into_iter()
+                .map(|segment| (segment.boundary, segment.points))
+                .collect(),
         }
     }
 }

crates/fj-core/src/algorithms/approx/curve/mod.rs

@@ -2,8 +2,10 @@
 
 mod approx;
 mod cache;
+mod points;
 mod segment;
 
 pub use self::{
-    approx::CurveApprox, cache::CurveApproxCache, segment::CurveApproxSegment,
+    approx::CurveApprox, cache::CurveApproxCache, points::CurveApproxPoints,
+    segment::CurveApproxSegment,
 };

crates/fj-core/src/algorithms/approx/curve/points.rs

@@ -0,0 +1,47 @@
+use fj_math::Point;
+
+use crate::{algorithms::approx::ApproxPoint, geometry::CurveBoundary};
+
+/// Points of a curve approximation
+#[derive(Clone, Debug, Default, Eq, PartialEq, Hash, Ord, PartialOrd)]
+pub struct CurveApproxPoints {
+    /// Points of a curve approximation
+    pub inner: Vec<ApproxPoint<1>>,
+}
+
+impl CurveApproxPoints {
+    /// Indicate whether there are any points
+    pub fn is_empty(&self) -> bool {
+        self.inner.is_empty()
+    }
+
+    /// Reverse the orientation of the approximation
+    pub fn reverse(&mut self) -> &mut Self {
+        self.inner.reverse();
+        self
+    }
+
+    /// Reduce the approximation to the subset defined by the provided boundary
+    pub fn make_subset(&mut self, boundary: CurveBoundary<Point<1>>) {
+        self.inner
+            .retain(|point| boundary.contains(point.local_form));
+    }
+
+    /// Merge the provided points
+    ///
+    /// If there is a true overlap between these points and the other points
+    /// then the overlapping part is taken from the other points.
+    pub fn merge(
+        &mut self,
+        other: &Self,
+        other_boundary: CurveBoundary<Point<1>>,
+    ) {
+        self.inner.retain(|point| {
+            // Only retain points that don't overlap with the other points, or
+            // we might end up with duplicates.
+            !other_boundary.contains(point.local_form)
+        });
+        self.inner.extend(&other.inner);
+        self.inner.sort();
+    }
+}

crates/fj-core/src/algorithms/approx/curve/segment.rs

@@ -4,6 +4,8 @@ use fj_math::Point;
 
 use crate::{algorithms::approx::ApproxPoint, geometry::CurveBoundary};
 
+use super::points::CurveApproxPoints;
+
 /// A segment of a curve approximation
 ///
 /// A curve is potentially infinite (at least its local coordinate space is
@@ -16,37 +18,15 @@ pub struct CurveApproxSegment {
     pub boundary: CurveBoundary<Point<1>>,
 
     /// The points that approximate the curve segment
-    pub points: Vec<ApproxPoint<1>>,
+    pub points: CurveApproxPoints,
 }
 
 impl CurveApproxSegment {
-    /// Indicate whether the segment is empty
-    pub fn is_empty(&self) -> bool {
-        let is_empty = self.boundary.is_empty();
-
-        if is_empty {
-            assert!(
-                self.points.is_empty(),
-                "Empty approximation still has points"
-            );
-        }
-
-        is_empty
-    }
-
     /// Indicate whether the segment is normalized
     pub fn is_normalized(&self) -> bool {
         self.boundary.is_normalized()
     }
 
-    /// Indicate whether this segment overlaps another
-    ///
-    /// Segments that touch (i.e. their closest boundary is equal) count as
-    /// overlapping.
-    pub fn overlaps(&self, other: &Self) -> bool {
-        self.boundary.overlaps(&other.boundary)
-    }
-
     /// Reverse the orientation of the approximation
     pub fn reverse(&mut self) -> &mut Self {
         self.boundary = self.boundary.reverse();
@@ -61,54 +41,11 @@ impl CurveApproxSegment {
     pub fn normalize(&mut self) -> &mut Self {
         if !self.is_normalized() {
             self.boundary = self.boundary.normalize();
-            self.points.reverse();
+            self.points.inner.reverse();
         }
 
         self
     }
-
-    /// Reduce the approximation to the subset defined by the provided boundary
-    pub fn make_subset(&mut self, boundary: CurveBoundary<Point<1>>) {
-        assert!(
-            self.is_normalized(),
-            "Expected normalized segment for making subset."
-        );
-        assert!(
-            boundary.is_normalized(),
-            "Expected subset to be defined by normalized boundary."
-        );
-
-        self.boundary = self.boundary.subset(boundary);
-        self.points
-            .retain(|point| self.boundary.contains(point.local_form));
-    }
-
-    /// Merge the provided segment into this one
-    ///
-    /// It there is a true overlap between both segments (as opposed to them
-    /// just touching), then the overlapping part is taken from the other
-    /// segment, meaning parts of this one get overwritten.
-    pub fn merge(&mut self, other: &Self) {
-        assert!(
-            self.overlaps(other),
-            "Shouldn't merge segments that don't overlap."
-        );
-        assert!(
-            self.is_normalized(),
-            "Can't merge into non-normalized segment."
-        );
-        assert!(other.is_normalized(), "Can't merge non-normalized segment.");
-
-        self.boundary = self.boundary.union(other.boundary);
-
-        self.points.retain(|point| {
-            // Only retain points that don't overlap with the other segment, or
-            // we might end up with duplicates.
-            !other.boundary.contains(point.local_form)
-        });
-        self.points.extend(&other.points);
-        self.points.sort();
-    }
 }
 
 impl Ord for CurveApproxSegment {
@@ -139,7 +76,9 @@ impl<const D: usize> From<(CurveBoundary<Point<1>>, [ApproxPoint<1>; D])>
     ) -> Self {
         Self {
             boundary,
-            points: points.into_iter().collect(),
+            points: CurveApproxPoints {
+                inner: points.into_iter().collect(),
+            },
         }
     }
 }

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

@@ -16,7 +16,9 @@ use crate::{
 };
 
 use super::{
-    curve::{CurveApprox, CurveApproxCache, CurveApproxSegment},
+    curve::{
+        CurveApprox, CurveApproxCache, CurveApproxPoints, CurveApproxSegment,
+    },
     Approx, ApproxPoint, Tolerance,
 };
 
@@ -54,11 +56,11 @@ impl Approx for (&Edge, &Surface) {
                     .get_curve_approx(edge.curve().clone(), edge.boundary());
 
                 match cached.segments.pop() {
-                    Some(segment) if cached.segments.is_empty() => {
+                    Some((boundary, points)) if cached.segments.is_empty() => {
                         // If the cached approximation has a single segment,
                         // that means everything we need is available, and we
                         // can use the cached approximation as-is.
-                        segment
+                        CurveApproxSegment { boundary, points }
                     }
                     _ => {
                         // If we make it here, there are holes in the
@@ -85,6 +87,7 @@ impl Approx for (&Edge, &Surface) {
 
             segment
                 .points
+                .inner
                 .into_iter()
                 .map(|point| {
                     let point_surface =
@@ -194,7 +197,10 @@ fn approx_curve(
             ApproxPoint::new(point_curve, point_global)
         })
         .collect();
-    CurveApproxSegment { boundary, points }
+    CurveApproxSegment {
+        boundary,
+        points: CurveApproxPoints { inner: points },
+    }
 }
 
 /// Cache for edge approximations