From 46131bf4bb25ab181e81c918a34540819319c226 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Oct 2023 10:09:07 +0200 Subject: [PATCH 1/8] Refactor to prepare for follow-on change --- crates/fj-core/src/algorithms/approx/edge.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index d1bb79c0f..8e787f9d8 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -32,6 +32,7 @@ impl Approx for (&Edge, &Surface) { cache: &mut Self::Cache, ) -> Self::Approximation { let (edge, surface) = self; + let tolerance = tolerance.into(); let start_position_surface = edge.start_position(); let start_position = From 7b99dde2f0b77c47d24b992fafee9978f0253f83 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Oct 2023 10:09:18 +0200 Subject: [PATCH 2/8] Refactor to avoid using problematic return value The return value of `insert_curve_approx` stands in the way of a refactoring I'm working on, and this commit is the first step towards removing it. --- crates/fj-core/src/algorithms/approx/edge.rs | 58 ++++++++++---------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 8e787f9d8..cb00e90bb 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -52,7 +52,7 @@ impl Approx for (&Edge, &Surface) { let first = ApproxPoint::new(start_position_surface, start_position); let rest = { - let segment = { + let segment = loop { let cached = cache .get_curve_approx(edge.curve().clone(), edge.boundary()); @@ -60,34 +60,36 @@ impl Approx for (&Edge, &Surface) { // within the edge boundary. This approximation might or might // not be complete. - match cached.into_single_segment(edge.boundary()) { - Some(segment) => { - // We've asked the approximation to give us a single - // segment that covers the boundary, and we got it. We - // can use it as-is. - segment - } - None => { - // If we make it here, there are holes in the - // approximation, in some way or another. We could be - // really surgical and fill in exactly those holes, and - // in the future we might want to, for performance - // reasons. - // - // For now, let's just approximate *all* we need and - // insert that into the cache. The cache takes care of - // merging that with whatever is already there. - cache.insert_curve_approx( - edge.curve().clone(), - approx_curve( - &edge.path(), - surface, - edge.boundary(), - tolerance, - ), - ) - } + if let Some(segment) = + cached.into_single_segment(edge.boundary()) + { + // We've asked the approximation to give us a single + // segment that covers the boundary, and we got it. We + // can use it as-is. + break segment; } + + // If we make it here, there are holes in the approximation, in + // some way or another. We could be really surgical and fill in + // exactly those holes, and in the future we might want to, for + // performance reasons. + // + // For now, let's just approximate *all* we need and insert that + // into the cache. The cache takes care of merging that with + // whatever is already there. + cache.insert_curve_approx( + edge.curve().clone(), + approx_curve( + &edge.path(), + surface, + edge.boundary(), + tolerance, + ), + ); + + // We will never complete more than one full loop here. If we + // don't return the segment the first time, we'll insert it + // immediately, and it will be there on the second iteration. }; segment From 270ada96200a1208d6182eb66eb090a48891e668 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Oct 2023 10:10:18 +0200 Subject: [PATCH 3/8] Remove unused return value --- crates/fj-core/src/algorithms/approx/edge.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index cb00e90bb..6427882b7 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -249,8 +249,8 @@ impl EdgeApproxCache { &mut self, handle: Handle, approx: CurveApproxSegment, - ) -> CurveApproxSegment { - self.curve_approx.insert(handle, approx) + ) { + self.curve_approx.insert(handle, approx); } } From 3c81c93cee6056eec89373b3fc8fa82a97eb161c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Oct 2023 10:17:29 +0200 Subject: [PATCH 4/8] Don't test ret val of `CurveApproxCache::insert` I'm about to remove it. This makes one of the tests fully redundant (the same functionality is already covered by a `get` test), while the other already uses `get` to cover the same functionality. --- .../src/algorithms/approx/curve/cache.rs | 42 +------------------ 1 file changed, 1 insertion(+), 41 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 6cabd7476..b9c80f005 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -69,42 +69,6 @@ pub mod tests { 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::from(( - CurveBoundary::from([[2.], [3.]]), - [ - 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::from(( - boundary, - [ - 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(); @@ -132,11 +96,7 @@ pub mod tests { 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); + cache.insert(curve.clone(), new_segment); // Also, the new segment should not have replaced the existing on in the // cache. From 93d1f4684f33d171add02c251fb947933c1d85aa Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Oct 2023 10:18:51 +0200 Subject: [PATCH 5/8] Remove unused return value --- 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 b9c80f005..71c78a9ed 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -43,7 +43,7 @@ impl CurveApproxCache { &mut self, curve: Handle, mut new_segment: CurveApproxSegment, - ) -> CurveApproxSegment { + ) { let curve = HandleWrapper::from(curve); // Overlapping approximations need to result in the same points, @@ -52,7 +52,7 @@ impl CurveApproxCache { // approximated segment before doing *anything* with it. new_segment.normalize(); - self.inner.entry(curve).or_default().merge(new_segment) + self.inner.entry(curve).or_default().merge(new_segment); } } From addbdc3c261c2cd550bb8dd27d0274bbf782573f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Oct 2023 10:19:44 +0200 Subject: [PATCH 6/8] Remove unused return value --- .../fj-core/src/algorithms/approx/curve/approx.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/approx.rs b/crates/fj-core/src/algorithms/approx/curve/approx.rs index 00f5d25b6..b0983437d 100644 --- a/crates/fj-core/src/algorithms/approx/curve/approx.rs +++ b/crates/fj-core/src/algorithms/approx/curve/approx.rs @@ -32,18 +32,9 @@ impl CurveApprox { } /// Merge the provided segment into the approximation - pub fn merge( - &mut self, - new_segment: CurveApproxSegment, - ) -> CurveApproxSegment { - let (merged_boundary, merged_segment) = self - .segments + pub fn merge(&mut self, new_segment: CurveApproxSegment) { + self.segments .merge(new_segment.boundary, new_segment.points); - - CurveApproxSegment { - boundary: merged_boundary, - points: merged_segment, - } } } From e1ae30e8d5c90e4e82655499f9d72ec35debd41b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Oct 2023 10:20:24 +0200 Subject: [PATCH 7/8] Remove unused return value --- crates/fj-core/src/geometry/boundary/multiple.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/fj-core/src/geometry/boundary/multiple.rs b/crates/fj-core/src/geometry/boundary/multiple.rs index 466059b8a..327d03b9d 100644 --- a/crates/fj-core/src/geometry/boundary/multiple.rs +++ b/crates/fj-core/src/geometry/boundary/multiple.rs @@ -70,7 +70,7 @@ impl CurveBoundaries { &mut self, new_boundary: CurveBoundary>, new_payload: T, - ) -> (CurveBoundary>, T) { + ) { let mut overlapping_payloads = VecDeque::new(); let mut i = 0; @@ -103,8 +103,6 @@ impl CurveBoundaries { self.inner.push((merged_boundary, merged_payload.clone())); self.inner.sort(); - - (merged_boundary, merged_payload) } } From 87f4aab2e93be0407f6bc992eb88f66371b00796 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Oct 2023 10:20:39 +0200 Subject: [PATCH 8/8] Remove redundant `clone` --- crates/fj-core/src/geometry/boundary/multiple.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-core/src/geometry/boundary/multiple.rs b/crates/fj-core/src/geometry/boundary/multiple.rs index 327d03b9d..2c0607408 100644 --- a/crates/fj-core/src/geometry/boundary/multiple.rs +++ b/crates/fj-core/src/geometry/boundary/multiple.rs @@ -101,7 +101,7 @@ impl CurveBoundaries { merged_payload.merge(&payload, boundary); } - self.inner.push((merged_boundary, merged_payload.clone())); + self.inner.push((merged_boundary, merged_payload)); self.inner.sort(); } }