diff --git a/crates/solidcore/src/curve/curve/p5.rs b/crates/solidcore/src/curve/curve/p5.rs index 326b2a39f6..45a82cfa1a 100644 --- a/crates/solidcore/src/curve/curve/p5.rs +++ b/crates/solidcore/src/curve/curve/p5.rs @@ -325,6 +325,15 @@ impl CurveMemoKey { if !k.memoize_curve_caches || !memo_pure(curve) { return None; } + Self::of_kernel(k) + } + + /// The kernel part of a memo key, for any memo of a build that reads `k` only + /// through its tolerances and context scalars; `None` when the memo is off. + pub(crate) fn of_kernel(k: &Kernel) -> Option { + if !k.memoize_curve_caches { + return None; + } let c = &k.context; Some(Self { tolerances: k.tolerances, diff --git a/crates/solidcore/src/kernel.rs b/crates/solidcore/src/kernel.rs index 5f6b30d24e..40ad3367ca 100644 --- a/crates/solidcore/src/kernel.rs +++ b/crates/solidcore/src/kernel.rs @@ -194,8 +194,10 @@ pub struct Kernel { /// [`Cell`] because the intersectors take `&Kernel`. pub cone_parallel_midpoint_test: Cell, /// Not upstream: whether `make_curve_cache` keeps a memo of each curve cache in the - /// curve value (`true` unless the `curve-memo-off` feature is on). The memo never - /// changes a result, only the time; `false` builds every cache fresh, to check that. + /// curve value, and `CoincidenceCheck` a memo of each surface's boundary curves in + /// the surface value (`true` unless the `curve-memo-off` feature is on). The memos + /// never change a result, only the time; `false` builds everything fresh, to check + /// that. pub memoize_curve_caches: bool, } diff --git a/crates/solidcore/src/surface/surface/p2.rs b/crates/solidcore/src/surface/surface/p2.rs index 825f0b63df..37f3eab583 100644 --- a/crates/solidcore/src/surface/surface/p2.rs +++ b/crates/solidcore/src/surface/surface/p2.rs @@ -233,6 +233,7 @@ use crate::surface::surf_of_extrusion::SurfOfExtrusion; use crate::surface::surf_of_revolution::p3::intersect_revolved; use crate::surface::surf_of_revolution::{Revolved, SurfOfRevolution}; use crate::surface::surface::p1::{StepSurface, SurfaceIntersectContext, SurfaceSolveContext}; +use crate::surface::surface::p8::BoundaryMemo; use crate::surface::surface::p8::FaceSolveCache; use crate::surface::surface_cache::p1::SurfaceCache; use crate::surface::torus::Torus; @@ -352,6 +353,9 @@ pub struct Surface { /// The identity a context issued this surface object for its notifications /// (upstream passes the object itself, known by address); a copy has none. notify_identity: IdentitySlot, + /// Not upstream: the memo of the natural boundary curves `CoincidenceCheck` makes + /// (`BoundaryMemo`); a copy starts an empty one, an edit replaces it. + boundary_memo: BoundaryMemo, } /// What a surface's cache slot (`SmAObject::m_pCacheObj`) holds, as far as a reader of @@ -454,6 +458,7 @@ impl Clone for Surface { owner_zone_tol_3d: None, cache_slot: CacheSlot::default(), notify_identity: IdentitySlot::new(), + boundary_memo: BoundaryMemo::new(), }; s.update_owned_bases(); s @@ -694,6 +699,7 @@ impl Surface { owner_zone_tol_3d: self.owner_zone_tol_3d, cache_slot: self.cache_slot.share(), notify_identity: IdentitySlot::new(), + boundary_memo: BoundaryMemo::new(), }) } @@ -740,6 +746,7 @@ impl Surface { owner_zone_tol_3d: self.owner_zone_tol_3d, cache_slot: self.cache_slot.share(), notify_identity: IdentitySlot::new(), + boundary_memo: BoundaryMemo::new(), } } } @@ -890,6 +897,7 @@ impl Surface { owner_zone_tol_3d: None, cache_slot: CacheSlot::default(), notify_identity: IdentitySlot::new(), + boundary_memo: BoundaryMemo::new(), } } @@ -1087,6 +1095,11 @@ impl Surface { self.cache_slot.same(&other.cache_slot) } + /// The boundary curve memo of `CoincidenceCheck` (see `BoundaryMemo`). + pub(crate) const fn boundary_memo(&self) -> &BoundaryMemo { + &self.boundary_memo + } + /// The concrete kind. #[must_use] pub const fn kind(&self) -> &SurfaceKind { @@ -1095,6 +1108,8 @@ impl Surface { /// The concrete kind, mutable. pub fn kind_mut(&mut self) -> &mut SurfaceKind { + // an edit makes another surface value: start an empty boundary memo + self.boundary_memo = BoundaryMemo::new(); &mut self.kind } @@ -2059,6 +2074,7 @@ impl Surface { k: &Kernel, new_step_uv_domain: &Extent2d, ) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.adjust_step_uv_domain(new_step_uv_domain), SurfaceKind::CurveBounded(_) @@ -2082,6 +2098,7 @@ impl Surface { /// /// Those of the kind's override (the base default is `SM_ERR`). pub fn reverse(&mut self, k: &Kernel, param: SurfParamType) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.reverse(param), SurfaceKind::CurveBounded(_) | SurfaceKind::Step(_) => base_edit(), @@ -2144,6 +2161,7 @@ impl Surface { /// /// Those of the kind's override (the base default is `SM_ERR`). pub fn reparameterize(&mut self, k: &Kernel, new_domain: &Extent2d) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.reparameterize(new_domain), SurfaceKind::CurveBounded(_) @@ -2168,6 +2186,7 @@ impl Surface { /// /// Those of the kind's override (the base default is `SM_ERR`). pub fn swap_uv(&mut self, k: &Kernel) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.swap_uv(), SurfaceKind::CurveBounded(_) | SurfaceKind::Step(_) => base_edit(), @@ -2243,6 +2262,7 @@ impl Surface { scale: Option, depth: usize, ) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::Cone(s) => s.transform(k, rotate_n_move, scale), SurfaceKind::Sphere(s) => s.transform(k, rotate_n_move, scale), @@ -2282,6 +2302,7 @@ impl Surface { trim_domain: &mut Extent2d, options: TrimOptions, ) -> Result<()> { + self.boundary_memo = BoundaryMemo::new(); match &mut self.kind { SurfaceKind::BSpline(s) => s.trim_with_domain_with(trim_domain, options), SurfaceKind::CurveBounded(_) | SurfaceKind::Step(_) => base_edit(), diff --git a/crates/solidcore/src/surface/surface/p8.rs b/crates/solidcore/src/surface/surface/p8.rs index a309cffdef..e88f425a74 100644 --- a/crates/solidcore/src/surface/surface/p8.rs +++ b/crates/solidcore/src/surface/surface/p8.rs @@ -16,6 +16,12 @@ //! | [`extract_iso_curve`] | `sm_ExtractIsoCurve` (static) | //! | [`intersect_srf_bndry_crv`] | `sm_IntersectSrfBndryCrv` (static) | //! +//! Deviations (not upstream, beyond the port): `CoincidenceCheck` keeps the surfaces' +//! natural boundary curves in a memo in each surface value (`BoundaryMemo`), where +//! upstream makes them again for every check. The curves are a function of the surface +//! value, the domain and the kernel's tolerances, so a hit gives what a fresh build +//! gives; `Kernel::memoize_curve_caches` turns it off with the curve cache memo. +//! //! Deviations: out-parameters become return values. The caller's object handles //! (`O`, the objects named in solutions) replace the `this` and `&crCurve` pointers //! upstream stores in them. Solvers read the patches of the surface cache they were @@ -64,6 +70,7 @@ use super::p2::{ use super::p4::{is_point_on_normal, NoObject}; use super::p5::DropPointOptions; use super::p6::SampleOptions; +use crate::curve::curve::p5::CurveMemoKey; use crate::curve::curve::{Curve, CurveKind}; use crate::curve::curve_cache::CurveCache; use crate::curve::line::Line; @@ -96,6 +103,8 @@ use crate::surface::plane::Coincidence; use crate::surface::surface::p7::SpGlobalSolver; use crate::surface::surface_cache::p1::SurfaceCache; use crate::surface::surface_cache::BezierPatch; +use std::borrow::Cow; +use std::sync::OnceLock; /// A surface's solve cache kept by a caller across [`Surface::global_line_intersect_held`] /// calls (upstream keeps one cache on each surface): `None` until the first call @@ -304,6 +313,104 @@ fn b_spline_part(curve: &Curve) -> Option<&BSplineCurve> { /// The 3D side curves of the natural UV trim loop of `surface` over `domain` /// (`CreateNaturalUVTrimCurves(ctx, domain, SM_SP_V, FALSE, ..)`); the status is /// not checked upstream. +/// The key of a memoised [`trim_curves`] result (not upstream): the kernel inputs of the +/// build ([`CurveMemoKey::of_kernel`]) and the domain's bits. +#[derive(Debug, Clone, Copy, PartialEq)] +struct BoundaryMemoKey { + kernel: CurveMemoKey, + domain: [u64; 4], +} + +impl BoundaryMemoKey { + /// The key of `trim_curves(k, surface, domain)`, or `None` when it must not be + /// memoised: the memo is off, or the surface is not a pure value. B-spline, plane, + /// cone, sphere and torus surfaces build their sides from their own value (their + /// `CreateIsoParametricCurve` overrides read nothing else); the other kinds hold + /// curves, other surfaces or volumes, or approximate through the base body. + fn of(k: &Kernel, surface: &Surface, domain: &Extent2d) -> Option { + let pure_kind = matches!( + surface.kind(), + SurfaceKind::BSpline(_) + | SurfaceKind::Plane(_) + | SurfaceKind::Cone(_) + | SurfaceKind::Sphere(_) + | SurfaceKind::Torus(_) + ); + if !pure_kind { + return None; + } + Some(Self { + kernel: CurveMemoKey::of_kernel(k)?, + domain: [ + domain.min().x.to_bits(), + domain.min().y.to_bits(), + domain.max().x.to_bits(), + domain.max().y.to_bits(), + ], + }) + } +} + +/// One memoised [`trim_curves`] result. +#[derive(Debug)] +struct BoundaryMemoEntry { + key: BoundaryMemoKey, + curves: Vec>, +} + +/// The memo of a surface's natural boundary curves for `CoincidenceCheck` (not +/// upstream: upstream makes the four curves again for every check, here every face +/// pair of a boolean). It lives in the [`Surface`] value and is filled once; a copy +/// starts an empty one and an edit through `&mut Surface` replaces it. The curves are a +/// function of the surface value and the key, so a hit gives what a fresh build gives, +/// and the curves carry their own curve cache memos, so their caches are built once +/// too. Lent by reference: the check only reads the curves. One entry per live surface, +/// freed with it; no `unsafe`, and the surface stays `Send`. +#[derive(Default)] +pub(crate) struct BoundaryMemo(OnceLock); + +impl core::fmt::Debug for BoundaryMemo { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + // the memo is not part of the surface's observable state + f.write_str("BoundaryMemo") + } +} + +impl BoundaryMemo { + /// An empty memo. + pub(crate) const fn new() -> Self { + Self(OnceLock::new()) + } +} + +/// [`trim_curves`] through the surface's [`BoundaryMemo`]: borrowed from the memo on a +/// hit (or after filling it), built and owned when the key differs or the surface is +/// not memoised. +fn boundary_curves<'s>( + k: &Kernel, + surface: &'s Surface, + domain: &Extent2d, +) -> Result]>> { + let Some(key) = BoundaryMemoKey::of(k, surface, domain) else { + return trim_curves(k, surface, domain).map(Cow::Owned); + }; + let memo = &surface.boundary_memo().0; + if let Some(entry) = memo.get() { + if entry.key == key { + return Ok(Cow::Borrowed(&entry.curves)); + } + return trim_curves(k, surface, domain).map(Cow::Owned); + } + let curves = trim_curves(k, surface, domain)?; + if let Err(entry) = memo.set(BoundaryMemoEntry { key, curves }) { + return Ok(Cow::Owned(entry.curves)); + } + match memo.get() { + Some(entry) => Ok(Cow::Borrowed(&entry.curves)), + None => trim_curves(k, surface, domain).map(Cow::Owned), + } +} + fn trim_curves(k: &Kernel, surface: &Surface, domain: &Extent2d) -> Result>> { let t = ignored(surface.create_natural_uv_trim_curves(k, domain, SurfParamType::V, false))?; Ok(t.map(|t| t.curves_3d.into_iter().map(Some).collect()) @@ -480,8 +587,11 @@ impl Surface { // Now we're looking for any partial overlap. // Create two sets of boundary curves. - let mut bdrys0 = trim_curves(k, self, &domain0)?; - let mut bdrys1 = trim_curves(k, other, &domain1)?; + // (not upstream: kept on the surfaces across checks, see BoundaryMemo) + let all0 = boundary_curves(k, self, &domain0)?; + let all1 = boundary_curves(k, other, &domain1)?; + let mut bdrys0: Vec> = all0.iter().map(Option::as_ref).collect(); + let mut bdrys1: Vec> = all1.iter().map(Option::as_ref).collect(); // We check angles a lot, so get a good angular tolerance. let bbox = bbox0.union(&bbox1); @@ -3691,4 +3801,75 @@ mod tests { ); } } + + /// Two overlapping bilinear patches in the same plane, the second one shifted. + fn overlapping_pair() -> (Surface, Surface) { + let patch = |x: f64| { + Surface::from( + BSplineSurface::create_bilinear_surface( + Vec3::new(x, 0.0, 0.0), + Vec3::new(x + 2.0, 0.0, 0.0), + Vec3::new(x, 3.0, 0.0), + Vec3::new(x + 2.0, 3.0, 0.0), + ) + .unwrap(), + ) + }; + (patch(0.0), patch(1.0)) + } + + #[test] + fn boundary_memo_hit_is_the_fresh_build() { + let on = Kernel { + memoize_curve_caches: true, + ..Kernel::default() + }; + let off = Kernel { + memoize_curve_caches: false, + ..Kernel::default() + }; + let (a, b) = overlapping_pair(); + let want = format!( + "{:?}", + a.base_coincidence_check(&off, &b, 1e-6, false).unwrap() + ); + assert!(a.boundary_memo().0.get().is_none()); + let (mut a, b) = overlapping_pair(); + let first = format!( + "{:?}", + a.base_coincidence_check(&on, &b, 1e-6, false).unwrap() + ); + assert!(a.boundary_memo().0.get().is_some()); + assert!(b.boundary_memo().0.get().is_some()); + let second = format!( + "{:?}", + a.base_coincidence_check(&on, &b, 1e-6, false).unwrap() + ); + assert_eq!(first, want); + assert_eq!(second, want); + // a copy starts empty, an edit empties the memo + assert!(a.clone().boundary_memo().0.get().is_none()); + let _ = a.kind_mut(); + assert!(a.boundary_memo().0.get().is_none()); + // other tolerances miss and build again, with the same result + let mut loose = Kernel { + memoize_curve_caches: true, + ..Kernel::default() + }; + loose.tolerances.zone_tol_3d *= 10.0; + let (a2, b2) = overlapping_pair(); + let _ = a2.base_coincidence_check(&on, &b2, 1e-6, false).unwrap(); + let fresh = format!( + "{:?}", + overlapping_pair() + .0 + .base_coincidence_check(&loose, &b2, 1e-6, false) + .unwrap() + ); + let missed = format!( + "{:?}", + a2.base_coincidence_check(&loose, &b2, 1e-6, false).unwrap() + ); + assert_eq!(missed, fresh); + } } diff --git a/crates/solidcore/tests/curve_cache_memo.rs b/crates/solidcore/tests/curve_cache_memo.rs index e40e8cab9d..696d7f603c 100644 --- a/crates/solidcore/tests/curve_cache_memo.rs +++ b/crates/solidcore/tests/curve_cache_memo.rs @@ -95,17 +95,30 @@ fn operands(k: &mut Kernel, name: &str) -> solidcore::Result<(Brep, Brep, Op)> { api::create_box(k, p(-30.0, -30.0, 10.0), 60.0, 60.0, 20.0)?, Op::Intersection, ), + // analytic surfaces against B-spline surfaces (both memoise their boundaries) + "skin-minus-cyl" => ( + skin(k, 20.0, 12.0, 0.0, 40.0, 5)?, + api::create_cylinder(k, p(0.0, 0.0, -5.0), 4.0, 50.0)?, + Op::Difference, + ), + "skin-and-sphere" => ( + skin(k, 20.0, 12.0, 0.0, 40.0, 5)?, + api::create_sphere(k, p(5.0, 0.0, 20.0), 12.0)?, + Op::Intersection, + ), _ => unreachable!("unknown case {name}"), }) } -const CASES: [&str; 6] = [ +const CASES: [&str; 8] = [ "box-cyl-hole", "sphere-and-box", "torus-minus-box", "cyl-union-cyl", "skin-minus-skin", "skin-and-box", + "skin-minus-cyl", + "skin-and-sphere", ]; /// Every observable output of one case as text (`{:?}` prints each `f64` so that it diff --git a/plan/findings/surface.surface_p8--coincidence-boundary-memo.md b/plan/findings/surface.surface_p8--coincidence-boundary-memo.md new file mode 100644 index 0000000000..fc2d1044ca --- /dev/null +++ b/plan/findings/surface.surface_p8--coincidence-boundary-memo.md @@ -0,0 +1,31 @@ +# surface.surface_p8: CoincidenceCheck keeps each surface's boundary curves (2026-10-25) + +Unit: `surface.surface_p8` (`SmSurface::CoincidenceCheck`, base body), with +`surface.surface_p2` (the `Surface` field). Not upstream: this goes beyond the C++. + +**Upstream.** `CoincidenceCheck` (`SmSurface.cpp:7553-7580`) calls +`CreateNaturalUVTrimCurves` on both surfaces for every check and deletes the curves at +the end (`SmObjsDelete`), so their `SmCacheMgr` entries go with them. A boolean runs the +check for every face pair, so the C++ builds the same boundary curves and their curve +caches again for every pair. On the #948 loft at 20 sections both kernels made 600 +curve-cache builds there (gdb on `Tessellate`), 30 % of the C++ samples at 40 sections. + +**Now.** Each `Surface` value keeps the result of its natural boundary curves in a memo +(`BoundaryMemo`, a `OnceLock`), keyed by the domain's bits and the kernel inputs of the +build (`CurveMemoKey::of_kernel`: tolerances and the context scalars). The check +borrows the curves (it only reads them), and the curves carry their own curve cache +memos (`plan/findings/curve.curve_p5--curve-cache-memo.md`), so each boundary curve's +cache is built once per surface value. Only B-spline, plane, cone, sphere and torus +surfaces are memoised: their iso-curve overrides read nothing but their own value. A +copy of a surface starts with an empty memo; an edit through `&mut Surface` replaces it. +One entry per live surface, freed with it. No `unsafe`; `Surface` stays `Send`. + +**Why no result can depend on it.** `CreateNaturalUVTrimCurves` takes `&Kernel` and the +surface; its result is a function of the surface value, the domain and the key, so a +hit gives what a fresh build gives. `Kernel::memoize_curve_caches` (and the +`curve-memo-off` feature) turns it off; the unit test +`boundary_memo_hit_is_the_fresh_build` and `tests/curve_cache_memo.rs` compare both +positions bit for bit. + +**After.** Same loft and method: 128 builds in `CoincidenceCheck` (C++ 600), 937 in the +whole run (C++ 1,393).