From 381a72feb97afcb21486137cebc23ae61241394d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 13 Sep 2023 11:26:34 +0200 Subject: [PATCH 01/18] Fix typo in comment --- crates/fj-core/src/algorithms/approx/edge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 78df2afc2..c2c372f9b 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -66,7 +66,7 @@ impl Approx for (&Edge, &Surface) { // the exact same 3D approximation, instead of generating a slightly // different one from the different 2D `Edge`. // - // So what if we had two coincident `fEdge`s that aren't congruent? + // So what if we had two coincident `Edge`s that aren't congruent? // Meaning, they overlap partially, but not fully. Then obviously, // they wouldn't refer to the same combination of curve and // boundary. And since those are the key in our cache, those `Edge`s From 2c49b405158be1f5b394d756e842afac6832073b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:23:45 +0200 Subject: [PATCH 02/18] Simplify return value of `CurveApproxCache::get` --- .../src/algorithms/approx/curve/cache.rs | 38 +++++++++---------- crates/fj-core/src/algorithms/approx/edge.rs | 22 +++++------ 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/curve/cache.rs b/crates/fj-core/src/algorithms/approx/curve/cache.rs index 28d7dc6d4..4cbb52488 100644 --- a/crates/fj-core/src/algorithms/approx/curve/cache.rs +++ b/crates/fj-core/src/algorithms/approx/curve/cache.rs @@ -22,10 +22,10 @@ impl CurveApproxCache { &self, curve: &Handle, boundary: &CurveBoundary>, - ) -> Option { + ) -> CurveApprox { let curve = HandleWrapper::from(curve.clone()); - let mut approx = self.inner.get(&curve).cloned()?; + let mut approx = self.inner.get(&curve).cloned().unwrap_or_default(); approx.make_subset(boundary); // Approximations within the cache are stored in normalized form. If the @@ -35,7 +35,7 @@ impl CurveApproxCache { approx.reverse(); } - Some(approx) + approx } /// Insert an approximated segment of the curve into the cache @@ -143,9 +143,9 @@ pub mod tests { let cached = cache.get(&curve, &boundary); assert_eq!( cached, - Some(CurveApprox { + CurveApprox { segments: vec![existing_segment] - }) + } ); } @@ -180,7 +180,7 @@ pub mod tests { let cached = cache.get(&curve, &CurveBoundary::from([[0.], [1.]])); assert_eq!( cached, - Some(CurveApprox { + CurveApprox { segments: vec![ CurveApproxSegment { boundary: CurveBoundary::from([[0.], [0.25]]), @@ -197,7 +197,7 @@ pub mod tests { )], } ] - }) + } ); } @@ -250,7 +250,7 @@ pub mod tests { let cached = cache.get(&curve, &boundary); assert_eq!( cached, - Some(CurveApprox { + CurveApprox { segments: vec![CurveApproxSegment { boundary, points: vec![ @@ -262,7 +262,7 @@ pub mod tests { ApproxPoint::new([1.375], [1.375, 1.375, 1.375]), ], }] - }) + } ); } @@ -301,9 +301,9 @@ pub mod tests { let cached = cache.get(&curve, &boundary); assert_eq!( cached, - Some(CurveApprox { + CurveApprox { segments: vec![segment] - }) + } ); } @@ -343,13 +343,13 @@ pub mod tests { let cached = cache.get(&curve, &boundary.reverse()); assert_eq!( cached, - Some(CurveApprox { + CurveApprox { segments: vec![{ let mut segment = segment; segment.reverse(); segment }] - }) + } ); } @@ -377,7 +377,7 @@ pub mod tests { let cached = cache.get(&curve, &CurveBoundary::from([[-0.5], [0.5]])); assert_eq!( cached, - Some(CurveApprox { + CurveApprox { segments: vec![CurveApproxSegment { boundary: CurveBoundary::from([[0.], [0.5]]), points: vec![ @@ -385,7 +385,7 @@ pub mod tests { ApproxPoint::new([0.375], [0.375, 0.375, 0.375]), ], }] - }) + } ); } @@ -413,7 +413,7 @@ pub mod tests { let cached = cache.get(&curve, &CurveBoundary::from([[0.5], [1.5]])); assert_eq!( cached, - Some(CurveApprox { + CurveApprox { segments: vec![CurveApproxSegment { boundary: CurveBoundary::from([[0.5], [1.0]]), points: vec![ @@ -421,7 +421,7 @@ pub mod tests { ApproxPoint::new([0.875], [0.875, 0.875, 0.875]), ], }] - }) + } ); } @@ -449,7 +449,7 @@ pub mod tests { let cached = cache.get(&curve, &CurveBoundary::from([[0.25], [0.75]])); assert_eq!( cached, - Some(CurveApprox { + CurveApprox { segments: vec![CurveApproxSegment { boundary: CurveBoundary::from([[0.25], [0.75]]), points: vec![ @@ -457,7 +457,7 @@ pub mod tests { ApproxPoint::new([0.625], [0.625, 0.625, 0.625]), ], }] - }) + } ); } } diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index c2c372f9b..d1be98348 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -245,17 +245,17 @@ impl EdgeApproxCache { handle: Handle, boundary: CurveBoundary>, ) -> Option { - self.curve_approx.get(&handle, &boundary).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 - }) + let approx = self.curve_approx.get(&handle, &boundary); + + let mut segments = approx.segments.into_iter(); + + let segment = segments.next()?; + assert!( + segments.next().is_none(), + "Cached approximations should have exactly 1 segment" + ); + + Some(segment) } fn insert_curve_approx( From 35655cdbc8231a1eb1cc92f88b9d44d4d76cf2e1 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:24:00 +0200 Subject: [PATCH 03/18] Update panic message --- crates/fj-core/src/algorithms/approx/edge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index d1be98348..f70125aa3 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -252,7 +252,7 @@ impl EdgeApproxCache { let segment = segments.next()?; assert!( segments.next().is_none(), - "Cached approximations should have exactly 1 segment" + "Cached approximations should have at most 1 segment" ); Some(segment) From d074c733b6d9082b883b332e0c23efb9dd174d68 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:24:43 +0200 Subject: [PATCH 04/18] Make variable name more accurate --- 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 f70125aa3..2217c5d89 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -84,9 +84,9 @@ impl Approx for (&Edge, &Surface) { // even if those boundaries aren't identical. The cache needs to be // able to deliver partial results for a given boundary, then // generating (and caching) the rest of it on the fly. - let cached_approx = + let cached_segment = cache.get_curve_approx(edge.curve().clone(), edge.boundary()); - let approx = match cached_approx { + let approx = match cached_segment { Some(approx) => approx, None => { let approx = approx_curve( From a27beec79e9af3d220488be6c189ef5105dc0e29 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:25:01 +0200 Subject: [PATCH 05/18] Make variable name more accurate --- crates/fj-core/src/algorithms/approx/edge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 2217c5d89..5ea2bc006 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -87,7 +87,7 @@ impl Approx for (&Edge, &Surface) { let cached_segment = cache.get_curve_approx(edge.curve().clone(), edge.boundary()); let approx = match cached_segment { - Some(approx) => approx, + Some(segment) => segment, None => { let approx = approx_curve( &edge.path(), From a753d740ea7b5bb81878167c915170c2807774b0 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:26:44 +0200 Subject: [PATCH 06/18] Refactor to prepare for follow-on change --- crates/fj-core/src/algorithms/approx/edge.rs | 34 +++++++++++--------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 5ea2bc006..dec14edec 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -16,7 +16,7 @@ use crate::{ }; use super::{ - curve::{CurveApproxCache, CurveApproxSegment}, + curve::{CurveApprox, CurveApproxCache, CurveApproxSegment}, Approx, ApproxPoint, Tolerance, }; @@ -84,8 +84,22 @@ impl Approx for (&Edge, &Surface) { // even if those boundaries aren't identical. The cache needs to be // able to deliver partial results for a given boundary, then // generating (and caching) the rest of it on the fly. - let cached_segment = - cache.get_curve_approx(edge.curve().clone(), edge.boundary()); + let cached_segment = 'segment: { + let approx = cache + .get_curve_approx(edge.curve().clone(), edge.boundary()); + + let mut segments = approx.segments.into_iter(); + + let Some(segment) = segments.next() else { + break 'segment None; + }; + assert!( + segments.next().is_none(), + "Cached approximations should have at most 1 segment" + ); + + Some(segment) + }; let approx = match cached_segment { Some(segment) => segment, None => { @@ -244,18 +258,8 @@ impl EdgeApproxCache { &self, handle: Handle, boundary: CurveBoundary>, - ) -> Option { - let approx = self.curve_approx.get(&handle, &boundary); - - let mut segments = approx.segments.into_iter(); - - let segment = segments.next()?; - assert!( - segments.next().is_none(), - "Cached approximations should have at most 1 segment" - ); - - Some(segment) + ) -> CurveApprox { + self.curve_approx.get(&handle, &boundary) } fn insert_curve_approx( From fcb615a016ce33a1c1593319ce8c23e3e1a27302 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:31:42 +0200 Subject: [PATCH 07/18] Inline redundant variable --- crates/fj-core/src/algorithms/approx/edge.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index dec14edec..620bc1e24 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -102,15 +102,15 @@ impl Approx for (&Edge, &Surface) { }; let approx = match cached_segment { Some(segment) => segment, - None => { - let approx = approx_curve( + None => cache.insert_curve_approx( + edge.curve().clone(), + approx_curve( &edge.path(), surface, edge.boundary(), tolerance, - ); - cache.insert_curve_approx(edge.curve().clone(), approx) - } + ), + ), }; approx From 7f38d0a17a92d717b6567418ebf46ce35cd0d3ba Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:33:13 +0200 Subject: [PATCH 08/18] Update variable name --- 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 620bc1e24..ac92f444d 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -85,10 +85,10 @@ impl Approx for (&Edge, &Surface) { // able to deliver partial results for a given boundary, then // generating (and caching) the rest of it on the fly. let cached_segment = 'segment: { - let approx = cache + let cached = cache .get_curve_approx(edge.curve().clone(), edge.boundary()); - let mut segments = approx.segments.into_iter(); + let mut segments = cached.segments.into_iter(); let Some(segment) = segments.next() else { break 'segment None; From beb0ed71be7058fcb3aae1f7353939a5948014b0 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:33:37 +0200 Subject: [PATCH 09/18] Update variable name --- 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 ac92f444d..9a0ee4de4 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -100,7 +100,7 @@ impl Approx for (&Edge, &Surface) { Some(segment) }; - let approx = match cached_segment { + let segment = match cached_segment { Some(segment) => segment, None => cache.insert_curve_approx( edge.curve().clone(), @@ -113,7 +113,7 @@ impl Approx for (&Edge, &Surface) { ), }; - approx + segment .points .into_iter() .map(|point| { From cacbc6bd7af62e48e0d3b47e97305e85428b4a1c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:37:44 +0200 Subject: [PATCH 10/18] Refactor to simplify --- crates/fj-core/src/algorithms/approx/edge.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 9a0ee4de4..5b2145a64 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -84,21 +84,19 @@ impl Approx for (&Edge, &Surface) { // even if those boundaries aren't identical. The cache needs to be // able to deliver partial results for a given boundary, then // generating (and caching) the rest of it on the fly. - let cached_segment = 'segment: { + let cached_segment = { let cached = cache .get_curve_approx(edge.curve().clone(), edge.boundary()); let mut segments = cached.segments.into_iter(); - let Some(segment) = segments.next() else { - break 'segment None; - }; + let segment = segments.next(); assert!( segments.next().is_none(), "Cached approximations should have at most 1 segment" ); - Some(segment) + segment }; let segment = match cached_segment { Some(segment) => segment, From 1e3004085baa3513a8b358d881308cb6fad66b13 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:42:37 +0200 Subject: [PATCH 11/18] Shorten error message --- crates/fj-core/src/algorithms/approx/edge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 5b2145a64..6017f407e 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -93,7 +93,7 @@ impl Approx for (&Edge, &Surface) { let segment = segments.next(); assert!( segments.next().is_none(), - "Cached approximations should have at most 1 segment" + "Cached approximations should have 1 segment max" ); segment From b308d8c2d68b38c1e970917b28d7b6a011d6ed33 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:43:12 +0200 Subject: [PATCH 12/18] Refactor to prepare for follow-on change --- crates/fj-core/src/algorithms/approx/edge.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 6017f407e..d03cd98e1 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -90,13 +90,17 @@ impl Approx for (&Edge, &Surface) { let mut segments = cached.segments.into_iter(); - let segment = segments.next(); - assert!( - segments.next().is_none(), - "Cached approximations should have 1 segment max" - ); - - segment + match segments.next() { + Some(segment) => { + assert!( + segments.next().is_none(), + "Cached approximations should have 1 segment max" + ); + + Some(segment) + } + None => None, + } }; let segment = match cached_segment { Some(segment) => segment, From 2a461c8e2b985910e4e8c0375898fbd58eff52f4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:43:57 +0200 Subject: [PATCH 13/18] Refactor to simplify --- crates/fj-core/src/algorithms/approx/edge.rs | 26 +++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index d03cd98e1..ed5bf5119 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -84,7 +84,7 @@ impl Approx for (&Edge, &Surface) { // even if those boundaries aren't identical. The cache needs to be // able to deliver partial results for a given boundary, then // generating (and caching) the rest of it on the fly. - let cached_segment = { + let segment = { let cached = cache .get_curve_approx(edge.curve().clone(), edge.boundary()); @@ -97,22 +97,18 @@ impl Approx for (&Edge, &Surface) { "Cached approximations should have 1 segment max" ); - Some(segment) + segment } - None => None, - } - }; - let segment = match cached_segment { - Some(segment) => segment, - None => cache.insert_curve_approx( - edge.curve().clone(), - approx_curve( - &edge.path(), - surface, - edge.boundary(), - tolerance, + None => cache.insert_curve_approx( + edge.curve().clone(), + approx_curve( + &edge.path(), + surface, + edge.boundary(), + tolerance, + ), ), - ), + } }; segment From b7363f0c1cdf6faf7795643013a0a2eb506304ae Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:45:01 +0200 Subject: [PATCH 14/18] Refactor to simplify --- crates/fj-core/src/algorithms/approx/edge.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index ed5bf5119..47adfe796 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -85,15 +85,13 @@ impl Approx for (&Edge, &Surface) { // able to deliver partial results for a given boundary, then // generating (and caching) the rest of it on the fly. let segment = { - let cached = cache + let mut cached = cache .get_curve_approx(edge.curve().clone(), edge.boundary()); - let mut segments = cached.segments.into_iter(); - - match segments.next() { + match cached.segments.pop() { Some(segment) => { assert!( - segments.next().is_none(), + cached.segments.is_empty(), "Cached approximations should have 1 segment max" ); From d918e6159da60290bd08c12362bab1a8282e0631 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:45:48 +0200 Subject: [PATCH 15/18] Refactor to prepare for follow-on change --- crates/fj-core/src/algorithms/approx/edge.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 47adfe796..9e710c527 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -89,14 +89,10 @@ impl Approx for (&Edge, &Surface) { .get_curve_approx(edge.curve().clone(), edge.boundary()); match cached.segments.pop() { - Some(segment) => { - assert!( - cached.segments.is_empty(), - "Cached approximations should have 1 segment max" - ); - - segment - } + Some(segment) if cached.segments.is_empty() => segment, + Some(_) => panic!( + "Cached approximations should have 1 segment max" + ), None => cache.insert_curve_approx( edge.curve().clone(), approx_curve( From 6bcbdb1783e60c96efd41a6b628b1c119d8c9d9f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:47:09 +0200 Subject: [PATCH 16/18] Add comment --- crates/fj-core/src/algorithms/approx/edge.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 9e710c527..36de12052 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -89,7 +89,12 @@ impl Approx for (&Edge, &Surface) { .get_curve_approx(edge.curve().clone(), edge.boundary()); match cached.segments.pop() { - Some(segment) if cached.segments.is_empty() => segment, + Some(segment) if cached.segments.is_empty() => { + // If the cached approximation has a single segment, + // that means everything we need is available, and we + // can use the cached approximation as-is. + segment + } Some(_) => panic!( "Cached approximations should have 1 segment max" ), From 6e47b0906ed5002f77c0c8d51f31a3eb78d25d85 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:52:28 +0200 Subject: [PATCH 17/18] Lift half-edge congruency limitation See the removed comment, or the (still available) documentation in `Edge` to learn more about that limitation. --- crates/fj-core/src/algorithms/approx/edge.rs | 67 ++++++-------------- 1 file changed, 20 insertions(+), 47 deletions(-) diff --git a/crates/fj-core/src/algorithms/approx/edge.rs b/crates/fj-core/src/algorithms/approx/edge.rs index 36de12052..2cc597348 100644 --- a/crates/fj-core/src/algorithms/approx/edge.rs +++ b/crates/fj-core/src/algorithms/approx/edge.rs @@ -49,41 +49,6 @@ impl Approx for (&Edge, &Surface) { let first = ApproxPoint::new(start_position_surface, start_position); let rest = { - // We cache approximated `Edge`s using the `Curve`s they reference - // and their boundary on that curve as the key. That bakes in the - // undesirable assumption that all coincident `Edge`s are also - // congruent. Let me explain. - // - // When two `Edge`s are coincident, we need to make sure their - // approximations are identical where they overlap. Otherwise, we'll - // get an invalid triangle mesh in the end. Hence, we cache - // approximations. - // - // Caching works like this: We check whether there already is a - // cache entry for the curve/boundary. If there isn't, we create the - // 3D approximation from the 2D `Edge`. Next time we check for a - // coincident `Edge`, we'll find the cache and use that, getting - // the exact same 3D approximation, instead of generating a slightly - // different one from the different 2D `Edge`. - // - // So what if we had two coincident `Edge`s that aren't congruent? - // Meaning, they overlap partially, but not fully. Then obviously, - // they wouldn't refer to the same combination of curve and - // boundary. And since those are the key in our cache, those `Edge`s - // would not share an approximation where they overlap, leading to - // exactly the problems that the cache is supposed to prevent. - // - // As of this writing, it is a documented (but not validated) - // limitation, that coincident `Edge`s must always be congruent. - // However, we're going to need to lift this limitation going - // forward, as it is, well, too limiting. This means things here - // will need to change. - // - // What we need here, is more intelligent caching, that can reuse - // already cached curve approximations where the boundaries overlap, - // even if those boundaries aren't identical. The cache needs to be - // able to deliver partial results for a given boundary, then - // generating (and caching) the rest of it on the fly. let segment = { let mut cached = cache .get_curve_approx(edge.curve().clone(), edge.boundary()); @@ -95,18 +60,26 @@ impl Approx for (&Edge, &Surface) { // can use the cached approximation as-is. segment } - Some(_) => panic!( - "Cached approximations should have 1 segment max" - ), - None => cache.insert_curve_approx( - edge.curve().clone(), - approx_curve( - &edge.path(), - surface, - edge.boundary(), - tolerance, - ), - ), + _ => { + // 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, + ), + ) + } } }; From 9cea234944a9891e02bdc8f357e1f4ba46025702 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 14 Sep 2023 11:56:42 +0200 Subject: [PATCH 18/18] Update documentation of `Edge` --- crates/fj-core/src/objects/kinds/edge.rs | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/crates/fj-core/src/objects/kinds/edge.rs b/crates/fj-core/src/objects/kinds/edge.rs index d7e895984..863afd825 100644 --- a/crates/fj-core/src/objects/kinds/edge.rs +++ b/crates/fj-core/src/objects/kinds/edge.rs @@ -10,25 +10,8 @@ use crate::{ /// /// When multiple faces, which are bound by edges, are combined to form a solid, /// the `Edge`s that bound the face on the surface are then coincident with the -/// `Edge`s of other faces, where those faces touch. Those coincident `Edge`s -/// are different representations of the same edge, and this fact must be -/// represented in the following way: -/// -/// - The coincident `Edge`s must refer to the same `Curve`. -/// - The coincident `Edge`s must have the same boundary. -/// -/// There is another, implicit requirement hidden here: -/// -/// `Edge`s that are coincident, i.e. located in the same space, must always be -/// congruent. This means they must coincide *exactly*. The overlap must be -/// complete. None of the coincident `Edge`s must overlap with just a section of -/// another. -/// -/// # Implementation Note -/// -/// The limitation that coincident `Edge`s must be congruent is currently -/// being lifted: -/// +/// `Edge`s of other faces, where those faces touch. Suche coincident `Edge`s +/// must always refer to the same `Curve`. #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Edge { path: SurfacePath,