Add subdivision surfaces via OpenSubdiv (opensubdiv-rs 0.1.2) - #127
Open
doubleailes wants to merge 2 commits into
Open
Add subdivision surfaces via OpenSubdiv (opensubdiv-rs 0.1.2)#127doubleailes wants to merge 2 commits into
doubleailes wants to merge 2 commits into
Conversation
Integrate the pure-Rust opensubdiv-rs port of OpenSubdiv's Far/Sdc layers (git tag 0.1.2 — zero dependencies, forbid(unsafe_code)) as the renderer's subdivision method, applied at USD import. A Mesh prim authoring crust:subdivisionLevel (int, default 0, clamped to 6) is uniformly refined and snapped to the limit surface, with smooth per-vertex shading normals flowing through every geometry path (committed prototypes, deferred bakes via the inverse-transpose, and the non-invertible-transform bake, which recomputes them post-transform). Opt-in per prim rather than triggered by subdivisionScheme, since USD's fallback scheme is catmullClark and honouring it alone would subdivide virtually every authored mesh; the scheme still picks the algorithm (catmullClark/bilinear/loop, none warns and keeps the cage). USD creases (per-run or per-edge sharpness), corners and interpolateBoundary are honoured; holes and faceVaryingLinearInterpolation are out of scope. Ptex stays correct under refinement: FaceMap gains optional explicit per-triangle corner UVs mapping refined triangles back into their base cage face's unit square (via a synthetic face-varying channel refined with linear-everywhere rules plus composed child->parent face maps), and check_face_count now takes the authored cage's face count by signature. Refinement happens in mesh_source before interning, so direct, deferred and prototype paths all see it exactly once and MeshKey dedupes on the refined arrays. Malformed cages and refiner errors warn and degrade to the cage. CRUST_SUBDIV=0 forces level 0 everywhere (A/B kill switch). samples/subdivision.usda shows cubes at levels 0-3 beside a fully edge-creased cube whose limit surface is the cage itself. Every existing sample renders bit-identical against pre-change goldens at 16 spp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Rz6Y7NCzAjsxadgfrHoxL
Contributor
PR Summary by QodoAdd opt-in USD mesh subdivision via opensubdiv-rs (0.1.2) with Ptex-safe face mapping
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Contributor
Code Review by Qodo
1. Loop Ptex IDs wrong
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+909
to
+913
| // A subdivided face table numbers *refined* faces; rewrite it to the | ||
| // base-cage ids Ptex actually indexes before anything caches it. | ||
| let faces = match (&src.subdiv_faces, faces) { | ||
| (Some(sub), Some(map)) => Some(remap_subdivided_faces(map, sub)), | ||
| (_, faces) => faces, |
Contributor
There was a problem hiding this comment.
1. Loop ptex ids wrong 🐞 Bug ≡ Correctness
For subdivisionScheme=loop meshes with a per-face (Ptex) texture, the importer triangulates the *refined* mesh with want_faces=true but cannot remap refined face IDs back to authored cage face IDs because Loop refinement does not produce SubdivFaces. This makes hits report incorrect face_ids (some alias other cage faces; others exceed num_faces and fall back), while check_face_count still passes because it compares against the authored cage face count.
Agent Prompt
### Issue description
Loop subdivision intentionally does not generate refined-face → base-cage-face remap metadata (`SubdivFaces`) and per-child param UVs, but the importer still builds a `FaceMap` from the refined topology when a material has `face_texture()`. This causes `HitRecord.face_id` to be interpreted as a Ptex face index even though it refers to refined faces, leading to wrong texturing (aliasing and/or fallback sampling) without a warning.
### Issue Context
- Loop subdivision is allowed (for all-triangle cages), and `want_face_uvs` is passed through from `want_faces`.
- In the remap step, the face table is only rewritten when `src.subdiv_faces` is `Some(_)`. For Loop it is `None`, so the refined IDs leak through.
- `check_face_count` now compares Ptex `num_faces()` against the authored *cage* face count, so a correct Ptex file for the cage will not warn even though runtime `face_id`s are wrong.
### Fix Focus Areas
- crates/crust-core/src/scene/subdiv.rs[132-135]
- crates/crust-core/src/scene/usd_import.rs[1246-1310]
- crates/crust-core/src/scene/usd_import.rs[909-915]
### Suggested fix
Implement one of these (prefer #1 if you want correctness over honoring Loop refinement):
1) **If `scheme == Loop` and `want_faces == true`, warn and degrade to cage** (return `cage(points, counts, indices)` in `mesh_source`). This preserves correct Ptex indexing and avoids silent wrong shading.
2) Alternatively, **disable face-table creation** for Loop subdivision (call `triangulate(..., want_faces=false)` or drop `faces` before caching) and warn that Ptex on Loop subdivision is unsupported; this avoids incorrect sampling but loses per-face texturing.
3) Full feature fix: implement a Loop-compatible base-face mapping + sub-triangle UV mapping (likely via a synthetic face-varying channel seeded with triangle-corner UVs and refined linearly, plus a composed child→parent face map), then plumb through a `remap_subdivided_faces` equivalent for triangles.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Two measurement layers, both repeatable: - An allocation-counting probe (test-only #[global_allocator] wrapping System) at the subdivide() boundary, run explicitly as an ignored test. Measured on a 64x64 quad cage at levels 1-4: ~313 B of transient requested bytes per refined face (the refiner retains every level - a x4/3 series - plus the position copies at the tail), ~556-592 B/face when the Ptex sub-face UV channel is on (the synthetic fvar channel is a full parallel hierarchy), and 48 / 84 B/face resident in the returned mesh. The probe asserts ceilings ~25% above those values, so a regression that starts retaining extra per-level data fails loudly. - scripts/gen_subdiv_stress.py generates a measurable quad-cage scene (1.18 M refined quads at level 3, bake + instanced paths) for the existing --stats methodology, A/B'd with CRUST_SUBDIV=0: traverse-phase peak +310 MiB (within 6% of the probe's model) and kernel-resident memory scaling exactly x4 per level (34.67 MiB at level 1 -> 2.17 GiB at level 4). The whole-process peak on subdivided scenes is the pre-existing SBVH build transient, not opensubdiv (commit peak 2.19 GiB vs traversal peak 1.16 GiB at level 4); recorded in CLAUDE.md next to the existing build transient note, along with the numbers and re-run commands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Rz6Y7NCzAjsxadgfrHoxL
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
2 participants
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.
Integrate the pure-Rust opensubdiv-rs port of OpenSubdiv's Far/Sdc layers
(git tag 0.1.2 — zero dependencies, forbid(unsafe_code)) as the renderer's
subdivision method, applied at USD import.
A Mesh prim authoring crust:subdivisionLevel (int, default 0, clamped to 6)
is uniformly refined and snapped to the limit surface, with smooth
per-vertex shading normals flowing through every geometry path (committed
prototypes, deferred bakes via the inverse-transpose, and the
non-invertible-transform bake, which recomputes them post-transform).
Opt-in per prim rather than triggered by subdivisionScheme, since USD's
fallback scheme is catmullClark and honouring it alone would subdivide
virtually every authored mesh; the scheme still picks the algorithm
(catmullClark/bilinear/loop, none warns and keeps the cage). USD
creases (per-run or per-edge sharpness), corners and interpolateBoundary
are honoured; holes and faceVaryingLinearInterpolation are out of scope.
Ptex stays correct under refinement: FaceMap gains optional explicit
per-triangle corner UVs mapping refined triangles back into their base
cage face's unit square (via a synthetic face-varying channel refined with
linear-everywhere rules plus composed child->parent face maps), and
check_face_count now takes the authored cage's face count by signature.
Refinement happens in mesh_source before interning, so direct, deferred
and prototype paths all see it exactly once and MeshKey dedupes on the
refined arrays. Malformed cages and refiner errors warn and degrade to
the cage. CRUST_SUBDIV=0 forces level 0 everywhere (A/B kill switch).
samples/subdivision.usda shows cubes at levels 0-3 beside a fully
edge-creased cube whose limit surface is the cage itself. Every existing
sample renders bit-identical against pre-change goldens at 16 spp.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_015Rz6Y7NCzAjsxadgfrHoxL