From c929c9198bcdb1227daf38414caa974290ee1777 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 09:59:12 +0200 Subject: [PATCH 01/12] Add unit tests for `CurveApproxCache` --- .../src/algorithms/approx/curve/cache.rs | 171 ++++++++++++++++++ 1 file changed, 171 insertions(+) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 18411771d..8fd4e2216 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -78,3 +78,174 @@ impl CurveApproxCache { .unwrap_or(new_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); + } + + #[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()); + + // 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 + }] + }) + ); + } +} From ff2682d22746ba7f11a4861205bce645a4c23922 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 10:20:46 +0200 Subject: [PATCH 02/12] Refactor to improve clarity --- crates/fj-core/src/algorithms/approx/curve/cache.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 8fd4e2216..3d42613b1 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -50,6 +50,8 @@ 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 @@ -63,7 +65,7 @@ impl CurveApproxCache { }; self.inner - .insert((curve.into(), new_segment.boundary), approx) + .insert((curve, new_segment.boundary), approx) .map(|approx| { let mut segments = approx.segments.into_iter(); let segment = segments From 483a500f3356ae7f4c10e4f663bd3cb15159dc12 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 10:22:32 +0200 Subject: [PATCH 03/12] Fix typo --- crates/fj-core/src/algorithms/approx/curve/cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 3d42613b1..e266dd494 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -54,7 +54,7 @@ impl CurveApproxCache { new_segment.normalize(); - // We assume that curve approximation segments never overlap, so so we + // We assume that curve approximation segments never overlap, so we // don't have to do any merging of this segment with a possibly existing // approximation for this curve. // From bd566dfb02464681c6eb412ad3d48c6dd545e015 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 10:25:12 +0200 Subject: [PATCH 04/12] Improve wording in comment --- crates/fj-core/src/algorithms/approx/curve/cache.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index e266dd494..5590a8998 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -54,8 +54,8 @@ impl CurveApproxCache { new_segment.normalize(); - // We assume that curve approximation segments never overlap, so we - // don't have to do any merging of this segment with a possibly existing + // We assume that approximated curve segments never overlap, 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 From 6a9181751a8fca9e9c0148ddb127755a3b1f5db2 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 10:49:01 +0200 Subject: [PATCH 05/12] Refactor to prepare for follow-on change --- crates/fj-core/src/algorithms/approx/curve/cache.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 5590a8998..17019347e 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -51,6 +51,7 @@ impl CurveApproxCache { mut new_segment: CurveApproxSegment, ) -> CurveApproxSegment { let curve = HandleWrapper::from(curve); + let cache_key = (curve, new_segment.boundary); new_segment.normalize(); @@ -65,7 +66,7 @@ impl CurveApproxCache { }; self.inner - .insert((curve, new_segment.boundary), approx) + .insert(cache_key, approx) .map(|approx| { let mut segments = approx.segments.into_iter(); let segment = segments From 3684c8ef80f9c548dbaf549a5e2a567df0689a6c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 10:50:09 +0200 Subject: [PATCH 06/12] Fix curve cache insertion bug Curve insertion *returned* the correct curve segment, which means either the existing segment, if there is one, or the new one being inserted. But it actually *inserted* the wrong one, i.e. the new one. I don't think this lead to any bugs, due to the current usage patterns of the cache API, but it's wrong none the less, and a bug waiting to happen. --- .../src/algorithms/approx/curve/cache.rs | 64 +++++++++++++------ 1 file changed, 46 insertions(+), 18 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 17019347e..3025437a5 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -55,30 +55,48 @@ impl CurveApproxCache { new_segment.normalize(); - // We assume that approximated curve segments never overlap, 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()], - }; - - self.inner - .insert(cache_key, approx) - .map(|approx| { - let mut segments = approx.segments.into_iter(); - let segment = segments + let existing_approx = self.inner.remove(&cache_key); + let (approx, segment) = match existing_approx { + Some(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. As + // a consequence of this, and the current structure of the + // cache, we can assume that the existing approximation + // contains exactly one segment that is congruent with the one + // we are meant to insert. This means we can just extract that + // segment and return it to the caller. + // + // 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 segments = approx.segments.iter().cloned(); + let existing_segment = segments .next() .expect("Empty approximations should not exist in cache"); assert!( segments.next().is_none(), "Cached approximations should have exactly 1 segment" ); - segment - }) - .unwrap_or(new_segment) + + (approx, existing_segment) + } + None => { + // No approximation for this curve exists. We need to create a + // new one. + let approx = CurveApprox { + segments: vec![new_segment.clone()], + }; + + (approx, new_segment) + } + }; + + self.inner.insert(cache_key, approx); + + segment } } @@ -163,6 +181,16 @@ pub mod tests { // 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] From b4d98d21fabfdde95cac428da249a29b3784e143 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 10:59:44 +0200 Subject: [PATCH 07/12] Rename variable to be more explicit --- crates/fj-core/src/algorithms/approx/curve/cache.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 3025437a5..8e27f04b7 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -57,7 +57,7 @@ impl CurveApproxCache { let existing_approx = self.inner.remove(&cache_key); let (approx, segment) = match existing_approx { - Some(approx) => { + Some(existing_approx) => { // An approximation for this curve already exists. We need to // merge the new segment into it. @@ -72,7 +72,7 @@ impl CurveApproxCache { // of this method, due to documented limitations elsewhere in // the system. - let mut segments = approx.segments.iter().cloned(); + let mut segments = existing_approx.segments.iter().cloned(); let existing_segment = segments .next() .expect("Empty approximations should not exist in cache"); @@ -81,7 +81,7 @@ impl CurveApproxCache { "Cached approximations should have exactly 1 segment" ); - (approx, existing_segment) + (existing_approx, existing_segment) } None => { // No approximation for this curve exists. We need to create a From 2b0315e75a96e13a71074ca615a1409cf484addc Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 11:00:05 +0200 Subject: [PATCH 08/12] Rename variable to be more explicit --- crates/fj-core/src/algorithms/approx/curve/cache.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 8e27f04b7..9289ac348 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -86,11 +86,11 @@ impl CurveApproxCache { None => { // No approximation for this curve exists. We need to create a // new one. - let approx = CurveApprox { + let new_approx = CurveApprox { segments: vec![new_segment.clone()], }; - (approx, new_segment) + (new_approx, new_segment) } }; From 0c6d72dd1609d3844cf811f937694cbf795fb6bd Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 11:21:00 +0200 Subject: [PATCH 09/12] Fix wording in comment --- crates/fj-core/src/algorithms/approx/curve/cache.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 9289ac348..358851902 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -61,12 +61,13 @@ impl CurveApproxCache { // An approximation for this curve already exists. We need to // merge the new segment into it. - // We assume that approximated curve segments never overlap. As - // a consequence of this, and the current structure of the - // cache, we can assume that the existing approximation - // contains exactly one segment that is congruent with the one - // we are meant to insert. This means we can just extract that - // segment and return it to the caller. + // We assume that approximated curve segments never overlap, + // unless they are completely congruent. As a consequence of + // this, and the current structure of the cache, we can assume + // that the existing approximation contains exactly one segment + // that is congruent with the one we are meant to insert. This + // means we can just extract that segment and return it to the + // caller. // // For now, this is a valid assumption, as it matches the uses // of this method, due to documented limitations elsewhere in From 105af0fa03adcdb51173dbcf52001d4d57e389ea Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 11:29:54 +0200 Subject: [PATCH 10/12] Refactor to prepare for follow-on change --- .../fj-core/src/algorithms/approx/curve/cache.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 358851902..06e577180 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -33,15 +33,14 @@ 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, normalized_boundary)).cloned()?; - approx - }, - ) + if !was_already_normalized { + approx.reverse(); + } + + Some(approx) } /// Insert an approximated segment of the curve into the cache From 7269315d403a08242d45d5ab4f8f6611cfd7bf11 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 12:02:14 +0200 Subject: [PATCH 11/12] Merge `CurveApprox`imations in `CurveApproxCache` This is a pure refactoring, but it paves the way for merging curve approximations in more interesting ways in the future. --- .../src/algorithms/approx/curve/cache.rs | 45 ++++++++++--------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 06e577180..0507993c3 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,8 +32,11 @@ impl CurveApproxCache { let was_already_normalized = boundary.is_normalized(); let normalized_boundary = boundary.normalize(); - let mut approx = - self.inner.get(&(curve, normalized_boundary)).cloned()?; + let mut approx = self.inner.get(&curve).cloned()?; + + approx + .segments + .retain(|segment| segment.boundary == normalized_boundary); if !was_already_normalized { approx.reverse(); @@ -50,38 +52,41 @@ impl CurveApproxCache { mut new_segment: CurveApproxSegment, ) -> CurveApproxSegment { let curve = HandleWrapper::from(curve); - let cache_key = (curve, new_segment.boundary); + let cache_key = curve; new_segment.normalize(); let existing_approx = self.inner.remove(&cache_key); let (approx, segment) = match existing_approx { - Some(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, and the current structure of the cache, we can assume - // that the existing approximation contains exactly one segment - // that is congruent with the one we are meant to insert. This - // means we can just extract that segment and return it to the - // caller. + // 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 segments = existing_approx.segments.iter().cloned(); - let existing_segment = segments - .next() - .expect("Empty approximations should not exist in cache"); - assert!( - segments.next().is_none(), - "Cached approximations should have exactly 1 segment" - ); + 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, existing_segment) + (existing_approx, segment) } None => { // No approximation for this curve exists. We need to create a From b18468d711c45157bf894dd555f736965235f9a6 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 7 Sep 2023 12:03:28 +0200 Subject: [PATCH 12/12] Inline redundant variable --- crates/fj-core/src/algorithms/approx/curve/cache.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 0507993c3..97b561bb7 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -52,11 +52,10 @@ impl CurveApproxCache { mut new_segment: CurveApproxSegment, ) -> CurveApproxSegment { let curve = HandleWrapper::from(curve); - let cache_key = curve; new_segment.normalize(); - let existing_approx = self.inner.remove(&cache_key); + 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 @@ -99,7 +98,7 @@ impl CurveApproxCache { } }; - self.inner.insert(cache_key, approx); + self.inner.insert(curve, approx); segment }