diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 18411771d..97b561bb7 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -13,8 +13,7 @@ use super::{CurveApprox, CurveApproxSegment}; /// Cache for curve approximations #[derive(Default)] pub struct CurveApproxCache { - inner: - BTreeMap<(HandleWrapper, CurveBoundary>), CurveApprox>, + inner: BTreeMap, CurveApprox>, } impl CurveApproxCache { @@ -33,15 +32,17 @@ impl CurveApproxCache { let was_already_normalized = boundary.is_normalized(); let normalized_boundary = boundary.normalize(); - self.inner.get(&(curve, normalized_boundary)).cloned().map( - |mut approx| { - if !was_already_normalized { - approx.reverse(); - } + let mut approx = self.inner.get(&curve).cloned()?; - approx - }, - ) + approx + .segments + .retain(|segment| segment.boundary == normalized_boundary); + + if !was_already_normalized { + approx.reverse(); + } + + Some(approx) } /// Insert an approximated segment of the curve into the cache @@ -50,31 +51,236 @@ impl CurveApproxCache { curve: Handle, mut new_segment: CurveApproxSegment, ) -> CurveApproxSegment { + let curve = HandleWrapper::from(curve); + new_segment.normalize(); - // We assume that curve approximation segments never overlap, so so we - // don't have to do any merging of this segment with a possibly existing - // approximation for this curve. - // - // For now, this is a valid assumption, as it matches the uses of this - // method, due to documented limitations elsewhere in the system. - let approx = CurveApprox { - segments: vec![new_segment.clone()], + let existing_approx = self.inner.remove(&curve); + let (approx, segment) = match existing_approx { + Some(mut existing_approx) => { + // An approximation for this curve already exists. We need to + // merge the new segment into it. + + // We assume that approximated curve segments never overlap, + // unless they are completely congruent. As a consequence of + // this, we don't have to do any merging with existing segments + // here. + // + // For now, this is a valid assumption, as it matches the uses + // of this method, due to documented limitations elsewhere in + // the system. + + let mut existing_segment = None; + for segment in existing_approx.segments.iter().cloned() { + if segment.boundary == new_segment.boundary { + existing_segment = Some(segment); + } + } + + let segment = match existing_segment { + Some(segment) => segment, + None => { + existing_approx.segments.push(new_segment.clone()); + 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 + } +} + +#[cfg(test)] +pub mod tests { + use crate::{ + algorithms::approx::{ + curve::{CurveApprox, CurveApproxCache, CurveApproxSegment}, + ApproxPoint, + }, + geometry::CurveBoundary, + objects::Curve, + operations::Insert, + services::Services, + }; + + #[test] + fn insert_curve_already_exists_but_no_segment_merge_necessary() { + let mut services = Services::new(); + + let mut cache = CurveApproxCache::default(); + let curve = Curve::new().insert(&mut services); + + // An approximation of our curve already exists. + cache.insert( + curve.clone(), + CurveApproxSegment { + boundary: CurveBoundary::from([[2.], [3.]]), + points: vec![ + ApproxPoint::new([2.25], [2.25, 2.25, 2.25]), + ApproxPoint::new([2.75], [2.75, 2.75, 2.75]), + ], + }, + ); + + // Here's another approximated segment for the same curve, but i doesn't + // overlap with the already existing one. + let boundary = CurveBoundary::from([[0.], [1.]]); + let segment = CurveApproxSegment { + boundary, + points: vec![ + ApproxPoint::new([0.25], [0.25, 0.25, 0.25]), + ApproxPoint::new([0.75], [0.75, 0.75, 0.75]), + ], + }; + + // When inserting the second segment, we expect to get it back + // unchanged. + let inserted = cache.insert(curve.clone(), segment.clone()); + assert_eq!(inserted, segment); + } + + #[test] + fn insert_congruent_segment_already_exists() { + let mut services = Services::new(); + + let mut cache = CurveApproxCache::default(); + let curve = Curve::new().insert(&mut services); + + // An approximation of our curve already exists. + let boundary = CurveBoundary::from([[0.], [1.]]); + let existing_segment = CurveApproxSegment { + boundary, + points: vec![ + ApproxPoint::new([0.25], [0.25, 0.25, 0.25]), + ApproxPoint::new([0.75], [0.75, 0.75, 0.75]), + ], + }; + cache.insert(curve.clone(), existing_segment.clone()); + + // Here's another approximated segment for the same curve that is + // congruent with the existing one. + let new_segment = CurveApproxSegment { + boundary, + points: vec![ + ApproxPoint::new([0.24], [0.24, 0.24, 0.24]), + ApproxPoint::new([0.76], [0.76, 0.76, 0.76]), + ], + }; + + // When inserting the second segment, we expect to get the original one + // back. + let inserted = cache.insert(curve.clone(), new_segment); + assert_eq!(inserted, existing_segment); + + // Also, the new segment should not have replaced the existing on in the + // cache. + let cached = cache.get(&curve, &boundary); + assert_eq!( + cached, + Some(CurveApprox { + segments: vec![existing_segment] + }) + ); + } + + #[test] + fn get_exact_match() { + let mut services = Services::new(); + + let mut cache = CurveApproxCache::default(); + let curve = Curve::new().insert(&mut services); + + // An approximation of our curve already exists. + cache.insert( + curve.clone(), + CurveApproxSegment { + boundary: CurveBoundary::from([[2.], [3.]]), + points: vec![ + ApproxPoint::new([2.25], [2.25, 2.25, 2.25]), + ApproxPoint::new([2.75], [2.75, 2.75, 2.75]), + ], + }, + ); + + // Here's a second segment that doesn't overlap the existing one. + let boundary = CurveBoundary::from([[0.], [1.]]); + let segment = CurveApproxSegment { + boundary, + points: vec![ + ApproxPoint::new([0.25], [0.25, 0.25, 0.25]), + ApproxPoint::new([0.75], [0.75, 0.75, 0.75]), + ], + }; + cache.insert(curve.clone(), segment.clone()); + + // When asking for an approximation with the same boundary as the second + // segment we added, we expect to get it back exactly. + let cached = cache.get(&curve, &boundary); + assert_eq!( + cached, + Some(CurveApprox { + segments: vec![segment] + }) + ); + } + + #[test] + fn get_exact_match_except_reversed() { + let mut services = Services::new(); + + let mut cache = CurveApproxCache::default(); + let curve = Curve::new().insert(&mut services); + + // An approximation of our curve already exists. + cache.insert( + curve.clone(), + CurveApproxSegment { + boundary: CurveBoundary::from([[2.], [3.]]), + points: vec![ + ApproxPoint::new([2.25], [2.25, 2.25, 2.25]), + ApproxPoint::new([2.75], [2.75, 2.75, 2.75]), + ], + }, + ); + + // Here's a second segment that doesn't overlap the existing one. + let boundary = CurveBoundary::from([[0.], [1.]]); + let segment = CurveApproxSegment { + points: vec![ + ApproxPoint::new([0.25], [0.25, 0.25, 0.25]), + ApproxPoint::new([0.75], [0.75, 0.75, 0.75]), + ], + boundary, }; + cache.insert(curve.clone(), segment.clone()); - self.inner - .insert((curve.into(), new_segment.boundary), approx) - .map(|approx| { - let mut segments = approx.segments.into_iter(); - let segment = segments - .next() - .expect("Empty approximations should not exist in cache"); - assert!( - segments.next().is_none(), - "Cached approximations should have exactly 1 segment" - ); - segment + // When asking for an approximation with the same boundary of the second + // segment we added but reversed, we expect to get back the segment, but + // reversed. + let cached = cache.get(&curve, &boundary.reverse()); + assert_eq!( + cached, + Some(CurveApprox { + segments: vec![{ + let mut segment = segment; + segment.reverse(); + segment + }] }) - .unwrap_or(new_segment) + ); } }