Merge pull request #2020 from hannobraun/approx

Finish work on curve approximation cache

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

@@ -1,11 +1,13 @@
+use std::collections::VecDeque;
+
 use fj_math::Point;
 
 use crate::geometry::CurveBoundary;
 
 use super::CurveApproxSegment;
 
 /// Partial approximation of a curve
-#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
+#[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>,
@@ -37,18 +39,31 @@ impl CurveApprox {
         &mut self,
         new_segment: CurveApproxSegment,
     ) -> CurveApproxSegment {
-        let mut existing_segment = None;
-        for segment in &mut self.segments {
+        let mut overlapping_segments = VecDeque::new();
+
+        let mut i = 0;
+        loop {
+            let Some(segment) = self.segments.get(i) else {
+                break;
+            };
+
             if segment.overlaps(&new_segment) {
-                segment.merge(&new_segment);
-                existing_segment = Some(segment.clone());
+                let segment = self.segments.swap_remove(i);
+                overlapping_segments.push_back(segment);
+                continue;
             }
+
+            i += 1;
+        }
+
+        let mut merged_segment = new_segment;
+        for segment in overlapping_segments {
+            merged_segment.merge(&segment);
         }
 
-        existing_segment.unwrap_or_else(|| {
-            self.segments.push(new_segment.clone());
-            new_segment
-        })
+        self.segments.push(merged_segment.clone());
+        self.segments.sort();
+        merged_segment
     }
 }
 

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

@@ -52,25 +52,7 @@ impl CurveApproxCache {
         // approximated segment before doing *anything* with it.
         new_segment.normalize();
 
-        let (approx, segment) = match self.inner.remove(&curve) {
-            Some(mut existing_approx) => {
-                let segment = existing_approx.merge(new_segment);
-                (existing_approx, segment)
-            }
-            None => {
-                // No approximation for this curve exists. We need to create a
-                // new one.
-                let new_approx = CurveApprox {
-                    segments: vec![new_segment.clone()],
-                };
-
-                (new_approx, new_segment)
-            }
-        };
-
-        self.inner.insert(curve, approx);
-
-        segment
+        self.inner.entry(curve).or_default().merge(new_segment)
     }
 }
 
@@ -167,6 +149,123 @@ pub mod tests {
         );
     }
 
