feat(brep): TrimmedCSGStump wires per-primitive Marschner trim-aware SDF#76
feat(brep): TrimmedCSGStump wires per-primitive Marschner trim-aware SDF#76
Conversation
Add TrimmedCSGStump and enrich_with_trim_frames which take a reconstructed CSGStump + its source shape and produce a differentiable composite whose cylinder / sphere / cone / torus primitives route through the Marschner signed-blend trim-aware SDF while plane primitives continue to use their raw half-space SDF. The asymmetry is deliberate and documented in the module docstring. Marschner replaces a primitive's untrimmed signed distance with a face-patch distance that is positive when the query projects outside the trim polygon. For curved primitives the untrimmed extension is the CSG phantom source, so the swap kills it. For plane primitives the untrimmed half-space is already the correct CSG ingredient — a closed convex polyhedron is exactly the intersection of its half- spaces — so swapping in a face-patch distance would break the DNF for any query inside the half-space but projecting outside the face. Curved REVERSED faces multiply by sign_flip = -1 in the trim-aware wrapper; to keep the DNF semantics the same, the matching column in the intersection matrix is negated. Plane REVERSED faces keep their raw SDF unchanged so no column flip is needed. Six tests on sample_box and box_with_holes lock in the wiring without asserting quantitative phantom reduction (left to PR D): - sample_box (plane-only, no phantom) -> trimmed SDF signs and volume match the untrimmed DifferentiableCSGStump within grid noise. - box_with_holes -> trimmed volume differs from untrimmed by more than 1 unit (i.e. the Marschner path actually activates), and a query directly above the box on a hole's axis is classified as outside by the trim-aware stump. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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 TrimmedCSGStump, which integrates Marschner signed-blend trim-aware SDFs into the CSG-Stump composition to eliminate phantom material from curved primitives like cylinders and spheres. While the implementation correctly handles face orientations and provides a differentiable volume integration path, the review identifies a significant issue: the current SDF dispatch for curved primitives relies on static frames and ignores the primitive objects, which breaks differentiability for those shapes. Additionally, the volume method should be updated to automatically compute bounding boxes when they are not explicitly provided, ensuring consistency with existing API patterns.
Two items from Gemini, both adopted: - **Gradient flow (HIGH)**: the old design kept both primitives and frames with independently copied JAX arrays. Curved primitive SDFs read from the frame, so gradients w.r.t. the primitives tuple were zeroed out. Drop the primitives field entirely; frames are the single source of truth for differentiable parameters. Plane dispatch now reads the outward-normal and origin from the frame directly as well, so every slot's gradient flows through frame fields. A regression test verifies jax.grad over the stump produces finite gradients on at least one frame's normal. - **Auto-bounds (MEDIUM)**: volume() accepts optional lo / hi and falls back on the bounding box captured at construction time. enrich_with_trim_frames stashes the reconstructed stump's bbox_lo / bbox_hi when available (OCCT metadata path) and uses _primitives_bounds only as a secondary fallback, avoiding the warning emitted for infinite primitives such as planes. Matches the ergonomics of DifferentiableCSGStump. Unifying on frames also removes the plane/curved matrix-flip asymmetry: all REVERSED slots now flip their matrix column because every dispatch consumes the frame's convention (outward normal for planes, sign-flipped radial distance for curved). The module docstring continues to explain why plane slots deliberately skip the Marschner blend even though they read from the frame. Eight tests pass (prior six plus the two new ones for bounds and gradient flow); full suite 723 passed. Co-Authored-By: Claude <noreply@anthropic.com>
|
Gemini review resolved in this commit (HIGH + MEDIUM):
Unifying on frames also removes the earlier plane/curved matrix-flip asymmetry: all REVERSED slots now flip their matrix column because every dispatch consumes the frame's convention. The module docstring continues to explain why plane slots deliberately skip the Marschner blend even though they read from the frame. 8 tests pass (6 prior + 2 new); full suite 723. |
1 participant
Summary
TrimmedCSGStumpandenrich_with_trim_frames: take a reconstructedCSGStump+ its source shape and produce a differentiable composite whose cylinder / sphere / cone / torus primitives route through the Marschner signed blend while plane primitives continue to use their raw half-space SDF.sign_flip = -1in the trim-aware wrapper; to keep the DNF semantics unchanged, the matching column in the intersection matrix is negated. Plane REVERSED faces keep their raw SDF unchanged so no column flip is needed.sample_box(plane-only, no phantom — trimmed sdf signs and volume match the untrimmed stump within grid noise) andbox_with_holes(trimmed volume differs from untrimmed by more than 1 unit so the Marschner path actually activates, and a query directly above the box on a hole's axis is classified as outside).Verification
uv run pytest721 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