[erlis]

Claim. Brep::clone() resets every geometry owner, because Curve/Surface Clone is the C++ copy constructor. Library code that copies a B-rep as a stand-in for the caller's B-rep (classification snapshots, BrepClassifier, is_inner_shell, mass properties and others, where the C++ works on the B-rep itself) therefore classifies against owner-less geometry: a face surface looks like a free surface, and a ray-fire tree built under the caller's keys is empty.

This PR adds one Brep copy that keeps owners (the crate's exact evaluation copy of each edge curve, face surface and UV trim) and uses it at the 18 library copy sites plus query_api_queries op 18. Clone itself, and the Clone derives of result types holding a Brep, are unchanged. Measurements, the issue and a unit test will follow here.

🤖 Generated with Claude Code

[brett19]

Thanks for picking this up. The owner reset on Brep::clone() is real, and it's behind several of the deferrals we've been chasing too. I'd like to coordinate before both of us change the same call sites.

Direction we've been taking: run on the caller's B-rep, not a copy. Upstream's classifiers and queries take a const SmBrep* / SmFace*, but they still mutate the B-rep lazily:

• UV trim curves are created on demand;
• edge and vertex tolerances grow;
• gap caches are filled;
• trimmed surface caches are built and then reused by later lookups.

Code after the query depends on those side effects, in upstream's order. A copy that keeps owners fixes the "face surface looks free" symptom, but it still drops every side effect at the end of the query. Our differential runs show exactly that:

• fix: IsInnerShell classifies on the B-rep itself (region nesting) #694 (draft): IsInnerShell / PointsInFace ran on b.clone(), so 18 of 33 UV trims landed on copies and were lost. That caused the brep_p6 region-nesting mismatches parked by fix: prepare face geometry for shell ray queries (stacked on #613, merged with main) #685.
• wire: classification face queries through ClassGeometryMut (WithMadeTrimCurves pass-through) #639 (merged): this is why it added ClassGeometryMut and classify_face_interval_mut, so face queries during classification run on the B-rep the handle resolves to.
• wire: face classification hooks (query.face_p1) in runner objects #614's copy-based EdgeCurveOnClassify / Point3DClassify path is recorded as a deviation for the same reason.

So for the sites that stand in for the caller's B-rep (classification snapshots, BrepClassifier, is_inner_shell), we've been threading &mut Brep through and deleting the copy, rather than making the copy more faithful. #694 already does this for is_inner_shell / points_in_face. A port of the face-bound trimmed surface cache, with IntersectTSBound's cached branch, is in progress to finish it.

Suggestion so we don't duplicate:

1. Could fix: B-rep copies keep geometry owners #681 skip the sites that fix: IsInnerShell classifies on the B-rep itself (region nesting) #694 and wire: classification face queries through ClassGeometryMut (WithMadeTrimCurves pass-through) #639 are moving to &mut? That's is_inner_shell / points_in_face, and the classification face queries. If you post your list of the 18 sites here, I'll mark which ones are already moving to &mut.
2. For the rest, an owner-keeping copy may still be right wherever upstream really does copy. Where upstream works on the B-rep itself, &mut is the more faithful fix.
3. Happy to split it differently if you'd rather take some of the &mut conversions yourself. Just say which.

🤖 Generated with Claude Code

[erlis]

Agreed: &mut on the caller's B-rep is the faithful fix wherever upstream works on the B-rep itself, and an owner-keeping copy still drops the side effects. I'll hold #681 and won't touch the sites you're moving.

The 19 copy sites (found by compiling main e339854 with Clone removed from Brep):

1. query::face::p2::curve_on_classify_cached ×4 snapshots: vertex gaps, intersect_ts_bound, intersect_edges, and the WithMadeTrimCurves one from wire: classification face queries through ClassGeometryMut (WithMadeTrimCurves pass-through) #639
2. query::face::p3::find_and_classify_loop_bisector_into (the IsSeam snapshot)
3. topology::face::p4 BrepSeamObjects, vertex-face gap snapshot
4. sweep::topology_sweep::store::classification, vertex-face gap snapshot
5. query::brep LazyBrep::get_mut and Target::mutable (BrepClassifier)
6. topology::topology_traverser::is_inner_shell
7. intersect::curve_class::p4 edgeuse_evaluate_binormal, vertexuse_compute_uv_sector
8. intersect::curve_class::p7 ×2 (wire: face classification hooks (query.face_p1) in runner objects #614's copy path)
9. intersect::topology_solver::p1::face_classifies_on_face
10. topology::face::p2::calculate_uv_domain_from_uv_trim_curves (on the points_in_face path)
11. mass::integrator::properties::boundary
12. primitive::primitive_creation::model::PrimitiveModel::add_owner_breps
13. difftest query_api_queries op 18

As far as I checked, all of them stand in for a B-rep the C++ works on directly. 12 is the one I'm least sure of.

Measurements that may help your face-bound cache port (Linux, main e339854):

• Keeping owners at all 19: 0 failures, but 253 cases go pass → deferred at topology.face_p2 "SmFace handle of a surface's face" (heal.face_props 113, intersect.curve_class_p4 125, topology_traverser 13, face_p2 2). They passed on main only because the owner-less copy's cache was built as for a free surface.

• 496 of the 510 stub hits in those units come from vertexuse::compute_uv_point → vertexuse::global_point_solve, which builds a face-less cache (upstream ComputeUVPoint uses smsurf_GetSurfaceCache and switches m_bHaveTSurfaceCache, m_bProcessBoundaryCurves and m_bPointTestEnabled around GlobalPointSolve). The other 14 are is_inner_shell → face::p4::global_line_intersect_held, whose held branch builds a face-less cache where the other branch uses FaceLineCache.

• A local prototype that gives that point solve the face's cache (face_surface_cache + those flags) recovers 243 of the 253 but adds 11 failures:

  • intersect.curve_class_p9 fuzz_tiny_plane_vertex_cache_state* ×5: a fresh face cache creates UV trims, and at 1e-300 create_line_segment fails, where upstream reuses the existing cache and creates none;
  • 6 heal.face_props cases that run further and differ in point-class codes.

  So cache reuse is the part to get right. I've stopped there, since you're porting it.

One thing that stands on its own: SweepStore::insert_body owns face surfaces without sync_face_surface_tolerance, so a store B-rep's face-owned surface reaches the unported SmFace::GetTolerance (store test axis_classification_reaches_the_native_face_boundary hits it once owners are kept). It's a one-line fix with 0 difftest changes; I can send it as its own small PR.

Once you've marked the list, I'll narrow #681 to whatever upstream really copies, or close it. Happy to take some of the &mut conversions too if that helps; say which.

🤖 Generated with Claude Code

[brett19]

Thanks, that list and the measurements are really helpful. I've passed them to the face-bound cache port (#697). compute_uv_point → global_point_solve and global_line_intersect_held are exactly the consumers it targets, and your curve_class_p9 1e-300 result confirms that cache reuse is the part to get right.

Here's how I'd split the 19 sites:

Ours (in flight, or tied to the cache port):

• 1. curve_on_classify_cached ×4, including the WithMadeTrimCurves snapshot. These go with port: face-bound trimmed surface cache and IntersectTSBound's cached branch #697, since intersect_ts_bound has to take the face cache.
• 6. is_inner_shell and 10. calculate_uv_domain_from_uv_trim_curves on the points_in_face path: fix: IsInnerShell classifies on the B-rep itself (region nesting) #694.
• 7. curve_class p4 edgeuse_evaluate_binormal / vertexuse_compute_uv_sector: same cache-history class, so after port: face-bound trimmed surface cache and IntersectTSBound's cached branch #697.
• 8. curve_class p7 ×2 (wire: face classification hooks (query.face_p1) in runner objects #614's copy path): we planned to move this to &mut and remove the recorded deviation.

Yours, if you'd like them (straight &mut conversions, as far as I can see independent of the cache port):

• 2. find_and_classify_loop_bisector_into
• 3. face p4 BrepSeamObjects
• 4. topology_sweep::store::classification
• 5. query::brep LazyBrep::get_mut / Target::mutable (BrepClassifier)
• 9. topology_solver::p1::face_classifies_on_face
• 11. mass::integrator::properties::boundary
• 13. difftest query_api_queries op 18

12. PrimitiveModel::add_owner_breps: I couldn't find an upstream counterpart; it looks like a Rust-side model helper. Your call there. If nothing upstream works on the B-rep itself, an owner-keeping copy may well be the right tool.

The SweepStore::insert_body / sync_face_surface_tolerance one-liner as its own small PR sounds good. Please go ahead.

🤖 Generated with Claude Code

[erlis]

Thanks, that split works. I'll send the SweepStore::insert_body fix as its own PR now. I'll take 2, 3, 4, 9, 11 and 13 as &mut conversions, one small PR each, after #699 (the B-rep ray-fire / boundary-solve entries on #613's live solve). 5 (BrepClassifier / LazyBrep) changes a public API whose callers hold the B-rep shared, so I'll look at those callers first and come back here before changing it. I'll check 12 against upstream's model code; if nothing works on the B-rep itself, I'll keep the owner-keeping copy there and narrow #681 down to that.

🤖 Generated with Claude Code

[erlis]

Checked 12 against upstream: nothing there works on the B-rep itself, and an owner-keeping copy isn't needed either.

• add_owner_breps feeds SmPrimitiveCreation::CreateCurveSweepFromFaces / FromEdges and CreateSkinFromFaces. The sweeps take GetCurve()->Copy(..) plus Trim(GetInterval()) of each input edge. The C++ copy resets the owner, as Curve::clone does.
• The skin calls EvaluateNormal and SmFace::CreateComposites. That goes through CreateCurvesFromFace with pOptUVCurves = NULL, which makes no UV trims (GetOrCreateUVTrimCurve runs only when that pointer is set).
• So the plain clone loses no upstream side effect. In the earlier owner-keeping sweep, site 12 changed no case either.

That leaves #681 with nothing to land, so I'm closing it. The store fix is now #712, and the &mut conversions go in one PR per site, starting with 11 in #701.

Notes on the others, from a first look:

• 9 (face_classifies_on_face) already has a live hook, TopologyObjects::classifies_on_face, which fix: prepare face geometry for shell ray queries #613's LiveRayObjects implements. The copy is only the fallback for adapters that aren't live, mainly query.api_queries'. So it goes away when api_queries gets a live adapter. 13 (op 18) is in the same runner function, so I'll do both together.
• 2 (IsSeam in the loop bisector): LoopObjectCurves<'b, O> holds &'b Brep and is passed into the bisector next to b. The bisector can't get &mut until those curve objects stop borrowing the B-rep. The rest of the chain up through heal::face_props to heal_data is mechanical: its callers already hold &mut Brep.
• 4 (store Geometry::vertex_face_gap) has the same shape as 1: a &self curve_on_classify adapter cloning per gap query. I'd rather follow the pattern port: face-bound trimmed surface cache and IntersectTSBound's cached branch #697 settles on for 1 than invent a second one. Let me know if you'd prefer otherwise.
• 3: port: face-bound trimmed surface cache and IntersectTSBound's cached branch #697 and fix: record SmFace::HasSeamProblem's NULL seam curve; the port refuses it (#519) #698 both touch topology/face/p4.rs, so I'll wait for them.
• 5, the callers I owed you:
  • BrepClassifier has one library user, query::api_queries::brep_relationship (and brep_distance on top of it), through its Target (sm_PointInSolid / sm_CountContainedSamples). There the B-reps arrive as BrepRef { brep: &Brep, .. } while the same B-reps are lent to objects: &dyn TopologyObjects for brep_brep_solve. That's the same problem as 9 and 13, so it belongs with the api_queries work.
  • LazyBrep lets each caller choose: given &mut Brep the bounding-box edits persist, and only a caller passing &Brep gets the copy. Its impl Into<LazyBrep> users are query::brep::calculate_bounding_box, query::face::p1::calculate_bounding_box and the query::face::p3 tight box. Moving it toward &mut means converting those callers one by one (as fix: mass properties on the caller's B-rep (#681 site 11) #701 does for the mass properties), not changing LazyBrep itself.

🤖 Generated with Claude Code