Exchange - #7
Open
chenkasirer wants to merge 18 commits into
Open
Conversation
Replace the ToBrep() + JoinBreps face-rebuild path with RhinoCommon's low-level construction API (AddSurface, Faces.Add, AddEdgeCurve, AddTrimCurve, Trims.Add, Trims.AddSingularTrim), ported from compas_rhino.geometry.brep.builder._RhinoBrepBuilder. See ADR-0002. The old path cropped each surface to the (u, v) bounding box of its outer loop's trim endpoints — a rectangular parametric crop that cannot represent a trimmed face. A filleted box round-tripped 26 faces -> 18, volume 7.563414 -> 7.063681. It now round-trips exactly. Two workarounds go away with it: - The hand-tuned 1e-6 JoinBreps tolerance: the builder shares edges by index rather than rediscovering them by proximity, so it runs at TOL.absolute with nothing to fudge. - Dropped singular trims: rhino_extract_topology and rhino_brep_to_data no longer skip edgeless trims. A sphere's 2 pole trims now serialize (edge: -1 plus the vertex they collapse to) and rebuild via AddSingularTrim. BrepTrim grows .vertex / .is_singular to carry them. The document does not pin a plane's parameterization — the "plane" tag is point + normal, fixing no x-axis — so planar faces re-derive their pcurves against the rebuilt plane instead of reusing the serialized ones. Non-planar faces use the serialized pcurves directly, where the NURBS control points do pin the parameterization. occ_rebuild now skips edge: -1 trims explicitly rather than letting edges[-1] silently bind the last edge. Format stays at v5. Two test fixes, both pre-existing faults the old code masked: - UNIT_BOX_OCC_DATA's bottom face wound against its own normal. The old rebuild discarded loop winding, so it read as outward anyway; the builder honors the document, exposing it as one flipped face (volume 2/3). - The version assertion still said 4 after the writer moved to 5. Verification: pytest -m occ passes (217). Rhino-marked tests do NOT run here — this machine has no rhinoinside and -m 'not rhino' skips them by default — so all 47 were executed inside a live Rhino 8 via the LAMCP bridge instead, where they pass. A real pytest -m rhino run is still owed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Loop role was encoded positionally in v5 -- readers took loops[0] as the outer loop while neither writer guaranteed that order. v6 tags each loop "outer" | "inner" and makes curve_2d non-nullable, so both are stated rather than assumed. Both writers emit the tags (OCC via BRepTools.OuterWire_s, Rhino via BrepLoopType) and both readers select the outer loop by tag, raising on a face with zero or several. The pieces the backends must agree on move to compas_brep/exchange.py so the agreement is one definition instead of a convention each side re-implements -- which is how v5 drifted in the first place. Both legacy concessions live there and nowhere else: the positional fallback for v4/v5 loops, and the null pcurve those versions allowed. Non-nullability is a property of a v6 document, not of the reader: the committed v4 fixture is planar-only with curve_2d: null throughout. Fix a hole that added volume instead of removing it. A box with a through-hole never round-tripped on OCC (volume 7.434513 -> 7.623009, shape invalid); the numbers are identical before this change, so the criterion surfaced a pre-existing defect rather than a new one. A planar face rebuilds from its 3D wires, whose winding says nothing about which wire is a hole, and MakeFace adds a wire without reorienting it -- so one of the two holed faces added the hole's area. ShapeFix_Face.FixOrientation now settles each wire's role; delta is now 4.5e-08. pytest -m occ -q: 227 passed. pytest -m rhino was NOT run as pytest (no rhinoinside here, and -m 'not rhino' skips it by default); the 53 Rhino-marked tests were executed inside a live Rhino 8 via the LAMCP bridge, where all 53 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI has no Rhino license and -m 'not rhino' skips Rhino tests by default even locally, so a live cross-backend round-trip is a test that never runs. Commit real Rhino-authored v6 documents instead (box, filleted box, sphere, box with a through-hole, authored in live Rhino 8) and read them back from OCC-marked tests. That is the only mechanism that catches "Rhino writes a tag OCC cannot read" on CI. A Rhino-marked test regenerates each fixture and asserts it still matches; --refresh-fixtures rewrites them intentionally. The comparison is structure- exact and float-tolerant, so it detects drift in our writer rather than Rhino's last digit. The schema test pins the format's tag set, not any one backend's. Tags a backend cannot write yet are present as strict xfails rather than omitted: cylinder/cone/sphere/torus on Rhino (slices 04, 05) and circle/arc/ellipse edge curves on both (slice 06). Verified in live Rhino that each genuinely fails today, so no xfail is hiding a passing case. The harness found two defects on its first run, both pre-existing: - OCC's rebuild flips the orientation of curved faces, costing a filleted box 26% of its volume. Not cross-backend — an OCC-authored filleted box round- trips through OCC the same way. Recorded as strict xfail; it needs its own issue, as slices 04-06 do not cover it. - Brep.from_surface had never worked on Rhino: plugins.py imported _compas_nurbs_surface_to_rhino, a name that does not exist. Fixed here because the schema test cannot cover the nurbs tag without it. The v4 document moves to tests/fixtures/legacy_v4_box.json and is now read by OCC on CI. It stays hand-written: no backend has written v4 for two versions, so there is nothing to author it with. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rhino's _extract_surface emitted plane/nurbs only and its rebuild understood
nothing else, so every analytic tag OCC wrote was dropped without error. A
cylindrical face now extracts, writes, reads and rebuilds as a cylinder.
The tag was the easy half. A COMPAS CylindricalSurface is parameterized
(angle, height) and OCC's pcurves already are; Rhino parameterizes a wall by
arc length, so its u is radius*angle. Emitting the tag with Rhino's pcurves
untouched would put every trim at the wrong angle -- worse than the nurbs tag,
because it looks right. _cylinder_and_param_map recovers the map by probing and
checks it across the whole domain rather than assuming it. Faces whose map is
not affine (a fillet: rational NURBS, angle non-linear, u/v swapped) stay
tagged nurbs, which reproduces them exactly.
Trying to read an OCC document in Rhino -- which nothing had ever done --
surfaced two pre-existing cross-backend defects:
- curve_2d meant opposite things in the two writers. OCC writes a pcurve in
its edge's direction; Rhino wrote it in the trim's. Rhino's own planar
rebuild already assumed edge-direction, so the non-planar writer was the
lone dissenter. Edge-direction wins.
- Rhino's pcurve domain was unrelated to its edge curve's. OCC requires them
to share a parameterization and read the mismatch as an edge with no range,
failing to sew the face at all. _align_pcurve_to_edge remaps knots only.
Together these mean no Rhino-authored non-planar face was ever readable by OCC
correctly -- most of slice 03's "OCC flips curved faces": the filleted box goes
4.054412 -> 7.063681 (true 7.563414) on this change alone. Not all of it; both
it and the sphere stay strict xfail.
The four committed fixtures are refreshed accordingly (a contract change), and
tests/fixtures/occ_cylinder.json is new: an OCC-authored document a Rhino test
reads, mirroring the Rhino-authored ones OCC reads on CI.
pytest -m occ -q: 261 passed, 6 xfailed. pytest -m rhino was NOT run -- no
rhinoinside here. All 78 Rhino-marked tests were executed in live Rhino 8 via
the LAMCP bridge: 78 pass, 6 xfail, 0 XPASS.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sphere, cone, and torus faces now extract, write, read, and rebuild as their COMPAS analytic types through the Rhino backend, matching OCC's coverage (the representational-fidelity bar of ADR-0001). The document's analytic parameter space (u the angle about the frame's z-axis; v a height, latitude, pipe-angle, or slant distance per tag) is stated once in exchange.py and pinned against OCC's native gp_ surfaces by test_exchange_parameterization.py on CI. Rhino parameterizes all of these by arc length, so the cylinder's probe-then-verify param map is generalized to all four tags via that shared evaluator; a fillet stays nurbs because its map is not affine. Rebuild revolves the document's own generating curve rather than trusting Rhino's Sphere/Torus/Cone.ToRevSurface, which disagree with the document in v (torus arc-length in both directions; cone measured from the apex). Two pre-existing defects surfaced by reading OCC documents in Rhino for the first time, both fixed: - OCC spells a pole/apex as a zero-length edge where Rhino uses a singular trim. brep_to_rhino now bridges the two, so an OCC sphere/cone rebuilds instead of raising. Sphere and torus are the first shapes OCC->Rhino has produced as valid solids. - A planar cap's surface patch was sized from loop vertices, but a circular cap has one seam vertex on its rim; the cone's radius pushed the circle off the patch and trimmed it small. Sized from the boundary curves now. Rhino-authored sphere/cone/torus fixtures committed (sphere flips nurbs -> sphere, its OCC rebuild volume now clearing the 1e-3 bar); OCC-authored mirrors committed for the Rhino read direction. All four analytic schema xfails are gone on both backends. pytest -m occ -q: 284 passed, 5 xfailed. Rhino-marked tests are not run here (no rhinoinside; -m 'not rhino' skips them) — all were verified in live Rhino 8 via the LAMCP bridge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Widen the exchange document's edge curves from `line | nurbs` to `line | circle | arc | ellipse | nurbs`. An exact cylinder crossed the wire carrying degree-11 polynomial approximations of its own seams; a NURBS circle's parameter is not its angle, so seam and wall pcurve traced the same circle at different rates and the rebuilt wall came back wrong. An analytic edge carries its conic AND the parameter interval the edge runs over. The interval is not redundant with the conic: a trim's pcurve is written over it, and the intervals real kernels hand out do not fit COMPAS Arc's 0 <= angle <= 2*pi (OCC writes a sphere's meridian over [3pi/2, 5pi/2], a tilted cut's ellipse over [3pi/2, 3pi/2 + 2pi]). So `circle` and `arc` both carry a COMPAS Circle and differ only in whether the interval is a full turn; COMPAS Arc is deliberately unused, since using it would force a frame rotation and a matching pcurve shift that buy nothing. The curve parameter space is stated once in exchange.py and pinned against the real kernel on CI, mirroring what slice 05 did for surfaces. Measured on OCC: cylinder round-trip volume error 1.24e-07 -> 2.2e-16, cone 1.55e-08 -> 5.6e-17 while regaining an edge it used to lose. The Rhino half is written but NEVER RUN — no bridge, no license, no rhinoinside. Its writer verifies the recovered parameterization across the whole edge and falls back to `nurbs` (exact for these curves) rather than emit a tag it cannot stand behind. The Rhino-authored fixtures could not be regenerated and still carry `nurbs` seams. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ends slice 06's "unrun on Rhino" state with the first live-kernel execution
of that code, via the LAMCP bridge:
- Rhino's planar-face pcurve projection never handled circle/ellipse edges,
so slice 06's widened edge tags made any shape with an analytic cap edge
(e.g. box_with_hole) hard-crash in the Rhino backend. Fixed.
- OCC's gp_Ax3-to-Frame conversion ignored left-handed ("indirect")
placements, misplacing fillet faces whose axis disagrees with x cross y.
Fixed, with the pcurve-mirroring correction it requires.
- OCC's area/volume/centroid queries ran GProp with no tolerance, converging
~0.9% off on rational NURBS solids. Now converges at TOL.absolute.
Refreshed all 7 Rhino fixtures from live Rhino; 5 changed (nurbs seams became
exact circle/arc), tightening volume_atol to 1e-6 accordingly. Swapped the
schema test's 'arc' source to a sphere (measured: fillet blend edges are not
angle-affine in Rhino) and marked 'ellipse' as a genuine, measured Rhino gap
rather than a speculative one.
Filed .agents/issues/brep-exchange/07 for the one remaining defect found:
reversed analytic faces (holes) rebuild valid-but-not-solid in Rhino. Measured
this is not caused by any change here (reproduces identically with the
face-reversal loop-winding fix reverted) and is likely the same defect slices
04/05 flagged and could never reproduce against a real kernel.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
brep_to_occ minted a fresh, independent native TopoDS_Edge/TopoDS_Vertex per face for every trim, relying on BRepBuilderAPI_Sewing's 1e-6 tolerance to merge near-duplicates back together across faces. Two consecutive Brep.__from_data__().__data__ round trips were enough to collapse a real cone into a degenerate point. brep_to_occ now caches native vertices/edges by document identity within a single rebuild, so two faces referencing the same document edge/vertex get the same native object. Getting this right also required: building a proper two-pcurve UpdateEdge representation for a periodic surface's seam edge (walked twice by the same face), and building a genuine BRep_Builder degenerate edge for a pole/apex instead of dropping its trim -- which also fixes the long-standing seam-invalidity defect on box_with_hole carried since slice 02 (its strict xfail is removed, now passing). Closes .agents/issues/brep-exchange/08-occ-rebuild-does-not-share-edges.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Store the committed fixtures as full dtype-tagged Brep documents (compas.data.json_dump) rather than raw exchange dicts, and load them back through json_load so the fixture tests exercise the real Brep.__from_data__/.__data__ round trip. Adds @pytest.mark.occ to the well-formedness tests (they now rebuild via the backend) and a _rebuilt_once helper so drift checks compare like against like. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP: inter-backend exchange capability for brep Rhino <=> OCC, prioritizing analytical definition of the "building blocks"