Prepare applied USD schema fallback precedence - #3572
Closed
adenzler-nvidia wants to merge 18 commits into
Closed
Conversation
Add an opt-in composed resolver path that gives applied schemas ownership of their unauthored properties. Prefer registered PXR fallbacks and use a built-in catalog when vendor schema plugins are unavailable, while keeping the existing resolver behavior unchanged by default.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds applied-schema fallback catalogs and ownership-aware USD resolution. Legacy results remain during compatibility periods, while the importer records affected properties and emits ChangesUSD schema resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant USDStage
participant SchemaResolverManager
participant UsdSchemaRegistry
participant USDImporter
USDStage->>SchemaResolverManager: resolve property
SchemaResolverManager->>UsdSchemaRegistry: inspect applied-schema fallback
UsdSchemaRegistry-->>SchemaResolverManager: return fallback value
SchemaResolverManager-->>USDImporter: return legacy value and record provenance
USDImporter-->>USDImporter: emit DeprecationWarning
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/_src/usd/schema_resolver.py (1)
342-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify
schema_fallbacksprecedence in the docstring. For schemas registered with USD'sSchemaRegistry, the registered fallback value wins over the supplied table (see_pxr_fallback: the supplied catalog is consulted via_mapping_fallbackonly when the registry has no value). A caller supplying e.g.{"NewtonJointAPI": {"newton:armature": 9.0}}will still resolve to the registered0.0, which the current "Overrides or additions to the built-in USD fallback catalog" wording does not convey. Suggest noting these overrides apply only when the schema/property is unregistered (i.e., the vendor plugin is unavailable).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/usd/schema_resolver.py` around lines 342 - 357, Clarify the schema_fallbacks argument in create_schema_resolution’s docstring: supplied values are used only when USD’s SchemaRegistry has no registered fallback for the schema/property, such as when the vendor plugin is unavailable. State that registered USD fallback values take precedence over the supplied mapping, rather than describing it as an unconditional override.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/_src/usd/schema_resolver.py`:
- Around line 342-357: Clarify the schema_fallbacks argument in
create_schema_resolution’s docstring: supplied values are used only when USD’s
SchemaRegistry has no registered fallback for the schema/property, such as when
the vendor plugin is unavailable. State that registered USD fallback values take
precedence over the supplied mapping, rather than describing it as an
unconditional override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b92f6556-1b86-4b90-ab36-6e5031ab1865
📒 Files selected for processing (11)
CHANGELOG.mddocs/api/newton_usd.rstdocs/concepts/usd_parsing.rstnewton/_src/sim/builder.pynewton/_src/usd/_schema_fallbacks.pynewton/_src/usd/schema_resolver.pynewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/tests/test_import_usd.pynewton/tests/test_schema_resolver.pynewton/usd.py
Preserve existing add_usd resolver results while auditing where applied USD schema fallbacks will change precedence. Emit one migration warning per import and leave the source-neutral facade to the follow-up change.
adenzler-nvidia
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/_src/usd/schema_resolver.py (1)
444-480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a short docstring for this non-obvious method.
_record_legacy_fallbackimplements a subtle backward/forward-compatibility check (short-circuiting based on resolver order vs. applied-schema ownership) with no docstring, unlike sibling helpers such as_collect_on_first_use/_resolve_valuewhich have one-liners. A brief explanation of what "legacy fallback" divergence means here would help future maintainers reason about the control flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/usd/schema_resolver.py` around lines 444 - 480, The non-obvious _record_legacy_fallback method lacks documentation describing its compatibility check. Add a brief docstring explaining that it records properties where legacy fallback resolution diverges from the current resolver’s value or ownership, while preserving the existing control flow and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 11-12: Update the Changed entry in CHANGELOG.md to include
migration guidance matching the USD parsing documentation: instruct users to
author the intended property value explicitly so it remains preserved when
fallback precedence changes.
---
Nitpick comments:
In `@newton/_src/usd/schema_resolver.py`:
- Around line 444-480: The non-obvious _record_legacy_fallback method lacks
documentation describing its compatibility check. Add a brief docstring
explaining that it records properties where legacy fallback resolution diverges
from the current resolver’s value or ownership, while preserving the existing
control flow and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9a4fdba2-2df5-46b4-a7f4-db14d54d2bfb
📒 Files selected for processing (7)
CHANGELOG.mddocs/concepts/usd_parsing.rstnewton/_src/usd/_schema_fallbacks.pynewton/_src/usd/schema_resolver.pynewton/_src/utils/import_usd.pynewton/tests/test_import_usd.pynewton/tests/test_schema_resolver.py
💤 Files with no reviewable changes (1)
- newton/_src/usd/_schema_fallbacks.py
Route legacy auditing and composed resolution through one private policy. Keep specialized joint paths on that policy so the future fallback switch requires no public API or scattered importer edits.
Reuse authored reads while auditing future schema fallbacks, and keep PXR-only custom getters on the legacy path when they cannot evaluate a source-neutral fallback.
3 tasks
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Distinguish expected future-resolution gaps from resolver failures so compatibility auditing cannot hide callback bugs. Treat registered properties without raw fallbacks as missing and report unauditable legacy values explicitly.\n\nMake first-party partial-schema and external-asset coverage assert the exact migration warning while preserving strict handling for every unexpected warning.
Let USD consumers adopt applied-schema fallback precedence during the compatibility period. Keep legacy resolution as the default while making its deprecation actionable, and migrate the Unitree examples to exercise the target behavior.
Distinguish the release default from explicit composed and legacy choices. This keeps the later default flip internal while allowing consumers to pin either behavior during migration.
Keep the public migration switch as a conventional false-by-default boolean. Consumers can opt into applied-schema fallback precedence explicitly while existing imports retain their current behavior.
Store the migration choice as the public boolean instead of a second policy enum, and declare the source-neutral reader callback on the resolver descriptor. Reuse authored reads in specialized joint auditing, share velocity-limit handling, and attribute migration warnings to consumer code.
Preserve the builder margin when PhysX's offset fallback selects its engine default, avoiding invalid shape margins. Match the built-in joint velocity fallback to the current PhysX schema when its plugin is unavailable.
This was referenced Jul 22, 2026
Normalize legacy and composed fallback values with the same importer rules before deciding whether migration guidance is needed. Keep warnings for effective changes while allowing equivalent upgrades to run cleanly.
Adopt the shared joint DOF resolver added on main while preserving migration-aware velocity and limit fallback handling for ordinary, merged, and D6 joints.
Remove explicit negative warning assertions from schema fallback tests. The suite-wide strict warning policy now detects regressions while positive assertions remain for warnings that are required behavior.
Rely on USD schema definitions for authoritative fallbacks instead of maintaining a copied catalog. Keep unregistered resolvers functional through their compatibility defaults after importer defaults. Exercise the composed behavior in functional tests and reserve migration warning assertions for focused compatibility coverage.
Resolve applied PhysX and MJC defaults at their resolver priority when schema plugins are unavailable. This keeps registration from changing import behavior and restores MuJoCo joint-limit parity.
Distinguish missing, blocked, and authored-null values during composed resolution so explicit blocks do not resurrect schema defaults. Keep MuJoCo zero-solref behavior and lock the unregistered PhysX velocity fallback with integration coverage.
3 tasks
3 tasks
3 tasks
Member
|
Closing in favor of #3888. |
3 tasks
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
3 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.
Description
Prepares applied-schema fallback precedence while preserving current
ModelBuilder.add_usd()results by default. Newton computes the future composed result internally: authored values follow resolver priority, while an applied, registered USD schema owns the fallback for an unauthored property. Fallbacks come fromUsd.SchemaRegistrycomposed prim definitions; Newton no longer carries a copied Newton, PhysX, and MuJoCo fallback catalog.ModelBuilder.add_usd(..., use_applied_schema_fallbacks=True)opts into the composed behavior without migration warnings. The boolean defaults toFalseduring the compatibility period, so existing imports retain legacy precedence. Newton emits one aggregatedDeprecationWarningwhen a registered schema fallback would change the effective result; explicitly authoring the affected values preserves asset behavior across the transition.Unregistered schemas remain functional as a second-class compatibility path: authored values still participate in resolver priority, and resolver compatibility defaults remain after importer defaults when the schema applies. They do not claim first-class schema-fallback ownership. Registering the PhysX and MuJoCo codeless schema plugins is the path to authoritative fallback behavior without duplicated defaults.
The registry lookup is part of the PXR adapter, not the resolution policy. The source-neutral resolution layer consumes schema applicability and fallback values through callbacks, so a later PXR-free adapter can supply the same registered schema metadata without changing precedence or restoring a private fallback catalog.
One private compatibility path governs generic properties, joint gains and limits, and velocity-limit sentinels. Specialized authored reads are cached while auditing, and warning attribution points to the consumer call site. The composed resolver remains independent of the migration choice so the legacy branch and audit machinery can be removed after the default transition.
Existing custom resolvers, including PXR-only getters, remain compatible on the default path. Registered fallbacks take precedence in the opt-in path; unsupported PXR-only fallback getters are reported explicitly.
PR relationship and dependencies
main, including matching multi-apply schema ownership metadata.Checklist
CHANGELOG.mdhas been updatedMigration behavior
Applying a registered schema without authoring one of its properties currently allows an importer or resolver compatibility default to win. This PR detects where the schema's registered USD fallback will win after the compatibility period, preserves the current result, and asks the asset author to make the intended value explicit. Consumers can opt into the target behavior now with
use_applied_schema_fallbacks=True.