Parse the newton-usd-schemas#1434
Conversation
📝 WalkthroughWalkthroughAdds optional runtime registration of the Newton USD schema and updates USD schema resolution, attribute names, and importer parsing to read newton-scoped attributes (e.g., time_steps_per_second, gravity_enabled, max_hull_vertices, contact_margin) and new material fields (torsional_friction, rolling_friction). Tests and test assets updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Init as newton._src.usd.__init__
participant SchemaReg as newton_usd_schemas
participant Parser as newton._src/utils/import_usd.py
participant Resolver as newton._src/usd/schemas.py
participant Model as PhysicsModel
App->>Init: import newton.usd
Init->>SchemaReg: try import newton_usd_schemas
alt import succeeds
SchemaReg-->>Init: register Newton USD schema
else import fails
SchemaReg-->>Init: ImportError swallowed
end
App->>Parser: build_model(usd_stage)
Parser->>Resolver: resolve_attributes(prim)
Note over Resolver: map USD attrs -> canonical keys\n(time_steps_per_second, gravity_enabled, contact_margin, max_hull_vertices, material frictions)
Resolver-->>Parser: attribute values
Parser->>Parser: parse_material & parse_shape (read torsional/rolling friction, contact_margin, max_hull_vertices)
Parser->>Model: construct(parsed_attrs)
Model-->>App: PhysicsModel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
newton-usd-schemas
1b8d8fb to
f27eb44
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/usd/utils.py (1)
431-443: Add defensive checks to filter attributes and validate authored values.
GetAuthoredPropertiesInNamespace()returns both attributes and relationships; the code should guard against non-attribute properties and properties without authored values before callingattr.Get(). The same file already uses this pattern elsewhere (e.g.,if attr.IsValid() and attr.HasAuthoredValue()). Useisinstance(attr, Usd.Attribute)to filter relationships, and checkIsValid()andHasAuthoredValue()before accessing property values.
🤖 Fix all issues with AI agents
In `@newton/_src/usd/__init__.py`:
- Around line 16-19: Remove the unnecessary noqa comment from the side-effect
import in newton/_src/usd/__init__.py: keep the import newton_usd_schemas (to
register the Newton schema plugin) but delete the trailing "# noqa: F401" so the
linter no longer reports an unused directive; ensure the from pxr import Usd
line remains unchanged.
In `@newton/_src/utils/import_usd.py`:
- Around line 942-947: The code currently computes physics_dt by dividing 1.0 by
the value returned from R.get_value for key "time_steps_per_second" which can be
None or <= 0; update the logic around the R.get_value call that reads
time_steps_per_second (used to compute physics_dt) to guard against None or
non-positive values: capture the raw value into a local (e.g.,
time_steps_per_second_raw), if it is None or <= 0 replace it with a sensible
fallback (for example 60.0 or another project default) and optionally emit a
warning when verbose is true, then compute physics_dt = 1.0 /
time_steps_per_second_safe; ensure you reference physics_scene_prim and
PrimType.SCENE in the same block so behavior remains unchanged when a valid
value exists.
In `@newton/tests/test_schema_resolver.py`:
- Around line 1339-1383: The test test_max_hull_vertices assumes that when
resolver priority is [SchemaResolverPhysx(), SchemaResolverNewton()] the
resolved max_hull_vertices will be Newton's sentinel (-1/32), but PhysX
currently wins and yields 64; update the failing assertion in
test_max_hull_vertices (the assertion after creating the
physxConvexHullCollision:hullVertexLimit attribute) to expect 64, or
alternatively change the resolver order to [SchemaResolverNewton(),
SchemaResolverPhysx()] if you intend Newton to override; locate the check via
the test name and the SchemaResolverManager([SchemaResolverPhysx(),
SchemaResolverNewton()]) usage and adjust the assertion accordingly.
- Around line 1286-1338: The test test_contact_margin currently expects None but
the resolver returns the unset sentinel (-inf); update the assertion to expect
the sentinel instead (e.g. assertEqual(contact_margin, float("-inf"))) at the
call to resolver.get_value in test_contact_margin, or alternatively call
resolver.get_value with an explicit default if supported; locate the failing
line by searching for test_contact_margin and the resolver.get_value(collider,
PrimType.SHAPE, "contact_margin") invocation and replace the
assertEqual(contact_margin, None) with an assertion for float("-inf") (or pass a
default value to get_value).
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@newton/_src/usd/__init__.py`:
- Around line 16-21: The import line "import newton_usd_schemas # noqa: F401"
contains an unnecessary noqa suppressor; remove the trailing "# noqa: F401" so
Ruff RUF100 is satisfied and keep the import as-is (inside the try/except that
sets Usd = None on ImportError) so the module registration via import
newton_usd_schemas still occurs and the Usd symbol from "from pxr import Usd"
remains handled in the except block.
In `@newton/_src/utils/import_usd.py`:
- Around line 941-946: The computation of physics_dt uses 1.0 / R.get_value(...)
which can raise ZeroDivisionError or TypeError if time_steps_per_second is 0 or
None; update the logic around the call that sets physics_dt (and related call to
R.get_value with physics_scene_prim and PrimType.SCENE) to first fetch the raw
value into a local (e.g., time_steps_per_second_value), validate it (not None
and not 0), and only then compute physics_dt = 1.0 / value, otherwise use a safe
fallback default (or log/raise a clear error); similarly ensure gravity_enabled
read remains bool by coercing or defaulting when R.get_value returns None.
In `@newton/tests/test_import_usd.py`:
- Around line 2966-3168: The test file contains unused "# noqa: PLC0415" markers
on pxr import lines (e.g. the imports in test_contact_margin_parsing,
test_scene_gravity_enabled_parsing, test_scene_time_steps_per_second_parsing,
test_scene_max_solver_iterations_parsing and the earlier test that imports
UsdShade) which Ruff flags as RUF100; remove those trailing "# noqa: PLC0415"
comments from the relevant import statements (the lines starting with "from pxr
import Usd, UsdGeom, UsdPhysics" and "from pxr import Usd, UsdGeom, UsdPhysics,
UsdShade") so the imports remain but the unused noqa markers are deleted.
In `@newton/tests/test_schema_resolver.py`:
- Around line 1286-1338: The test incorrectly expects None when
SchemaResolverPhysx is first in the SchemaResolverManager; change the assertion
after creating resolver = SchemaResolverManager([SchemaResolverPhysx(),
SchemaResolverNewton()]) in test_contact_margin so it expects PhysX's mapping
default (negative infinity) instead of None—i.e., assert that
resolver.get_value(collider, PrimType.SHAPE, "contact_margin") equals
float("-inf") (or use an equivalent is-inf negative check).
- Around line 1339-1383: The test's initial expectation is wrong: when resolver
= SchemaResolverManager([SchemaResolverPhysx(), SchemaResolverNewton()]) the
PhysX mapping default for max_hull_vertices is 64, so update the assertion after
calling resolver.get_value(collider, PrimType.SHAPE, "max_hull_vertices") in
test_max_hull_vertices to expect 64 instead of -1; leave the subsequent explicit
sets and other resolver-order assertions unchanged.
🧹 Nitpick comments (2)
newton/_src/usd/utils.py (1)
395-437: Cache schema property names once per prim to avoid repeated registry lookups.Line 395–Line 406 re-scan SchemaRegistry for every attribute; precomputing the property-name set once per prim reduces overhead without changing behavior.
♻️ Proposed refactor
- def is_schema_attribute(prim, attr_name: str) -> bool: - """Check if attribute is defined by a registered schema.""" - # Check the prim's type schema - prim_def = Usd.SchemaRegistry().FindConcretePrimDefinition(prim.GetTypeName()) - if prim_def and attr_name in prim_def.GetPropertyNames(): - return True - - # Check all applied API schemas - for schema_name in prim.GetAppliedSchemas(): - api_def = Usd.SchemaRegistry().FindAppliedAPIPrimDefinition(schema_name) - if api_def and attr_name in api_def.GetPropertyNames(): - return True - - return False + prim_def = Usd.SchemaRegistry().FindConcretePrimDefinition(prim.GetTypeName()) + schema_props = set(prim_def.GetPropertyNames()) if prim_def else set() + for schema_name in prim.GetAppliedSchemas(): + api_def = Usd.SchemaRegistry().FindAppliedAPIPrimDefinition(schema_name) + if api_def: + schema_props.update(api_def.GetPropertyNames()) + + def is_schema_attribute(attr_name: str) -> bool: + """Check if attribute is defined by a registered schema.""" + return attr_name in schema_props @@ - for attr in prim.GetAuthoredPropertiesInNamespace("newton"): - if is_schema_attribute(prim, attr.GetName()): - continue - attr_name = attr.GetName() + for attr in prim.GetAuthoredPropertiesInNamespace("newton"): + attr_name = attr.GetName() + if is_schema_attribute(attr_name): + continuenewton/_src/utils/import_usd.py (1)
1417-1422: Consider documenting the sentinel value pattern.Using
float("-inf")as a sentinel to detect "not authored" is unconventional. Consider adding a brief comment explaining why this pattern is used, or using a more explicit sentinel likeNoneif the schema resolver supports it.if collect_schema_attrs: R.collect_prim_attrs(prim) + # float("-inf") is the schema default sentinel indicating "not authored" contact_margin = R.get_value(prim, prim_type=PrimType.SHAPE, key="contact_margin", verbose=verbose) if contact_margin == float("-inf"): contact_margin = builder.default_shape_cfg.contact_margin
73fb4a7 to
4f2b437
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/usd/schemas.py (1)
225-261: Fix the timestep default to match transformer units.
The SchemaAttribute default is passed throughusd_value_transformer(confirmed at schema_resolver.py:240). Since the transformer expects timestep in seconds (lambda s: int(1.0 / s)), the default must also be in seconds. Currently,500(a frequency) is passed through the transformer, yieldingint(1.0 / 500) = 0Hz—a broken fallback. Change the default to0.002(equivalent to 500 Hz in seconds).
♻️ Duplicate comments (2)
newton/_src/usd/__init__.py (1)
16-19: Remove unused# noqa: F401(already noted).newton/tests/test_import_usd.py (1)
2966-2966: Remove unused# noqa: PLC0415directives.Ruff reports these
noqamarkers are unused across multiple import statements in the new tests. These should be removed for lint cleanliness.Affected lines: 2966, 3026, 3074, 3114, 3148, 3175
♻️ Proposed fix
- from pxr import Usd, UsdGeom, UsdPhysics, UsdShade # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics, UsdShadeApply similar changes to the other affected lines.
Also applies to: 3026-3026, 3074-3074, 3114-3114, 3148-3148, 3175-3175
4f2b437 to
5b5ed98
Compare
5b5ed98 to
16ac528
Compare
Also removed irrelevant omniverse propreties
Adapt the SchemaResolvers to the proper schema attributes Maintain un-official newton attribute parsing for joints & shapes for now. I don't understand why these were added, but tests fail without them, so its a bit late to change.
16ac528 to
0c90adc
Compare
|
rebased on main (twice in 10 minutes!) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/utils/import_usd.py (1)
1419-1449: Guard against missing resolver mappings for contact_margin.If
schema_resolversis empty or lacks a SHAPE mapping (e.g., MJC-only),R.get_value(...)returnsNone, which then flows intoShapeConfigand can break downstream numeric operations. Add aNonefallback alongside the-infsentinel check.🛠️ Proposed fix
- contact_margin = R.get_value(prim, prim_type=PrimType.SHAPE, key="contact_margin", verbose=verbose) - if contact_margin == float("-inf"): + contact_margin = R.get_value(prim, prim_type=PrimType.SHAPE, key="contact_margin", verbose=verbose) + if contact_margin is None or contact_margin == float("-inf"): contact_margin = builder.default_shape_cfg.contact_margin
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_usd.py`:
- Around line 36-37: Uncomment and restore the public re-export of
SchemaResolverNewton (and the other schema resolver classes) so users can pass
instances to parse_usd via the schema_resolvers parameter: locate the commented
import and export lines for SchemaResolverNewton in the module that exposes the
public USD API, re-enable the import from ..usd.schemas (SchemaResolverNewton)
and add it to the module's exports (e.g., __all__ or explicit export list) so
SchemaResolverNewton is available to callers of parse_usd and documented as part
of the public API.
♻️ Duplicate comments (2)
newton/_src/usd/__init__.py (1)
16-20: Remove unused# noqa: F401directive.The static analysis indicates
F401is not enabled, making the suppression unnecessary. This issue was already flagged in previous reviews.🧹 Proposed fix
try: # register the newton schema plugin before any other USD code is executed - import newton_usd_schemas # noqa: F401 + import newton_usd_schemas except ImportError: passnewton/tests/test_import_usd.py (1)
2968-2968: Remove unusednoqamarkers to keep lint clean.
These# noqa: PLC0415directives are unused and trigger RUF100; drop them.🧹 Proposed cleanup
- from pxr import Usd, UsdGeom, UsdPhysics, UsdShade # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics, UsdShade @@ - from pxr import Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics @@ - from pxr import Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics @@ - from pxr import Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics @@ - from pxr import Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics @@ - from pxr import Gf, Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Gf, Usd, UsdGeom, UsdPhysics#!/bin/bash # Verify the unused noqa markers in this file rg -n "noqa: PLC0415" newton/tests/test_import_usd.pyAlso applies to: 3028-3028, 3076-3076, 3116-3116, 3150-3150, 3177-3177
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_usd.py`:
- Around line 1419-1425: get_value can return None when schema resolvers are
disabled, so in the contact_margin handling (around R.get_value(...,
prim_type=PrimType.SHAPE, key="contact_margin")), treat None the same as the
unset sentinel float("-inf") and fallback to
builder.default_shape_cfg.contact_margin; update the contact_margin assignment
logic in the block using R.get_value and also the similar occurrence around line
1445 to check for contact_margin is None || contact_margin == float("-inf") and
then set contact_margin = builder.default_shape_cfg.contact_margin so downstream
ShapeConfig receives a numeric value.
♻️ Duplicate comments (2)
newton/_src/usd/__init__.py (1)
16-19: Remove the unused# noqa: F401.Ruff reports the directive as unused; keep the side‑effect import but drop the noqa to avoid RUF100.
🧹 Proposed cleanup
- import newton_usd_schemas # noqa: F401 + import newton_usd_schemasnewton/tests/test_import_usd.py (1)
2968-2968: Remove unused# noqa: PLC0415markers.Ruff reports these as unused; removing them keeps lint clean while preserving the local optional imports.
🧹 Proposed cleanup
- from pxr import Usd, UsdGeom, UsdPhysics, UsdShade # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics, UsdShade @@ - from pxr import Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics @@ - from pxr import Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics @@ - from pxr import Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics @@ - from pxr import Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Usd, UsdGeom, UsdPhysics @@ - from pxr import Gf, Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Gf, Usd, UsdGeom, UsdPhysicsAlso applies to: 3028-3028, 3076-3076, 3116-3116, 3150-3150, 3177-3177
Description
Adds a dependency on
newton-usd-schemas& updates the USD parser to use the schemas.SchemaResolverNewtonis enabled by default, so examples don't have to understand the resolver mechanism to get the basics workingCloses #1321, though I expect more updates needed each time we update the schemas.
There are a few open questions and/or controvertial changes here, so I'll do a self review to get some threads started.
Before your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.