Merge pull request #1997 from hannobraun/approx

Replace `GlobalEdge` with `Curve` in edge approximation code

crates/fj-core/src/algorithms/approx/edge.rs

@@ -11,7 +11,7 @@ use fj_math::Point;
 
 use crate::{
     geometry::{CurveBoundary, GlobalPath, SurfacePath},
-    objects::{GlobalEdge, HalfEdge, Surface, Vertex},
+    objects::{Curve, HalfEdge, Surface, Vertex},
     storage::{Handle, HandleWrapper},
 };
 
@@ -43,45 +43,44 @@ impl Approx for (&HalfEdge, &Surface) {
         let first = ApproxPoint::new(position_surface, position_global);
 
         let points = {
-            // We cache approximated `HalfEdge`s using the `GlobalEdge`s they
-            // reference as the key. That bakes in the undesirable assumption
-            // that all coincident `HalfEdge`s are also congruent. Let me
-            // explain.
+            // We cache approximated `HalfEdge`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 `HalfEdge`s are
+            // also congruent. Let me explain.
             //
             // When two `HalfEdge`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 `GlobalEdge`. If there isn't, we create the
+            // cache entry for the curve/boundary. If there isn't, we create the
             // 3D approximation from the 2D `HalfEdge`. Next time we check for a
             // coincident `HalfEdge`, 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 `HalfEdge`.
             //
             // So what if we had two coincident `HalfEdge`s that aren't
             // congruent? Meaning, they overlap partially, but not fully. Then
-            // obviously, they wouldn't refer to the same `GlobalEdge`, because
-            // they are not the same edge, in global coordinates. And since the
-            // `GlobalEdge` is the key in our cache, those `HalfEdge`s would not
-            // share an approximation where they overlap, leading to exactly the
-            // problems that the cache is supposed to avoid.
+            // obviously, they wouldn't refer to the same combination of curve
+            // and boundary. And since those are the key in our cache, those
+            // `HalfEdge`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 `HalfEdge`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 based on `Curve`
-            // and the edge boundaries, instead of `GlobalEdge`. The cache needs
-            // to be able to deliver partial results for a given boundary, then
+            // 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_edge(
-                half_edge.global_form().clone(),
-                half_edge.boundary(),
-            );
+            let cached_approx =
+                cache.get_edge(half_edge.curve().clone(), half_edge.boundary());
             let approx = match cached_approx {
                 Some(approx) => approx,
                 None => {
@@ -92,7 +91,7 @@ impl Approx for (&HalfEdge, &Surface) {
                         tolerance,
                     );
                     cache.insert_edge(
-                        half_edge.global_form().clone(),
+                        half_edge.curve().clone(),
                         half_edge.boundary(),
                         approx,
                     )
@@ -218,7 +217,7 @@ fn approx_edge(
 #[derive(Default)]
 pub struct EdgeCache {
     edge_approx: BTreeMap<
-        (HandleWrapper<GlobalEdge>, CurveBoundary<Point<1>>),
+        (HandleWrapper<Curve>, CurveBoundary<Point<1>>),
         CurveApproxSegment,
     >,
     vertex_approx: BTreeMap<HandleWrapper<Vertex>, Point<3>>,
@@ -230,10 +229,10 @@ impl EdgeCache {
         Self::default()
     }
 
-    /// Access the approximation for the given [`GlobalEdge`], if available
+    /// Access the approximation for the given edge, if available
     fn get_edge(
         &self,
-        handle: Handle<GlobalEdge>,
+        handle: Handle<Curve>,
         boundary: CurveBoundary<Point<1>>,
     ) -> Option<CurveApproxSegment> {
         if let Some(approx) =
@@ -252,10 +251,10 @@ impl EdgeCache {
         None
     }
 
-    /// Insert the approximation of a [`GlobalEdge`]
+    /// Insert the approximation of an edge
     fn insert_edge(
         &mut self,
-        handle: Handle<GlobalEdge>,
+        handle: Handle<Curve>,
         boundary: CurveBoundary<Point<1>>,
         approx: CurveApproxSegment,
     ) -> CurveApproxSegment {