Skip to content

fix: pcp, value clip and usda writer correctness - #139

Merged
mxpv merged 9 commits into
mxpv:mainfrom
bresilla:fix/correctness
Oct 4, 2026
Merged

mxpv merged 9 commits into
mxpv:mainfrom
bresilla:fix/correctness

Conversation

@bresilla

@bresilla bresilla commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Hi man, now that I'm using this more across multiple projects I'm upstreaming some of the fixes I've been carrying locally. This one is just correctness fixes, and I'll have a few more PRs with some features later.

Six correctness fixes, one per commit, so any of them can be cherry-picked on its own (6 builds on 5). Each was checked against C++ OpenUSD 25.05.01 (Python bindings and usdcat), and each commit's test fails on main and passes with the fix.

1. fix(pcp): keep empty path out of index cache

Querying an attribute on the pseudo-root caches a prim index for the empty path, and muting a sublayer afterwards panics in index_store.rs. ensure_index now skips the empty path. In C++, a stage with a sublayer, then GetPseudoRoot().GetAttribute("x").Get(), then mute, unmute, mute raises no error. Test: mute_after_pseudo_root_attribute_query in usd/stage.rs.

2. fix(pcp): interpolate across clip activations

Between two active clips the value comes only from the clip active at the query time, so clips 0:1 / 20:3 and 0:5 / 20:7 with active = [(0, 0), (10, 1)] give 1.5 at stage time 5. value_in_set now brackets the query with the clip set's stage sample times, reads each bracketing value from the clip active at that time and interpolates between them. Across a times jump discontinuity it does not interpolate, and the earlier clip time holds up to the jump, as C++ does. In C++ the fixture above gives (0, 1.0), (5, 3.5), (9, 5.5), (10, 6.0), (15, 6.5), (20, 7.0), the values the new test clip_switch_interpolates_activation_samples in pcp/index_cache.rs expects, and clip_multi holds -15 just before its jump at 16 (C++ adds a sample at 15.999999995559108).

This commit also updates clip_interpolate_missing_boundary_is_a_sample, which expected 0.0 at 9.999 where C++ gives 49.994998931884766 (and 50 at 10). That test still asserts time_sample_times == [0, 10, 20] while C++ reports [0, 20], which this PR leaves as it is.

3. fix(usda): write customData on references

The usda writer drops customData on references. write_reference now writes it when present and leaves out an identity layer offset, as C++ does: SdfLayer::ExportToString writes @./b.usda@ ( customData = { string note = "identity" } ), and C++ reads the Rust output back with the same values. Test: reference_custom_data_roundtrip parses C++'s own output, round-trips it and checks that no offset = 0 is written.

4. fix(pcp): match identity payload offsets

delete payload = @content.usda@</Box> (offset = 0; scale = 1) does not remove a weaker prepend payload = @content.usda@</Box>, because Some(LayerOffset::default()) is compared as different from None. compose_site now normalises an identity offset to None before list-op matching. In C++, Sdf.Payload("./p.usda") == Sdf.Payload("./p.usda", layerOffset=Sdf.LayerOffset(0, 1)) is True, and usdcat --flatten of the same fixture gives an empty /Root. Test: identity_payload_offset_matches_omitted in usd/stage.rs.

5. fix(pcp): report missing reference targets

A reference or payload whose target prim does not exist in an external layer composes silently, with no composition error, both for @a.usda@</Nope> and for a layer with no default prim. prim_indexer now reports empty external targets for references and payloads, and checks sub-root targets after all composition tasks finish so a target whose specs arrive through a later arc is not reported. In C++, UsdStage::GetCompositionErrors() reports Unresolved reference prim path @a.usda@</Nope>. Test: absent_external_targets_report_reference_and_payload_errors in pcp/prim_index.rs.

6. fix(pcp): report missing same-layer targets

