Repository navigation
port: topology.face_p4 - #239
Conversation
…otes and report - WR-180/181 collided with UB-180/181 of open PRs tangency-labs#229/tangency-labs#239 (one UB/WR number space): now WR-186/187, reserved for tangency-labs#205 in the swarm ledger. - offset.offset_executive_p1: probe_piecewise_empty defers at query.brep now that PiecewiseMerge's stitch is wired; the report and notes say so. 0 other changes per case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjjfMoEv8cPgHcyNPF2hvE
… notes) - rustfmt the include!-ed runner and FFI unit files. - surface::surface::p4: the face_p2 porter's NOTE lists query::face::p1::point_3d_classify (SmFace.cpp:12801) among the callers that switch the cache's point test off; the module Deviations say so. - The six UNPORTED(query.face_p1) sites on opaque face handles (boolean::topology_intersector::p1, intersect::curve_class::p7, intersect::topology_solver::p1, geom::object) are DISPATCH GAPs, as in lift_to_face; their deferral labels are unchanged. - ClassifyProblem: name PR tangency-labs#239 (topology.face_p4) next to PR tangency-labs#132 in the module table, findings and the query.face_p1 / query.face_p3 notes. - Point3d FFI guard: note it is stricter than the C++ needs and should be narrowed once those cases compare. - mass.integrator runner: a comment instead of the unused-name binding. Difftest unchanged (0 failed; --green 210 units, 113 pass, 97 partial). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ut range) - plan/modules.toml: the query.face_p3 note names PR tangency-labs#132 and tangency-labs#239 once, in one sentence; the topology.shell note says the face-use box is wired. - geom::tol: SmTol::GetScaledZero(SmRegion/SmShell) no longer carries UNPORTED(query.face_p1); the comment says their box callees exist in query::face::p1 and where to port them when a caller needs them. - query.face_p1 runner: curve cases outside the shim's inputs (kind, non-finite data, mask outside 0..=7) are rejected on both sides, as the FFI does. Difftest unchanged: query.face_p1 1535/689, SENSITIVE 1535/1535; --green 213 units, 114 pass, 99 partial, 0 failed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I am preparing to reuse this port in #101 after the #259 integration, rather than duplicating the face traversal implementation. I reviewed head
I am closing the already-prepared #259/#149 integration first, then can bind the face integration to a tested head. The 92 directed Point3D fixtures in #101 remain deferred in the published checkpoint; I am not claiming that moving the first dependency boundary proves their numerical parity. Your reported counts remain evidence for your head, separate from our upcoming combined run. |
|
Two output-contract issues are present in
Our integration has not wired the remaining p1 seam callers through this API. I am separating the FaceProps/trait adaptations from the ownership and partial-output correction so that the latter can be reviewed explicitly. The current |
bherbruck
left a comment
There was a problem hiding this comment.
Thanks for taking this one over and for the detailed write-up. The back-edge list and the WR-176 / WR-177 / UB-180 notes made the review much quicker. Reviewed at c0c470736. The new head 7e3d3005b only merges main (#160, #219) and STATUS.md, and leaves this unit's files unchanged, so everything below applies to it.
What passed (x86-64, main through cfa9d068, merge-tree with origin/main clean):
cargo fmt --all --check, clippy-D warnings(plain and--features solidcore-ffi/cpp,difftest/cpp),cargo test -p solidcore(1440 passed), wasm32 check.--unit topology.face_p4reproduces your numbers exactly: 4696 cases, 3807 pass, 889 deferred, 0 failed.--green: 223 units, 114 passed, 109 partial, 0 failed.- I also ran 23,040 extra adversarial cases through the C++ (10
.smbfiles with and without rebuilt trim curves, faces 0-3): lines with tiny (1e-9) and huge (1e9) vectors, far-away points, reversed, zero-width and 1e-12 intervals, tolerances 0 / 1e-15 / 10 / negative; sectors with tolerances 0 / 1e-15 / 100 / -1 over every orientation and validate / use_uv / uv_ok combination; every forced-periodicity seam combination; everyIsConnectedToslot; every list getter with out-of-range loop selectors. No crashes or panics on either side. 9,612 pass, 13,406 defer (as expected, mostlySectorClassifyCurveOn), and 22 fail, all on the same cause (must-fix 2).
adversarial (not committed): cases=23040 passed=9612 deferred=13406 failed=22 (all: GlobalLineIntersect with a negative tolerance on a planar face)
PARTIAL topology.face_p4 cases=4696 passed=3807 failed=0 deferred=889 max_rel_err=0e0
PARTIAL topology.trimming_tools cases=3263 passed=2850 failed=0 deferred=413 max_rel_err=0e0
PARTIAL topology.face_p3 cases=1781 passed=823 failed=0 deferred=958 max_rel_err=0e0
PARTIAL topology.topology_traverser cases=909 passed=895 failed=0 deferred=14 max_rel_err=0e0
PARTIAL heal.stitch_p1 cases=300 passed=234 failed=0 deferred=66 max_rel_err=0e0
PARTIAL offset.offset_geometry_creation cases=861 passed=438 failed=0 deferred=423 max_rel_err=0e0
difftest: 223 unit(s), 114 passed, 109 partial, 0 failed
The rest of the port reads faithfully. I checked GetBrep, the five list getters, IsConnectedTo's short-circuit order, the GlobalLineIntersect removal/insertion index logic, the FindVertexSector shortcuts, the 3D pole/seam branch (including the block-scoped sSurfDomain), the binormal skip, and the whole HasSeamProblem torus case against SmFace.cpp. The HasSeamProblem ↔ SetProps_Stage2 recursion stops at depth 2 because set_props_stage2_with sets Stage2A before the seam call. The two UNPORTED(query.face_p3) stubs are tagged correctly (query.face_p3 is todo), and the code after them is ported. I found no overlapping co-ports in the other open PRs.
Must-fix
-
FindVertexSector's interval swaps where upstream keeps the natural interval.crates/solidcore/src/topology/face/p4.rs:846-851buildsivlwithExtent1d::from_min_max, which swaps reversed ends. Upstream (SmFace.cpp:9496-9500) starts fromsIvl = cr3DCurve.GetNaturalInterval()and callssIvl.SetMinMax(..). That call fails onmin > maxand leavessIvlas the natural interval (SmExtent1d.cpp:247-252). The shim reaches this already:at_start=0withopposite=0, orat_start=1withopposite=1, gives reversed ends. Upstream then passes the whole natural interval toSectorClassifyCurveOn, but the port passes the swapped edge interval. Every such case defers today, so the difftest can't see it yet. It will diverge oncequery.face_p3lands. Fix:let mut ivl = natural; let _ = ivl.set_min_max(a, b);(status ignored, as upstream), with a Deviations note like the ones insurface/cone/p1.rs:341andsurface/surf_of_revolution/p3.rs:601. -
SmZoneTol3dby-value parameters are not converted.GlobalLineIntersecttakesSmZoneTol3d sSrcZoneTol3d(SmFace.cpp:11700) andFindVertexSectortakesSmZoneTol3d sZoneTol3d(SmFace.cpp:9420). Passing a double through those parameters runsSmZoneTol3d(double):SM_USE_DEFAULTbecomes the system zone tolerance, thenfabs(SmTol.h:143-171, ported asgeom::tol::zone_tol3d).p4.rs:427andp4.rs:599take the rawf64. Concrete case:plane_square.smb, face 0,Line { p: [0.2, 0.1, 1], v: [0, 0, -1], tol: Some(-1e-6) }. The C++ returns one solution,t=1, uv=(4.9, 4.8), because the plane gets1e-6. The port returns none, becausedist > -1e-6rejects it. The same happens for 22 of the adversarial line cases onplane_square,disk,box_with_large_filletandMouse. Fix: applytol::zone_tol3d(k, ..)at entry in both functions, asblend/hcr.rs:347does, and add a negative-tolerance line case to the runner. -
rustfmt on the include!d runner.
rustfmt --edition 2021 --check crates/difftest/src/units/topology_face_p4.rsreports 15 hunks: theuse solidcore::topology::{..}list and several struct patterns inop_args/rust_record. They look like they were formatted at a widermax_width.cargo fmtskips include!d unit files, so CI does not catch this. Runningrustfmt --edition 2021 crates/difftest/src/units/topology_face_p4.rsfixes it. The FFI unit file is already clean.
Should-fix / nits
IsConnectedTowith a shell target: a face use without shell is just "not equal" upstream (pFaceuse1->GetShell() == target, no dereference). Only the region branch dereferences it.faceuse_shell(p4.rs:290) errors for both, and the shim's guard (topology_face_p4.cpp:237) skips both, plus a nullfu2whenfu1already matches. On valid B-reps this can't happen, but the guard could be narrowed to what actually crashes.- 2D curves in
FindVertexSector: lifting a 2D curve needs aSurfaceKind::BSplinehere (p4.rs:831), but upstream'sSetAlltakes anySmSurface. APlaneface therefore getsInvalidInput. The shim always passes the edge's 3D curve, so this branch isn't compared. A line in the Deviations header would help the next porter. - 18
UNPORTED(topology.face_p4)markers stay after the unit becomes partial. They are listed inmodules.tomlwith reasons, which is fine for now. Once a classification-object mapper exists, theface_p1.rs:244/trimming_tools.rs:393/heal/face_props.rs:1081HasSeamProblemsites look like the next candidates.
🤖 Generated with Claude Code
|
Rechecked One precision correction to my earlier seam comment: the native assignment does more than transfer the seam handle. At the pinned The storage and partial-output issues from the previous comment remain in this head: I have prepared a separate operation-owned registry and additive The useful regressions here should resolve the returned curve and UV handles after the call, retain distinct U/V and repeated-call identities, inspect the outputs after a later U/V error, and verify cache/trim writes on the original B-rep. These are source-review findings against the pinned native implementation, separate from the passing CI reported for this PR. |
|
Thanks for the quick turnaround, and for merging #132 and wiring Earlier must-fix items: all three are fixed.
The should-fix items were addressed as well: the New since What passed (x86-64):
No must-fix items remain. It's good to merge once CI is green (it's green on Should-fix (follow-up is fine)
🤖 Generated with Claude Code |
|
Thanks for the follow-ups since Earlier should-fix items: both are fixed (
New since then: the What passed (x86-64):
No must-fix items. It's good to merge once CI is green (it's green on Nits (optional)
🤖 Generated with Claude Code |
…m.tol on main Review nit (tangency-monitor, at 590caa7): the notes named surface.surface_cache_p2 while the report's blocked_by says geom.tol, the owner tag of SmTol::GetZoneTol3d(const SmObject*) on main. Regenerated reports; STATUS unit totals (237: 116 green, 121 partial). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pfe7dodPG5rpuN9B8QcnuX
, tangency-labs#239) into unit/blend.fillet_solver_p3 Conflict: the offset.offset_geometry_creation notes (main's topology.face_p4 sentence and this branch's tangency-labs#192 one were appended at the same place; main's taken, this branch's re-added with fresh counts after --green) and its generated report (main's taken; regenerated by --green). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s#239 topology.face_p4, tangency-labs#229 boolean.merge_p2) into fix/topology.brep_p2-delta-minors Conflict: plan/modules.toml (offset.offset_geometry_creation): main's notes with this branch's issue tangency-labs#76 link re-applied. boolean/merge/p3.rs takes main's side: tangency-labs#229 wired topology.brep_p7's merge_copy_faces, which uses the notification environment, so this branch's comments on the CopyFaces merge branch no longer held. The branch now carries only the issue tangency-labs#76 links. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rsection-loop Brings in batch tangency-labs#364 (tangency-labs#239 topology.face_p4, tangency-labs#229 boolean.merge_p2). The merge_p4 note keeps this branch's text, with main's appended tangency-labs#229 sentence added. The p2 report is regenerated on the merged tree: 905 cases, 728 pass, 0 fail, 177 deferred. Against the previous head, 3 CreateUVTrimCurves deferrals move from topology.face_p4 (GetUpwardLoopuses, now ported) to topology.face_p2 (GetTolerance), and the notes' breakdown is updated. merge_p4 is identical to main. No newly unblocked stub is wired here (merge-only round). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…topology.face_p4, tangency-labs#229 boolean.merge_p2) into unit/topology.brep_p3 Conflicts: - difftest boolean_merge_p1 runner: main's flagged Canon walk (of / reachable / of_native, with the unreachable tail); graph() uses its reachable mode (the stricter criterion of d1ae46e), graph_reachable() is kept for its callers, the unreached_records diagnostic and the regression test are kept. - topology/brep/p7.rs: main's shorter unported list plus tangency-labs#204's wire-edges bullet. - four reports: main's; plan/modules.toml: main's notes with tangency-labs#204's UPDATEs appended. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…logy.face_p4, tangency-labs#229 boolean.merge_p2) into unit/topology.brep_p7-curve-class Conflicts: crates/solidcore/src/topology/brep/p7.rs (module docs: main's CopyFaces / boolean.merge_p2 sentence and this PR's curve classification wiring; the stand-in interval stubs main still described are gone) and plan/modules.toml (topology.brep_p7 notes: main's text with tangency-labs#229's update, then this PR's sentences, plus a merge-round sentence). Gated: cargo check (workspace, features cpp), fmt, clippy (both), cargo test -p difftest --features cpp (177 passed), 1557 tests, wasm; topology.brep_p7 338/233/0/105 (main 316/237/79: only this PR's 26 curve cases differ, deferred at ReSet), perturb 233/233, FUZZ-PASS 2000. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the follow-up commit. I reviewed head Context: the port itself landed on What's left in this PR: compared with
Both reports this commit regenerates ( Earlier nit: fixed. The notes now credit the 1118 zone-tolerance deferrals to There are no must-fix items. It's good to merge; CI is already green on this head. Nit (optional)
🤖 Generated with Claude Code |
…gency-labs#229) into unit/topology.face_p4 plan/STATUS.md: main's unit totals (238: 116 green, 122 partial), review nit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
STATUS keeps both lines; offset.offset_geometry_creation takes main's notes (tangency-labs#239 face_p4) plus this PR's re-run (595 pass / 279 deferred), report regenerated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QwJFBTKLZ9FFK3nVd1X4
topology.face_p4, tangency-labs#229 boolean.merge_p2) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Takes over the claim in #134 (no commits since 10-16; see the comment there).
Ports the 10
SmFacemembers oftopology.face_p4intotopology::face::p4:GetBrep,GetEdges,GetVerticesGetUpwardLoopuses,GetUpwardVertexuses,GetUpwardEdgeusesIsConnectedTo,GlobalLineIntersectFindVertexSector,HasSeamProblemBack-edges. 26 of the 44
UNPORTED(topology.face_p4)sites are wired (28 came with this PR; 18 more arrived as main merged #196, #145, #130, #155 and the #259 batch):GetEdgesinbrep_cache,trimming_tools,face_p1,heal_data_p3,geom.tolandquery.face_p2GetVertices,GetBrepand the upward-use getters inface_p1GlobalLineIntersectinis_inner_shell, which classifies on a copy of the B-repGetBrep/GetVertices/GetEdgesinheal.stitch_p1,topology.brep_p5,brep_p7andbrep_p10;FindVertexSectorintopology.brep_p9GetEdgesandGetBrepintopology.face_p3The existing partial ports
topology_traverser::face_edgesandquery::face::p2::face_verticesnow delegate to the full ones. The other 18 take generic handles that can't reach a B-rep, or, inbrep_p5'sFindVertexSector, have no kernel to pass; each is listed inmodules.toml.In the other direction,
HasSeamProblemnow callsintersect.curve_class_p4'sGetLoopCrossings(merged in #259). The runner's classification objects answer part 5'svertex_face_gap(#85) and #137'stopology_entity/topology_handle, so the 50 periodic seam cases run up tocurve_class_p4's ownintersect.curve_class_p2stubs.Second review's should-fixes.
has_seam_problemgives the V seam its own temporaries (2/3), andSeamCheckreturns both seams' curves, because upstream's classifications own them.is_inner_shellruns all its rays on one copy of the B-rep instead of cloning it for every ray/face pair.query.face_p3(#132, merged since). face_p4's two calls into it are now wired:sm_FaceClassifyUVLineforGlobalLineIntersect's range solutions, using face_p2's UV line handles (nowpub(crate)), andSectorClassifyCurveOnforFindVertexSector. face_p4 no longer has anyUNPORTEDmarkers of its own. Its 80query.face_p3deferrals now defer oncurve.crv_on_surfinsideSectorClassifyCurveOn. The duplicateClassifyProblemco-port is dropped in favour ofquery::face::p1's (re-exported).Review fixes (bherbruck, at c0c4707):
GlobalLineIntersectandFindVertexSectornow convert their tolerance asSmZoneTol3ddoes (fabs,SM_USE_DEFAULT), and the runner has 810 new tolerance line cases;FindVertexSector's interval keeps the natural interval whenSetMinMaxrefuses a reversed pair; theIsConnectedToshell / region null checks (port and shim) now match upstream's dereferences.Fewer passes after #269. Since #147 (merged in batch #269), the surface cache's copy keeps the surface's owner. Its subdivision test now reaches
SmTol::GetZoneTol3dof the face-owned surface,UNPORTED(surface.surface_cache_p2), which main now tagsgeom.tol. So 329 cases that used to pass, matching the system tolerance only by coincidence, now defer. That is main's change, not this PR's.Other units after the wiring (0 failed throughout):
topology.face_p3823 passed (654 on main without this PR),topology.trimming_tools2850 (was 2842; the shim now runsCreateUVTrimCurves, which Rust no longer defers, and both sides compare the UV trim curves),topology.topology_traverser895 (was 889),heal.stitch_p1234 (was 232). Inoffset.offset_geometry_creation,UpdateFaceSurfaceDomainnow runs past the face_p3 getters. On a face imported as its B-spline part, it defers underharness:analytic-geometry-export, asFaceuseOffsetalready does: its virtual surface calls and the face surface the record compares would be the B-spline kind's, not the analyticSmSurfOfRevolution(after main's exactSmSurfOfRevolutionimport, only inexact faces defer there: 585 passed, 565 on main). Intopology.brep_p9,MakeVertexLoopandMakeEdgeInFaceWithVUnow run past the face_p4 / face_p3 calls they used to defer at. That exposed three runner gaps, and 8 cases failed on CI:MakeEdgeInFaceWithVU's edge, loop and face;Face::loops/Loop::edgescaches that the importer leaves empty were being compared;The runner now records the outputs, empties those caches before comparing, and compares only the reachable topology (
graph_reachable). Result: 681 passed, 669 on main.In
face_p1,SwapUVused to discardGetBrep()'s result; a face without a B-rep now reports the null dereference that upstream would hit.HasSeamProblem↔SetProps_Stage2. Upstream they call each other. A newFaceProps::set_props_stage2_withtakes the seam call as a callback, whichhas_seam_problemsupplies. The seam curves areClassObject::temporaryhandles, as incurve_class::p8.trimming_tools. WiringGetEdgesmakesClearUVTrimCurvesrunnable in Rust, so the shim now runs it and the runner compares the resulting UV trim curves. Before, it was a guarded "not run" case.Upstream defects (
plan/upstream-issues.md):HasSeamProblemreturnsSM_SUCCESS(1000) as itsSmBoolean, so it is always TRUE on the face-properties path. Found by the difftest.FindVertexSector's block-scopedsSurfDomainis empty on later passes.GlobalLineIntersectreadssCC[0]of an empty classification.x86-64 fix. The first CI run failed 326
GlobalLineIntersectcases on x86-64 only. The shim built the line fromSmPoint3d p(ad.num(), ad.num(), ad.num()). C++ leaves the order in which those arguments are evaluated unspecified, and x86-64 g++ reads them right to left, so the C++ side intersected the line through(z, y, x). The shim now reads each value into a local first (plan/findings.md, General). The counts above are from x86-64 (Linux Mint 22.3 / Ubuntu 24.04, GCC 13.3, as CI), and the committed report reproduces byte for byte.Verification (x86-64, on d27e87a, main through 7b57d8d): fmt, rustfmt of the include!d runners, clippy (workspace,
--features difftest/cpp),cargo test -p solidcore(1551 passed), wasm32 and--green(237 units, 116 passed, 121 partial, 0 failed).🤖 Generated with Claude Code