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, - } } } diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 6cabd7476..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); } } @@ -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. diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index d1bb79c0f..6427882b7 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 = @@ -51,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()); @@ -59,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 @@ -246,8 +249,8 @@ impl EdgeApproxCache { &mut self, handle: Handle, approx: CurveApproxSegment, - ) -> CurveApproxSegment { - self.curve_approx.insert(handle, approx) + ) { + self.curve_approx.insert(handle, approx); } } diff --git a/crates/fj-core/src/geometry/boundary/multiple.rs b/crates/fj-core/src/geometry/boundary/multiple.rs index 466059b8a..2c0607408 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; @@ -101,10 +101,8 @@ 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(); - - (merged_boundary, merged_payload) } }