Skip to content

Commit

Permalink
Merge pull request #2021 from hannobraun/approx
Browse files Browse the repository at this point in the history 
Lift limitation, that coincident `Edge`s must be congruent
  • Loading branch information
@hannobraun
hannobraun authored Sep 14, 2023
2 parents e031ac5 + 9cea234 commit 35f2f31
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 99 deletions.
38 changes: 19 additions & 19 deletions crates/fj-core/src/algorithms/approx/curve/cache.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -35,7 +35,7 @@ impl CurveApproxCache {
approx.reverse();
}

Some(approx)
approx
}

/// Insert an approximated segment of the curve into the cache
Expand Down Expand Up @@ -143,9 +143,9 @@ pub mod tests {
let cached = cache.get(&curve, &boundary);
assert_eq!(
cached,
Some(CurveApprox {
CurveApprox {
segments: vec![existing_segment]
})
}
);
}

Expand Down Expand Up @@ -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]]),
Expand All @@ -197,7 +197,7 @@ pub mod tests {
)],
}
]
})
}
);
}

Expand Down Expand Up @@ -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![
Expand All @@ -262,7 +262,7 @@ pub mod tests {
ApproxPoint::new([1.375], [1.375, 1.375, 1.375]),
],
}]
})
}
);
}

Expand Down Expand Up @@ -301,9 +301,9 @@ pub mod tests {
let cached = cache.get(&curve, &boundary);
assert_eq!(
cached,
Some(CurveApprox {
CurveApprox {
segments: vec![segment]
})
}
);
}

Expand Down Expand Up @@ -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
}]
})
}
);
}

Expand Down Expand Up @@ -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]),
],
}]
})
}
);
}

Expand Down Expand Up @@ -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]),
],
}]
})
}
);
}

Expand Down Expand Up @@ -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]),
],
}]
})
}
);
}
}
96 changes: 35 additions & 61 deletions crates/fj-core/src/algorithms/approx/edge.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::{
};

use super::{
curve::{CurveApproxCache, CurveApproxSegment},
curve::{CurveApprox, CurveApproxCache, CurveApproxSegment},
Approx, ApproxPoint, Tolerance,
};

Expand Down Expand Up @@ -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| {
Expand Down Expand Up @@ -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(
Expand Down
21 changes: 2 additions & 19 deletions crates/fj-core/src/objects/kinds/edge.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 35f2f31

Please sign in to comment.