+    #[test]
+    fn insert_merge_non_overlapping_segments() {
+        let mut services = Services::new();
+
+        let mut cache = CurveApproxCache::default();
+        let curve = Curve::new().insert(&mut services);
+
+        // Insert segments that are not overlapping. The segments themselves
+        // don't need to be merged, but both segments still need to be merged
+        // into the same curve approximation.
+        //
+        // Let's make sure they are out of order, to make sure that is taken
+        // care of when doing the merge.
+        cache.insert(
+            curve.clone(),
+            CurveApproxSegment {
+                boundary: CurveBoundary::from([[0.75], [1.]]),
+                points: vec![ApproxPoint::new([0.875], [0.875, 0.875, 0.875])],
+            },
+        );
+        cache.insert(
+            curve.clone(),
+            CurveApproxSegment {
+                boundary: CurveBoundary::from([[0.], [0.25]]),
+                points: vec![ApproxPoint::new([0.125], [0.125, 0.125, 0.125])],
+            },
+        );
+
+        let cached = cache.get(&curve, &CurveBoundary::from([[0.], [1.]]));
+        assert_eq!(
+            cached,
+            Some(CurveApprox {
+                segments: vec![
+                    CurveApproxSegment {
+                        boundary: CurveBoundary::from([[0.], [0.25]]),
+                        points: vec![ApproxPoint::new(
+                            [0.125],
+                            [0.125, 0.125, 0.125]
+                        )],
+                    },
+                    CurveApproxSegment {
+                        boundary: CurveBoundary::from([[0.75], [1.]]),
+                        points: vec![ApproxPoint::new(
+                            [0.875],
+                            [0.875, 0.875, 0.875]
+                        )],
+                    }
+                ]
+            })
+        );
+    }
+
+    #[test]
+    fn insert_merge_overlapping_segments() {
+        let mut services = Services::new();
+
+        let mut cache = CurveApproxCache::default();
+        let curve = Curve::new().insert(&mut services);
+
+        // Insert two non-overlapping segments to prepare for the actual test.
+        // Make sure they are not normalized and out of order, to exercise that
+        // functionality.
+        cache.insert(
+            curve.clone(),
+            CurveApproxSegment {
+                boundary: CurveBoundary::from([[1.5], [1.0]]),
+                points: vec![
+                    ApproxPoint::new([1.375], [1.375, 1.375, 1.375]),
+                    ApproxPoint::new([1.125], [1.125, 1.125, 1.125]),
+                ],
+            },
+        );
+        cache.insert(
+            curve.clone(),
+            CurveApproxSegment {
+                boundary: CurveBoundary::from([[0.5], [0.]]),
+                points: vec![
+                    ApproxPoint::new([0.375], [0.375, 0.375, 0.375]),
+                    ApproxPoint::new([0.125], [0.125, 0.125, 0.125]),
+                ],
+            },
+        );
+
+        // Now insert a third segment that overlaps both of them (touching
+        // counts as overlapping). They should all get merged into a single
+        // segment.
+        cache.insert(
+            curve.clone(),
+            CurveApproxSegment {
+                boundary: CurveBoundary::from([[1.0], [0.5]]),
+                points: vec![
+                    ApproxPoint::new([0.875], [0.875, 0.875, 0.875]),
+                    ApproxPoint::new([0.625], [0.625, 0.625, 0.625]),
+                ],
+            },
+        );
+
+        let boundary = CurveBoundary::from([[0.], [1.5]]);
+        let cached = cache.get(&curve, &boundary);
+        assert_eq!(
+            cached,
+            Some(CurveApprox {
+                segments: vec![CurveApproxSegment {
+                    boundary,
+                    points: vec![
+                        ApproxPoint::new([0.125], [0.125, 0.125, 0.125]),
+                        ApproxPoint::new([0.375], [0.375, 0.375, 0.375]),
+                        ApproxPoint::new([0.625], [0.625, 0.625, 0.625]),
+                        ApproxPoint::new([0.875], [0.875, 0.875, 0.875]),
+                        ApproxPoint::new([1.125], [1.125, 1.125, 1.125]),
+                        ApproxPoint::new([1.375], [1.375, 1.375, 1.375]),
+                    ],
+                }]
+            })
+        );
+    }
+
     #[test]
     fn get_exact_match() {
         let mut services = Services::new();
@@ -255,7 +354,7 @@ pub mod tests {
     }
 
     #[test]
-    fn partial_match_that_overlaps_start() {
+    fn get_partial_match_that_overlaps_start() {
         let mut services = Services::new();
 
         let mut cache = CurveApproxCache::default();
@@ -291,7 +390,7 @@ pub mod tests {
     }
 
     #[test]
-    fn partial_match_that_overlaps_end() {
+    fn get_partial_match_that_overlaps_end() {
         let mut services = Services::new();
 
         let mut cache = CurveApproxCache::default();
@@ -327,7 +426,7 @@ pub mod tests {
     }
 
     #[test]
-    fn partial_match_in_the_middle() {
+    fn get_partial_match_in_the_middle() {
         let mut services = Services::new();
 
         let mut cache = CurveApproxCache::default();
@@ -361,86 +460,4 @@ pub mod tests {
             })
         );
     }
-
-    #[test]
-    fn merge_insertions() {
-        let mut services = Services::new();
-
-        let mut cache = CurveApproxCache::default();
-        let curve = Curve::new().insert(&mut services);
-
-        cache.insert(
-            curve.clone(),
-            CurveApproxSegment {
-                boundary: CurveBoundary::from([[0.5], [1.]]),
-                points: vec![ApproxPoint::new([0.75], [0.75, 0.75, 0.75])],
-            },
-        );
-        cache.insert(
-            curve.clone(),
-            CurveApproxSegment {
-                boundary: CurveBoundary::from([[0.], [0.5]]),
-                points: vec![ApproxPoint::new([0.25], [0.25, 0.25, 0.25])],
-            },
-        );
-
-        let cached = cache.get(&curve, &CurveBoundary::from([[0.], [1.]]));
-        assert_eq!(
-            cached,
-            Some(CurveApprox {
-                segments: vec![CurveApproxSegment {
-                    boundary: CurveBoundary::from([[0.], [1.]]),
-                    points: vec![
-                        ApproxPoint::new([0.25], [0.25, 0.25, 0.25]),
-                        ApproxPoint::new([0.75], [0.75, 0.75, 0.75])
-                    ],
-                }]
-            })
-        );
-    }
-
-    #[test]
-    fn merge_insertions_that_are_not_normalized() {
-        let mut services = Services::new();
-
-        let mut cache = CurveApproxCache::default();
-        let curve = Curve::new().insert(&mut services);
-
-        cache.insert(
-            curve.clone(),
-            CurveApproxSegment {
-                boundary: CurveBoundary::from([[1.], [0.5]]),
-                points: vec![
-                    ApproxPoint::new([0.875], [0.875, 0.875, 0.875]),
-                    ApproxPoint::new([0.625], [0.625, 0.625, 0.625]),
-                ],
-            },
-        );
-        cache.insert(
-            curve.clone(),
-            CurveApproxSegment {
-                boundary: CurveBoundary::from([[0.5], [0.]]),
-                points: vec![
-                    ApproxPoint::new([0.375], [0.375, 0.375, 0.375]),
-                    ApproxPoint::new([0.125], [0.125, 0.125, 0.125]),
-                ],
-            },
-        );
-
-        let cached = cache.get(&curve, &CurveBoundary::from([[0.], [1.]]));
-        assert_eq!(
-            cached,
-            Some(CurveApprox {
-                segments: vec![CurveApproxSegment {
-                    boundary: CurveBoundary::from([[0.], [1.]]),
-                    points: vec![
-                        ApproxPoint::new([0.125], [0.125, 0.125, 0.125]),
-                        ApproxPoint::new([0.375], [0.375, 0.375, 0.375]),
-                        ApproxPoint::new([0.625], [0.625, 0.625, 0.625]),
-                        ApproxPoint::new([0.875], [0.875, 0.875, 0.875]),
-                    ],
-                }]
-            })
-        );
-    }
 }

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

@@ -100,6 +100,10 @@ impl CurveApproxSegment {
     }
 
     /// 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),
@@ -119,12 +123,12 @@ impl CurveApproxSegment {
 
         self.boundary.inner = [min, max];
 
-        self.points
-            .extend(other.points.iter().copied().filter(|point| {
-                // Only add points that come from `other`. Otherwise we might
-                // end up with duplicate points.
-                point.local_form < a_min || point.local_form > a_max
-            }));
+        self.points.retain(|point| {
+            // Only retain points that don't overlap with the other segment, or
+            // we might end up with duplicates.
+            point.local_form < b_min || point.local_form > b_max
+        });
+        self.points.extend(&other.points);
         self.points.sort();
     }
 }