Lift limitation, that coincident Edges must be congruent

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

@@ -22,10 +22,10 @@ impl CurveApproxCache {
         &self,
         curve: &Handle<Curve>,
         boundary: &CurveBoundary<Point<1>>,
-    ) -> Option<CurveApprox> {
+    ) -> 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,15 +377,15 @@ 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![
                         ApproxPoint::new([0.125], [0.125, 0.125, 0.125]),
                         ApproxPoint::new([0.375], [0.375, 0.375, 0.375]),
                     ],
                 }]
-            })
+            }
         );
     }
 
@@ -413,15 +413,15 @@ 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![
                         ApproxPoint::new([0.625], [0.625, 0.625, 0.625]),
                         ApproxPoint::new([0.875], [0.875, 0.875, 0.875]),
                     ],
                 }]
-            })
+            }
         );
     }
 
@@ -449,15 +449,15 @@ 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![
                         ApproxPoint::new([0.375], [0.375, 0.375, 0.375]),
                         ApproxPoint::new([0.625], [0.625, 0.625, 0.625]),
                     ],
                 }]
-            })
+            }
         );
     }
 }

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,
 };
 
@@ -49,57 +49,41 @@ 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 `fEdge`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 cached_approx =
-                cache.get_curve_approx(edge.curve().clone(), edge.boundary());
-            let approx = match cached_approx {
-                Some(approx) => approx,
-                None => {
-                    let approx = approx_curve(
-                        &edge.path(),
-                        surface,
-                        edge.boundary(),
-                        tolerance,
-                    );
-                    cache.insert_curve_approx(edge.curve().clone(), approx)
+            let segment = {
+                let mut cached = cache
+                    .get_curve_approx(edge.curve().clone(), edge.boundary());
+
+                match cached.segments.pop() {
+                    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
+                    }
+                    _ => {
+                        // 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,
+                            ),
+                        )
+                    }
                 }
             };
 
-            approx
+            segment
                 .points
                 .into_iter()
                 .map(|point| {
@@ -244,18 +228,8 @@ impl EdgeApproxCache {
         &self,
         handle: Handle<Curve>,
         boundary: CurveBoundary<Point<1>>,
-    ) -> Option<CurveApproxSegment> {
-        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
-        })
+    ) -> CurveApprox {
+        self.curve_approx.get(&handle, &boundary)
     }
 
     fn insert_curve_approx(

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:
-/// <https://github.com/hannobraun/fornjot/issues/1937>
+/// `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,