usda: parse typed tuple values inside time samples#59
Merged
Conversation
`.timeSamples = { ... }` previously dispatched each per-time value
through `parse_property_metadata_value`, which is the type-blind
metadata-block parser and has no case for tuple syntax. Typed-tuple
samples — `quatf[]` rotations `(w, x, y, z)`, `float3[]` translations
`(r, g, b)`, `matrix4d` xforms — bailed with `Unsupported property
metadata value token: Punctuation('(')`.
Thread the property's `TypeInfo` from the call site through to
`parse_time_samples` and dispatch each per-time value through
`parse_value(info)` so it lands in the matching `Value::QuatfVec` /
`Vec3fVec` / `Matrix4d` variant.
Pixar's `UsdSkelExamples/HumanFemale.walk.usd` is the canonical
broken case; with the fix it parses to a usable stage. Adds a
parser-level regression covering quatf[], float3[], and matrix4d
time samples.
Contributor
Author
|
Quick note: this only touches USDA. USDC was already fine, since each per-time value in the binary stream carries its own type tag and decodes through the typed value path. So both formats parse typed-tuple time samples correctly after this lands. |
There was a problem hiding this comment.
Pull request overview
This PR fixes the USDA parser’s handling of .timeSamples = { ... } for attributes whose per-sample values require type-aware parsing (notably tuple-based syntaxes like quatf[], float3[], and matrix4d). It threads the attribute’s already-parsed TypeInfo into parse_time_samples and parses each sample using the same typed parse_value(...) path used for default values.
Changes:
- Pass the attribute
TypeInfointoparse_time_samples(...)at the.timeSamplesparsing site. - Update
parse_time_samplesto dispatch each sample viaparse_value(info)(type-aware), instead ofparse_property_metadata_value()(type-blind). - Add a regression test covering tuple-array quaternion samples, tuple-array float3 samples, and matrix4d row tuple samples.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The first revision of this PR moved every per-time value through parse_value(info), which broke the spec corpus's attributes.usda fixture. That fixture deliberately authors bare scalars (5.67, -7) and `None` against a typed `vector3f` property to check parser tolerance — strict typed dispatch rejected them. Peek the next token first: only route through the typed path when the property's type expects a tuple AND the next token actually opens one (`(` or `[`). Otherwise fall back to parse_property_metadata_value(), preserving the historical tolerance. Adds a parse_lenient_time_samples_keep_scalar_and_none regression covering the attributes.usda shape that was failing CI.
bresilla
force-pushed
the
fix/typed-timesample-tuples
branch
from
f9d3388 to
7b81499
Compare
Owner
|
Nice! |
mxpv
added a commit
that referenced
this pull request
Apr 26, 2026
Broadens the type-aware time-sample dispatch from PR #59 to cover scalar array types (int[], float[], token[], bool[], …) in addition to tuple types. Each per-time value whose declared type is an array and whose literal opens with `[` now routes through `parse_value`, landing in the precise `IntVec` / `FloatVec` / `TokenVec` / `BoolVec` variant rather than the type-blind metadata path's `Int64Vec` / `DoubleVec` / `StringVec` fallbacks. The lenient fallback (bare scalars and `None` authored against typed properties, as in the spec corpus's `attributes.usda`) is preserved: the dispatch only kicks in when the next token opens a literal whose shape matches the declared type. Renames `next_is_typed_tuple_value` to `next_is_typed_value` since it now covers more than tuples, and updates docs accordingly.
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.
What
.timeSamples = { ... }was dispatching each per-time value throughparse_property_metadata_value, which is type-blind and has no case for tuple syntax. So typed-tuple samples likequatf[]rotations(w, x, y, z),float3[]translations(x, y, z), andmatrix4drows bailed withUnsupported property metadata value token: Punctuation('(').The fix threads the property's
TypeInfo(already in scope at the call site) through toparse_time_samplesand dispatches each sample viaparse_value(info), so they land in the rightQuatfVec/Vec3fVec/Matrix4dvariant.Why I needed this
Trying to load Pixar's UsdSkel test asset
HumanFemale.walk.usd, whose rotation and translation timeSamples are arrays of tuples. Without this fix, basically every UsdSkel rig in the wild fails to parse.Test
Adds
parse_typed_tuple_time_samplescoveringquatf[],float3[], andmatrix4dtime samples. All 338 lib tests pass,cargo fmt --checkis clean.