Internal references and payloads such as </Nope> are not reported either. Root targets are now reported like external ones, and sub-root targets use the end-of-composition check from commit 5. C++ reports a missing same-layer target at the root, below a prim and for payloads, and does not report it when a variant supplies the target. Test: absent_internal_targets_report_reference_and_payload_errors is built from that C++ fixture and expects RootMissing, SubMissing and PayloadMissing to be reported and SubPresent and FromVariantRef not to be.

PS: you can cherry pick each of them if you want separate from each other...

Copilot AI balanced review requested due to automatic review settings October 4, 2026 15:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Clip reads eagerly load all clips, and internal unresolved-target diagnostics can identify the wrong layer.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Corrects composition, value-clip resolution, and USDA serialization behavior to match C++ OpenUSD.

Changes:

  • Fixes index caching, payload-offset matching, and unresolved-target diagnostics.
  • Interpolates values across clip activations.
  • Preserves reference customData during USDA serialization.
File Description
crates/​openusd/​tests/​stage.rs Updates clip interpolation expectations.
crates/​openusd/​src/​usda/​writer.rs Serializes reference custom data.
crates/​openusd/​src/​usd/​stage.rs Adds cache and payload regression tests.
crates/​openusd/​src/​pcp/​prim_indexer.rs Reports unresolved arc targets.
crates/​openusd/​src/​pcp/​prim_index.rs Tests missing target diagnostics.
crates/​openusd/​src/​pcp/​index_cache.rs Skips empty paths and tests clip switching.
crates/​openusd/​src/​pcp/​compose_site.rs Normalizes identity payload offsets.
crates/​openusd/​src/​pcp/​clip.rs Interpolates across clip activation boundaries.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/openusd/src/pcp/clip.rs Outdated
Comment thread crates/openusd/src/pcp/prim_indexer.rs Outdated
Bracket clip values from samples in the active window, consulting the
next activation only when it supplies an interpolation bound. Honor
manifest blocks while gathering those samples. Unused, distant, and
manifest-blocked clips remain unopened, so a malformed asset outside
the query's dependencies cannot fail the read.

Use the target stack's root layer for unresolved reference and payload
diagnostics, including sub-root targets. Internal arcs on a stage with
a session layer now identify the root layer in their errors.

Define payload equality in sdf so an omitted layer offset compares equal
to an explicit identity offset. Direct comparisons and list-op deletion
and deduplication share this behavior, with no normalization in pcp.

Add regression coverage for lazy clip reads, manifest-blocked malformed
clips, session-layer diagnostics, and payload equality and list ops.

Validation: cargo test --workspace --all-targets --all-features;
cargo clippy --workspace --all-targets --all-features -- -D warnings;
cargo fmt --all -- --check --files-with-diff.
mxpv added 2 commits October 4, 2026 10:48
sdf::Payload::layer_offset is a plain LayerOffset, as on Reference and
C++ SdfPayload, so an omitted offset and an explicit identity offset
compare equal without a hand-written PartialEq.

The reference/payload arc computes its target stack's root layer once
and names it in every target diagnostic, including arc cycles and
prohibited relocation sources, which named the stack's strongest layer
(the session layer for the root stack).

Clip resolution shares one active-clip lookup and one contributes
check, and reads plain sample times when bracketing an activation. The
usda writer emits sublayer, reference, and payload metadata through one
helper.
Report an unresolved reference or payload target from a task that only
the top-level build runs, after every other task, as C++
EvalUnresolvedPrimPathError does. A nested build defers its variants,
so a reference composed inside another reference's target whose prim a
variant supplies was reported as unresolved.

The check reads each node's path at introduction, which now skips
variant selections the way C++ _GetPathAtIntroDepth does. An index at
a variant selection path starts from its own site without ancestral
opinions, as in C++ Pcp_BuildPrimIndex.
@mxpv

mxpv commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Thanks! and welcome back!

@mxpv
mxpv merged commit 37d0f2a into mxpv:main Oct 4, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants