diff --git a/crates/fj-core/src/algorithms/approx/curve/approx.rs b/crates/fj-core/src/algorithms/approx/curve/approx.rs index d5f6264bb..a966bda3a 100644 --- a/crates/fj-core/src/algorithms/approx/curve/approx.rs +++ b/crates/fj-core/src/algorithms/approx/curve/approx.rs @@ -1,3 +1,5 @@ +use std::collections::VecDeque; + use fj_math::Point; use crate::geometry::CurveBoundary; @@ -5,7 +7,7 @@ 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, @@ -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 } } diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 470747e09..28d7dc6d4 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/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]), - ], - }] - }) - ); - } } diff --git a/crates/fj-core/src/algorithms/approx/curve/segment.rs b/crates/fj-core/src/algorithms/approx/curve/segment.rs index 37b4cf780..05a3dd6e6 100644 --- a/crates/fj-core/src/algorithms/approx/curve/segment.rs +++ b/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(); } }