Merge pull request #2051 from hannobraun/approx

Remove redundant return values in curve approx code

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,
- }
  }
 }
 

crates/fj-core/src/algorithms/approx/curve/cache.rs

@@ -43,7 +43,7 @@ impl CurveApproxCache {
  &mut self,
  curve: Handle<Curve>,
  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.

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,42 +52,44 @@ 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());
 
  // `cached` is the approximation of the curve that is available
  // 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<Curve>,
  approx: CurveApproxSegment,
- ) -> CurveApproxSegment {
- self.curve_approx.insert(handle, approx)
+ ) {
+ self.curve_approx.insert(handle, approx);
  }
 }
 

crates/fj-core/src/geometry/boundary/multiple.rs

@@ -70,7 +70,7 @@ impl<T: CurveBoundariesPayload> CurveBoundaries<T> {
  &mut self,
  new_boundary: CurveBoundary<Point<1>>,
  new_payload: T,
- ) -> (CurveBoundary<Point<1>>, T) {
+ ) {
  let mut overlapping_payloads = VecDeque::new();
 
  let mut i = 0;
@@ -101,10 +101,8 @@ impl<T: CurveBoundariesPayload> CurveBoundaries<T> {
  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)
  }
 }