feat(geom): UsdGeom schema reader behind geom feature#74
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a feature-gated UsdGeom schema reader (including xformOp evaluation and many concrete prim readers) and centralizes shared 4×4 matrix math so both UsdGeom and UsdSkel can reuse it.
Changes:
- Introduces
schemas::geombehind a newgeomCargo feature, plus a comprehensive integration fixture + tests. - Adds
crate::mathwith shared row-major 4×4 helpers; refactorsskelmath call sites to use it. - Updates docs/roadmap to reflect UsdGeom support and wires up the new geom integration test target.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/skel_reader.rs | Switches identity matrix constant to the new shared openusd::math location. |
| tests/geom_reader.rs | Adds integration tests covering UsdGeom readers (Imageable/Boundable, xforms, shapes, mesh/primvars, curves, instancing). |
| src/schemas/skel/skinning_query.rs | Uses crate::math::IDENTITY_MAT4 instead of the skel-local constant. |
| src/schemas/skel/skinning.rs | Replaces in-module mat4 helpers with imports from crate::math; removes duplicated mat4 tests. |
| src/schemas/skel/anim_query.rs | Switches mat4 imports to crate::math to share implementation. |
| src/schemas/mod.rs | Registers geom schema module behind the geom feature gate. |
| src/schemas/geom/xform.rs | Implements UsdGeomXformable xformOpOrder evaluation including !invert! and !resetXformStack!. |
| src/schemas/geom/types.rs | Adds decoded structs/enums for UsdGeom return types and token conversions. |
| src/schemas/geom/tokens.rs | Adds centralized UsdGeom token constants. |
| src/schemas/geom/shapes.rs | Adds intrinsic shape readers (Cube/Sphere/Cylinder/Capsule/Cone/Plane). |
| src/schemas/geom/read.rs | Adds cross-cutting readers (visibility/purpose/extent/proxyPrim/kind) and find_geom_prims. |
| src/schemas/geom/mod.rs | Defines the public UsdGeom module surface and re-exports. |
| src/schemas/geom/mesh.rs | Adds Mesh/GeomSubset/PrimvarsAPI readers. |
| src/schemas/geom/instancer.rs | Adds PointInstancer reader including proto targets and prune lists. |
| src/schemas/geom/curves.rs | Adds curve/patch/points/tet-mesh readers with defaults and fallbacks. |
| src/schemas/geom/camera.rs | Adds Camera reader + reads all authored attrs with spec defaults. |
| src/math.rs | Introduces shared row-major mat4 helpers + unit tests. |
| src/lib.rs | Exposes the new math module in the crate root docs and module list. |
| fixtures/usdGeom_scene.usda | Adds a dedicated fixture scene for geom integration tests. |
| ROADMAP.md | Marks UsdGeom as implemented and documents the supported surface. |
| Cargo.toml | Adds geom feature and wires geom_reader integration tests to it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+196
to
+198
| } else if let Ok(Some(_)) = stage.field::<Value>(path.clone(), A_VISIBILITY) { | ||
| out.imageables.push(p); | ||
| } else if let Ok(Some(_)) = stage.field::<Value>(path.clone(), A_PURPOSE) { |
Comment on lines
+75
to
+83
| let n = (*count as usize).max(0); | ||
| let p = order.get(i).copied().unwrap_or(4) as usize; | ||
| let nk = n + p; | ||
| if k_cursor + nk <= knots.len() && p > 0 && n > 0 { | ||
| out.push([knots[k_cursor + p - 1], knots[k_cursor + n]]); | ||
| } else { | ||
| out.push([0.0, 1.0]); | ||
| } | ||
| k_cursor += nk; |
| None => (false, op_name), | ||
| }; | ||
|
|
||
| let attr = prim.append_property(base)?; |
| }; | ||
|
|
||
| if inverted { | ||
| Ok(mat4_inverse(&m).unwrap_or(IDENTITY_MAT4)) |
Comment on lines
+112
to
+117
| let u_knots = read_double_vec(stage, prim, A_U_KNOTS)?.unwrap_or_default(); | ||
| let v_knots = read_double_vec(stage, prim, A_V_KNOTS)?.unwrap_or_default(); | ||
| let u_range = read_vec2d_scalar(stage, prim, A_U_RANGE)? | ||
| .unwrap_or_else(|| clamped_inner_span(&u_knots, u_vertex_count as usize, u_order as usize)); | ||
| let v_range = read_vec2d_scalar(stage, prim, A_V_RANGE)? | ||
| .unwrap_or_else(|| clamped_inner_span(&v_knots, v_vertex_count as usize, v_order as usize)); |
Comment on lines
+138
to
+140
| fn find_geom_prims_buckets_by_type() -> Result<()> { | ||
| let stage = open()?; | ||
| let prims = find_geom_prims(&stage)?; |
Signed-off-by: Maksym Pavlenko <[email protected]>
Hermite tangent fallback now walks per-curve segments via curveVertexCounts so a multi-curve prim's terminal tangents don't accidentally point into the next curve. Primvar Interpolation::default() flipped from Vertex to Constant to match the UsdGeomPrimvar spec default.
Pixar's PointInstancer spec mandates that authored orientationsf (quatf[]) wins over the legacy orientations (quath[]); the float-precision attribute exists specifically to override the half-precision one. The reader had the priority reversed.
Pixar's schema declares inactiveIds as int64listop prim metadata, so the canonical authoring shape is a list-op (prepend/append/delete). The reader previously only matched plain Int64Vec/IntVec, silently dropping every list-op opinion and leaving masked instances visible.
Replace ReadMesh.interpolate_boundary / face_varying_linear_interpolation / triangle_subdivision_rule (previously Option<String>) with typed token enums. Unauthored opinions now substitute Pixar's documented defaults — edgeAndCorner, cornersPlus1, catmullClark — instead of staying None.
The walker previously classified a prim as imageable if visibility or purpose held any Value type, but read_visibility / read_purpose accept only Token / String — a wrong-typed opinion like `bool visibility = false` would put the prim into the imageables bucket while value resolution returned Inherited. Match Token / String here so walker and reader agree.
compute_purpose previously walked past an authored-but-unrecognised purpose token on the local prim and inherited the parent's value, while read_purpose maps the same input to Purpose::Default. The two readers now agree: any authored opinion stops the walk and falls back to Default on unknown tokens. Both walks also drop the raw `cur.as_str() == "/"` check in favour of Path::parent() returning None at the pseudo-root — the typed API is the canonical termination signal and CLAUDE.md forbids raw path string compares.
Pixar's UsdGeomNurbsCurves carries an optional double[] pointWeights — when authored, each control vertex's weight turns the curve rational (the only way to represent shapes like exact circles). The reader previously dropped this attribute entirely, silently flattening rational curves to polynomial.
Pixar's UsdGeomXformOp allows xformOp:rotate* to be authored as either float or double, and UsdGeomCamera declares shutter:open / shutter:close as double. The readers previously narrowed both to f32 before the math ran, silently truncating mantissa bits a Pixar-compatible reader would have kept. xform.rs now keeps rotation angles in f64 end-to-end (value extract, to_radians, sin_cos, matrix). ReadCamera.shutter_open / shutter_close move to f64. Other camera attributes stay f32 because their schema type is float. Also tightens compute_local_to_parent_transform: a !resetXformStack! sentinel is only valid at index 0 per the UsdGeomXformable spec; a non-leading occurrence now surfaces an error instead of being silently dropped.
Primvar elementSize stored as Int64 was previously cast to i32 with a wrapping `as` — an adversarial or buggy author could write a value above i32::MAX and read it back as a negative or rolled-over i32 used unchecked as a stride. Treat overflow as "no opinion" via i32::try_from.
UsdGeomPlane overrides Gprim's `doubleSided = false` default with `true` — Plane is the one Gprim where the inherited default flips. ReadPlane now exposes the field and read_plane substitutes the spec default when `doubleSided` is unauthored.
translation, scale, axis rotation, and quaternion-to-matrix helpers were duplicated between geom/xform.rs and skel/anim_query.rs (with one only trivial naming difference). Move all six into crate::math under a consistent mat4_ prefix — mat4_translation, mat4_scale, mat4_rotation_x/y/z, mat4_from_quat — and drop the locals from both schema modules. Also adjusts mat4_transform_vec's docstring: "e.g. a normal" was misleading because normals under non-uniform scale need the inverse-transpose; mention that caveat explicitly so callers don't get surprised.
UsdGeomXformOp can be authored with double precision translate, scale, and quaternion values. The reader previously narrowed those values through f32 before building matrix4d output, losing mantissa bits for large coordinates and high-precision transforms. UsdGeomGprim also has schema fallbacks that should apply when opinions are unauthored: doubleSided defaults to true, and UsdGeomPrimvar interpolation defaults to constant. Match those fallbacks so composed meshes are not read as single-sided or face-varying unless the layer says so.
Owner
|
Thanks for contributing this! This is awesome. |
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
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.
UsdGeom is the foundation every USD consumer needs — meshes, transforms, cameras, instancing. Without it the crate can parse a stage but can't surface its actual content to a renderer or DCC. This PR fills that gap behind a
geomCargo feature, in the same shape asskel/physics/lux.The first commit extracts the matrix helpers from
schemas::skel::skinninginto a new top-levelcrate::mathmodule so the xform evaluator can share them withoutgeomhaving to depend onskel. anim_query + skel_reader test imports are updated in the same commit so every commit on the branch builds clean.Read-only — no authoring helpers, same as the other schema readers. Defaults across every prim match Pixar's
schema.usdaexactly.While implementing the
xformOp:rotateXYZevaluator I hit a discrepancy between Pixar's schema docs and their C++ source — opened PixarAnimationStudios/OpenUSD#4087 upstream. This PR follows the C++ behaviour (which is what consumers see in practice).Covered by 58 integration tests against a fixture that exercises every prim type. fmt and clippy both come back clean.