Merge pull request #1925 from tdamsma/claude/copy-faces-merge-key-aliasing

fix(topology): copy_faces_from guards shared namings so merged copies no longer alias source attributes (#1953; no API change)

crates/solidcore/src/boolean/merge/p3.rs

@@ -1233,14 +1233,26 @@ pub fn boolean_lists_with_notifications(
                         // Defaults: pOptNewFaces=NULL, bUseMerge=TRUE,
                         // pOptMergeObject=NULL. Returned outputs are not inspected.
                         if let Some(notify) = notifications.as_deref_mut() {
-                            bp7::copy_faces(
+                            // (not upstream: upstream's objects are their own
+                            // identities) the result under the caller's names, the
+                            // component's objects under their own (`notify_ref`, its
+                            // faces `face_ref`), as `copy_component` names them
+                            let source_key = notify.key_of;
+                            let mut component_env = crate::topology::edge::NotifyEnv {
+                                records: &mut *notify.records,
+                                attributes: &mut *notify.attributes,
+                                context: notify.context.as_deref_mut(),
+                                key_of: &crate::topology::loopuse::notify_ref,
+                            };
+                            bp7::copy_faces_from(
                                 k,
                                 &mut result,
                                 &connected,
                                 &mut component,
                                 true,
+                                source_key,
                                 &face_ref,
-                                notify,
+                                &mut component_env,
                             )?;
                         } else {
                             // the arena entry keeps no notification state: the copy

crates/solidcore/src/primitive/primitive_creation/model.rs

@@ -801,10 +801,17 @@ impl PrimitiveCreationAccess for PrimitiveModel<'_> {
         destination: BrepId,
     ) -> Result<()> {
         // `SmBrep::CopyFaces(faces, destination, NULL, FALSE)`; its result is unused.
-        self.with_breps_mut(source, destination, |k, src, dst, _, this_face, _, env| {
-            crate::topology::brep::p7::copy_faces(k, src, faces, dst, false, this_face, env)
+        // (the source under `source_ref`, where the model entered its attributes)
+        self.with_breps_mut(
+            source,
+            destination,
+            |k, src, dst, source_key, this_face, _, env| {
+                crate::topology::brep::p7::copy_faces_from(
+                    k, src, faces, dst, false, source_key, this_face, env,
+                )
                 .map(drop)
-        })
+            },
+        )
     }
     fn attach_offset_map(&mut self, face: FaceRef, map: OffsetMapAttribute) -> Result<()> {
         self.brep(face.0)?.face(face.1)?;

crates/solidcore/src/topology/brep/p3.rs

@@ -1450,7 +1450,7 @@ pub fn api_brep_copy(
 /// # Errors
 ///
 /// `InvalidInput` when no such shift exists.
-fn private_source_shift(
+pub(crate) fn private_source_shift(
     live: &[EntityRef],
     source_key: &dyn Fn(EntityRef) -> ObjectRef,
     key_of: &dyn Fn(EntityRef) -> ObjectRef,

crates/solidcore/src/topology/brep/p7.rs

@@ -63,7 +63,12 @@
 //!   (`topology.brep_p4`). `GlueVertices` calls `topology.brep_p5` with the caller's
 //!   notification environment. `CopyFaces` with the merge builds `boolean.merge_p2`'s
 //!   merge and calls its `MakeFaceuse` (so it takes the kernel mutably, for the
-//!   context's `DoingBoolean`); its copies are notified in `env`'s one identity space.
+//!   context's `DoingBoolean`). (not upstream: upstream's objects are their own
+//!   identities) `CopyFaces` names the copies by `env.key_of` (`this_face` for faces)
+//!   and the source by `env.key_of` too, or by its own naming in [`copy_faces_from`];
+//!   two namings that coincide on the source's objects name the source in a private
+//!   domain for the copy, and are refused where a source object's attribute list is on
+//!   a key the copies' naming gives out (the copies would share it).
 //!   `MergeSelfIntersectionCurves` classifies each UV curve with
 //!   `intersect.curve_class_p4`'s constructor and `query.face_p2`'s `CurveOnClassify`,
 //!   then `intersect.curve_class_p1`'s `SetCurve` and `intersect.curve_class_p5`'s
@@ -117,6 +122,7 @@ use crate::boolean::topology_intersector::p1::{Relations, TopologyIntersector};
 use crate::curve::crv_on_surf::CrvOnSurf;
 use crate::curve::curve::Curve;
 use crate::error::{ignore_status, ignored, EntityRef, Result, SolidError};
+use crate::geom::attribute::ObjectKey;
 use crate::geom::extent1d::Extent1d;
 use crate::geom::extent2d::Extent2d;
 use crate::geom::object::{NotifyOperation, ObjectRef};
@@ -2048,9 +2054,15 @@ pub fn glue_edges_general(
 /// never lowered). Returns the
 /// new faces of `dest`, and whether every face was copied.
 ///
+/// (not upstream: upstream's objects are their own identities.) `env.key_of` names both
+/// B-reps here, so a copy and a source object of one arena id would share a key; the
+/// source is named apart for the copy as [`copy_faces_from`] describes. A caller whose
+/// source objects carry attributes names them apart itself ([`copy_faces_from`]).
+///
 /// # Errors
 ///
-/// `SM_ERR` for a face of another B-rep; [`SolidError::Unported`] (see the module docs).
+/// `SM_ERR` for a face of another B-rep; [`SolidError::Unported`] (see the module docs);
+/// `InvalidInput` as [`copy_faces_from`].
 pub fn copy_faces(
     k: &mut Kernel,
     src: &mut Brep,
@@ -2070,9 +2082,23 @@ pub fn copy_faces(
 /// (`crate::topology::brep::p2::CopyNotifyState`, `source_key` its
 /// `copy_source_ref`), as upstream's `SM_NO_COPY` gives each new object its source's.
 ///
+/// (not upstream: upstream's objects are their own identities.) Namings that coincide
+/// on `src`'s objects (`source_key` and `env.key_of` / `this_face` alike, as
+/// [`copy_faces`] has them) would make a copy and a source object of one arena id share
+/// a key, and `SM_NO_COPY` would cross their attribute lists both ways (#1869 review).
+/// `src` is then named in a private domain for the copy (as `api_brep_copy` does, #1884),
+/// with the caller's records and attribute lists of it lent there and taken back after.
+/// Where a source object's list is on a key that naming gives the copies too, the call
+/// is refused: the source and its copies cannot keep their own lists apart in one store.
+/// `this_face` owns each new face's surface: [`super::validation::check_brep`] resolves
+/// owners by `dest`'s naming scheme, so a key outside it (a caller's own domain on a
+/// [`crate::topology::NamingScheme::Notify`] B-rep) reads as no owner (Test #35).
+///
 /// # Errors
 ///
-/// As [`copy_faces`].
+/// As [`copy_faces`]; `InvalidInput` (before anything changes) for coinciding namings
+/// where a source object's attribute list is on a key the copies' naming gives out, or
+/// when no private domain is left for `src`'s names.
 #[allow(clippy::too_many_arguments)] // copy_faces's arguments and the source identities
 pub fn copy_faces_from(
     k: &mut Kernel,
@@ -2083,6 +2109,81 @@ pub fn copy_faces_from(
     source_key: &dyn Fn(EntityRef) -> ObjectRef,
     this_face: &dyn Fn(FaceId) -> ObjectRef,
     env: &mut ed::NotifyEnv<'_>,
+) -> Result<(Vec<FaceId>, bool)> {
+    // (not upstream) the copies' names (`env.key_of`, `this_face` for faces) against
+    // the source's: where they coincide on the source's objects, a copy would take the
+    // key of the source object of its arena id.
+    let dest_key_of = env.key_of;
+    let dest_name = |e: EntityRef| match e {
+        EntityRef::Face(f) => this_face(f),
+        other => dest_key_of(other),
+    };
+    let live = p2::live_entities(src);
+    let shift = super::p3::private_source_shift(&live, source_key, &dest_name)?;
+    let Some(shift) = shift else {
+        return copy_faces_named(k, src, faces, dest, use_merge, source_key, this_face, env);
+    };
+    // a source object's list on a key the copies' naming also gives out would be a
+    // copy's list too
+    let dest_keys: std::collections::BTreeSet<ObjectKey> =
+        live.iter().map(|&e| dest_name(e).key).collect();
+    let listed: Vec<EntityRef> = live
+        .iter()
+        .copied()
+        .filter(|&e| !env.attributes.attributes(source_key(e).key).is_empty())
+        .collect();
+    if listed
+        .iter()
+        .any(|&e| dest_keys.contains(&source_key(e).key))
+    {
+        return Err(SolidError::invalid_input(
+            OP,
+            "the source and its copies are named alike where the source has attributes",
+        ));
+    }
+    let shifted = |e: EntityRef| {
+        let r = source_key(e);
+        ObjectRef {
+            type_code: r.type_code,
+            key: ObjectKey(r.key.0 + shift),
+        }
+    };
+    // the caller's records and lists of the source, lent to its private names for the
+    // copy and taken back after
+    for &e in &live {
+        if let Some(&r) = env.records.get(&source_key(e).key) {
+            env.records.insert(shifted(e).key, r);
+        }
+    }
+    let mut lent = Vec::new();
+    for &e in &listed {
+        let key = shifted(e).key;
+        for h in env.attributes.attributes(source_key(e).key) {
+            env.attributes.add_attribute(key, h)?;
+            lent.push((key, h));
+        }
+    }
+    let copied = copy_faces_named(k, src, faces, dest, use_merge, &shifted, this_face, env);
+    for (key, h) in lent {
+        env.attributes.remove_attribute(key, h, true)?;
+    }
+    for &e in &live {
+        env.records.remove(&shifted(e).key);
+    }
+    copied
+}
+
+/// [`copy_faces_from`] once the source's names (`source_key`) are apart from the copies'.
+#[allow(clippy::too_many_arguments)] // copy_faces_from's arguments
+fn copy_faces_named(
+    k: &mut Kernel,
+    src: &mut Brep,
+    faces: &[FaceId],
+    dest: &mut Brep,
+    use_merge: bool,
+    source_key: &dyn Fn(EntityRef) -> ObjectRef,
+    this_face: &dyn Fn(FaceId) -> ObjectRef,
+    env: &mut ed::NotifyEnv<'_>,
 ) -> Result<(Vec<FaceId>, bool)> {
     // pDestinationBrep->SetTolerance(GetTolerance()) = (tol, TRUE, TRUE): grow only
     brep_set_tolerance(k, dest, src.tolerance(), true, true)?;

crates/solidcore/tests/copy_faces_merge_key_aliasing.rs

@@ -0,0 +1,255 @@
+//! `SmBrep::CopyFaces(faces, pDestinationBrep, .., bUseMerge = TRUE)` (#1869 review): every
+//! copy is its own object upstream, so each new face and edge carries its own source's
+//! `SM_NO_COPY` attributes and the source keeps its own. solidcore's `copy_faces` named
+//! the new edges and vertices with the caller's one `key_of`, the source's naming: a copy
+//! took the key of the source object of its arena id, and the attribute lists crossed
+//! both ways (a copied edge got two attributes, the source edge the copy's). With the
+//! source named apart (`copy_faces_from`) each copy carries its own source's; one naming
+//! for both (`copy_faces`) names the source apart for the copy, and is refused where the
+//! source has attribute lists on keys the copies are given.
+#![allow(clippy::unwrap_used, clippy::expect_used, clippy::indexing_slicing)]
+
+use solidcore::geom::attribute::{Attribute, AttributeStore, Behavior, ObjectKey};
+use solidcore::geom::object::ObjectRef;
+use solidcore::geom::tol::USE_DEFAULT;
+use solidcore::math::Point3;
+use solidcore::primitive::api_primitives::create_box;
+use solidcore::topology::brep::p2::ArenaNotifyState;
+use solidcore::topology::brep::p3::new_brep;
+use solidcore::topology::brep::p7::{copy_faces, copy_faces_from};
+use solidcore::topology::brep::validation::{check_brep, CheckLevel};
+use solidcore::topology::edge::NotifyEnv;
+use solidcore::topology::loopuse::notify_ref;
+use solidcore::topology::topology::NotifyRecords;
+use solidcore::topology::{Brep, EdgeId, FaceId};
+use solidcore::{EntityRef, Kernel, SolidError};
+
+/// The source's names, apart from the destination's (`notify_ref`).
+fn source_ref(e: EntityRef) -> ObjectRef {
+    let r = notify_ref(e);
+    ObjectRef {
+        type_code: r.type_code,
+        key: ObjectKey(r.key.0 + (1 << 45)),
+    }
+}
+
+/// A 10 mm box whose arena ids start at 0 (a copy of a box's faces into a new B-rep;
+/// `create_box` leaves its first edge ids unused), as a B-rep read from a file has.
+fn compact_box(k: &mut Kernel) -> Brep {
+    let mut b = create_box(k, Point3::new(0.0, 0.0, 0.0), 10.0, 10.0, 10.0).unwrap();
+    let faces = b.face_list().to_vec();
+    let mut dest = new_brep(k, USE_DEFAULT, USE_DEFAULT).unwrap();
+    let mut state = ArenaNotifyState::new(&dest);
+    let mut env = state.env(None);
+    let face = |f: FaceId| notify_ref(EntityRef::Face(f));
+    copy_faces(k, &mut b, &faces, &mut dest, true, &face, &mut env).unwrap();
+    dest.refresh_cached_topology();
+    assert_eq!(dest.edge_list().first(), Some(&EdgeId::from_index(0)));
+    dest
+}
+
+/// Tags each face and edge of `b` under `name` with a copy attribute of its own:
+/// 1000 + the face's index, 2000 + the edge's.
+fn tag(store: &mut AttributeStore, b: &Brep, name: &dyn Fn(EntityRef) -> ObjectRef) {
+    for &f in b.face_list() {
+        let h = store
+            .insert(Attribute::long(
+                1000 + u64::from(f.index()),
+                0,
+                Behavior::Copy,
+            ))
+            .unwrap();
+        store
+            .add_attribute(name(EntityRef::Face(f)).key, h)
+            .unwrap();
+    }
+    for &e in b.edge_list() {
+        let h = store
+            .insert(Attribute::long(
+                2000 + u64::from(e.index()),
+                0,
+                Behavior::Copy,
+            ))
+            .unwrap();
+        store
+            .add_attribute(name(EntityRef::Edge(e)).key, h)
+            .unwrap();
+    }
+}
+
+fn ids(store: &AttributeStore, key: ObjectKey) -> Vec<u64> {
+    let mut v: Vec<u64> = store
+        .attributes(key)
+        .into_iter()
+        .map(|h| store.get(h).unwrap().attribute_id())
+        .collect();
+    v.sort_unstable();
+    v
+}
+
+/// Each source face and edge still carries exactly its own tag under `name`.
+fn assert_source_untouched(
+    store: &AttributeStore,
+    b: &Brep,
+    name: &dyn Fn(EntityRef) -> ObjectRef,
+) {
+    for &f in b.face_list() {
+        let key = name(EntityRef::Face(f)).key;
+        assert_eq!(
+            ids(store, key),
+            vec![1000 + u64::from(f.index())],
+            "source {f:?}"
+        );
+    }
+    for &e in b.edge_list() {
+        let key = name(EntityRef::Edge(e)).key;
+        assert_eq!(
+            ids(store, key),
+            vec![2000 + u64::from(e.index())],
+            "source {e:?}"
+        );
+    }
+}
+
+/// The source edge whose curve runs between the ends of `copy`'s.
+fn source_edge_of(k: &Kernel, src: &Brep, dest: &Brep, copy: EdgeId) -> EdgeId {
+    use solidcore::topology::edge;
+    let ends = |b: &Brep, e: EdgeId| {
+        let c = edge::curve(b, e).unwrap().unwrap();
+        let ivl = edge::interval(b, e).unwrap();
+        let mut p = [
+            c.evaluate_point(k, ivl.min()).unwrap(),
+            c.evaluate_point(k, ivl.max()).unwrap(),
+        ];
+        p.sort_by(|a, b| (a.x, a.y, a.z).partial_cmp(&(b.x, b.y, b.z)).unwrap());
+        p
+    };
+    let want = ends(dest, copy);
+    *src.edge_list()
+        .iter()
+        .find(|&&e| {
+            let got = ends(src, e);
+            got.iter().zip(&want).all(|(a, b)| a.distance(*b) < 1e-9)
+        })
+        .unwrap()
+}
+
+#[test]
+fn copies_through_the_merge_carry_only_their_own_sources_attributes() {
+    for which in [1usize, 3, 5] {
+        let mut k = Kernel::default();
+        let mut src = compact_box(&mut k);
+        let mut dest = new_brep(&mut k, USE_DEFAULT, USE_DEFAULT).unwrap();
+        let mut records = NotifyRecords::new();
+        let mut store = AttributeStore::default();
+        tag(&mut store, &src, &source_ref);
+        let face = src.face_list()[which];
+        let mut env = NotifyEnv {
+            records: &mut records,
+            attributes: &mut store,
+            context: None,
+            key_of: &notify_ref,
+        };
+        let this_face = |f: FaceId| notify_ref(EntityRef::Face(f));
+        let (made, all) = copy_faces_from(
+            &mut k,
+            &mut src,
+            &[face],
+            &mut dest,
+            true,
+            &source_ref,
+            &this_face,
+            &mut env,
+        )
+        .unwrap();
+        assert!(all);
+        dest.refresh_cached_topology();
+        assert_eq!(made.len(), 1);
+        assert_eq!(
+            ids(&store, notify_ref(EntityRef::Face(made[0])).key),
+            vec![1000 + u64::from(face.index())]
+        );
+        assert_eq!(dest.edge_list().len(), 4);
+        for &copy in dest.edge_list() {
+            let from = source_edge_of(&k, &src, &dest, copy);
+            assert_eq!(
+                ids(&store, notify_ref(EntityRef::Edge(copy)).key),
+                vec![2000 + u64::from(from.index())],
+                "copy {copy:?} of {from:?}"
+            );
+        }
+        assert_source_untouched(&store, &src, &source_ref);
+        for b in [&mut dest, &mut src] {
+            let check = check_brep(&mut k, b, CheckLevel::Level2).unwrap();
+            assert_eq!(check.failure_count(), 0, "{:?}", check.lines());
+        }
+    }
+}
+
+#[test]
+fn one_naming_for_both_breps_is_refused_where_the_source_has_attributes() {
+    let mut k = Kernel::default();
+    let mut src = compact_box(&mut k);
+    let mut dest = new_brep(&mut k, USE_DEFAULT, USE_DEFAULT).unwrap();
+    let mut records = NotifyRecords::new();
+    let mut store = AttributeStore::default();
+    tag(&mut store, &src, &notify_ref);
+    let face = src.face_list()[3];
+    let mut env = NotifyEnv {
+        records: &mut records,
+        attributes: &mut store,
+        context: None,
+        key_of: &notify_ref,
+    };
+    let this_face = |f: FaceId| notify_ref(EntityRef::Face(f));
+    let r = copy_faces(
+        &mut k,
+        &mut src,
+        &[face],
+        &mut dest,
+        true,
+        &this_face,
+        &mut env,
+    );
+    assert!(matches!(r, Err(SolidError::InvalidInput { .. })), "{r:?}");
+    // before anything changed
+    dest.refresh_cached_topology();
+    assert!(dest.face_list().is_empty() && dest.edge_list().is_empty());
+    assert_source_untouched(&store, &src, &notify_ref);
+}
+
+#[test]
+fn one_naming_for_both_breps_without_attributes_still_copies() {
+    // the #1869 caller: one arena naming, no attributes; the source is named apart for
+    // the copy
+    let mut k = Kernel::default();
+    let mut src = compact_box(&mut k);
+    let mut dest = new_brep(&mut k, USE_DEFAULT, USE_DEFAULT).unwrap();
+    let mut state = ArenaNotifyState::new(&src);
+    let mut env = state.env(None);
+    let this_face = |f: FaceId| notify_ref(EntityRef::Face(f));
+    let face = src.face_list()[3];
+    let (made, all) = copy_faces(
+        &mut k,
+        &mut src,
+        &[face],
+        &mut dest,
+        true,
+        &this_face,
+        &mut env,
+    )
+    .unwrap();
+    assert!(all);
+    assert_eq!(made.len(), 1);
+    dest.refresh_cached_topology();
+    assert_eq!(
+        [
+            dest.face_list().len(),
+            dest.edge_list().len(),
+            dest.vertex_list().len()
+        ],
+        [1, 4, 4]
+    );
+    let check = check_brep(&mut k, &mut dest, CheckLevel::Level2).unwrap();
+    assert_eq!(check.failure_count(), 0, "{:?}", check.lines());
+}