From 809207ff481ea1e1403cd5dcc0e932e4963da263 Mon Sep 17 00:00:00 2001 From: Brett Lawson Date: Fri, 30 Oct 2026 07:00:18 +0100 Subject: [PATCH] fix(topology): name geometry owners by one identity, so ops on caller-keyed bodies check valid (#1631) An operation on a NamingScheme::Caller body (an api_brep_copy that records its caller's keys, #1585) left the edge uses it made without a recorded key, and check_brep refused the result with InvalidInput. Upstream's owner identity is the entity itself, so binder and checker always agree. Brep::owner_identity is now that single identity: under Caller the recorded key, else (and under every other scheme) the scheme's default. The UV trim binder (owner_ref_in), the merge's namespace adoption (faces, edges and edge-use trims, now also under Caller), the fillet's piecewise renaming, default_face_owner/default_edge_owner and check_brep all use it; check_brep no longer refuses. MakeEdgeInFace records the caller key it names its UV trims by, as it does for the edge's curve. The arena scheme now names edge-use trims by the notification identity the check uses (was the raw (16005, index)). Regression test: tests/issue_1631_caller_keyed_ops_check.rs (union of copies and copy with box, fillet session and fillet_edges, shell, extrude-onto, union of two plain boxes); each fails before this change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MNXLXj2sJf943y2dDF36Vy --- .../insertion/geometry/topology_objects.rs | 6 +- .../fillet_executive/session/piecewise.rs | 27 ++- .../src/boolean/merge/p4/kernel_adapter.rs | 46 ++-- crates/solidcore/src/topology.rs | 51 +++++ crates/solidcore/src/topology/brep/p1.rs | 41 ++-- crates/solidcore/src/topology/brep/p9.rs | 10 +- .../topology/brep/validation/assert_valid.rs | 70 ++---- crates/solidcore/src/topology/edgeuse/p1.rs | 27 +-- .../tests/issue_1584_check_brep_naming.rs | 13 +- .../issue_1631_caller_keyed_ops_check.rs | 210 ++++++++++++++++++ 10 files changed, 351 insertions(+), 150 deletions(-) create mode 100644 crates/solidcore/tests/issue_1631_caller_keyed_ops_check.rs diff --git a/crates/solidcore/src/blend/fillet_executive/session/insertion/geometry/topology_objects.rs b/crates/solidcore/src/blend/fillet_executive/session/insertion/geometry/topology_objects.rs index 6fcff0fae3..8c97f6d1f8 100644 --- a/crates/solidcore/src/blend/fillet_executive/session/insertion/geometry/topology_objects.rs +++ b/crates/solidcore/src/blend/fillet_executive/session/insertion/geometry/topology_objects.rs @@ -339,8 +339,10 @@ mod tests { // A UV trim curve named by its B-rep's naming scheme (`SetUVTrimCurve`): the // intermediate box has an edge use of the same index and notify identity, so // the name resolves only in the walked curve's own body. - let named = crate::topology::edgeuse::p1::owner_ref(NamingScheme::Notify, eu); - assert!(named.is_some()); + let named = Some(crate::topology::scheme_owner_identity( + NamingScheme::Notify, + EntityRef::Edgeuse(eu), + )); assert_ne!(named, Some(eu_ref)); view.target.edgeuse_mut(eu).unwrap().uv_trim_curve = Some(line_owned_by(named)); assert_eq!( diff --git a/crates/solidcore/src/blend/fillet_executive/session/piecewise.rs b/crates/solidcore/src/blend/fillet_executive/session/piecewise.rs index 3ab00568da..49a589aa52 100644 --- a/crates/solidcore/src/blend/fillet_executive/session/piecewise.rs +++ b/crates/solidcore/src/blend/fillet_executive/session/piecewise.rs @@ -56,8 +56,6 @@ use crate::sweep::topology_sweep::access::SweepAccess; use crate::sweep::topology_sweep::store::{SweepIdentities, SweepStore}; use crate::topology::brep::p3::{seed_records, topology_ref}; use crate::topology::brep::{p1, p2, p5}; -use crate::topology::brep_cache::{arena_identity, CacheObject}; -use crate::topology::loopuse::notify_ref; use crate::topology::{ edgeuse, face, vertex, Brep, BrepId, EdgeId, FaceId, NamingScheme, RegionId, }; @@ -221,14 +219,16 @@ fn normalize_session_owners( Ok(()) } -/// The identity a B-rep of naming `naming` owns an edge's or face's geometry under, -/// `None` for a caller's or an unknown scheme. -fn naming_owner(naming: NamingScheme, e: EntityRef) -> Option { - match (naming, e) { - (NamingScheme::Arena, EntityRef::Edge(x)) => Some(arena_identity(CacheObject::Edge(x))), - (NamingScheme::Arena, EntityRef::Face(x)) => Some(arena_identity(CacheObject::Face(x))), - (NamingScheme::Notify, e) => Some(notify_ref(e)), - _ => None, +/// The identity a B-rep of naming `naming` that records `recorded` for `e` owns `e`'s +/// geometry under ([`Brep::owner_identity`]), `None` for an unknown scheme. +fn naming_owner( + naming: NamingScheme, + recorded: Option, + e: EntityRef, +) -> Option { + match naming { + NamingScheme::Unknown => None, + naming => Some(crate::topology::owner_identity_of(naming, recorded, e)), } } @@ -260,7 +260,7 @@ fn surface_of(brep: &Brep, face: FaceId) -> Result> { /// identity with an equal raw key names a different owner and is not accepted. fn own_identities(b: &Brep, e: EntityRef) -> Vec { let mut ids = Vec::new(); - ids.extend(naming_owner(b.naming(), e)); + ids.extend(naming_owner(b.naming(), b.geometry_owner_key(e), e)); ids.extend(b.geometry_owner_key(e)); ids } @@ -519,6 +519,9 @@ impl FilletSession<'_> { // the target's session identities, which return to its naming afterwards. let target_serial = self.objects.target_serial; let naming = target.naming(); + // The keys a caller-named target recorded before the merge, its entities' + // names afterwards (a merge-made entity has none and takes the scheme's). + let recorded = target.geometry_owner_keys().clone(); let session_owner = |e| topology_ref(target_serial, e); let issued = issue_actual_curve_identities(&mut k.context, &mut target) .and_then(|()| issue_actual_curve_identities(&mut k.context, &mut intermediate)); @@ -574,7 +577,7 @@ impl FilletSession<'_> { drop(store); let mut target = target.ok_or_else(|| invalid("target lost in the merge"))?; let restored = normalize_session_owners(&mut target, session_owner, |e| { - naming_owner(naming, e).unwrap_or_else(|| session_owner(e)) + naming_owner(naming, recorded.get(&e).copied(), e).unwrap_or_else(|| session_owner(e)) }); target.set_naming(naming); *self.objects.target = target; diff --git a/crates/solidcore/src/boolean/merge/p4/kernel_adapter.rs b/crates/solidcore/src/boolean/merge/p4/kernel_adapter.rs index 24ce41eb45..66f9fd5f55 100644 --- a/crates/solidcore/src/boolean/merge/p4/kernel_adapter.rs +++ b/crates/solidcore/src/boolean/merge/p4/kernel_adapter.rs @@ -131,12 +131,12 @@ pub(crate) fn entity_of_notification_key( /// own naming: a face's surface (an edge's curve) owned under that face's (edge's) /// [`notification_key`] in `namespace` is the record's own geometry (upstream's owner /// is the record), so it takes the record's name in `b`'s recorded scheme -/// ([`Brep::naming`](crate::topology::Brep::naming): notification or arena identity) -/// and the face's tolerance is re-recorded on it. An edge use's UV trim curve owned -/// under the edge use's key takes the edge use's notification identity (upstream's owner -/// is the edge use; `SmEdgeuse::AssertValid` test #6). Called when the namespace ends with -/// the merge (its records and attributes go with it); a B-rep of another or unknown -/// scheme is left as it is. +/// ([`Brep::owner_identity`]: notification or arena identity, or a caller's recorded +/// key) and the face's tolerance is re-recorded on it. An edge use's UV trim curve +/// owned under the edge use's key likewise takes the edge use's name (upstream's owner +/// is the edge use; `SmEdgeuse::AssertValid` test #6). Called when the namespace ends +/// with the merge (its records and attributes go with it); a B-rep of unknown scheme +/// is left as it is. /// /// # Errors /// @@ -145,13 +145,8 @@ pub(crate) fn adopt_namespace_owners(b: &mut Brep, namespace: u32, other: bool) use crate::topology::NamingScheme; let name = |e: EntityRef| -> Option { match b.naming() { - NamingScheme::Notify => Some(crate::topology::loopuse::notify_ref(e)), - NamingScheme::Arena => Some(match e { - EntityRef::Face(f) => arena_identity(CacheObject::Face(f)), - EntityRef::Edge(x) => arena_identity(CacheObject::Edge(x)), - _ => return None, - }), - _ => None, + NamingScheme::Unknown => None, + _ => Some(b.owner_identity(e)), } }; let faces: Vec<(FaceId, ObjectRef)> = bp::faces(b, None)? @@ -191,16 +186,16 @@ pub(crate) fn adopt_namespace_owners(b: &mut Brep, namespace: u32, other: bool) } } // An edge use's UV trim curve is owned by the edge use (`SmEdgeuse` test #6); one - // the merge set under the namespace takes the edge use's own name. Both schemes - // name edge uses by their notification identity (the check's keys for an arena - // B-rep fall back to it too). - if matches!(b.naming(), NamingScheme::Notify | NamingScheme::Arena) { + // the merge set under the namespace takes the edge use's own name + // ([`Brep::owner_identity`]: its notification identity under the notification and + // arena schemes, its recorded key or else that identity under a caller's). + if b.naming() != NamingScheme::Unknown { let uses: Vec<_> = b.edgeuses().iter().map(|(id, _)| id).collect(); for eu in uses { let merged = notification_key(namespace, other, EntityRef::Edgeuse(eu)); + let own = b.owner_identity(EntityRef::Edgeuse(eu)); if let Some(trim) = b.edgeuse_mut(eu)?.uv_trim_curve.as_mut() { if trim.owner() == Some(merged) { - let own = crate::topology::loopuse::notify_ref(EntityRef::Edgeuse(eu)); trim.set_owner(Some(own), None); } } @@ -221,7 +216,6 @@ struct GeometryOwnerScope<'a, 'keys> { } impl<'a, 'keys> GeometryOwnerScope<'a, 'keys> { fn new(body: &'a mut Brep, keys: NotificationKeys<'keys>, other: bool) -> Result { - use crate::topology::NamingScheme; let naming = body.naming(); let mut scope = Self { body, @@ -232,11 +226,8 @@ impl<'a, 'keys> GeometryOwnerScope<'a, 'keys> { }; if matches!(keys, NotificationKeys::Namespace(_)) { for edge in crate::topology::brep::p1::edges(scope.body, None)? { - let expected = if naming == NamingScheme::Notify { - crate::topology::loopuse::notify_ref(EntityRef::Edge(edge)) - } else { - arena_identity(CacheObject::Edge(edge)) - }; + let expected = + crate::topology::scheme_owner_identity(naming, EntityRef::Edge(edge)); if let Some(curve) = scope.body.edge_mut(edge)?.curve.as_mut() { if curve.owner() == Some(expected) { scope.edges.push((edge, expected)); @@ -245,11 +236,8 @@ impl<'a, 'keys> GeometryOwnerScope<'a, 'keys> { } } for face in crate::topology::brep::p1::faces(scope.body, None)? { - let expected = if naming == NamingScheme::Notify { - crate::topology::loopuse::notify_ref(EntityRef::Face(face)) - } else { - arena_identity(CacheObject::Face(face)) - }; + let expected = + crate::topology::scheme_owner_identity(naming, EntityRef::Face(face)); let rebound = scope .body .face_mut(face)? diff --git a/crates/solidcore/src/topology.rs b/crates/solidcore/src/topology.rs index 8d6071e4ab..6db903890c 100644 --- a/crates/solidcore/src/topology.rs +++ b/crates/solidcore/src/topology.rs @@ -697,6 +697,43 @@ pub enum NamingScheme { Arena, } +/// The identity `entity` owns geometry under in a B-rep of naming `naming` that +/// records `recorded` for it: [`Brep::owner_identity`] without the B-rep. +#[must_use] +pub(crate) fn owner_identity_of( + naming: NamingScheme, + recorded: Option, + entity: EntityRef, +) -> crate::geom::object::ObjectRef { + match (naming, recorded) { + (NamingScheme::Caller, Some(key)) => key, + _ => scheme_owner_identity(naming, entity), + } +} + +/// The identity a B-rep of naming `naming` names `entity` by when it records none: +/// the Euler operators' notification identity ([`loopuse::notify_ref`]) under +/// [`NamingScheme::Notify`], else the arena identity of the crate's imported B-reps +/// ([`brep_cache::arena_identity`]) for edges, faces, vertices and the B-rep, and the +/// notification identity for the other entities (edge uses among them). +#[must_use] +pub(crate) fn scheme_owner_identity( + naming: NamingScheme, + entity: EntityRef, +) -> crate::geom::object::ObjectRef { + use brep_cache::{arena_identity, CacheObject}; + if naming == NamingScheme::Notify { + return loopuse::notify_ref(entity); + } + match entity { + EntityRef::Edge(x) => arena_identity(CacheObject::Edge(x)), + EntityRef::Face(x) => arena_identity(CacheObject::Face(x)), + EntityRef::Vertex(x) => arena_identity(CacheObject::Vertex(x)), + EntityRef::Brep(_) => arena_identity(CacheObject::Brep), + other => loopuse::notify_ref(other), + } +} + /// A boundary representation (`SmBrep`): arenas of shells, faces, loops, edges and /// vertices linked by typed ids. #[derive(Debug)] @@ -987,6 +1024,20 @@ impl Brep { Ok(()) } + /// The identity `entity` owns geometry under (upstream: the entity itself, + /// `pGeom->GetOwner() == this`): under [`NamingScheme::Caller`] the key recorded + /// for it where it was built or its geometry bound, else, and under every other + /// scheme, the scheme's own identity (the notification identity under + /// [`NamingScheme::Notify`], otherwise the arena identity of an edge, face or + /// vertex and the notification identity of any other entity). Geometry binders name new + /// geometry by it and [`check_brep`](crate::topology::brep::validation::check_brep) + /// checks owners against it, so the two always agree (upstream's identity exists + /// from the moment the entity does). + #[must_use] + pub fn owner_identity(&self, entity: EntityRef) -> crate::geom::object::ObjectRef { + owner_identity_of(self.naming, self.geometry_owner_key(entity), entity) + } + /// Producer-supplied owner identity recorded when geometry was bound to `entity`. #[must_use] pub(crate) fn geometry_owner_key( diff --git a/crates/solidcore/src/topology/brep/p1.rs b/crates/solidcore/src/topology/brep/p1.rs index 51d48c4563..6e083cbebf 100644 --- a/crates/solidcore/src/topology/brep/p1.rs +++ b/crates/solidcore/src/topology/brep/p1.rs @@ -63,7 +63,7 @@ //! reported by [`regions`]. //! - [`surfaces`] / [`curves`]: upstream's `GetOwner() == pFace` / `pEdge` test compares //! the geometry's owner with the face's / edge's -//! [`arena_identity`] (the kernel's +//! [`arena_identity`](crate::topology::brep_cache::arena_identity) (the kernel's //! identity of arena entities; a B-rep whose geometry carries no owner reports no //! surfaces or curves, as upstream for unowned geometry); the attribute filter //! ([`GeometryAttributeFilter`]) looks the geometry up under its own keys. @@ -141,7 +141,7 @@ use crate::query::brep::{self as qbrep, BoundingBoxOptions}; use crate::surface::surface::p2::{BlendEndType, CurvatureType, DerivSurfDefinition}; use crate::surface::surface::p4::NoObject; use crate::surface::surface::Surface; -use crate::topology::brep_cache::{arena_identity, CacheObject}; +use crate::topology::brep_cache::CacheObject; use crate::topology::edge::{self, AttributeFilter}; use crate::topology::edgeuse::p1 as eu1; use crate::topology::edgeuse::p2::BinormalEval; @@ -155,8 +155,7 @@ use crate::topology::topology_traverser::{Entity, Marks}; use crate::topology::vertex; use crate::topology::vertexuse; use crate::topology::{ - Brep, EdgeId, EdgeuseId, FaceId, LoopId, NamingScheme, RegionId, ShellId, VertexId, Vertexuse, - VertexuseId, + Brep, EdgeId, EdgeuseId, FaceId, LoopId, RegionId, ShellId, VertexId, Vertexuse, VertexuseId, }; /// `SmBoolean` `TRUE` / `FALSE`. @@ -671,42 +670,36 @@ impl core::fmt::Debug for GeometryAttributeFilter<'_> { } } -/// The default expected owner of `f`'s surface under `b`'s recorded naming scheme, -/// falling back to a proven caller owner recorded on `b`. +/// The default expected owner of `f`'s surface: its identity in `b`'s recorded naming +/// scheme ([`Brep::owner_identity`]), falling back to a proven owner recorded on `b`. pub(crate) fn default_face_owner(b: &Brep, f: FaceId) -> ObjectRef { - let fallback = if b.naming() == NamingScheme::Notify { - loopuse::notify_ref(EntityRef::Face(f)) - } else { - arena_identity(CacheObject::Face(f)) - }; + let entity = EntityRef::Face(f); + let expected = b.owner_identity(entity); let actual = b.face(f).ok().and_then(|x| x.surface.as_ref()?.owner()); - if actual != Some(fallback) { - if let Some(proven) = b.geometry_owner_key(EntityRef::Face(f)) { + if actual != Some(expected) { + if let Some(proven) = b.geometry_owner_key(entity) { if actual == Some(proven) { return proven; } } } - fallback + expected } -/// The default expected owner of `e`'s curve under `b`'s recorded naming scheme, -/// falling back to a proven caller owner recorded on `b`. +/// The default expected owner of `e`'s curve: its identity in `b`'s recorded naming +/// scheme ([`Brep::owner_identity`]), falling back to a proven owner recorded on `b`. pub(crate) fn default_edge_owner(b: &Brep, e: EdgeId) -> ObjectRef { - let fallback = if b.naming() == NamingScheme::Notify { - loopuse::notify_ref(EntityRef::Edge(e)) - } else { - arena_identity(CacheObject::Edge(e)) - }; + let entity = EntityRef::Edge(e); + let expected = b.owner_identity(entity); let actual = b.edge(e).ok().and_then(|x| x.curve.as_ref()?.owner()); - if actual != Some(fallback) { - if let Some(proven) = b.geometry_owner_key(EntityRef::Edge(e)) { + if actual != Some(expected) { + if let Some(proven) = b.geometry_owner_key(entity) { if actual == Some(proven) { return proven; } } } - fallback + expected } /// `GetSurfaces`: the surfaces owned by their faces (`pSurface->GetOwner() == pFace`, diff --git a/crates/solidcore/src/topology/brep/p9.rs b/crates/solidcore/src/topology/brep/p9.rs index 53c2c8f015..f63be53f61 100644 --- a/crates/solidcore/src/topology/brep/p9.rs +++ b/crates/solidcore/src/topology/brep/p9.rs @@ -1632,7 +1632,11 @@ pub fn make_edge_in_face_with_vu_with_curve_identity( } else { eu2 }; - trim.set_owner(Some((env.key_of)(EntityRef::Edgeuse(top))), None); + // SetOwner(pEu): named by the caller's key, recorded as for the edge's curve + // (`set_curve_owner`) so the B-rep's checks name the edge use alike + let top_owner = (env.key_of)(EntityRef::Edgeuse(top)); + trim.set_owner(Some(top_owner), None); + b.set_geometry_owner_key(EntityRef::Edgeuse(top), Some(top_owner)); b.edgeuse_mut(top)?.uv_trim_curve = Some(trim); b.edgeuse_mut(top)?.drop_curve_fail = drop_fail; let mut copy = @@ -1661,7 +1665,9 @@ pub fn make_edge_in_face_with_vu_with_curve_identity( } else { eu3 }; - copy.set_owner(Some((env.key_of)(EntityRef::Edgeuse(bottom))), None); + let bottom_owner = (env.key_of)(EntityRef::Edgeuse(bottom)); + copy.set_owner(Some(bottom_owner), None); + b.set_geometry_owner_key(EntityRef::Edgeuse(bottom), Some(bottom_owner)); b.edgeuse_mut(bottom)?.uv_trim_curve = Some(copy); b.edgeuse_mut(bottom)?.drop_curve_fail = fail_copy; original_trim_owner = Some(top); diff --git a/crates/solidcore/src/topology/brep/validation/assert_valid.rs b/crates/solidcore/src/topology/brep/validation/assert_valid.rs index ded1834e8c..39d3a975d8 100644 --- a/crates/solidcore/src/topology/brep/validation/assert_valid.rs +++ b/crates/solidcore/src/topology/brep/validation/assert_valid.rs @@ -48,7 +48,6 @@ mod shell; mod vertex; use std::borrow::Cow; -use std::collections::BTreeMap; use super::{object_info, validate_pointers, Object, SUCCESS}; use crate::error::{EntityRef, Result}; @@ -58,7 +57,7 @@ use crate::geom::assert_array::{ use crate::geom::object::ObjectRef; use crate::kernel::Kernel; use crate::math::UNDEF_DOUBLE; -use crate::topology::{Brep, BrepId, NamingScheme}; +use crate::topology::{Brep, BrepId}; /// `SM_LEVEL_GIVEN`: every test, no base classes. const LEVEL_GIVEN: u8 = 4; @@ -174,63 +173,24 @@ fn report_line(r: &AssertReport) -> String { ) } -/// The arena identities of the crate's imported B-reps -/// ([`crate::topology::brep_cache::arena_identity`]). -fn arena(e: EntityRef) -> ObjectRef { - use crate::topology::brep_cache::{arena_identity, CacheObject}; - match e { - EntityRef::Edge(x) => arena_identity(CacheObject::Edge(x)), - EntityRef::Face(x) => arena_identity(CacheObject::Face(x)), - EntityRef::Vertex(x) => arena_identity(CacheObject::Vertex(x)), - EntityRef::Brep(_) => arena_identity(CacheObject::Brep), - other => crate::topology::loopuse::notify_ref(other), - } -} - -/// The caller keys a [`NamingScheme::Caller`] B-rep recorded for every edge, face and -/// edge use (`Brep::geometry_owner_keys`, the only entities the check names as -/// geometry owners); `None` when one of them has none. -fn recorded_caller_keys(b: &Brep) -> Option> { - let entities = b - .edges() - .ids() - .map(EntityRef::Edge) - .chain(b.faces().ids().map(EntityRef::Face)) - .chain(b.edgeuses().ids().map(EntityRef::Edgeuse)); - let mut keys = BTreeMap::new(); - for e in entities { - keys.insert(e, b.geometry_owner_key(e)?); - } - Some(keys) -} - -/// [`check_brep_with_keys`] with the geometry-owner identities of `b`'s naming scheme: -/// the Euler operators' notification identities for a [`NamingScheme::Notify`] -/// B-rep, the caller keys recorded where a [`NamingScheme::Caller`] B-rep was built -/// (an imported body, [`crate::topology::brep::p3::api_brep_copy`]), else the arena -/// identities ([`crate::topology::brep_cache::arena_identity`]). +/// [`check_brep_with_keys`] with the geometry-owner identities of `b`'s naming scheme +/// ([`Brep::owner_identity`]): the Euler operators' notification identities for a +/// [`NamingScheme::Notify`](crate::topology::NamingScheme::Notify) B-rep; for a +/// [`NamingScheme::Caller`](crate::topology::NamingScheme::Caller) B-rep the caller +/// key recorded where the entity was built or its geometry bound (an imported body, +/// [`crate::topology::brep::p3::api_brep_copy`], an operation on either), else the +/// arena identities ([`crate::topology::brep_cache::arena_identity`]), which the +/// binders name an entity with no recorded key by too. /// /// # Errors /// -/// [`crate::SolidError::InvalidInput`] for a [`NamingScheme::Caller`] B-rep without a -/// recorded key for each edge, face and edge use (pass the maker's keys to -/// [`check_brep_with_keys`]); those of [`check_brep_with_keys`]. +/// Those of [`check_brep_with_keys`]. pub fn check_brep(k: &mut Kernel, b: &mut Brep, level: CheckLevel) -> Result { - match b.naming() { - NamingScheme::Notify => { - check_brep_with_keys(k, b, level, &crate::topology::loopuse::notify_ref) - } - NamingScheme::Caller => { - let recorded = recorded_caller_keys(b).ok_or(crate::SolidError::invalid_input( - "topology::brep::validation::check_brep", - "body names geometry owners by its maker's keys and does not record them \ - all; use check_brep_with_keys", - ))?; - let keys = |e: EntityRef| recorded.get(&e).copied().unwrap_or_else(|| arena(e)); - check_brep_with_keys(k, b, level, &keys) - } - _ => check_brep_with_keys(k, b, level, &arena), - } + let naming = b.naming(); + let recorded = b.geometry_owner_keys().clone(); + let keys = + |e: EntityRef| crate::topology::owner_identity_of(naming, recorded.get(&e).copied(), e); + check_brep_with_keys(k, b, level, &keys) } /// The capi `smlib_brep_check(brep, level)`: [`validate_pointers`] at `level` (its diff --git a/crates/solidcore/src/topology/edgeuse/p1.rs b/crates/solidcore/src/topology/edgeuse/p1.rs index 183ef75be5..78aeaf692d 100644 --- a/crates/solidcore/src/topology/edgeuse/p1.rs +++ b/crates/solidcore/src/topology/edgeuse/p1.rs @@ -94,7 +94,6 @@ use crate::curve::curve::Curve; use crate::error::{EntityRef, Result, SolidError}; -use crate::geom::attribute::ObjectKey; use crate::geom::extent1d::Extent1d; use crate::geom::global_solver::SolverOperation; use crate::geom::local_solve_nd::EvalNFunctions; @@ -830,28 +829,14 @@ pub fn set_uv_trim_curve( Ok(detached) } -/// The identity edge use `eu` of `b` owns geometry under: for a -/// [`NamingScheme::Caller`] B-rep the caller's key recorded for it where the B-rep was -/// built (`Brep::geometry_owner_key`; `None` when none is recorded), else -/// [`owner_ref`] of `b`'s naming. +/// The identity edge use `eu` of `b` owns geometry under ([`Brep::owner_identity`]: +/// for a [`NamingScheme::Caller`] B-rep the key recorded for it, else its scheme's, +/// the identity `check_brep` checks the owner against); `None` when the B-rep records +/// no scheme. pub(crate) fn owner_ref_in(b: &Brep, eu: EdgeuseId) -> Option { match b.naming() { - NamingScheme::Caller => b.geometry_owner_key(EntityRef::Edgeuse(eu)), - naming => owner_ref(naming, eu), - } -} - -/// The identity edge use `eu` owns geometry under in a B-rep with naming `naming` -/// ([`NamingScheme`]); `None` when the B-rep records no scheme, or names its owners by -/// a caller's keys (see [`owner_ref_in`]). -pub(crate) fn owner_ref(naming: NamingScheme, eu: EdgeuseId) -> Option { - match naming { - NamingScheme::Unknown | NamingScheme::Caller => None, - NamingScheme::Notify => Some(crate::topology::loopuse::notify_ref(EntityRef::Edgeuse(eu))), - NamingScheme::Arena => Some(ObjectRef { - type_code: TYPE, - key: ObjectKey(u64::from(eu.index())), - }), + NamingScheme::Unknown => None, + _ => Some(b.owner_identity(EntityRef::Edgeuse(eu))), } } diff --git a/crates/solidcore/tests/issue_1584_check_brep_naming.rs b/crates/solidcore/tests/issue_1584_check_brep_naming.rs index 149c1c886d..f07a089bc5 100644 --- a/crates/solidcore/tests/issue_1584_check_brep_naming.rs +++ b/crates/solidcore/tests/issue_1584_check_brep_naming.rs @@ -19,7 +19,7 @@ use solidcore::topology::brep_data_write::FromBrepEnv; use solidcore::topology::edgeuse::p3::get_or_create_uv_trim_curve; use solidcore::topology::topology::NotifyRecords; use solidcore::topology::{EdgeId, EdgeuseId, FaceId, NamingScheme}; -use solidcore::{Brep, EntityRef, SolidError}; +use solidcore::{Brep, EntityRef}; /// A caller naming: the notification layout shifted by `base`. fn named(e: EntityRef, base: u64) -> ObjectRef { @@ -204,11 +204,14 @@ fn foreign_trim_owner_still_fails_test_6() { } #[test] -fn caller_body_without_keys_is_refused() { +fn caller_body_without_keys_is_checked_under_the_default_identities() { let mut k = Kernel::default(); - // a body built through the Euler operators records no caller keys + // a body that records no caller keys is not refused (#1631): the check names each + // entity by the scheme's default identity, as the binders name an entity without + // a recorded key, and the box's geometry is owned by those let mut b = create_box(&mut k, Vec3::new(0.0, 0.0, 0.0), 10.0, 20.0, 30.0).unwrap(); b.set_naming(NamingScheme::Caller); - let err = check_brep(&mut k, &mut b, CheckLevel::Level2).unwrap_err(); - assert!(matches!(err, SolidError::InvalidInput { .. }), "{err:?}"); + let r = check_brep(&mut k, &mut b, CheckLevel::Level2).unwrap(); + assert!(r.is_valid(), "{:?}", r.lines()); + assert!(r.lines().is_empty(), "{:?}", r.lines()); } diff --git a/crates/solidcore/tests/issue_1631_caller_keyed_ops_check.rs b/crates/solidcore/tests/issue_1631_caller_keyed_ops_check.rs new file mode 100644 index 0000000000..f7260ba46e --- /dev/null +++ b/crates/solidcore/tests/issue_1631_caller_keyed_ops_check.rs @@ -0,0 +1,210 @@ +//! tangency-labs/solidcore#1631: an operation on a `NamingScheme::Caller` body (an +//! `SmApiBrepCopy` that records its caller's keys, #1585) keeps that scheme, and +//! `check_brep` checks the result as the C++ `smlib_brep_check` does: every geometry +//! owner is its entity (`pGeom->GetOwner() == this`). The edges, faces and edge uses +//! an operation creates record no caller key; they are named by the scheme's default +//! identity, by the binders and by the check alike (`Brep::owner_identity`), so the +//! check finds the result valid instead of refusing it with `InvalidInput`. +#![allow(clippy::unwrap_used, clippy::panic, clippy::indexing_slicing)] // test helpers + +#[path = "support/offset_fixture.rs"] +mod offset_fixture; + +use std::f64::consts::PI; + +use solidcore::blend::fillet_executive::api::fillet_edges; +use solidcore::blend::fillet_executive::session::FilletSession; +use solidcore::boolean::api_brep::{boolean, boolean_union}; +use solidcore::curve::curve::Curve; +use solidcore::curve::line::Line; +use solidcore::geom::a_object::BooleanOperation; +use solidcore::geom::attribute::{AttributeStore, ObjectKey}; +use solidcore::geom::object::ObjectRef; +use solidcore::mass::integrator::properties::{brep_volume, PropertyAttributes}; +use solidcore::math::{Point3, Vec3}; +use solidcore::primitive::api_primitives::create_box; +use solidcore::sweep::api_brep::create_linear_sweep; +use solidcore::topology::brep::p2::ArenaNotifyState; +use solidcore::topology::brep::p3::{api_brep_copy, topology_ref}; +use solidcore::topology::brep::p5::{stitch_and_orient, StitchAndOrientOptions}; +use solidcore::topology::brep::validation::{check_brep, CheckLevel}; +use solidcore::topology::{edge, vertex, Brep, EdgeId, NamingScheme}; +use solidcore::{EntityRef, Kernel}; + +/// Each copy names its objects in its own key domain (as SolidCraft's copies do). +fn named(e: EntityRef, domain: u64) -> ObjectRef { + let r = topology_ref(0, e); + ObjectRef { + type_code: r.type_code, + key: ObjectKey(domain + r.type_code as u64 * 1_000_000 + (r.key.0 & 0xffff_ffff)), + } +} + +/// `SmApiBrepCopy` of `b` named in domain `n`: a `NamingScheme::Caller` body with its +/// keys recorded (#1585). +fn copy(k: &mut Kernel, b: &Brep, n: u64) -> Brep { + let target = move |e| named(e, n * 100_000_000); + let mut state = ArenaNotifyState::new(&Brep::new()); + let mut env = state.env(None); + env.key_of = ⌖ + let c = api_brep_copy(k, &mut env, Some(b), &|e| named(e, 0)).unwrap(); + assert_eq!(c.naming(), NamingScheme::Caller); + c +} + +fn volume(k: &Kernel, b: &mut Brep) -> f64 { + let store = AttributeStore::default(); + let attrs = PropertyAttributes { + store: &store, + key_of: &|_| ObjectKey(0), + }; + brep_volume(k, b, 1e-9, &attrs).unwrap() +} + +/// `check_brep` at Level2 finds `b` valid, with no failed test. +fn assert_checks_valid(k: &mut Kernel, b: &mut Brep, what: &str) { + let c = check_brep(k, b, CheckLevel::Level2) + .unwrap_or_else(|e| panic!("{what}: check_brep refused the body: {e}")); + assert!(c.is_valid(), "{what}: {:#?}", c.lines()); + assert!(c.lines().is_empty(), "{what}: {:#?}", c.lines()); +} + +fn assert_volume(k: &Kernel, b: &mut Brep, analytic: f64, what: &str) { + let v = volume(k, b); + assert!( + (v - analytic).abs() <= 1e-9 * analytic, + "{what}: volume {v}, analytic {analytic}" + ); +} + +fn boxes(k: &mut Kernel) -> (Brep, Brep) { + let a = create_box(k, Point3::new(0.0, 0.0, 0.0), 10.0, 10.0, 10.0).unwrap(); + let b = create_box(k, Point3::new(5.0, 5.0, 5.0), 10.0, 10.0, 10.0).unwrap(); + (a, b) +} + +/// Two 10 mm cubes overlapping in a 5 mm cube: 2 * 1000 - 125. +const UNION_VOLUME: f64 = 1875.0; + +/// The issue's first repro: the union of two caller-keyed copies. +#[test] +fn union_of_two_keyed_copies_checks_valid() { + let mut k = Kernel::default(); + let (a, b) = boxes(&mut k); + let (mut ca, cb) = (copy(&mut k, &a, 1), copy(&mut k, &b, 2)); + assert_checks_valid(&mut k, &mut ca, "copy"); + let mut u = boolean(&mut k, ca, cb, BooleanOperation::Union) + .unwrap() + .result; + assert_eq!(u.naming(), NamingScheme::Caller); + assert_volume(&k, &mut u, UNION_VOLUME, "union(copy, copy)"); + assert_checks_valid(&mut k, &mut u, "union(copy, copy)"); +} + +#[test] +fn union_of_keyed_copy_with_box_checks_valid() { + let mut k = Kernel::default(); + let (a, b) = boxes(&mut k); + let ca = copy(&mut k, &a, 1); + let mut u = boolean_union(&mut k, ca, b).unwrap(); + assert_volume(&k, &mut u, UNION_VOLUME, "union(copy, box)"); + assert_checks_valid(&mut k, &mut u, "union(copy, box)"); +} + +/// Side finding: the union of two plain boxes (a notification-named result) left the +/// original box's geometry without owner (`SmBrep` test #9, `SmEdge` test #61). +#[test] +fn union_of_two_boxes_checks_valid() { + let mut k = Kernel::default(); + let (a, b) = boxes(&mut k); + let mut u = boolean_union(&mut k, a, b).unwrap(); + assert_volume(&k, &mut u, UNION_VOLUME, "union(box, box)"); + assert_checks_valid(&mut k, &mut u, "union(box, box)"); +} + +/// The box's top front edge, along x at y = 0, z = `top`. +fn top_front_edge(b: &Brep, top: f64) -> EdgeId { + b.edges() + .ids() + .find(|&e| { + let (s, t) = edge::start_end_vertices(b, e).unwrap(); + let (p, q) = ( + vertex::point(b, s.unwrap()).unwrap(), + vertex::point(b, t.unwrap()).unwrap(), + ); + p.y.abs() < 1e-9 && q.y.abs() < 1e-9 && p.z == top && q.z == top + }) + .unwrap() +} + +/// The issue's second repro: a 30 x 20 x 10 copy with its top front edge rounded r2, +/// 6000 - (1 - pi/4) * 2^2 * 30. +#[test] +fn fillet_session_on_keyed_copy_checks_valid() { + let mut k = Kernel::default(); + let a = create_box(&mut k, Point3::new(0.0, 0.0, 0.0), 30.0, 20.0, 10.0).unwrap(); + let mut c = copy(&mut k, &a, 1); + let e = top_front_edge(&c, 10.0); + { + let mut s = FilletSession::new(&mut k, &mut c).unwrap(); + s.prepare_constant_radius_edges(&mut k, &[e], 2.0, 1, 1, 1.0) + .unwrap(); + s.create_corners(&k, None).unwrap(); + s.do_filleting(&mut k, None).unwrap(); + } + stitch_and_orient(&mut k, &mut c, &StitchAndOrientOptions::default()).unwrap(); + assert_eq!(c.naming(), NamingScheme::Caller); + let analytic = 6000.0 - (1.0 - PI / 4.0) * 4.0 * 30.0; + assert_volume(&k, &mut c, analytic, "fillet session"); + assert_checks_valid(&mut k, &mut c, "fillet session"); +} + +/// `fillet_edges` (`SmApiFilletEdges`) of a 10 mm keyed copy's edge, r1: +/// 1000 - (1 - pi/4) * 10. +#[test] +fn fillet_edges_of_keyed_copy_checks_valid() { + let mut k = Kernel::default(); + let a = create_box(&mut k, Point3::new(0.0, 0.0, 0.0), 10.0, 10.0, 10.0).unwrap(); + let mut c = copy(&mut k, &a, 1); + let e = top_front_edge(&c, 10.0); + fillet_edges(&mut k, &mut c, &[e], 1.0, 1, 1, 1.0).unwrap(); + assert_eq!(c.naming(), NamingScheme::Caller); + let analytic = 1000.0 - (1.0 - PI / 4.0) * 10.0; + assert_volume(&k, &mut c, analytic, "fillet_edges"); + assert_checks_valid(&mut k, &mut c, "fillet_edges"); +} + +/// A keyed copy of a 10 mm box shelled inward by 1 with its top open: +/// 1000 - 8 * 8 * 9. +#[test] +fn shell_of_keyed_copy_checks_valid() { + let mut k = Kernel::default(); + let a = create_box(&mut k, Point3::new(0.0, 0.0, 0.0), 10.0, 10.0, 10.0).unwrap(); + let c = copy(&mut k, &a, 1); + let top = offset_fixture::planar_faces_at(&k, &c, 2, 10.0); + assert_eq!(top.len(), 1); + let mut s = offset_fixture::shell(&mut k, c, -1.0, &top).unwrap(); + assert_eq!(s.naming(), NamingScheme::Caller); + assert_volume(&k, &mut s, 1000.0 - 576.0, "shell"); + assert_checks_valid(&mut k, &mut s, "shell"); +} + +fn line(a: Point3, b: Point3) -> Curve { + Curve::from(Line::create_line_segment(3, a, b, None).unwrap()) +} + +/// A 4 x 4 boss extruded 5 from the top of a keyed copy of a 10 mm box and united +/// with it: 1000 + 4 * 4 * 5. +#[test] +fn extrude_onto_keyed_copy_checks_valid() { + let mut k = Kernel::default(); + let a = create_box(&mut k, Point3::new(0.0, 0.0, 0.0), 10.0, 10.0, 10.0).unwrap(); + let c = copy(&mut k, &a, 1); + let p = [(2.0, 2.0), (6.0, 2.0), (6.0, 6.0), (2.0, 6.0)].map(|(x, y)| Point3::new(x, y, 10.0)); + let sides: Vec = (0..4).map(|i| line(p[i], p[(i + 1) % 4])).collect(); + let refs: Vec> = sides.iter().map(Some).collect(); + let boss = create_linear_sweep(&mut k, &refs, Vec3::new(0.0, 0.0, 1.0), 5.0, true).unwrap(); + let mut u = boolean_union(&mut k, c, boss).unwrap(); + assert_volume(&k, &mut u, 1080.0, "extrude onto copy"); + assert_checks_valid(&mut k, &mut u, "extrude onto copy"); +}