Merge pull request #1896 from tdamsma/claude/wr212-flag-first-round-validate

fix(blend): the WR-212 rollback copy no longer strips curve owners from a round it keeps (fillet_refuse_non_manifold_result)

crates/solidcore/src/blend/fillet_executive/api.rs

@@ -80,14 +80,17 @@ const OP: &str = "blend::fillet_executive::api";
 /// `fixes.fillet_refuse_non_manifold_result` (WR-212, not upstream), a refusal of a
 /// result that is not a closed manifold solid puts back the body as it was before the
 /// call (a [`Brep::clone_for_rollback`] taken first) and names no entity, as the
-/// refused result's edge is gone with it. Every other result is `fillet`'s own.
+/// refused result's edge is gone with it. Every other result is `fillet`'s own, and
+/// a result the check keeps is the flag-off result: the copy shares nothing with
+/// `brep`, so `fillet` edits the caller's own objects (curve owners included). The
+/// copy fails only on geometry nested deeper than `MAX_GEOMETRY_NESTING`.
 fn refusing_non_manifold_result(
     k: &mut Kernel,
     brep: &mut Brep,
     fillet: impl FnOnce(&mut Kernel, &mut Brep) -> Result<()>,
 ) -> Result<()> {
     let saved = if k.fixes.fillet_refuse_non_manifold_result {
-        Some(brep.clone_for_rollback())
+        Some(brep.clone_for_rollback()?)
     } else {
         None
     };

crates/solidcore/src/topology.rs

@@ -1208,14 +1208,25 @@ impl Brep {
     }
 
     /// A copy of this B-rep to put back with [`Brep::restore_from`] if an operation
-    /// on it is refused (not upstream: an operation's rollback). The edge and edge use
-    /// arenas are shared copy-on-write, so their curves keep their owners, and the face
-    /// surfaces keep theirs; the surfaces start with empty cache slots (a cache the
-    /// operation builds on the edited faces is not one of the copy's), and the copy's
-    /// objects, being copies, carry no notification identities.
-    #[must_use]
-    pub fn clone_for_rollback(&self) -> Self {
-        let mut copy = self.copy(true);
+    /// on it is refused (not upstream: an operation's rollback). Its edge curves and UV
+    /// trim curves are exact copies that keep their owners, and its face surfaces keep
+    /// theirs; the surfaces start with empty cache slots (a cache the operation builds
+    /// on the edited faces is not one of the copy's), and the copy's objects, being
+    /// copies, carry no notification identities.
+    ///
+    /// The copy shares no arena with this B-rep, so the operation that follows edits
+    /// this B-rep's own objects exactly as it would without the copy. (Sharing the edge
+    /// arenas copy-on-write, as [`Brep::clone_sharing_surface_caches`] does, would make
+    /// the operation's first edge edit copy them for this B-rep with the copy
+    /// constructor, which resets every curve's owner: a result the check then fails with
+    /// Test #61, Bad Edge, `ValidateGeometry`.)
+    ///
+    /// # Errors
+    ///
+    /// Those of the geometry copies (a geometry nesting deeper than
+    /// `MAX_GEOMETRY_NESTING`).
+    pub fn clone_for_rollback(&self) -> Result<Self> {
+        let mut copy = self.copy(false);
         for (id, face) in self.faces.iter() {
             if let (Some(original), Ok(face_copy)) = (face.surface.as_ref(), copy.face_mut(id)) {
                 if let Some(surface) = face_copy.surface.as_mut() {
@@ -1223,7 +1234,17 @@ impl Brep {
                 }
             }
         }
-        copy
+        for (id, edge) in self.edges.iter() {
+            if let Some(curve) = edge.curve.as_ref() {
+                copy.edge_mut(id)?.curve = Some(curve.clone_for_evaluation()?);
+            }
+        }
+        for (id, edgeuse) in self.edgeuses.iter() {
+            if let Some(curve) = edgeuse.uv_trim_curve.as_ref() {
+                copy.edgeuse_mut(id)?.uv_trim_curve = Some(curve.clone_for_evaluation()?);
+            }
+        }
+        Ok(copy)
     }
 
     /// Puts back the contents of `saved`, a [`Brep::clone_for_rollback`] of this

crates/solidcore/tests/issue_422_round_over_through_hole.rs

@@ -22,6 +22,7 @@ use solidcore::mass::integrator::properties::{brep_volume, PropertyAttributes};
 use solidcore::math::Point3;
 use solidcore::primitive::api_primitives::{create_box, create_cylinder};
 use solidcore::topology::brep::p2::is_manifold_solid;
+use solidcore::topology::brep::validation::{check_brep, CheckLevel};
 use solidcore::topology::{edge, vertex, EdgeId};
 use solidcore::{Brep, Kernel, Result, SolidError};
 
@@ -156,17 +157,34 @@ fn issue_422_round_into_the_hole_is_refused_with_the_fix() {
         "the body differs from before the call"
     );
     assert_eq!(volume(&k, &mut b).to_bits(), before_volume.to_bits());
+    let lines = check_brep(&mut k, &mut b, CheckLevel::Level2)
+        .unwrap()
+        .lines();
+    assert!(lines.is_empty(), "{lines:?}");
     // The restored body still takes a round that fits.
     fillet_edges(&mut k, &mut b, &[top], 2.0, 1, 1, 1.0).unwrap();
     assert!(is_manifold_solid(&b).unwrap());
 }
 
-/// A round clear of the hole (R2, the hole's wall at y = 17.5) still builds a solid.
+/// A round clear of the hole (R2, the hole's wall at y = 17.5) still builds a solid,
+/// and with the fix it is the round without it: the same check report (the refusal's
+/// rollback copy must not leave the kept result's curves without owner, Test #61).
 #[test]
 fn issue_422_round_clear_of_the_hole_is_kept() {
+    let mut reports = Vec::new();
     for fix in [false, true] {
         let (result, solid) = round(fix, 2.0);
         result.unwrap();
         assert!(solid, "fix {fix}");
+        let mut k = Kernel::default();
+        k.fixes.fillet_refuse_non_manifold_result = fix;
+        let (mut b, top) = block(&mut k);
+        fillet_edges(&mut k, &mut b, &[top], 2.0, 1, 1, 1.0).unwrap();
+        reports.push(
+            check_brep(&mut k, &mut b, CheckLevel::Level2)
+                .unwrap()
+                .lines(),
+        );
     }
+    assert_eq!(reports[1], reports[0]);
 }

crates/solidcore/tests/wr212_refusal_keeps_owners.rs

@@ -0,0 +1,115 @@
+//! `fixes.fillet_refuse_non_manifold_result` (WR-212) on a round it does not refuse: the
+//! result is the flag-off result, curve owners included. The refusal's rollback copy
+//! (`Brep::clone_for_rollback`) used to share the edge arenas copy-on-write, so the
+//! round's first edge edit copied them for the caller's body with the copy
+//! constructor, which resets every curve's owner. The first round of the WR-592 bar
+//! (`wr592_torus_rail_on_axis.rs`) then failed `check_brep` with eight
+//! `SmBrep::Test #61 ... Bad Edge fails pE->ValidateGeometry(FALSE) check [owner SmEdge]`
+//! lines, while the flag-off round (and the pinned C++) checks clean.
+#![allow(
+    clippy::unwrap_used,
+    clippy::expect_used,
+    clippy::panic,
+    clippy::indexing_slicing
+)]
+
+use solidcore::blend::fillet_executive::api::fillet_edges;
+use solidcore::curve::curve::Curve;
+use solidcore::geom::attribute::{AttributeStore, ObjectKey};
+use solidcore::mass::integrator::properties::{brep_volume, PropertyAttributes};
+use solidcore::math::{Point3, Vec3};
+use solidcore::primitive::api_curves::create_line_segment;
+use solidcore::sweep::api_brep::create_linear_sweep;
+use solidcore::topology::brep::validation::{check_brep, CheckLevel};
+use solidcore::topology::{edge, EdgeId};
+use solidcore::{Brep, EntityRef, Kernel};
+
+const PROFILE: [[f64; 2]; 4] = [
+    [0.4893, 0.0579],
+    [41.6497, 0.0579 + 3e-10],
+    [41.6497 + 1e-10, 16.0579],
+    [0.4893 + 2e-10, 16.0579 - 2e-10],
+];
+
+fn midpoint(k: &Kernel, b: &Brep, e: EdgeId) -> Point3 {
+    let ivl = edge::interval(b, e).unwrap();
+    let curve = edge::curve(b, e).unwrap().unwrap();
+    curve
+        .evaluate(k, 0.5 * (ivl.min() + ivl.max()), 0, true, false)
+        .unwrap()[0]
+}
+
+fn nearest_edge(k: &Kernel, b: &Brep, p: Point3) -> EdgeId {
+    b.edges()
+        .ids()
+        .min_by(|&x, &y| {
+            midpoint(k, b, x)
+                .distance(p)
+                .total_cmp(&midpoint(k, b, y).distance(p))
+        })
+        .unwrap()
+}
+
+fn bar(k: &mut Kernel) -> Brep {
+    let cs: Vec<Curve> = PROFILE
+        .iter()
+        .zip(PROFILE.iter().cycle().skip(1))
+        .map(|(a, b)| {
+            Curve::from(
+                create_line_segment(Vec3::new(a[0], a[1], 0.0), Vec3::new(b[0], b[1], 0.0))
+                    .unwrap(),
+            )
+        })
+        .collect();
+    let refs: Vec<Option<&Curve>> = cs.iter().map(Some).collect();
+    create_linear_sweep(k, &refs, Vec3::new(0.0, 0.0, 1.0), 10.0, true).unwrap()
+}
+
+/// The WR-592 bar's first round (its front top edge, r 2) with the flag as `fix` says.
+fn first_round(fix: bool) -> (Kernel, Brep) {
+    let mut k = Kernel::default();
+    k.fixes.fillet_refuse_non_manifold_result = fix;
+    let mut b = bar(&mut k);
+    let front_top = nearest_edge(&k, &b, Point3::new(20.0, 0.0579, 10.0));
+    fillet_edges(&mut k, &mut b, &[front_top], 2.0, 1, 1, 1.0).unwrap();
+    (k, b)
+}
+
+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-8, &attrs).unwrap()
+}
+
+/// Per edge: whether its curve's owner is the edge's owner identity.
+fn edge_owners(b: &Brep) -> Vec<(EdgeId, bool)> {
+    b.edges()
+        .ids()
+        .map(|e| {
+            let curve = edge::curve(b, e).unwrap().unwrap();
+            (
+                e,
+                curve.owner() == Some(b.owner_identity(EntityRef::Edge(e))),
+            )
+        })
+        .collect()
+}
+
+#[test]
+fn wr212_flag_keeps_a_kept_round_as_without_it() {
+    let (mut k_off, mut off) = first_round(false);
+    let (mut k_on, mut on) = first_round(true);
+    assert!(edge_owners(&off).iter().all(|&(_, owned)| owned));
+    assert_eq!(edge_owners(&on), edge_owners(&off));
+    assert_eq!(
+        volume(&k_on, &mut on).to_bits(),
+        volume(&k_off, &mut off).to_bits()
+    );
+    for (k, b) in [(&mut k_off, &mut off), (&mut k_on, &mut on)] {
+        let lines = check_brep(k, b, CheckLevel::Level2).unwrap().lines();
+        assert!(lines.is_empty(), "{lines:?}");
+    }
+}