feat(brep): extract torus trim-frame data for Marschner composition#73
Merged
feat(brep): extract torus trim-frame data for Marschner composition#73
Conversation
📝 WalkthroughWalkthroughAdds torus support to the trim frame extraction module by introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for torus surfaces by adding the TorusTrimFrame data structure and the extract_torus_trim_frame function. The implementation handles the extraction of geometric properties from OCCT faces and includes logic for sampling outer wires into 3D polylines. A comprehensive test suite with unit and property-based tests has been added to verify geometric invariants and parameterization. I have no feedback to provide.
Add TorusTrimFrame NamedTuple and extract_torus_trim_frame mirroring
the plane, cylinder, sphere, and cone pattern. OCCT's Geom_Torus
parameterises the surface as
S(u, v) = center
+ (major_radius + minor_radius * cos(v))
* (cos(u) * x_dir + sin(u) * y_dir)
+ minor_radius * sin(v) * axis
with both u (major angle around the polar axis) and v (minor angle
around the tube cross-section) periodic in [0, 2*pi].
14 unit tests on sample_torus.step (radius-5 major x radius-1 minor):
orthonormal (axis, x_dir, y_dir), positive radii, sign_flip tracks
face.Orientation(), polyline satisfies the tube-distance identity
sqrt((radial - R)^2 + axial^2) == minor_radius to 1e-6, and
polygon-to-polyline reconstruction matches the parametric formula.
Three Hypothesis property tests fuzz major in [2, 50] and minor in
[0.1, 1] and verify the same invariants.
No composition wrapper yet; extraction only.
Co-Authored-By: Claude <noreply@anthropic.com>
lv416e
force-pushed
the
feat/trim-frame-torus
branch
from
14f6160 to
e89bbed
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/brep/test_trim_frame_torus.py (1)
29-50: Type hint nit: returnTopoDS_Facerather thanobject.Both
sample_torus_faceand_torus_facereturn OCCT face objects, and the production API (extract_torus_trim_frame) is typed againstTopoDS_Face. Usingobjectloses type information for the runtime-typechecked test suite (jaxtyping/beartype per coding guidelines).📝 Suggested change
-from brepax._occt.backend import ( +from brepax._occt.backend import ( BRepAdaptor_Surface, BRepPrimAPI_MakeTorus, GeomAbs_Plane, GeomAbs_Torus, TopAbs_FACE, TopAbs_FORWARD, TopExp_Explorer, TopoDS, ) +from brepax._occt.types import TopoDS_Face @@ -def sample_torus_face() -> object: +def sample_torus_face() -> TopoDS_Face: @@ -def _torus_face(major: float, minor: float) -> object: +def _torus_face(major: float, minor: float) -> TopoDS_Face:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/brep/test_trim_frame_torus.py` around lines 29 - 50, Update the return type annotations from object to TopoDS_Face for the fixture sample_torus_face and the helper _torus_face so the test-suite and runtime type-checking use the concrete OCCT face type (matching the production API extract_torus_trim_frame); ensure TopoDS_Face is imported where these functions are defined or add the import if missing, and keep the runtime behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/brepax/brep/trim_frame.py`:
- Around line 223-227: Update the docstring that explains the ``sign_flip``
variable so the phrase "tube's tube-centre ring" is not redundant: change it to
either "torus's tube-centre ring" or simply "tube-centre ring" in the sentence
describing ``sign_flip`` for ``TopAbs_FORWARD`` and ``TopAbs_REVERSED`` faces
(the comment that references the ``Torus`` primitive's signed-distance
convention near the ``sign_flip`` description in trim_frame.py). Ensure the rest
of the wording (references to ``TopAbs_FORWARD``, ``TopAbs_REVERSED``, and
``Torus``) remains unchanged.
In `@tests/unit/brep/test_trim_frame_torus.py`:
- Around line 78-83: Add a new unit test in the TestSignFlip class that
constructs a reversed face from sample_torus_face (use
TopoDS.Face_s(sample_torus_face.Reversed())), call
extract_torus_trim_frame(reversed_face), assert the result is not None, and
assert jnp.isclose(tf.sign_flip, -1.0) to exercise the reversed-orientation
branch of extract_torus_trim_frame and verify sign_flip == -1.0.
---
Nitpick comments:
In `@tests/unit/brep/test_trim_frame_torus.py`:
- Around line 29-50: Update the return type annotations from object to
TopoDS_Face for the fixture sample_torus_face and the helper _torus_face so the
test-suite and runtime type-checking use the concrete OCCT face type (matching
the production API extract_torus_trim_frame); ensure TopoDS_Face is imported
where these functions are defined or add the import if missing, and keep the
runtime behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22bda02d-edf9-4468-bf9e-279dac01e488
📒 Files selected for processing (2)
src/brepax/brep/trim_frame.pytests/unit/brep/test_trim_frame_torus.py
Comment on lines
+223
to
+227
| ``sign_flip`` is ``+1`` for a ``TopAbs_FORWARD`` face (outward | ||
| normal points away from the tube's tube-centre ring, matching | ||
| the ``Torus`` primitive's signed-distance convention) and | ||
| ``-1`` for a ``TopAbs_REVERSED`` face (outward points toward | ||
| the tube-centre ring, as with the inside of a toroidal hollow). |
There was a problem hiding this comment.
Minor docstring wording: "tube's tube-centre ring" is redundant.
The phrase repeats "tube"; given this class describes a torus, "torus's tube-centre ring" (or just "tube-centre ring") reads more cleanly and matches the terminology used later in the extraction function comment (Line 643).
📝 Suggested wording fix
- ``sign_flip`` is ``+1`` for a ``TopAbs_FORWARD`` face (outward
- normal points away from the tube's tube-centre ring, matching
- the ``Torus`` primitive's signed-distance convention) and
- ``-1`` for a ``TopAbs_REVERSED`` face (outward points toward
- the tube-centre ring, as with the inside of a toroidal hollow).
+ ``sign_flip`` is ``+1`` for a ``TopAbs_FORWARD`` face (outward
+ normal points away from the tube-centre ring, matching the
+ ``Torus`` primitive's signed-distance convention) and ``-1``
+ for a ``TopAbs_REVERSED`` face (outward points toward the
+ tube-centre ring, as with the inside of a toroidal hollow).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``sign_flip`` is ``+1`` for a ``TopAbs_FORWARD`` face (outward | |
| normal points away from the tube's tube-centre ring, matching | |
| the ``Torus`` primitive's signed-distance convention) and | |
| ``-1`` for a ``TopAbs_REVERSED`` face (outward points toward | |
| the tube-centre ring, as with the inside of a toroidal hollow). | |
| ``sign_flip`` is ``+1`` for a ``TopAbs_FORWARD`` face (outward | |
| normal points away from the tube-centre ring, matching the | |
| ``Torus`` primitive's signed-distance convention) and ``-1`` | |
| for a ``TopAbs_REVERSED`` face (outward points toward the | |
| tube-centre ring, as with the inside of a toroidal hollow). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/brepax/brep/trim_frame.py` around lines 223 - 227, Update the docstring
that explains the ``sign_flip`` variable so the phrase "tube's tube-centre ring"
is not redundant: change it to either "torus's tube-centre ring" or simply
"tube-centre ring" in the sentence describing ``sign_flip`` for
``TopAbs_FORWARD`` and ``TopAbs_REVERSED`` faces (the comment that references
the ``Torus`` primitive's signed-distance convention near the ``sign_flip``
description in trim_frame.py). Ensure the rest of the wording (references to
``TopAbs_FORWARD``, ``TopAbs_REVERSED``, and ``Torus``) remains unchanged.
Comment on lines
+78
to
+83
| class TestSignFlip: | ||
| def test_sign_flip_matches_face_orientation(self, sample_torus_face) -> None: | ||
| tf = extract_torus_trim_frame(sample_torus_face) | ||
| expected = 1.0 if sample_torus_face.Orientation() == TopAbs_FORWARD else -1.0 | ||
| assert jnp.isclose(tf.sign_flip, expected) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no existing test already covers the REVERSED torus orientation.
rg -nP -C2 '(Reversed|TopAbs_REVERSED)' tests/unit/brep/ | rg -i torus -C3 || echo "No REVERSED coverage for torus found"Repository: lv416e/brepax
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Locate and read the test file to confirm the test exists and matches the snippet
fd -p "test_trim_frame_torus.py" tests/Repository: lv416e/brepax
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Check line count and read the specific test method
if [ -f "tests/unit/brep/test_trim_frame_torus.py" ]; then
wc -l tests/unit/brep/test_trim_frame_torus.py
echo "=== Lines 75-90 ==="
sed -n '75,90p' tests/unit/brep/test_trim_frame_torus.py
fiRepository: lv416e/brepax
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Find and examine extract_torus_trim_frame to verify the sign_flip ternary logic
rg -n "def extract_torus_trim_frame" --type py -A 20 | head -50Repository: lv416e/brepax
Length of output: 1536
🏁 Script executed:
#!/bin/bash
# Broader search: check for any TopAbs_REVERSED usage in the torus test or related files
rg -i "topabs" tests/unit/brep/test_trim_frame_torus.py
rg -i "reversed\|complemented" tests/unit/brep/test_trim_frame_torus.py || echo "No reversed/complemented found in test"Repository: lv416e/brepax
Length of output: 361
🏁 Script executed:
#!/bin/bash
# Find the sign_flip assignment in extract_torus_trim_frame
sed -n '598,700p' src/brepax/brep/trim_frame.py | grep -n "sign_flip" -B5 -A5Repository: lv416e/brepax
Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Check the sample_torus_face fixture definition
rg -n "sample_torus_face" tests/unit/brep/test_trim_frame_torus.py -B3 -A3 | head -40Repository: lv416e/brepax
Length of output: 1796
🏁 Script executed:
#!/bin/bash
# Look for fixture definitions in conftest or the test file
if [ -f "tests/unit/brep/conftest.py" ]; then
echo "=== conftest.py ==="
cat tests/unit/brep/conftest.py | head -100
fiRepository: lv416e/brepax
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Get more of the conftest file
if [ -f "tests/unit/brep/conftest.py" ]; then
cat tests/unit/brep/conftest.py
fiRepository: lv416e/brepax
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check the OCCT API to see if Reversed() method exists and returns the right type
rg -n "Reversed\|Complemented" src/brepax --type py | head -20Repository: lv416e/brepax
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Verify the test-first development requirement by checking if there are any TODO/FIXME comments for sign_flip == -1
rg -i "sign.flip|reversed.*torus|topabs_reversed" tests/unit/brep/ --type pyRepository: lv416e/brepax
Length of output: 2145
🏁 Script executed:
#!/bin/bash
# Compare cylinder tests to see how they test REVERSED orientation
rg -n "test_sign_flip" tests/unit/brep/test_trim_frame_cylinder.py -A 10Repository: lv416e/brepax
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Check if sphere and cone tests also have REVERSED coverage
rg -n "test_sign_flip" tests/unit/brep/test_trim_frame_sphere.py -A 10
rg -n "test_sign_flip" tests/unit/brep/test_trim_frame_cone.py -A 10Repository: lv416e/brepax
Length of output: 1245
🏁 Script executed:
#!/bin/bash
# Look at cylinder_face_sdf.py to see how they create REVERSED faces
rg -n "Reversed\|REVERSED" tests/unit/brep/test_cylinder_face_sdf.py -B2 -A2 | head -50Repository: lv416e/brepax
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check the box_with_holes_cylinder_faces fixture to understand how it provides REVERSED faces
rg -n "box_with_holes_cylinder_faces" tests/unit/brep/test_trim_frame_cylinder.py -B5 -A5Repository: lv416e/brepax
Length of output: 6065
🏁 Script executed:
#!/bin/bash
# Find the fixture definition for box_with_holes_cylinder_faces
rg -n "box_with_holes_cylinder_faces" tests/unit/brep/ -B3 -A8Repository: lv416e/brepax
Length of output: 12492
🏁 Script executed:
#!/bin/bash
# Check the cylinder_face_sdf test to see if they explicitly create REVERSED faces
rg -n "\.Reversed\(\)" tests/unit/brep/test_cylinder_face_sdf.py -B3 -A3Repository: lv416e/brepax
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if sphere and cone tests have any REVERSED face test coverage
rg -n "parametrize" tests/unit/brep/test_trim_frame_sphere.py -A 5
rg -n "parametrize" tests/unit/brep/test_trim_frame_cone.py -A 5Repository: lv416e/brepax
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check the actual orientations of the sample faces from the fixtures
rg -n "sample_sphere_face\|sample_cone_face\|sample_torus_face" tests/unit/brep/ --type py -B3 -A8 | grep -E "(fixture|step|REVERSED|FORWARD)" | head -30Repository: lv416e/brepax
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Examine the cylinder property-based tests to see if they test REVERSED explicitly
sed -n '181,250p' tests/unit/brep/test_trim_frame_cylinder.pyRepository: lv416e/brepax
Length of output: 3245
Add test for sign_flip == -1 (reversed torus face).
The test only covers TopAbs_FORWARD orientation because sample_torus.step produces a forward-facing torus. The -1 branch of the orientation ternary in extract_torus_trim_frame is unexercised. Use face.Reversed() to test the reversed-orientation path as well.
Example test
def test_sign_flip_reversed_face(self, sample_torus_face) -> None:
reversed_face = TopoDS.Face_s(sample_torus_face.Reversed())
tf = extract_torus_trim_frame(reversed_face)
assert tf is not None
assert jnp.isclose(tf.sign_flip, -1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/brep/test_trim_frame_torus.py` around lines 78 - 83, Add a new
unit test in the TestSignFlip class that constructs a reversed face from
sample_torus_face (use TopoDS.Face_s(sample_torus_face.Reversed())), call
extract_torus_trim_frame(reversed_face), assert the result is not None, and
assert jnp.isclose(tf.sign_flip, -1.0) to exercise the reversed-orientation
branch of extract_torus_trim_frame and verify sign_flip == -1.0.
lv416e
added a commit
that referenced
this pull request
Apr 24, 2026
#74) Add torus_face_sdf_from_frame (pure JAX, jittable) and torus_face_sdf (convenience wrapper) closing the torus branch of Marschner-style trim-aware SDF built in #73. The composition computes the torus primitive's signed distance sqrt((r - R)^2 + h^2) - minor, applies sign_flip, and feeds the (major-angle u, minor-angle v) UV pair into trim_aware_sdf. Three degenerate cases need the safe-square + double-where guards accumulated on this stack: - polar axis (r == 0): major angle u undefined; - tube-centre ring (r == major AND axial == 0): minor angle v undefined and the primitive distance has a sqrt(0); - torus centre (delta == 0): both angles undefined. 16 unit tests across two fixtures: - sample_torus.step (major=5, minor=1.5) exercises the untrimmed collapse on four off-seam queries, all matching OCCT BRepExtrema_DistShapeShape within 0.15. - A quarter torus built in-test via BRepPrimAPI_MakeTorus(5, 1, pi/2) exercises the phantom regime: query (-5, 0, 0) sits on the tube-centre ring at u=pi (outside the quarter's [0, pi/2]) where the untrimmed primitive classifies it as inside the tube (d_S < 0). The signed blend flips it positive. jax.grad through the query is finite at the torus centre, on the polar axis, on the tube-centre ring, and in the phantom regime. The pure composition JITs end to end. Co-authored-by: Claude <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.
Summary
TorusTrimFrameNamedTuple andextract_torus_trim_framemirroring plane, cylinder, sphere, and cone. OCCT's Geom_Torus parameterises the surface asS(u, v) = center + (major + minor*cos(v)) * (cos(u)*x_dir + sin(u)*y_dir) + minor*sin(v)*axis, with both angles periodic in[0, 2*pi].sample_torus.step(major 5, minor 1): orthonormality, positive radii, sign_flip tracks face.Orientation(), polyline satisfiessqrt((radial - R)^2 + axial^2) == minorto 1e-6, polygon-to-polyline reconstruction matches parametric formula. Three Hypothesis property tests fuzz major in [2, 50] and minor in [0.1, 1].torus_face_sdffollow-up will apply the same signed-blend pattern proven on plane, cylinder, sphere.Verification
uv run pytest647 passed, 1 skipped (matplotlib), 54 deselected (slow)uv run mypy src/0 errorsuv run ruff check src/ tests/cleanuv run ruff format src/ tests/cleanRelated
Summary by CodeRabbit
Release Notes
New Features
Tests