Register and parse solver-specific custom attributes#996
Conversation
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
…s condim attribute Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a CustomAttribute model and ONCE frequency; moves USD schema resolution into a new usd package (usd.utils, schema resolvers); updates USD/MJCF/URDF importers to parse and propagate custom attributes; introduces solver hook for MuJoCo condim; updates public exports, tests, examples, and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Builder as ModelBuilder
participant Importer as Importer (USD/MJCF/URDF)
participant USDUtils as usd.utils / SchemaResolverManager
participant Solver as SolverMuJoCo
participant Model as Model
App->>Builder: create builder
Solver->>Builder: register_custom_attributes(builder)
Note right of Builder `#E8F4FF`: Builder stores CustomAttribute entries (e.g., mjc:condim)
Importer->>USDUtils: collect_prim_attrs(stage)
USDUtils-->>Importer: schema_attrs + custom attribute declarations
Importer->>Builder: add_shape/add_body/add_joint(..., custom_attributes=...)
Builder->>Builder: add_custom_attribute(CustomAttribute)
Builder->>Model: finalize() -> Model contains aggregated custom attribute arrays (incl. ONCE)
App->>Solver: instantiate SolverMuJoCo(model)
Solver->>Model: read model.mujoco.condim -> inject into solver geom params
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (18)
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 |
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
newton/_src/utils/import_usd.py (1)
1141-1178: parent_prim may be undefined when collect_solver_specific_attrs=False.parent_prim is created inside the guarded block, but later referenced unconditionally (Line 1166). This will raise NameError when collection is disabled.
Apply:
- # Collect engine-specific attributes for the articulation root on first encounter - if collect_solver_specific_attrs: - R.collect_prim_solver_attrs(articulation_prim) - # Also collect on the parent prim (e.g. Xform with PhysxArticulationAPI) - try: - parent_prim = articulation_prim.GetParent() - except Exception: - parent_prim = None - if parent_prim is not None and parent_prim.IsValid(): - R.collect_prim_solver_attrs(parent_prim) + # Resolve parent prim once; used regardless of collection flag + try: + parent_prim = articulation_prim.GetParent() + except Exception: + parent_prim = None + # Collect engine-specific attributes for the articulation root on first encounter + if collect_solver_specific_attrs: + R.collect_prim_solver_attrs(articulation_prim) + if parent_prim is not None and parent_prim.IsValid(): + R.collect_prim_solver_attrs(parent_prim)newton/_src/sim/builder.py (2)
1419-1434: Critical: ONCE frequency attributes are skipped in add_builder due to control flow bug.Line 1421 should be
elif, notif. Currently, whenattr.frequency == ModelAttributeFrequency.ONCE, the code:
- Sets
offset = 0(line 1420)- Evaluates the second
if attr.frequency == ModelAttributeFrequency.BODY:(line 1421) → false- Evaluates all the
elifconditions → all false- Hits
else: continue(line 1434) and skips the remapping logic at lines 1436-1440This means ONCE frequency attributes are never merged when using
add_builder().Apply this diff:
if attr.frequency == ModelAttributeFrequency.ONCE: offset = 0 -if attr.frequency == ModelAttributeFrequency.BODY: +elif attr.frequency == ModelAttributeFrequency.BODY: offset = start_body_idx
4822-4837: Critical: ONCE frequency attributes are skipped in finalize due to control flow bug.Same issue as lines 1419-1434. Line 4824 should be
elif, notif. Whenfrequency == ModelAttributeFrequency.ONCE, the code setscount = 1then falls through toelse: continue(line 4837), skipping the array building at line 4839. This prevents ONCE frequency attributes from being added to the finalized Model.Apply this diff:
if frequency == ModelAttributeFrequency.ONCE: count = 1 -if frequency == ModelAttributeFrequency.BODY: +elif frequency == ModelAttributeFrequency.BODY: count = m.body_count
🧹 Nitpick comments (6)
newton/tests/test_mujoco_solver.py (4)
1663-1664: Prefer unittest assertions over bareassert.Use TestCase assertions for clearer failure messages.
- assert np.allclose(model.mjc.condim.numpy(), [6, 4, 3]) - assert np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]) + self.assertTrue(np.allclose(model.mjc.condim.numpy(), [6, 4, 3])) + self.assertTrue(np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]))
1691-1692: Prefer unittest assertions over bareassert.- assert np.allclose(model.mjc.condim.numpy(), [6, 4, 3]) - assert np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]) + self.assertTrue(np.allclose(model.mjc.condim.numpy(), [6, 4, 3])) + self.assertTrue(np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]))
1720-1721: Prefer unittest assertions over bareassert.- assert np.allclose(model.mjc.condim.numpy(), [6]) - assert np.allclose(solver.mjw_model.geom_condim.numpy(), [6]) + self.assertTrue(np.allclose(model.mjc.condim.numpy(), [6])) + self.assertTrue(np.allclose(solver.mjw_model.geom_condim.numpy(), [6]))
1645-1722: No duplicates found; optional refactoring to reduce setup repetition remains valid.Verification confirms
TestMuJoCoAttributesclass exists only once. The three test methods (from_code,from_mjcf,from_usd) each repeat the same registration and assertion calls—this works correctly but could benefit from a class-levelsetUpmethod or shared helper to reduce duplication:def setUp(self): self.builder = newton.ModelBuilder() newton.solvers.SolverMuJoCo.register_custom_attributes(self.builder)This refactoring is optional and improves maintainability without affecting functionality.
newton/_src/solvers/mujoco/solver_mujoco.py (2)
1997-2009: Graceful retrieval of custom attributes.The helper handles absent namespaces cleanly. Consider documenting expected shapes (per-shape arrays) in a short comment.
2186-2187: Validate condim range before applying to geoms.MuJoCo condim must be 1, 3, 4, or 6. Guarding prevents invalid inputs from USD/MJCF or code and aids debugging.
- if shape_condim is not None: - geom_params["condim"] = shape_condim[shape] + if shape_condim is not None: + cond = int(shape_condim[shape]) + # Valid MuJoCo condim values per docs: 1, 3, 4, or 6 + if cond in (1, 3, 4, 6): + geom_params["condim"] = cond + else: + warnings.warn(f"Ignoring invalid condim={cond} for shape {shape}; using default {geom_condim}.") + geom_params["condim"] = geom_condim
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
newton/_src/sim/__init__.py(2 hunks)newton/_src/sim/builder.py(2 hunks)newton/_src/sim/model.py(2 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(4 hunks)newton/_src/solvers/solver.py(2 hunks)newton/_src/utils/import_mjcf.py(1 hunks)newton/_src/utils/import_usd.py(7 hunks)newton/examples/robot/example_robot_g1.py(1 hunks)newton/tests/test_mujoco_solver.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
newton/_src/sim/__init__.py (1)
newton/_src/sim/model.py (3)
Model(154-854)ModelAttributeAssignment(33-48)ModelAttributeFrequency(51-72)
newton/_src/utils/import_usd.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(51-72)
newton/_src/solvers/mujoco/solver_mujoco.py (4)
newton/_src/sim/model.py (2)
ModelAttributeAssignment(33-48)ModelAttributeFrequency(51-72)newton/_src/sim/builder.py (2)
ModelBuilder(70-4896)add_custom_attribute(492-539)newton/_src/core/types.py (1)
override(35-36)newton/_src/solvers/solver.py (1)
register_custom_attributes(307-314)
newton/_src/sim/builder.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(51-72)
newton/tests/test_mujoco_solver.py (3)
newton/_src/sim/builder.py (2)
ModelBuilder(70-4896)add_body(1455-1527)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)newton/_src/utils/import_usd.py (1)
add_body(526-545)
newton/_src/solvers/solver.py (2)
newton/_src/sim/builder.py (1)
ModelBuilder(70-4896)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)
newton/examples/robot/example_robot_g1.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/_src/sim/builder.py
[error] 569-569: Custom attribute 'mjc:condim' is not defined. Please declare it first using add_custom_attribute().
🪛 GitHub Actions: Pull Request
newton/_src/sim/builder.py
[error] 569-569: Custom attribute 'mjc:condim' is not defined. Please declare it first using add_custom_attribute().
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (14)
newton/_src/utils/import_usd.py (4)
530-535: LGTM: body-level “newton:*” fallback is harmless and complements resolver.If you want to prevent overriding resolver results, guard with
if attr_name not in body_custom_attrs: ....
631-635: LGTM: joint-level “newton:*” fallback mirrors body handling.
1161-1165: LGTM: articulation-level fallbacks mirror joint/body. Ensure fix above for parent_prim scoping is applied.After applying the scoping fix, please check this path with collect_solver_specific_attrs=False.
Also applies to: 1174-1178
1417-1421: LGTM: shape-level “newton:*” fallback. Aligns with other levels.newton/examples/robot/example_robot_g1.py (1)
46-46: Good call: register solver attributes before loading assets.This ensures shape-level attrs like condim are recognized during import.
Confirm
SolverMuJoCo.register_custom_attributesis idempotent across multiple builders/usages.newton/_src/utils/import_mjcf.py (1)
308-312: Fix condim key and type to match registered custom attribute.Use un-namespaced key "condim" and cast to int; current code forwards a string under "mjc:condim", which likely won’t bind to builder-registered attr (dtype=int, namespace handled by USD only).
Apply:
shape_kwargs = { "key": geom_name, "body": link, "cfg": shape_cfg, - "custom_attributes": {}, + "custom_attributes": {}, } - condim = geom_attrib.get("condim") - if condim is not None: - shape_kwargs["custom_attributes"]["mjc:condim"] = condim + condim = geom_attrib.get("condim") + if condim is not None: + try: + shape_kwargs["custom_attributes"]["condim"] = int(condim) + except (TypeError, ValueError): + if verbose: + print(f"Warning: invalid condim '{condim}' on geom '{geom_name}', expected int")To confirm consistency, scan for namespaced vs un-namespaced usage across the repo:
newton/_src/solvers/solver.py (3)
19-19: LGTM!Import addition is correct and necessary for the new
register_custom_attributesclassmethod.
306-314: LGTM!The new
register_custom_attributesclassmethod is well-designed as a hook for solver implementations to register their custom attributes. The docstring is clear and thepassbody is appropriate for a base class hook.
296-304: Fix malformed docstring.The docstring closes immediately on line 297, making lines 298-303 dead code (a string literal inside the method body rather than a docstring). The actual docstring is empty.
Apply this diff:
def update_contacts(self, contacts: Contacts) -> None: - """ - Update a Contacts object with forces from the solver state. Where the solver state contains + """Update a Contacts object with forces from the solver state. Where the solver state contains other contact data, convert that data to the Contacts format. - + Args: contacts (Contacts): The object to update from the solver state. """ raise NotImplementedError() -Likely an incorrect or invalid review comment.
newton/_src/sim/__init__.py (1)
30-30: LGTM!These changes correctly expose
ModelAttributeAssignmentandModelAttributeFrequencyin the public API, enabling users to specify frequency and assignment when registering custom attributes.Also applies to: 44-45
newton/_src/sim/model.py (2)
83-96: Namespace container change looks good.Constructor rename and repr are consistent with usage in Model.add_attribute/_add_custom_attributes.
59-72: The comprehensive search found no evidence of the concern raised. All 100+ uses ofModelAttributeFrequencyin the codebase use enum member access (e.g.,ModelAttributeFrequency.BODY), not numeric.valueaccess. Searches for.valuepatterns and numeric literal comparisons returned zero results.The
CustomAttributeclass stores frequency as an enum type, and all comparisons throughout the codebase (builder.py, import_usd.py, selection.py, schema_resolver.py) use enum member comparisons likeattr.frequency == ModelAttributeFrequency.BODY. Enum member identity is independent of numeric value changes—renumbering only breaks code that stores or compares numeric values directly, which does not occur here.The review comment's premise about
.valueusage and breaking changes is not substantiated by the codebase.Likely an incorrect or invalid review comment.
newton/_src/solvers/mujoco/solver_mujoco.py (2)
34-37: Public API imports are appropriate.Bringing ModelAttributeAssignment, ModelAttributeFrequency, and ModelBuilder into scope aligns with the new registration API.
1226-1236: Solid: explicit solver-level attribute registration.Registering mjc:condim as SHAPE/MODEL with default 3 is clear and matches tests.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/sim/builder.py (2)
1419-1434: Critical: Broken if-elif chain prevents ONCE attributes from being merged.Line 1421 uses
ifinstead ofelif, breaking the control flow. Whenattr.frequencyisONCE:
- Line 1420 sets
offset = 0- Line 1421's
if attr.frequency == ModelAttributeFrequency.BODYevaluates toFalse- All subsequent
elifchecks fail (frequency is stillONCE)- Line 1434's
else: continueexecutes, skipping lines 1437-1440 whereoffsetis used to remap valuesThis prevents ONCE-frequency custom attributes from being merged when adding builders, likely causing the pipeline failure "Custom attribute 'mjc:condim' is not defined."
Apply this diff:
if attr.frequency == ModelAttributeFrequency.ONCE: offset = 0 -if attr.frequency == ModelAttributeFrequency.BODY: +elif attr.frequency == ModelAttributeFrequency.BODY: offset = start_body_idx elif attr.frequency == ModelAttributeFrequency.SHAPE:
4822-4837: Critical: Broken if-elif chain prevents ONCE attributes from being finalized.Line 4824 uses
ifinstead ofelif, creating the same control-flow bug as lines 1419-1434. WhenfrequencyisONCE:
- Line 4823 sets
count = 1- Line 4824's
if frequency == ModelAttributeFrequency.BODYevaluates toFalse- All subsequent
elifchecks fail- Line 4837's
else: continueexecutes, skipping line 4839 where the array is builtThis prevents ONCE-frequency custom attributes from being allocated in the finalized Model, breaking solver-specific custom attributes like MuJoCo's condim.
Apply this diff:
if frequency == ModelAttributeFrequency.ONCE: count = 1 -if frequency == ModelAttributeFrequency.BODY: +elif frequency == ModelAttributeFrequency.BODY: count = m.body_count elif frequency == ModelAttributeFrequency.SHAPE:
🧹 Nitpick comments (5)
newton/tests/test_mujoco_solver.py (2)
1645-1665: Prefer unittest assertions over bareassertfor clearer failures.Use
self.assertTrue(np.allclose(...), msg)to get better diffs and consistent reporting with the rest of the file.- assert np.allclose(model.mjc.condim.numpy(), [6, 4, 3]) - assert np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]) + self.assertTrue(np.allclose(model.mjc.condim.numpy(), [6, 4, 3]), + "Model condim should match per-shape values [6,4,3]") + self.assertTrue(np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]), + "MuJoCo geom condim should match per-shape values [6,4,3]")
1666-1692: Same assertion style recommendation for MJCF path.Switch to unittest assertions for consistency and clearer failures.
- assert np.allclose(model.mjc.condim.numpy(), [6, 4, 3]) - assert np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]) + self.assertTrue(np.allclose(model.mjc.condim.numpy(), [6, 4, 3]), + "Model condim (MJCF) should match [6,4,3]") + self.assertTrue(np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]), + "MuJoCo geom condim (MJCF) should match [6,4,3]")newton/_src/utils/import_usd.py (2)
631-635: JOINT custom attrs: also include JOINT_DOF/JOINT_COORD from resolver.You seed
joint_custom_attrsonly fromModelAttributeFrequency.JOINT(Line 629). Sincebuilder_custom_attr_jointincludes JOINT_DOF and JOINT_COORD, read those from the resolver too to keep behavior consistent (not only via the newton:* fallback).- joint_custom_attrs = R.get_custom_attributes_for_prim(joint_prim, ModelAttributeFrequency.JOINT) + joint_custom_attrs = {} + for freq in (ModelAttributeFrequency.JOINT, + ModelAttributeFrequency.JOINT_DOF, + ModelAttributeFrequency.JOINT_COORD): + joint_custom_attrs.update(R.get_custom_attributes_for_prim(joint_prim, freq))
530-534: Minor: factor repeated “newton:” extraction loops.BODY/JOINT/ARTICULATION/SHAPE repeat the same pattern. A small helper avoids drift and reduces boilerplate.
def merge_newton_prefixed_attrs(prim, names: list[str], dst: dict): for attr_name in names: usd_attr = f"newton:{attr_name}" if prim.HasAttribute(usd_attr): dst[attr_name] = prim.GetAttribute(usd_attr).Get()Then call it at the four sites.
Also applies to: 1162-1165, 1174-1177, 1417-1421
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1997-2009: Condim propagation looks correct; add a small guard for shape bounds.Accessing
shape_condim[shape]assumes 1:1 withmodel.shape_count. If a future selection mask is applied, an explicit bounds assert helps debugging.- if shape_condim is not None: - geom_params["condim"] = shape_condim[shape] + if shape_condim is not None: + # defensive check; shape ids must be in range + assert 0 <= shape < shape_condim.shape[0], "condim length must match shape_count" + geom_params["condim"] = int(shape_condim[shape])Also applies to: 2186-2188
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
newton/_src/sim/__init__.py(2 hunks)newton/_src/sim/builder.py(2 hunks)newton/_src/sim/model.py(2 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(4 hunks)newton/_src/solvers/solver.py(2 hunks)newton/_src/utils/import_mjcf.py(1 hunks)newton/_src/utils/import_usd.py(7 hunks)newton/examples/robot/example_robot_g1.py(1 hunks)newton/tests/test_mujoco_solver.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
newton/_src/solvers/solver.py (3)
newton/_src/sim/model.py (1)
Model(154-854)newton/_src/sim/builder.py (1)
ModelBuilder(70-4896)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)
newton/tests/test_mujoco_solver.py (3)
newton/_src/sim/builder.py (5)
ModelBuilder(70-4896)add_body(1455-1527)add_joint_revolute(1663-1750)add_shape_box(2901-2935)add_usd(900-1023)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)newton/_src/utils/import_usd.py (1)
add_body(526-545)
newton/_src/sim/__init__.py (1)
newton/_src/sim/model.py (3)
Model(154-854)ModelAttributeAssignment(33-48)ModelAttributeFrequency(51-72)
newton/examples/robot/example_robot_g1.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)
newton/_src/sim/builder.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(51-72)
newton/_src/utils/import_usd.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(51-72)
newton/_src/solvers/mujoco/solver_mujoco.py (4)
newton/_src/sim/model.py (2)
ModelAttributeAssignment(33-48)ModelAttributeFrequency(51-72)newton/_src/sim/builder.py (2)
ModelBuilder(70-4896)add_custom_attribute(492-539)newton/_src/core/types.py (1)
override(35-36)newton/_src/solvers/solver.py (1)
register_custom_attributes(307-314)
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/_src/sim/builder.py
[error] 569-569: Custom attribute 'mjc:condim' is not defined. Please declare it first using add_custom_attribute().
🪛 GitHub Actions: Pull Request
newton/_src/sim/builder.py
[error] 569-569: Custom attribute 'mjc:condim' is not defined. Please declare it first using add_custom_attribute().
[error] 1083-1083: Cannot resolve value for 'joint:transX_position' on prim '/joint_AD'; no authored value, no explicit default, and no solver mapping default.
[error] 569-569: Custom attribute 'mjc:condim' is not defined. Please declare it first using add_custom_attribute().
🔇 Additional comments (9)
newton/examples/robot/example_robot_g1.py (1)
46-46: LGTM! Proper placement of custom attribute registration.The call to
register_custom_attributesis correctly placed after theModelBuilderinstantiation and before model configuration, ensuring that MuJoCo-specific attributes are registered for subsequent parsing and propagation.newton/_src/sim/__init__.py (2)
30-30: LGTM! Public API exposure is correct.The new enums
ModelAttributeAssignmentandModelAttributeFrequencyare properly imported and will be accessible via the public API for solver implementations and user code.
44-45: LGTM! Exports added correctly.The
__all__list is updated to include both new enums, maintaining alphabetical ordering and ensuring proper public API exposure.newton/_src/sim/model.py (2)
59-72: Enum renumbering poses minimal risk to this codebase—all usages reference enum members by name.The verification confirms that throughout newton/ (both tests and implementation), the codebase consistently uses enum member names (e.g.,
ModelAttributeFrequency.BODY) rather than hardcoded integer values. The second search for hardcoded frequency values (0-6) returned zero results in the actual code.Since enum renumbering doesn't break name-based references, the breaking change primarily affects external API consumers or code that serializes enum numeric values. Within the Newton codebase itself, tests and all implementation code continue to function correctly.
No updates to examples, tests, or implementation are necessary due to this renumbering.
83-95: No breaking change identified in codebase.The review comment claims the
AttributeNamespaceconstructor parameter rename breaks code usingnamespace_name=..., but verification shows:
AttributeNamespaceis internal tomodel.py(not exported in__all__)- Zero keyword argument usages of
namespace_nameexist in the codebase- All instantiations (lines 764, 819) use positional arguments:
AttributeNamespace(namespace)- Zero external references to this class outside
model.pyThe parameter rename is a safe internal refactoring with no actual breaking impact.
Likely an incorrect or invalid review comment.
newton/_src/solvers/solver.py (2)
19-19: LGTM!Import addition is necessary for the new
register_custom_attributesclassmethod signature.
306-314: LGTM! Well-designed extension point for solver-specific attributes.The classmethod design allows solvers to register custom attributes before instantiation, and the base implementation with
passcorrectly provides an optional hook that subclasses can override (e.g.,SolverMuJoCo.register_custom_attributesregisters thecondimattribute).newton/tests/test_mujoco_solver.py (2)
25-25: LGTM on USD availability guard.Importing USD_AVAILABLE and gating with skipUnless is appropriate.
1693-1722: USD test looks correct and minimal.Covers the stage authoring path and verifies propagation end-to-end.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
newton/_src/utils/import_usd.py (1)
275-293: Add defensive check for builder.custom_attributes.The code assumes
builder.custom_attributesexists when creating the filtered lists. Consider adding a defensive check to handle cases where the builder might not have this attribute initialized.Apply this diff:
+ if not hasattr(builder, 'custom_attributes'): + builder_custom_attr_model = [] + builder_custom_attr_shape = [] + builder_custom_attr_body = [] + builder_custom_attr_joint = [] + builder_custom_attr_articulation = [] + else: # process custom attributes defined for different kinds of prim builder_custom_attr_model = [ key for key, attr in builder.custom_attributes.items() if attr.frequency == ModelAttributeFrequency.ONCE ] builder_custom_attr_shape = [ key for key, attr in builder.custom_attributes.items() if attr.frequency == ModelAttributeFrequency.SHAPE ] builder_custom_attr_body = [ key for key, attr in builder.custom_attributes.items() if attr.frequency == ModelAttributeFrequency.BODY ] builder_custom_attr_joint = [ key for key, attr in builder.custom_attributes.items() if attr.frequency in [ModelAttributeFrequency.JOINT, ModelAttributeFrequency.JOINT_DOF, ModelAttributeFrequency.JOINT_COORD] ] builder_custom_attr_articulation = [ key for key, attr in builder.custom_attributes.items() if attr.frequency == ModelAttributeFrequency.ARTICULATION ]newton/_src/solvers/mujoco/solver_mujoco.py (1)
1997-2009: Validate and normalize custom attribute before use.Guard against shape-count mismatches and non-int/invalid values; keeps spec generation robust.
Apply this diff:
# retrieve MuJoCo-specific attributes mujoco_attrs = getattr(model, "mjc", None) def get_custom_attribute(name: str) -> nparray | None: if mujoco_attrs is None: return None attr = getattr(mujoco_attrs, name, None) if attr is None: return None - return attr.numpy() + return attr.numpy() shape_condim = get_custom_attribute("condim") +if shape_condim is not None: + # enforce int32 dtype and validate basic constraints + shape_condim = shape_condim.astype(np.int32, copy=False) + if shape_condim.shape[0] != model.shape_count: + raise ValueError( + f"mjc:condim length mismatch (got {shape_condim.shape[0]}, expected {model.shape_count})" + ) + # MuJoCo accepts 1..6; warn if outside range + if np.any((shape_condim < 1) | (shape_condim > 6)): + warnings.warn("mjc:condim contains values outside [1, 6]; MuJoCo may clamp/ignore.", stacklevel=2)newton/tests/test_mujoco_solver.py (1)
1645-1723: Tests read well; prefer exact equality for ints.For integer arrays, use np.array_equal over np.allclose for stricter checks.
Apply this diff:
- assert np.allclose(model.mjc.condim.numpy(), [6, 4, 3]) - assert np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]) + assert np.array_equal(model.mjc.condim.numpy(), np.array([6, 4, 3], dtype=np.int32)) + assert np.array_equal(solver.mjw_model.geom_condim.numpy(), np.array([6, 4, 3], dtype=np.int32)) @@ - assert np.allclose(model.mjc.condim.numpy(), [6, 4, 3]) - assert np.allclose(solver.mjw_model.geom_condim.numpy(), [6, 4, 3]) + assert np.array_equal(model.mjc.condim.numpy(), np.array([6, 4, 3], dtype=np.int32)) + assert np.array_equal(solver.mjw_model.geom_condim.numpy(), np.array([6, 4, 3], dtype=np.int32)) @@ - assert np.allclose(model.mjc.condim.numpy(), [6]) - assert np.allclose(solver.mjw_model.geom_condim.numpy(), [6]) + assert np.array_equal(model.mjc.condim.numpy(), np.array([6], dtype=np.int32)) + assert np.array_equal(solver.mjw_model.geom_condim.numpy(), np.array([6], dtype=np.int32))
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
newton/_src/sim/__init__.py(2 hunks)newton/_src/sim/builder.py(2 hunks)newton/_src/sim/model.py(2 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(4 hunks)newton/_src/solvers/solver.py(2 hunks)newton/_src/utils/import_mjcf.py(1 hunks)newton/_src/utils/import_usd.py(7 hunks)newton/examples/robot/example_robot_g1.py(1 hunks)newton/tests/test_mujoco_solver.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
newton/_src/sim/builder.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(51-72)
newton/examples/robot/example_robot_g1.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)
newton/_src/utils/import_usd.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(51-72)
newton/_src/sim/__init__.py (1)
newton/_src/sim/model.py (3)
Model(154-854)ModelAttributeAssignment(33-48)ModelAttributeFrequency(51-72)
newton/tests/test_mujoco_solver.py (3)
newton/_src/sim/builder.py (5)
ModelBuilder(70-4896)add_body(1455-1527)add_shape_box(2901-2935)add_mjcf(1025-1109)add_usd(900-1023)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)newton/_src/utils/import_usd.py (1)
add_body(526-545)
newton/_src/solvers/mujoco/solver_mujoco.py (4)
newton/_src/sim/model.py (2)
ModelAttributeAssignment(33-48)ModelAttributeFrequency(51-72)newton/_src/sim/builder.py (2)
ModelBuilder(70-4896)add_custom_attribute(492-539)newton/_src/core/types.py (1)
override(35-36)newton/_src/solvers/solver.py (1)
register_custom_attributes(307-314)
newton/_src/solvers/solver.py (3)
newton/_src/sim/model.py (1)
Model(154-854)newton/_src/sim/builder.py (1)
ModelBuilder(70-4896)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1228-1236)
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/_src/sim/builder.py
[error] 569-569: Custom attribute processing failed during add_shape: Custom attribute 'mjc:condim' is not defined. Please declare it first using add_custom_attribute().
🪛 GitHub Actions: Pull Request
newton/_src/sim/builder.py
[error] 569-569: Custom attribute 'mjc:condim' is not defined. Please declare it first using add_custom_attribute().
🪛 Ruff (0.14.1)
newton/tests/test_mujoco_solver.py
1695-1695: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (11)
newton/_src/utils/import_usd.py (1)
275-293: LGTM! Clear custom attribute categorization.The logic correctly filters custom attributes by frequency type for each prim category. The inclusion of JOINT, JOINT_DOF, and JOINT_COORD frequencies for joint attributes is appropriate.
newton/examples/robot/example_robot_g1.py (1)
46-46: LGTM! Correct placement of custom attribute registration.The call to
register_custom_attributesis correctly placed immediately after creating the ModelBuilder and before loading the USD asset, ensuring custom attributes are available during the import process.newton/_src/sim/__init__.py (1)
30-30: LGTM! Clean public API expansion.The addition of
ModelAttributeAssignmentandModelAttributeFrequencyto the public API enables external code to register custom attributes with proper type safety.Also applies to: 44-45
newton/_src/utils/import_mjcf.py (1)
308-312: LGTM! Proper propagation of MuJoCo condim attribute.The code correctly:
- Initializes the
custom_attributesdictionary for all shapes- Uses the
mjc:condimnamespace convention consistently with the MuJoCo solver- Only populates the attribute when it exists in the MJCF
newton/_src/solvers/solver.py (3)
19-19: LGTM! Necessary import for the new API.The
ModelBuilderimport is required for the type hint in the newregister_custom_attributesclassmethod.
306-314: LGTM! Clean extension point for solver-specific attributes.The
register_custom_attributesclassmethod provides a clean hook for solver implementations to register their custom attributes. The base implementation withpassis appropriate, and the docstring is clear.
297-304: Fix docstring formatting.The docstring appears to have been split incorrectly, creating a stray string literal on line 297-298 that is separate from the function body. The docstring should be a single continuous triple-quoted string.
Apply this diff:
def update_contacts(self, contacts: Contacts) -> None: - """ - Update a Contacts object with forces from the solver state. Where the solver state contains + """Update a Contacts object with forces from the solver state. Where the solver state contains other contact data, convert that data to the Contacts format. - + Args: contacts (Contacts): The object to update from the solver state. """ raise NotImplementedError() -Likely an incorrect or invalid review comment.
newton/_src/solvers/mujoco/solver_mujoco.py (2)
34-37: Imports look correct for the new API surface.
2186-2188: condim propagation LGTM; consider bounds check.Setting per-geom condim is correct. Ensure values are within 1..6 (as above) to prevent unexpected solver behavior.
newton/_src/sim/model.py (2)
59-72: Enum value shifts can be breaking; confirm all consumers and data IO.Adding ONCE=0 shifts other values. Verify:
- No persisted artifacts or interop rely on prior numeric values.
- Importers/exporters/tests updated accordingly.
If any external formats store raw ints, consider temporary aliases or name-based serialization to avoid breakage.
83-96: Namespace API rename LGTM.Constructor/repr changes are consistent with Model.add_attribute/_add_custom_attributes usage.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
I think we need to define the mechanism that we use to create custom attributes for solver-specific attributes with already defined schema like mujoco; specifically, there is already The mechanism of using a physics scene prim as a place to read the declaration is likely more suitable for solvers without a schema. Note that our convention of prepending We should start testing some of @andrewkaufman assets for testing this |
|
We ideally should decouple solver-specific custom attribute creation from import_usd, IMO. |
adenzler-nvidia
left a comment
There was a problem hiding this comment.
a few questions, general direction looks good! Thanks!
|
Another question that just came up, especially with the new attribute frequency ONCE: Where should we resolve these if some of these values are constructor arguments to the solver? For example, take mjc:option:solver which is part of the USD schema, but currently this is a constructor argument for MuJoCo solver. Are we going to override the constructor argument in favor of the USD value, what's taking precendence, or are we dropping the constructor arguments? Here's my opinion:
Opinions? I kind of like the idea of explicit constructor args for the solver options compared to encoding them in the model. |
|
ok quick summary on the first part of the discussion about solver constructor args:
|
In SolverMuJoCo we can simply skip those attributes when declaring objects in MjSpec (to use MuJoCo's internal defaults), similar to how I only set the condim attribute if it exists as a custom attribute: shape_condim = get_custom_attribute("condim")
if shape_condim is not None:
geom_params["condim"] = shape_condim[shape]
body.add_geom(**geom_params)Other solvers can also just raise an exception if the user didn't register the custom attributes for this solver. |
…e Argument Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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_mjcf.py (1)
21-104: Switch to a hardened XML parser.
xml.etree.ElementTreeis still documented as unsafe for untrusted input because it remains vulnerable to entity-expansion DoS and related XML attacks.(docs.python.org) We should import the defusedxml shim (with a narrow fallback if the dependency is unavailable) before parsing incoming MJCF.-import xml.etree.ElementTree as ET +try: + from defusedxml import ElementTree as ET # hardened parser +except ImportError: # pragma: no cover + import xml.etree.ElementTree as ET @@ - file = ET.parse(source) + file = ET.parse(source) @@ - root = ET.fromstring(xml_content) + root = ET.fromstring(xml_content)
♻️ Duplicate comments (5)
newton/_src/usd/utils.py (1)
266-276: Fix translation extraction inget_transform.
mat[3, :3]reads the last row of the 4×4 matrix, which is always[0, 0, 0]for homogeneous transforms, so every imported prim ends up at the origin. Use the last column for translation so positions survive USD import.- pos = mat[3, :3] + pos = mat[:3, 3]newton/_src/sim/builder.py (2)
4942-4962: ONCE frequency handling in finalize looks correct.The mutually exclusive
elifchain ensures ONCE getscount = 1and does not fall through; this addresses the earlier issue.
1494-1542: Blocker: First-merge of custom attributes loses per-entity indices and aliases sub-builder state.When
merged is None, you stash the sub‑builder’sCustomAttributeinstance directly. Two problems:
- Indices in
attr.valuesare local to the sub‑builder (0…n-1) and must be offset into the receiver’s global indices; otherwise values land on the wrong entities (or are dropped during array build).- You keep a reference to the sub‑builder’s object, so later mutations/GC can leak across builders.
Fix by remapping indices and storing a copy. Minimal patch:
@@ - if merged is None: - self.custom_attributes[full_key] = attr - continue + # Determine the offset based on frequency (needed for both branches) + if attr.frequency == ModelAttributeFrequency.ONCE: + offset = 0 + elif attr.frequency == ModelAttributeFrequency.BODY: + offset = start_body_idx + elif attr.frequency == ModelAttributeFrequency.SHAPE: + offset = start_shape_idx + elif attr.frequency == ModelAttributeFrequency.JOINT: + offset = start_joint_idx + elif attr.frequency == ModelAttributeFrequency.JOINT_DOF: + offset = start_joint_dof_idx + elif attr.frequency == ModelAttributeFrequency.JOINT_COORD: + offset = start_joint_coord_idx + elif attr.frequency == ModelAttributeFrequency.ARTICULATION: + offset = start_articulation_idx + else: + continue + + if merged is None: + # Copy spec and remap values to this builder's global indices + from dataclasses import replace as _dc_replace + remapped = {offset + idx: val for idx, val in (attr.values or {}).items()} + self.custom_attributes[full_key] = _dc_replace(attr, values=remapped) + continue @@ - # Determine the offset based on frequency - if attr.frequency == ModelAttributeFrequency.ONCE: - offset = 0 - elif attr.frequency == ModelAttributeFrequency.BODY: - offset = start_body_idx - elif attr.frequency == ModelAttributeFrequency.SHAPE: - offset = start_shape_idx - elif attr.frequency == ModelAttributeFrequency.JOINT: - offset = start_joint_idx - elif attr.frequency == ModelAttributeFrequency.JOINT_DOF: - offset = start_joint_dof_idx - elif attr.frequency == ModelAttributeFrequency.JOINT_COORD: - offset = start_joint_coord_idx - elif attr.frequency == ModelAttributeFrequency.ARTICULATION: - offset = start_articulation_idx - else: - continue + # 'merged' exists: offset incoming values into receiver's global indicesThis preserves ONCE, BODY, SHAPE, JOINT*, and ARTICULATION semantics and prevents cross‑builder aliasing.
newton/usd.py (1)
38-42: Duplicate symbol in all.
"convert_warp_type"appears twice. Remove the duplicate.__all__ = [ - "convert_warp_type", - "convert_warp_type", + "convert_warp_type", "convert_warp_value",newton/_src/utils/import_utils.py (1)
68-75: Fix boolean vector parsing.We’re still calling
bool(token)for each element, so any non-empty string (even"0"or"false") becomesTrue. Python’s truthiness rules make all non-empty strings truthy.(geeksforgeeks.org) Please reuseget_boolfor each token so vector and matrix conversions honor the intended semantics.- if scalar_type is wp.bool or scalar_type is bool: - return warp_dtype(*get_vector(bool)) + if scalar_type is wp.bool or scalar_type is bool: + return warp_dtype(*[get_bool(tok) for tok in value.split()])
🧹 Nitpick comments (3)
newton/_src/usd/utils.py (1)
183-186: Rename the ambiguous temporary inget_quat.Rename
ltolengthto avoid Ruff E741 and improve clarity.- l = wp.length(quat) - if np.isfinite(l) and l > 0.0: + length = wp.length(quat) + if np.isfinite(length) and length > 0.0:newton/_src/sim/builder.py (2)
637-651: Type hint clarity for Sequence.
Sequenceis imported from..core.types; for readability and to avoid confusion withtyping.Sequence, prefercollections.abc.Sequence(already imported pattern is used forCallable,Iterable). Not urgent.- def get_custom_attributes_by_frequency( - self, frequencies: Sequence[ModelAttributeFrequency] + from collections.abc import Sequence as _Sequence # near imports + def get_custom_attributes_by_frequency( + self, frequencies: _Sequence[ModelAttributeFrequency] ) -> list[CustomAttribute]:
319-328: Style: long exception messages (Ruff TRY003).Consider shortening messages or creating dedicated exception types; current long strings trigger TRY003. Non-blocking.
Also applies to: 1513-1516
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/api/newton_usd.rst(1 hunks)newton/_src/sim/builder.py(13 hunks)newton/_src/sim/collide_unified.py(1 hunks)newton/_src/usd/__init__.py(1 hunks)newton/_src/usd/schemas.py(1 hunks)newton/_src/usd/utils.py(1 hunks)newton/_src/utils/import_mjcf.py(12 hunks)newton/_src/utils/import_usd.py(37 hunks)newton/_src/utils/import_utils.py(1 hunks)newton/tests/test_schema_resolver.py(26 hunks)newton/usd.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- newton/_src/sim/collide_unified.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api/newton_usd.rst
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/utils/import_utils.pynewton/tests/test_schema_resolver.pynewton/_src/usd/utils.pynewton/_src/utils/import_usd.pynewton/usd.pynewton/_src/usd/__init__.py
📚 Learning: 2025-10-27T18:45:18.446Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
Applied to files:
newton/tests/test_schema_resolver.pynewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/usd.pynewton/_src/usd/__init__.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_schema_resolver.pynewton/_src/usd/utils.pynewton/_src/utils/import_usd.pynewton/usd.pynewton/_src/usd/__init__.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_schema_resolver.pynewton/_src/usd/utils.pynewton/_src/utils/import_usd.pynewton/usd.pynewton/_src/usd/__init__.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/_src/sim/builder.pynewton/_src/utils/import_mjcf.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/usd.pynewton/_src/usd/__init__.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-07-21T19:11:04.077Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.077Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/usd/utils.pynewton/_src/utils/import_usd.pynewton/_src/utils/import_mjcf.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/usd/utils.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is properly re-exported from the spatial module via "from .spatial import (..., transform_twist, ...)" and included in __all__, making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/usd.pynewton/_src/usd/__init__.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is re-exported from the spatial module via "from .spatial import transform_twist", making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/usd.py
📚 Learning: 2025-08-14T17:38:36.106Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/__init__.py:25-29
Timestamp: 2025-08-14T17:38:36.106Z
Learning: The Newton project prefers incremental __all__ building using __all__ += [...] pattern to group exports with their related imports, rather than a single consolidated __all__ at the end of the file.
Applied to files:
newton/_src/usd/__init__.py
🧬 Code graph analysis (9)
newton/_src/utils/import_utils.py (1)
newton/_src/sim/builder.py (3)
ModelBuilder(70-5067)CustomAttribute(270-360)key(353-355)
newton/tests/test_schema_resolver.py (4)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-70)newton/_src/sim/builder.py (2)
ModelBuilder(70-5067)add_usd(1012-1131)newton/_src/usd/schema_resolver.py (7)
PrimType(33-49)SchemaResolverManager(168-272)schema_attrs(264-272)collect_prim_attrs(122-145)collect_prim_attrs(246-261)get_value(101-120)get_value(196-244)newton/_src/usd/schemas.py (3)
SchemaResolverNewton(26-102)SchemaResolverPhysx(105-189)SchemaResolverMjc(248-318)
newton/_src/sim/builder.py (2)
newton/_src/usd/schema_resolver.py (1)
SchemaResolver(70-145)newton/_src/sim/model.py (3)
Model(96-796)ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)
newton/_src/usd/schemas.py (1)
newton/_src/usd/schema_resolver.py (3)
PrimType(33-49)SchemaAttribute(53-67)SchemaResolver(70-145)
newton/_src/usd/utils.py (3)
newton/_src/core/types.py (2)
Axis(63-160)from_string(74-94)newton/_src/sim/model.py (2)
ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)newton/_src/sim/builder.py (2)
CustomAttribute(270-360)key(353-355)
newton/_src/utils/import_usd.py (2)
newton/_src/usd/schema_resolver.py (6)
PrimType(33-49)SchemaResolver(70-145)SchemaResolverManager(168-272)collect_prim_attrs(122-145)collect_prim_attrs(246-261)schema_attrs(264-272)newton/_src/usd/utils.py (16)
get_scale(220-235)get_float(90-90)get_float(94-94)get_float(97-115)get_vector(191-191)get_vector(195-195)get_vector(198-217)get_gprim_axis(238-251)get_custom_attribute_values(443-467)from_gfquat(145-155)get_float_with_fallback(118-142)get_custom_attribute_declarations(346-440)get_attribute(38-53)get_quat(159-159)get_quat(163-163)get_quat(166-187)
newton/_src/utils/import_mjcf.py (5)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-70)newton/_src/usd/schemas.py (1)
solref_to_stiffness_damping(192-225)newton/_src/utils/import_utils.py (3)
parse_custom_attributes(79-120)sanitize_xml_content(123-137)transform(110-111)newton/_src/sim/builder.py (5)
ModelBuilder(70-5067)CustomAttribute(270-360)get_custom_attributes_by_frequency(637-651)key(353-355)add_body(1556-1628)newton/_src/utils/import_usd.py (1)
add_body(418-432)
newton/usd.py (1)
newton/_src/usd/utils.py (16)
convert_warp_type(312-343)convert_warp_value(279-309)get_attribute(38-53)get_attributes_in_namespace(56-71)get_custom_attribute_declarations(346-440)get_custom_attribute_values(443-467)get_float(90-90)get_float(94-94)get_float(97-115)get_gprim_axis(238-251)get_quat(159-159)get_quat(163-163)get_quat(166-187)get_scale(220-235)get_transform(254-276)has_attribute(74-86)
newton/_src/usd/__init__.py (1)
newton/_src/usd/utils.py (16)
convert_warp_type(312-343)convert_warp_value(279-309)get_attribute(38-53)get_attributes_in_namespace(56-71)get_custom_attribute_declarations(346-440)get_custom_attribute_values(443-467)get_float(90-90)get_float(94-94)get_float(97-115)get_gprim_axis(238-251)get_quat(159-159)get_quat(163-163)get_quat(166-187)get_scale(220-235)get_transform(254-276)has_attribute(74-86)
🪛 Ruff (0.14.3)
newton/_src/utils/import_utils.py
57-57: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/sim/builder.py
325-327: Avoid specifying long messages outside the exception class
(TRY003)
628-628: Avoid specifying long messages outside the exception class
(TRY003)
1513-1516: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/usd/utils.py
184-184: Ambiguous variable name: l
(E741)
367-367: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
newton/_src/utils/import_usd.py
1029-1029: Do not catch blind exception: Exception
(BLE001)
newton/_src/utils/import_mjcf.py
100-100: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (1)
newton/_src/usd/__init__.py (1)
31-44: LGTM: clean re-exports.Public USD utils surface is consistent and free of duplicates.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
newton/_src/utils/import_usd.py (2)
321-335: Plane axis rotation computed but not applied (use rotated xform).You rotate xform for plane axis but still pass plane_xform (unrotated) to add_shape_plane, so the rotation is ignored. Apply the rotated transform.
- elif type_name == "plane": - axis = usd.get_gprim_axis(prim) - plane_xform = xform - # Apply axis rotation to transform - xform = wp.transform(xform.p, xform.q * quat_between_axes(Axis.Z, axis)) + elif type_name == "plane": + axis = usd.get_gprim_axis(prim) + # Apply axis rotation to transform + xform = wp.transform(xform.p, xform.q * quat_between_axes(Axis.Z, axis)) width = usd.get_float(prim, "width", 0.0) * scale[0] length = usd.get_float(prim, "length", 0.0) * scale[1] shape_id = builder.add_shape_plane( - body=parent_body_id, - xform=plane_xform, + body=parent_body_id, + xform=xform, width=width, length=length, cfg=visual_shape_cfg, key=path_name, )
1484-1490: Inverse inertia construction likely incorrect (avoid splatting arrays into mat33).wp.mat33(*inertia) unpacks rows and may not match expected constructor; earlier you use wp.mat33(inertia). Use the same for the inverse.
- if inertia.any(): - builder.body_inv_inertia[body_id] = wp.inverse(wp.mat33(*inertia)) + if inertia.any(): + builder.body_inv_inertia[body_id] = wp.inverse(wp.mat33(inertia)) else: builder.body_inv_inertia[body_id] = wp.mat33(*np.zeros((3, 3), dtype=np.float32))newton/_src/utils/import_mjcf.py (1)
610-616: Wrong child body for base_joint dict (uses XML root).base_joint["child"] must be the body index link, not the XML root element. This currently mis-connects the joint.
- elif isinstance(base_joint, dict): + elif isinstance(base_joint, dict): base_joint["parent"] = -1 - base_joint["child"] = root + base_joint["child"] = link base_joint["parent_xform"] = base_parent_xform base_joint["child_xform"] = base_child_xform base_joint["key"] = "base_joint" builder.add_joint(**base_joint)newton/_src/sim/builder.py (1)
1649-1661: Fix reST reference in argument doc.Use backticks in
:ref:links.- joint_type (JointType): The type of joint to add (see :ref:'joint-types'). + joint_type (JointType): The type of joint to add (see :ref:`joint-types`).
♻️ Duplicate comments (4)
newton/_src/utils/import_mjcf.py (1)
21-23: Harden XML parsing (use defusedxml).Switch to defusedxml.ElementTree for untrusted input; keep stdlib as fallback. This was already raised and tracked.
-import xml.etree.ElementTree as ET +try: + from defusedxml import ElementTree as ET +except Exception: + import xml.etree.ElementTree as ET # fallbacknewton/usd.py (1)
38-52: Duplicate entry in all.convert_warp_type is listed twice. Remove the duplicate.
__all__ = [ - "convert_warp_type", - "convert_warp_type", + "convert_warp_type", "convert_warp_value", "get_attribute", "get_attributes_in_namespace", "get_custom_attribute_declarations", "get_custom_attribute_values", "get_float", "get_gprim_axis", "get_quat", "get_scale", "get_transform", "has_attribute", ]newton/_src/usd/utils.py (1)
275-275: CRITICAL: Fix translation extraction in get_transform (duplicate).This critical issue was already flagged in previous review: Line 275 extracts the translation from the wrong part of the matrix. The translation is in the last column, not the last row:
- pos = mat[3, :3] + pos = mat[:3, 3]newton/_src/utils/import_utils.py (1)
73-74: Boolean vector parsing is still incorrect.Line 74 uses
get_vector(bool)which callsbool(x)on each token string, treating any non-empty string (including "0" or "false") as True. Apply the same fix used for scalars:if scalar_type is wp.bool or scalar_type is bool: - return warp_dtype(*get_vector(bool)) + return warp_dtype(*[get_bool(x) for x in value.split()])
🧹 Nitpick comments (8)
newton/_src/utils/import_usd.py (1)
370-397: Use integer dtype for mesh face indices.Face indices should be int32, not float32. Avoid float conversion to prevent precision issues.
- indices = np.array(mesh.GetFaceVertexIndicesAttr().Get(), dtype=np.float32) + indices = np.array(mesh.GetFaceVertexIndicesAttr().Get(), dtype=np.int32)Apply similarly in the shape path:
- indices = np.array(mesh.GetFaceVertexIndicesAttr().Get(), dtype=np.float32) + indices = np.array(mesh.GetFaceVertexIndicesAttr().Get(), dtype=np.int32)Also applies to: 1369-1396
newton/_src/utils/import_mjcf.py (1)
350-366: Per-geometry normals in Scene branch.When iterating m.geometry values, normals should come from m_geom, not m (scene). Also guard for missing normals.
- m_mesh = Mesh( - m_vertices, - m_faces, - m.vertex_normals, - color=np.array(m.visual.main_color) / 255.0, - maxhullvert=maxhullvert, - ) + normals = getattr(m_geom, "vertex_normals", None) + color = np.array(getattr(m_geom.visual, "main_color", [255, 255, 255, 255])) / 255.0 + m_mesh = Mesh( + m_vertices, + m_faces, + normals, + color=color, + maxhullvert=maxhullvert, + )newton/tests/test_schema_resolver.py (1)
116-116: Avoid returning values from unittest test methods.Returning result, builder from a test is unused and non-idiomatic. Drop the return.
- return result, builder + # no return needed in unittest test methodsnewton/_src/usd/utils.py (1)
184-185: Use a clearer variable name.The variable
lis ambiguous and can be confused with1orI. Use a more descriptive name:- l = wp.length(quat) - if np.isfinite(l) and l > 0.0: + quat_length = wp.length(quat) + if np.isfinite(quat_length) and quat_length > 0.0:newton/_src/sim/builder.py (4)
321-328: Silence unused variable; keep dtype validation.
_sizeis unused. Assign to_to satisfy linters.- try: - _size = wp.types.type_size_in_bytes(self.dtype) + try: + _ = wp.types.type_size_in_bytes(self.dtype)
341-351: Confirm zero-default construction across Warp types.
dtype(0)for vectors/matrices may vary across Warp versions. Please verifywp.vec*, matrices, and user-defined dtypes return a zero value here; otherwise considerdtype()or deriving a zero viawp.zeros(1, dtype)[0].
301-303: Clarify USD attribute default wording.Behavior produces
newton:<namespace>:<name>when namespaced, elsenewton:<name>. Update docstring to reflect both cases.- """Name of the corresponding USD attribute. If None, the USD attribute name ``"newton:<namespace>:<name>"`` is used.""" + """Name of the corresponding USD attribute. If None, defaults to ``"newton:<namespace>:<name>"`` when a + namespace is provided, otherwise ``"newton:<name>"``."""
325-327: Shorten exception messages to satisfy Ruff TRY003.Trim overly long messages; content remains clear.
- raise ValueError( - f"Invalid dtype: {self.dtype}. Must be a valid Warp dtype that is compatible with Warp arrays." - ) from e + raise ValueError(f"Invalid Warp dtype: {self.dtype}") from e- raise ValueError(f"Custom attribute '{key}' already exists with incompatible spec") + raise ValueError(f"Incompatible spec for custom attribute '{key}'")- raise ValueError( - f"Custom attribute '{full_key}' default mismatch when merging builders: " - f"existing={merged.default}, incoming={attr.default}" - ) + raise ValueError(f"Default mismatch for custom attribute '{full_key}' during merge")Also applies to: 628-628, 1513-1516
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/api/newton_usd.rst(1 hunks)newton/_src/sim/builder.py(13 hunks)newton/_src/sim/collide_unified.py(1 hunks)newton/_src/usd/__init__.py(1 hunks)newton/_src/usd/schemas.py(1 hunks)newton/_src/usd/utils.py(1 hunks)newton/_src/utils/import_mjcf.py(12 hunks)newton/_src/utils/import_usd.py(37 hunks)newton/_src/utils/import_utils.py(1 hunks)newton/tests/test_schema_resolver.py(26 hunks)newton/usd.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api/newton_usd.rst
- newton/_src/usd/init.py
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
📚 Learning: 2025-10-27T18:45:18.446Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
Applied to files:
newton/tests/test_schema_resolver.pynewton/usd.pynewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/tests/test_schema_resolver.pynewton/usd.pynewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/_src/utils/import_utils.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_schema_resolver.pynewton/usd.pynewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/_src/utils/import_utils.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_schema_resolver.pynewton/usd.pynewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/_src/utils/import_utils.pynewton/_src/usd/utils.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/usd.pynewton/_src/usd/schemas.pynewton/_src/sim/collide_unified.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is properly re-exported from the spatial module via "from .spatial import (..., transform_twist, ...)" and included in __all__, making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/usd.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is re-exported from the spatial module via "from .spatial import transform_twist", making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/usd.py
📚 Learning: 2025-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/_src/utils/import_mjcf.pynewton/_src/sim/builder.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-07-21T19:11:04.077Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.077Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/usd/utils.py
🧬 Code graph analysis (8)
newton/tests/test_schema_resolver.py (3)
newton/_src/sim/builder.py (2)
ModelBuilder(70-5067)add_usd(1012-1131)newton/_src/usd/schema_resolver.py (7)
PrimType(33-49)SchemaResolverManager(168-272)schema_attrs(264-272)collect_prim_attrs(122-145)collect_prim_attrs(246-261)get_value(101-120)get_value(196-244)newton/_src/usd/schemas.py (3)
SchemaResolverNewton(26-102)SchemaResolverPhysx(105-189)SchemaResolverMjc(248-318)
newton/usd.py (1)
newton/_src/usd/utils.py (16)
convert_warp_type(312-343)convert_warp_value(279-309)get_attribute(38-53)get_attributes_in_namespace(56-71)get_custom_attribute_declarations(346-440)get_custom_attribute_values(443-467)get_float(90-90)get_float(94-94)get_float(97-115)get_gprim_axis(238-251)get_quat(159-159)get_quat(163-163)get_quat(166-187)get_scale(220-235)get_transform(254-276)has_attribute(74-86)
newton/_src/utils/import_mjcf.py (4)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-70)newton/_src/usd/schemas.py (1)
solref_to_stiffness_damping(192-225)newton/_src/utils/import_utils.py (3)
parse_custom_attributes(79-120)sanitize_xml_content(123-137)transform(110-111)newton/_src/utils/import_usd.py (1)
add_body(418-432)
newton/_src/usd/schemas.py (1)
newton/_src/usd/schema_resolver.py (3)
PrimType(33-49)SchemaAttribute(53-67)SchemaResolver(70-145)
newton/_src/sim/builder.py (4)
newton/_src/usd/schema_resolver.py (1)
SchemaResolver(70-145)newton/_src/utils/__init__.py (1)
compute_world_offsets(237-303)newton/_src/sim/joints.py (4)
EqType(99-114)JointMode(81-95)JointType(20-44)get_joint_dof_count(47-78)newton/_src/sim/model.py (3)
Model(96-796)ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)
newton/_src/utils/import_usd.py (1)
newton/_src/usd/utils.py (18)
get_transform(254-276)get_scale(220-235)get_float(90-90)get_float(94-94)get_float(97-115)has_attribute(74-86)get_vector(191-191)get_vector(195-195)get_vector(198-217)get_gprim_axis(238-251)get_custom_attribute_values(443-467)from_gfquat(145-155)get_float_with_fallback(118-142)get_custom_attribute_declarations(346-440)get_attribute(38-53)get_quat(159-159)get_quat(163-163)get_quat(166-187)
newton/_src/utils/import_utils.py (1)
newton/_src/sim/builder.py (3)
ModelBuilder(70-5067)CustomAttribute(270-360)key(353-355)
newton/_src/usd/utils.py (3)
newton/_src/core/types.py (2)
Axis(63-160)from_string(74-94)newton/_src/sim/model.py (2)
ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)newton/_src/sim/builder.py (2)
CustomAttribute(270-360)key(353-355)
🪛 Ruff (0.14.3)
newton/_src/utils/import_mjcf.py
100-100: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
newton/_src/sim/builder.py
325-327: Avoid specifying long messages outside the exception class
(TRY003)
628-628: Avoid specifying long messages outside the exception class
(TRY003)
1513-1516: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/utils/import_usd.py
1029-1029: Do not catch blind exception: Exception
(BLE001)
newton/_src/utils/import_utils.py
57-57: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/usd/utils.py
184-184: Ambiguous variable name: l
(E741)
367-367: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (14)
newton/_src/sim/collide_unified.py (1)
47-57: LGTM! Clear and helpful documentation improvements.The per-member docstrings effectively document each broad-phase mode with accurate complexity notation and practical use case guidance. The concise class-level docstring combined with detailed member documentation enhances the public API surface.
newton/_src/utils/import_utils.py (2)
79-121: LGTM!The function correctly handles usd/mjcf/urdf parsing modes with appropriate transformers and provides a sensible default that delegates to
parse_warp_value_from_string.
123-137: LGTM!The XML sanitization logic correctly handles BOM removal, whitespace stripping, and leading comment removal with proper edge case handling.
newton/_src/usd/schemas.py (4)
26-102: Schema resolver structure looks good.The Newton schema resolver follows the established pattern with appropriate defaults and attribute mappings. Note that past review comments have already flagged questions about specific attributes (e.g., enable_gravity) for future refinement.
105-189: LGTM!The PhysX schema resolver correctly handles namespace prefixes, attribute transformations (e.g., inverting
disableGravity), and per-axis limit mappings.
192-246: LGTM!The solref conversion utilities correctly handle both standard mode (timeconst > 0) and direct mode (both negative) with appropriate validation and graceful handling of invalid inputs.
248-318: Review the past comments on MuJoCo schema accuracy.Past review comments have already identified concerns about attribute name accuracy, API coverage, and the solref transformation approach. These should be addressed as noted in the PR objectives (follow-up work with @andrewkaufman assets for testing).
newton/_src/usd/utils.py (7)
38-86: LGTM!The attribute access functions correctly handle USD prim attributes with proper validation checks. The use of
GetAuthoredPropertiesInNamespace()andattr.GetName()is consistent with the learned pattern where GetName() already returns the full namespaced attribute name.Based on learnings
89-142: LGTM!The float getter overloads are properly typed with appropriate finite value validation. The fallback iteration pattern is clean and correct.
190-217: LGTM!The vector getter properly validates finite components and returns consistent float32 numpy arrays.
238-251: LGTM!The function is appropriately named
get_gprim_axisto distinguish it from joint axis handling, addressing previous naming concerns.
279-343: LGTM!The value conversion utilities correctly handle USD-to-Warp type conversion with proper quaternion detection, vector inference, and scalar fallbacks.
346-440: LGTM!The custom attribute declaration parser correctly handles the
newton:namespace:attr_nameformat and readscustomDatametadata for assignment and frequency, with appropriate warnings for invalid or missing metadata.
443-467: LGTM!The custom attribute value extraction correctly applies transformers when provided and falls back to the generic
convert_warp_valueconverter, with proper validation checks.
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)
322-335: Apply the plane-axis rotation to the shape transform.We compute a rotated
xformfor planes, butadd_shape_plane()still receives the pre-rotation transform (plane_xform). As a result, authoredaxisvalues (e.g., X/Y) are ignored and planes stay oriented along +Z. Use the rotated transform when creating the shape.- axis = usd.get_gprim_axis(prim) - plane_xform = xform - # Apply axis rotation to transform - xform = wp.transform(xform.p, xform.q * quat_between_axes(Axis.Z, axis)) + axis = usd.get_gprim_axis(prim) + # Apply axis rotation to transform + rotated_xform = wp.transform(xform.p, xform.q * quat_between_axes(Axis.Z, axis)) width = usd.get_float(prim, "width", 0.0) * scale[0] length = usd.get_float(prim, "length", 0.0) * scale[1] shape_id = builder.add_shape_plane( body=parent_body_id, - xform=plane_xform, + xform=rotated_xform,This mirrors the capsule/cylinder/cone branches and keeps plane orientation consistent with USD authoring.
♻️ Duplicate comments (2)
newton/_src/utils/import_utils.py (1)
73-74: Fix boolean vector parsing
bool("0")andbool("false")both evaluate toTrue, so boolean vectors parsed viawp.vec*still misinterpret MJCF/URDF tokens like"0 1"or"false true". Reuse theget_bool()helper for each element so custom attribute parsing yields accurate Warp values.- if scalar_type is wp.bool or scalar_type is bool: - return warp_dtype(*get_vector(bool)) + if scalar_type is wp.bool or scalar_type is bool: + return warp_dtype(*[get_bool(x) for x in value.split()])newton/_src/sim/builder.py (1)
1495-1542: Fix remapping when first merging custom attribute valuesOn the first merge path we stash the sub-builder’s
CustomAttributeobject verbatim. Itsvaluesdict still references the source builder’s local indices (0…n‑1). As soon as the receiving builder already owns any entities, those indices point at the wrong slots and the overrides silently land on unrelated bodies/shapes/joints. Example: create two bodies in the target builder, thenadd_builder()a sub-builder whose BODY attribute overrides index 0; afterfinalize()the override hits body 0 of the original builder instead of the newly imported body. That’s data loss and breaks per-entity schema attributes. Please copy the attribute and apply the same offset remapping you already do for subsequent merges before storing it.Apply this diff:
for full_key, attr in builder.custom_attributes.items(): # Declare the attribute if it doesn't exist in the main builder merged = self.custom_attributes.get(full_key) if merged is None: - self.custom_attributes[full_key] = attr + offset = None + if attr.frequency == ModelAttributeFrequency.ONCE: + offset = 0 + elif attr.frequency == ModelAttributeFrequency.BODY: + offset = start_body_idx + elif attr.frequency == ModelAttributeFrequency.SHAPE: + offset = start_shape_idx + elif attr.frequency == ModelAttributeFrequency.JOINT: + offset = start_joint_idx + elif attr.frequency == ModelAttributeFrequency.JOINT_DOF: + offset = start_joint_dof_idx + elif attr.frequency == ModelAttributeFrequency.JOINT_COORD: + offset = start_joint_coord_idx + elif attr.frequency == ModelAttributeFrequency.ARTICULATION: + offset = start_articulation_idx + attr_copy = copy.deepcopy(attr) + source_values = attr.values or {} + if source_values and offset is not None: + attr_copy.values = {offset + idx: value for idx, value in source_values.items()} + else: + attr_copy.values = dict(source_values) + self.custom_attributes[full_key] = attr_copy continue # Prevent silent divergence if defaults differ
🧹 Nitpick comments (1)
newton/usd.py (1)
39-41: Remove duplicate export entry
convert_warp_typeis listed twice in__all__, which creates noise in the public surface. Please drop the duplicate line.- "convert_warp_type", - "convert_warp_type", + "convert_warp_type",
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/api/newton_usd.rst(1 hunks)newton/_src/sim/builder.py(13 hunks)newton/_src/sim/collide_unified.py(1 hunks)newton/_src/usd/__init__.py(1 hunks)newton/_src/usd/schemas.py(1 hunks)newton/_src/usd/utils.py(1 hunks)newton/_src/utils/import_mjcf.py(12 hunks)newton/_src/utils/import_usd.py(37 hunks)newton/_src/utils/import_utils.py(1 hunks)newton/tests/test_schema_resolver.py(26 hunks)newton/usd.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- newton/_src/sim/collide_unified.py
- docs/api/newton_usd.rst
🧰 Additional context used
🧠 Learnings (21)
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/usd/__init__.pynewton/tests/test_schema_resolver.pynewton/_src/usd/utils.pynewton/usd.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/usd/__init__.pynewton/_src/usd/schemas.pynewton/usd.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/usd/__init__.pynewton/tests/test_schema_resolver.pynewton/_src/usd/utils.pynewton/usd.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-08-14T17:38:36.106Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/__init__.py:25-29
Timestamp: 2025-08-14T17:38:36.106Z
Learning: The Newton project prefers incremental __all__ building using __all__ += [...] pattern to group exports with their related imports, rather than a single consolidated __all__ at the end of the file.
Applied to files:
newton/_src/usd/__init__.pynewton/usd.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is properly re-exported from the spatial module via "from .spatial import (..., transform_twist, ...)" and included in __all__, making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/_src/usd/__init__.pynewton/usd.py
📚 Learning: 2025-10-27T18:45:18.446Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
Applied to files:
newton/_src/usd/__init__.pynewton/tests/test_schema_resolver.pynewton/_src/usd/schemas.pynewton/_src/usd/utils.pynewton/usd.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/usd/__init__.pynewton/_src/utils/import_utils.pynewton/tests/test_schema_resolver.pynewton/_src/usd/utils.pynewton/usd.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/usd/__init__.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/tests/test_schema_resolver.py
📚 Learning: 2025-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/_src/sim/builder.pynewton/_src/utils/import_mjcf.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-07-21T19:11:04.077Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.077Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
Applied to files:
newton/_src/usd/schemas.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/utils/import_mjcf.pynewton/_src/usd/utils.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is re-exported from the spatial module via "from .spatial import transform_twist", making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/usd.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/utils/import_usd.py
🧬 Code graph analysis (9)
newton/_src/usd/__init__.py (1)
newton/_src/usd/utils.py (16)
convert_warp_type(312-343)convert_warp_value(279-309)get_attribute(38-53)get_attributes_in_namespace(56-71)get_custom_attribute_declarations(346-440)get_custom_attribute_values(443-467)get_float(90-90)get_float(94-94)get_float(97-115)get_gprim_axis(238-251)get_quat(159-159)get_quat(163-163)get_quat(166-187)get_scale(220-235)get_transform(254-276)has_attribute(74-86)
newton/_src/utils/import_utils.py (1)
newton/_src/sim/builder.py (3)
ModelBuilder(70-5067)CustomAttribute(270-360)key(353-355)
newton/tests/test_schema_resolver.py (2)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-70)newton/_src/usd/schema_resolver.py (7)
PrimType(33-49)SchemaResolverManager(168-272)schema_attrs(264-272)collect_prim_attrs(122-145)collect_prim_attrs(246-261)get_value(101-120)get_value(196-244)
newton/_src/sim/builder.py (4)
newton/_src/usd/schema_resolver.py (1)
SchemaResolver(70-145)newton/_src/utils/__init__.py (1)
compute_world_offsets(237-303)newton/_src/sim/joints.py (4)
EqType(99-114)JointMode(81-95)JointType(20-44)get_joint_dof_count(47-78)newton/_src/sim/model.py (3)
Model(96-796)ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)
newton/_src/usd/schemas.py (1)
newton/_src/usd/schema_resolver.py (3)
PrimType(33-49)SchemaAttribute(53-67)SchemaResolver(70-145)
newton/_src/utils/import_mjcf.py (4)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-70)newton/_src/usd/schemas.py (1)
solref_to_stiffness_damping(192-225)newton/_src/utils/import_utils.py (3)
parse_custom_attributes(79-120)sanitize_xml_content(123-137)transform(110-111)newton/_src/sim/builder.py (5)
ModelBuilder(70-5067)CustomAttribute(270-360)get_custom_attributes_by_frequency(637-651)key(353-355)add_body(1556-1628)
newton/_src/usd/utils.py (3)
newton/_src/core/types.py (2)
Axis(63-160)from_string(74-94)newton/_src/sim/model.py (2)
ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)newton/_src/sim/builder.py (3)
ModelBuilder(70-5067)CustomAttribute(270-360)key(353-355)
newton/usd.py (1)
newton/_src/usd/utils.py (16)
convert_warp_type(312-343)convert_warp_value(279-309)get_attribute(38-53)get_attributes_in_namespace(56-71)get_custom_attribute_declarations(346-440)get_custom_attribute_values(443-467)get_float(90-90)get_float(94-94)get_float(97-115)get_gprim_axis(238-251)get_quat(159-159)get_quat(163-163)get_quat(166-187)get_scale(220-235)get_transform(254-276)has_attribute(74-86)
newton/_src/utils/import_usd.py (2)
newton/_src/usd/schema_resolver.py (6)
PrimType(33-49)SchemaResolver(70-145)SchemaResolverManager(168-272)collect_prim_attrs(122-145)collect_prim_attrs(246-261)schema_attrs(264-272)newton/_src/usd/utils.py (17)
get_transform(254-276)get_scale(220-235)get_float(90-90)get_float(94-94)get_float(97-115)has_attribute(74-86)get_vector(191-191)get_vector(195-195)get_vector(198-217)get_gprim_axis(238-251)get_custom_attribute_values(443-467)from_gfquat(145-155)get_float_with_fallback(118-142)get_attribute(38-53)get_quat(159-159)get_quat(163-163)get_quat(166-187)
🪛 Ruff (0.14.3)
newton/_src/utils/import_utils.py
57-57: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/sim/builder.py
325-327: Avoid specifying long messages outside the exception class
(TRY003)
628-628: Avoid specifying long messages outside the exception class
(TRY003)
1513-1516: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/utils/import_mjcf.py
100-100: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
newton/_src/usd/utils.py
184-184: Ambiguous variable name: l
(E741)
367-367: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
newton/_src/utils/import_usd.py
1029-1029: Do not catch blind exception: Exception
(BLE001)
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
619-629: Guard against silent default divergence when re-declaring an attribute.If the same key is added twice with a different
default, the second call is silently ignored. Add a default-compatibility check here (mirroring the merge-time check) to fail fast.existing = self.custom_attributes.get(key) if existing: # validate that specification matches exactly - if ( - existing.frequency != attribute.frequency - or existing.dtype != attribute.dtype - or existing.assignment != attribute.assignment - or existing.namespace != attribute.namespace - ): + if ( + existing.frequency != attribute.frequency + or existing.dtype != attribute.dtype + or existing.assignment != attribute.assignment + or existing.namespace != attribute.namespace + ): raise ValueError(f"Custom attribute '{key}' already exists with incompatible spec") + # Also ensure defaults are compatible (avoid silent divergence) + try: + defaults_match = existing.default == attribute.default + if hasattr(defaults_match, "__iter__") and not isinstance(defaults_match, (str, bytes)): + defaults_match = all(defaults_match) + except (ValueError, TypeError): + defaults_match = False + if not defaults_match: + raise ValueError( + f"Custom attribute '{key}' already exists with a different default " + f"(existing={existing.default}, incoming={attribute.default})" + ) return
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/sim/builder.py(13 hunks)newton/_src/utils/import_usd.py(37 hunks)newton/tests/test_custom_attributes.py(20 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
📚 Learning: 2025-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/tests/test_custom_attributes.pynewton/_src/sim/builder.py
📚 Learning: 2025-09-22T21:03:18.367Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
Applied to files:
newton/tests/test_custom_attributes.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/tests/test_custom_attributes.py
📚 Learning: 2025-10-27T18:45:18.446Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
Applied to files:
newton/_src/utils/import_usd.pynewton/_src/sim/builder.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/utils/import_usd.py
🧬 Code graph analysis (3)
newton/tests/test_custom_attributes.py (1)
newton/_src/sim/builder.py (2)
add_custom_attribute(586-631)CustomAttribute(270-360)
newton/_src/utils/import_usd.py (3)
newton/_src/usd/schema_resolver.py (6)
PrimType(33-49)SchemaResolver(70-145)SchemaResolverManager(168-272)collect_prim_attrs(122-145)collect_prim_attrs(246-261)schema_attrs(264-272)newton/_src/usd/utils.py (17)
get_transform(254-276)get_scale(220-235)get_float(90-90)get_float(94-94)get_float(97-115)get_vector(191-191)get_vector(195-195)get_vector(198-217)get_gprim_axis(238-251)get_custom_attribute_values(443-467)from_gfquat(145-155)get_float_with_fallback(118-142)get_custom_attribute_declarations(346-440)get_attribute(38-53)get_quat(159-159)get_quat(163-163)get_quat(166-187)newton/_src/sim/builder.py (3)
ModelBuilder(70-5071)CustomAttribute(270-360)get_custom_attributes_by_frequency(637-651)
newton/_src/sim/builder.py (4)
newton/_src/usd/schema_resolver.py (1)
SchemaResolver(70-145)newton/_src/utils/__init__.py (1)
compute_world_offsets(237-303)newton/_src/sim/joints.py (4)
EqType(99-114)JointMode(81-95)JointType(20-44)get_joint_dof_count(47-78)newton/_src/sim/model.py (3)
Model(96-796)ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)
🪛 GitHub Actions: Pull Request
newton/_src/sim/builder.py
[warning] 3-3: docstring of newton._src.sim.builder.ModelBuilder.add_joint:3: WARNING: undefined label: 'joint-types' [ref.ref]
🪛 Ruff (0.14.3)
newton/_src/utils/import_usd.py
1029-1029: Do not catch blind exception: Exception
(BLE001)
newton/_src/sim/builder.py
325-327: Avoid specifying long messages outside the exception class
(TRY003)
628-628: Avoid specifying long messages outside the exception class
(TRY003)
1535-1538: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (4)
newton/_src/sim/builder.py (4)
269-361: SolidCustomAttributeAPI; dtype/default handling looks correct.Validation via
wp.types.type_size_in_bytes, sensible dtype defaults (including quats), and array construction are all appropriate. No issues found.
1039-1069: USD docs/readout look good; API reference aligns withSchemaResolverManager.schema_attrs.The parameterization and docs for
schema_resolversare clear and correct, and the output key rename toschema_attrsis consistent. LGTM.Also applies to: 1099-1101
1495-1546: Custom-attribute merge: offsets and default checks are correct.
- Proper offsetting for all frequencies, including
ONCE.- Uses
dataclasses.replaceto avoid mutating the source.- Default mismatch raises early.
Nice cleanup; this fixes the prior index-remap bug during first merge.
4947-4949: Correctelifhandling forONCEfrequency.The mutually exclusive chain now preserves the
ONCEbranch as intended. Good fix.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
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 (2)
newton/examples/robot/example_robot_allegro_hand.py (1)
92-102: Register custom attributes before parsing USD.Custom attributes must be registered on
allegro_handbefore callingadd_usd()to ensure MuJoCo-specific attributes (e.g.,mjc:condim) are parsed from the USD file. Currently, registration occurs on a different builder instance at line 117, after the USD has already been parsed.Apply this diff to fix the registration order:
allegro_hand = newton.ModelBuilder() +newton.solvers.SolverMuJoCo.register_custom_attributes(allegro_hand) asset_path = newton.utils.download_asset("wonik_allegro")newton/_src/utils/import_mjcf.py (1)
311-313: Bug: incoming_xform applied twice for world/static geomsWhen
link == -1the transform already incorporatesincoming_xform(Line 291). Lines 311–313 apply it again, doubling the transform and misplacing static geoms.Apply this diff to remove the second application:
- if incoming_xform is not None: - tf = incoming_xform * tfAlternatively, guard with
and link != -1if needed elsewhere.
♻️ Duplicate comments (2)
newton/_src/utils/import_mjcf.py (1)
100-106: Use defusedxml for untrusted XML (tracked)XML parsing still relies on
xml.etree. Prior discussion opened an issue to switch todefusedxml; no further action here.Confirm follow-up issue remains open and linked to this PR for post-merge hardening.
newton/_src/usd/utils.py (1)
265-275: Fix translation extraction inget_transform.The translation component is stored in the last column of a 4×4 transform matrix, but line 274 reads from the last row. This will produce incorrect positions for all transforms.
Apply this diff to extract the translation correctly:
if invert_rotation: rot = wp.quat_from_matrix(wp.mat33(mat[:3, :3].T.flatten())) else: rot = wp.quat_from_matrix(wp.mat33(mat[:3, :3].flatten())) - pos = mat[3, :3] + pos = mat[:3, 3] return wp.transform(pos, rot)
🧹 Nitpick comments (11)
newton/examples/robot/example_robot_allegro_hand.py (1)
116-118: Remove redundant registration.Since custom attributes are now registered on
allegro_hand(at line 93), this registration onbuilderis redundant. Thereplicate()method copies the registered attributes fromallegro_handtobuilder.Apply this diff to remove the redundant registration:
builder = newton.ModelBuilder() -newton.solvers.SolverMuJoCo.register_custom_attributes(builder) builder.replicate(allegro_hand, self.num_worlds)newton/_src/solvers/mujoco/solver_mujoco.py (2)
1236-1254: Custom attribute registration for “condim” looks correct; two small improvements
- Keep behavior but explicitly cast when consuming (see next comment).
- Optional: set
mjcf_attribute_name="condim"for self-documentation (behavior unchanged).Please confirm call-sites register these attributes on the builder before invoking MJCF/USD import so parsing remains solver-agnostic (as discussed in PR).
2211-2213: Cast condim to Python int when writing to specAvoids accidental numpy/warp scalar leakage into the MJCF spec.
[Suggest applying]
- if shape_condim is not None: - geom_params["condim"] = shape_condim[shape] + if shape_condim is not None: + geom_params["condim"] = int(shape_condim[shape])Please verify
mujoco.MjSpecaccepts the cast value across both CPU and warp backends.newton/_src/utils/import_mjcf.py (1)
536-542: solreflimit conversion — small optional guardCurrent usage is fine. Optionally coerce to plain tuple to avoid edge cases with warp vectors:
- solreflimit = parse_vec(joint_attrib, "solreflimit", (0.02, 1.0)) + solreflimit = tuple(parse_vec(joint_attrib, "solreflimit", (0.02, 1.0)))newton/_src/usd/utils.py (2)
184-184: Rename ambiguous variablelto improve readability.The single-letter variable name
lis ambiguous and can be confused with1orI. Use a descriptive name likelengthorquat_length.Apply this diff:
- l = wp.length(quat) - if np.isfinite(l) and l > 0.0: + length = wp.length(quat) + if np.isfinite(length) and length > 0.0:
366-366: Remove unusednoqadirective.The
noqa: PLC0415directive is unnecessary as the rule is not enabled in the current configuration.Apply this diff:
- from ..sim.builder import ModelBuilder # noqa: PLC0415 + from ..sim.builder import ModelBuildernewton/_src/sim/builder.py (5)
587-617: Guard against duplicate registration with different defaults.Re-registering the same attribute spec but with a different default silently no-ops; later merges will error. Fail fast here.
@@ def add_custom_attribute(self, attribute: CustomAttribute) -> None: - if existing: + if existing: # validate that specification matches exactly if ( existing.frequency != attribute.frequency or existing.dtype != attribute.dtype or existing.assignment != attribute.assignment or existing.namespace != attribute.namespace ): raise ValueError(f"Custom attribute '{key}' already exists with incompatible spec") - return + # also ensure defaults match to avoid divergence later + if existing.default != attribute.default: + raise ValueError(f"Custom attribute '{key}' already exists with a different default") + returnIf you want to allow changing defaults before any explicit values are set, we can permit it when
existing.valuesis empty. Let me know.Also applies to: 620-633
634-653: Type hint clarity for return type.Use the fully qualified nested type for better tool resolution with postponed annotations.
- ) -> list[CustomAttribute]: + ) -> list[ModelBuilder.CustomAttribute]:
666-674: Doc clarity: require fully qualified keys for namespaced attributes.Code looks up exact keys; bare names only work for non-namespaced attributes. Make this explicit.
- Attribute names can optionally include a namespace prefix in the format "namespace:attr_name". - If no namespace prefix is provided, the attribute is assumed to be in the default namespace (None). + Keys must match the registered attribute key exactly. + Use "namespace:attr_name" for namespaced attributes; bare "attr_name" only works for attributes registered without a namespace.
1496-1547: Avoid silent overwrites when merging ONCE/overlapping indices.If both builders author the same target index (e.g., ONCE -> 0), last-wins silently. Consider detecting conflicts.
@@ - merged.values.update({offset + idx: value for idx, value in attr.values.items()}) + for idx, value in attr.values.items(): + dest = offset + idx + if dest in merged.values and merged.values[dest] != value: + raise ValueError( + f"Conflicting value for custom attribute '{full_key}' at index {dest}" + ) + merged.values[dest] = valueDo you prefer “first-wins”, “last-wins”, or “error on conflict”? If “last-wins” is desired, we can keep current behavior but log at DEBUG.
1655-1655: Fix Sphinx cross-reference to JointType.Use a type ref instead of a (likely missing) label.
- joint_type (JointType): The type of joint to add (see :ref:`Joint types`). + joint_type (JointType): The type of joint to add (see :class:`JointType`).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
newton/__init__.py(1 hunks)newton/_src/sim/__init__.py(2 hunks)newton/_src/sim/builder.py(14 hunks)newton/_src/sim/collide_unified.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(4 hunks)newton/_src/usd/schema_resolver.py(1 hunks)newton/_src/usd/utils.py(1 hunks)newton/_src/utils/import_mjcf.py(12 hunks)newton/_src/utils/import_usd.py(40 hunks)newton/examples/robot/example_robot_allegro_hand.py(1 hunks)newton/examples/robot/example_robot_anymal_c_walk.py(1 hunks)newton/examples/robot/example_robot_anymal_d.py(1 hunks)newton/examples/robot/example_robot_cartpole.py(1 hunks)newton/examples/robot/example_robot_h1.py(1 hunks)newton/examples/robot/example_robot_humanoid.py(1 hunks)newton/examples/robot/example_robot_policy.py(1 hunks)newton/examples/robot/example_robot_ur10.py(1 hunks)newton/tests/test_custom_attributes.py(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/_src/sim/collide_unified.py
- newton/_src/sim/init.py
- newton/_src/usd/schema_resolver.py
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/examples/robot/example_robot_anymal_c_walk.pynewton/examples/robot/example_robot_h1.pynewton/examples/robot/example_robot_ur10.pynewton/examples/robot/example_robot_anymal_d.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/examples/robot/example_robot_cartpole.pynewton/examples/robot/example_robot_policy.pynewton/examples/robot/example_robot_humanoid.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/examples/robot/example_robot_anymal_c_walk.pynewton/examples/robot/example_robot_h1.pynewton/examples/robot/example_robot_allegro_hand.pynewton/examples/robot/example_robot_ur10.pynewton/examples/robot/example_robot_anymal_d.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/examples/robot/example_robot_cartpole.pynewton/examples/robot/example_robot_policy.pynewton/examples/robot/example_robot_humanoid.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/examples/robot/example_robot_h1.pynewton/examples/robot/example_robot_anymal_d.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_usd.pynewton/_src/utils/import_mjcf.pynewton/_src/sim/builder.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-27T19:05:44.697Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 535
File: newton/tests/test_examples.py:320-414
Timestamp: 2025-08-27T19:05:44.697Z
Learning: In newton/examples/__init__.py, the robot policy example is registered with the key "robot_policy" (not "robot.example_robot_policy"), so tests should reference it as name="robot_policy".
Applied to files:
newton/examples/robot/example_robot_allegro_hand.pynewton/examples/robot/example_robot_anymal_d.pynewton/examples/robot/example_robot_policy.py
📚 Learning: 2025-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/examples/robot/example_robot_allegro_hand.pynewton/tests/test_custom_attributes.pynewton/examples/robot/example_robot_policy.pynewton/_src/utils/import_mjcf.pynewton/_src/sim/builder.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/examples/robot/example_robot_anymal_d.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/examples/robot/example_robot_anymal_d.py
📚 Learning: 2025-10-27T14:07:25.236Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 940
File: newton/_src/solvers/implicit_mpm/solver_implicit_mpm.py:1477-1482
Timestamp: 2025-10-27T14:07:25.236Z
Learning: In Warp kernels, variables must be explicitly cast (e.g., `current_sum = int(0)`) to be mutable. Without the cast, Warp interprets literal values as constants, which cannot be modified within the kernel.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-10-27T18:45:18.446Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_usd.pynewton/_src/sim/builder.pynewton/_src/usd/utils.py
📚 Learning: 2025-09-22T21:03:18.367Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
Applied to files:
newton/tests/test_custom_attributes.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/tests/test_custom_attributes.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/utils/import_usd.pynewton/__init__.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/utils/import_usd.pynewton/_src/usd/utils.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-07-21T19:11:04.077Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.077Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/usd/utils.py
📚 Learning: 2025-08-14T17:38:36.106Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/__init__.py:25-29
Timestamp: 2025-08-14T17:38:36.106Z
Learning: The Newton project prefers incremental __all__ building using __all__ += [...] pattern to group exports with their related imports, rather than a single consolidated __all__ at the end of the file.
Applied to files:
newton/__init__.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is properly re-exported from the spatial module via "from .spatial import (..., transform_twist, ...)" and included in __all__, making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/__init__.py
📚 Learning: 2025-08-12T17:58:16.929Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/tests/test_ik.py:25-26
Timestamp: 2025-08-12T17:58:16.929Z
Learning: In the Newton physics engine codebase, it's acceptable for tests to import and use private `_src` internal modules and functions when needed for testing purposes, even though this breaks the public API boundary. This is an intentional project decision for test code.
Applied to files:
newton/__init__.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is re-exported from the spatial module via "from .spatial import transform_twist", making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/__init__.py
🧬 Code graph analysis (15)
newton/examples/robot/example_robot_anymal_c_walk.py (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1238-1254)newton/_src/solvers/solver.py (1)
register_custom_attributes(307-314)
newton/examples/robot/example_robot_h1.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1238-1254)
newton/examples/robot/example_robot_allegro_hand.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1238-1254)
newton/examples/robot/example_robot_ur10.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1238-1254)
newton/examples/robot/example_robot_anymal_d.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1238-1254)
newton/_src/solvers/mujoco/solver_mujoco.py (4)
newton/_src/sim/model.py (2)
ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)newton/_src/sim/builder.py (3)
ModelBuilder(71-5072)add_custom_attribute(587-632)CustomAttribute(271-361)newton/_src/core/types.py (1)
override(35-36)newton/_src/solvers/solver.py (1)
register_custom_attributes(307-314)
newton/tests/test_custom_attributes.py (2)
newton/_src/sim/model.py (2)
ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)newton/_src/sim/builder.py (5)
ModelBuilder(71-5072)add_custom_attribute(587-632)CustomAttribute(271-361)add_body(1561-1633)add_joint_revolute(1769-1856)
newton/examples/robot/example_robot_cartpole.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(1164-2939)register_custom_attributes(1238-1254)
newton/examples/robot/example_robot_policy.py (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1238-1254)newton/_src/solvers/solver.py (1)
register_custom_attributes(307-314)
newton/_src/utils/import_usd.py (2)
newton/_src/usd/schema_resolver.py (8)
PrimType(33-49)SchemaResolver(70-145)SchemaResolverManager(168-275)collect_prim_attrs(122-145)collect_prim_attrs(249-264)get_value(101-120)get_value(196-247)schema_attrs(267-275)newton/_src/usd/utils.py (17)
get_transform(253-275)get_scale(220-234)get_float(90-90)get_float(94-94)get_float(97-115)has_attribute(74-86)get_vector(191-191)get_vector(195-195)get_vector(198-217)get_gprim_axis(237-250)get_custom_attribute_values(442-466)from_gfquat(145-155)get_float_with_fallback(118-142)get_attribute(38-53)get_quat(159-159)get_quat(163-163)get_quat(166-187)
newton/_src/utils/import_mjcf.py (3)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-70)newton/_src/usd/schemas.py (1)
solref_to_stiffness_damping(192-225)newton/_src/utils/import_utils.py (3)
parse_custom_attributes(79-120)sanitize_xml_content(123-137)transform(110-111)
newton/_src/sim/builder.py (3)
newton/_src/usd/schema_resolver.py (1)
SchemaResolver(70-145)newton/_src/sim/model.py (3)
Model(96-796)ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)newton/_src/sim/joints.py (1)
JointType(20-44)
newton/_src/usd/utils.py (3)
newton/_src/core/types.py (2)
Axis(63-160)from_string(74-94)newton/_src/sim/model.py (2)
ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)newton/_src/sim/builder.py (3)
ModelBuilder(71-5072)CustomAttribute(271-361)key(354-356)
newton/__init__.py (1)
newton/_src/sim/model.py (2)
ModelAttributeAssignment(31-46)ModelAttributeFrequency(49-70)
newton/examples/robot/example_robot_humanoid.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(1238-1254)
🪛 Ruff (0.14.3)
newton/_src/utils/import_usd.py
1063-1063: Do not catch blind exception: Exception
(BLE001)
newton/_src/utils/import_mjcf.py
100-100: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
newton/_src/sim/builder.py
326-328: Avoid specifying long messages outside the exception class
(TRY003)
629-629: Avoid specifying long messages outside the exception class
(TRY003)
1536-1539: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/usd/utils.py
184-184: Ambiguous variable name: l
(E741)
366-366: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (28)
newton/examples/robot/example_robot_policy.py (1)
232-232: LGTM!The custom attribute registration is correctly placed after ModelBuilder creation and before
add_usd(), enabling MuJoCo-specific attributes (e.g., condim) to be parsed from the USD file.newton/examples/robot/example_robot_humanoid.py (1)
45-45: LGTM!The custom attribute registration is correctly placed after ModelBuilder creation and before
add_mjcf(), enabling MuJoCo-specific attributes to be parsed from the MJCF file.newton/examples/robot/example_robot_cartpole.py (1)
45-45: LGTM!The custom attribute registration is correctly placed after ModelBuilder creation and before
add_usd(), enabling MuJoCo-specific attributes to be parsed from the USD file.newton/examples/robot/example_robot_ur10.py (1)
77-77: LGTM!The custom attribute registration is correctly placed after ModelBuilder creation and before
add_usd(), enabling MuJoCo-specific attributes to be parsed from the USD file.newton/examples/robot/example_robot_anymal_d.py (1)
48-48: LGTM!The custom attribute registration is correctly placed after ModelBuilder creation and before
add_usd(), enabling MuJoCo-specific attributes to be parsed from the USD file.newton/examples/robot/example_robot_h1.py (1)
49-49: LGTM!The custom attribute registration is correctly placed after ModelBuilder creation and before
add_usd(), enabling MuJoCo-specific attributes to be parsed from the USD file.newton/examples/robot/example_robot_anymal_c_walk.py (1)
86-86: LGTM!The custom attribute registration is correctly placed after ModelBuilder creation and before
add_urdf(), enabling MuJoCo-specific attributes to be parsed from the URDF file.newton/__init__.py (1)
66-68: Top-level export of ModelAttribute — LGTM*Re-exporting ModelAttributeAssignment/Frequency from newton._src.sim and adding to all is consistent with the public API direction.
Also applies to: 76-89
newton/_src/solvers/mujoco/solver_mujoco.py (2)
35-38: Imports aligned with public API — LGTMUsing public ModelBuilder/ModelAttribute* here keeps the solver decoupled from private paths.
2022-2034: Attribute retrieval helper is fineFetching
model.mujoco.condimand returning.numpy()is appropriate for CPU-side spec building. No changes needed.newton/_src/utils/import_mjcf.py (6)
22-22: Fixes Ruff F821 — LGTMImporting
Anyresolves undefined name warnings.
31-34: USD/MJCF helpers wired in — LGTMThe added imports match the new custom-attribute and solref handling flow.
93-96: Use identity transform when xform is None — LGTMCleaner default; avoids constructing from unpacked None.
120-133: Prefetching builder custom-attrs by frequency — LGTMKeeps parsing generic and solver-agnostic.
200-202: Axis transformation composition fixed — LGTMComposing once avoids re-wrapping; matches prior guidance.
303-309: Generic custom-attr parsing for geoms — LGTMThis correctly leverages builder-declared attrs (incl. dtype coercion).
newton/tests/test_custom_attributes.py (3)
28-31: Migration to ModelBuilder.CustomAttribute — LGTMTests now use the public wrapper and cover multiple frequencies/assignments. Solid coverage.
Also applies to: 69-106, 108-131, 133-149
840-852: Shape color assertions — LGTMIndexing fixed (0..5) and expectations read clearly.
Also applies to: 855-858
859-899: Namespace tests — nice coverageVerifies nested namespaces on model/state and default namespace.
Also applies to: 936-964
newton/_src/utils/import_usd.py (2)
882-951: LGTM: Custom attribute list ordering is correct.The custom attribute frequency lists are now correctly built after parsing and registering declarations from the PhysicsScene prim (lines 882-884), ensuring that scene-declared attributes are included in the appropriate frequency lists.
257-416: LGTM: Systematic use of USD utility helpers improves consistency.The updated
load_visual_shapesfunction now consistently uses theusd.*helper functions for attribute access, transform extraction, and axis handling. The type annotation onincoming_xformalso improves clarity.newton/_src/sim/builder.py (7)
24-26: LGTM: typing and dataclasses imports.Imports look correct and are used (Iterable in API, replace in merge).
58-58: LGTM: USD SchemaResolver import.Keeps add_usd() solver-agnostic while enabling schema collection.
68-68: LGTM: ModelAttribute enums import.Consistent with new custom-attribute APIs.
1061-1068: LGTM: schema_resolvers doc and reference.Good use of :attr:
newton.usd.SchemaResolverManager.schema_attrsand clear default semantics.
4947-4966: LGTM: ONCE branch useselifchain and count=1.This preserves ONCE semantics during finalize().
1040-1041: Verify Sphinx label exists.Check that the docs define the label “usd_parsing”.
270-362: Two tweaks in CustomAttribute: quaternion default and shorter error text.
- Avoid private dtype internals for quaternion default; prefer stable API.
- Shorten the dtype error per TRY003.
Apply:
@@ - try: - _size = wp.types.type_size_in_bytes(self.dtype) - except TypeError as e: - raise ValueError( - f"Invalid dtype: {self.dtype}. Must be a valid Warp dtype that is compatible with Warp arrays." - ) from e + try: + _ = wp.types.type_size_in_bytes(self.dtype) + except TypeError as e: + raise ValueError(f"Invalid Warp dtype: {self.dtype}") from e @@ - if wp.types.type_is_quaternion(dtype): - return wp.quat_identity(dtype._wp_scalar_type_) + if wp.types.type_is_quaternion(dtype): + return wp.quat_identity()Please confirm expected usage in Warp:
Description
This PR adds the function
Solver.register_custom_attributes(ModelBuilder)which allows a solver to set up custom attributes in the builder so that the user can define them later on programmatically from the builder functions, or parse them from USD or MJCF.As an example, this PR adds MuJoCo
condimattribute as a per-shape custom attribute and parses it from USD and MJCF.The USD importer now handles user-defined custom attributes for shapes, bodies, joints, and articulation.
By adding another type of attribute frequency,
ONCE, we consider those attributes to be potential candidates for model-specific parameters which the USD importer can parse from the PhysicsScene prim (the attributes we return asscene_attributesare now considered for potential model-specific custom attributes).We assume custom attributes follow the name
"newton:namespace:custom_attr"in USD.Addresses #976.
Closes #538.
ToDo's:
Solver.register_custom_attributes(builder)Before your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Improvements
Tests
Breaking Changes