Parse springref and ref #1288
Merged
Parse springref and ref #1288
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThis PR introduces two new per-DOF MuJoCo custom attributes ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1590-1593: Per‑DOF propagation of springref/ref into MuJoCo joint_params is correct; zero as sentinel is acceptableUsing
joint_springref[ai]/joint_ref[ai]to setjoint_params["springref"]and["ref"]for both linear and angular DOFs (when arrays exist and the value is non‑zero) correctly respects the JOINT_DOF frequency and avoids changing behavior when attributes are absent. The zero‑value sentinel is reasonable given current defaults, but if non‑zero defaults are ever introduced you may want a separate “has value” mask instead of relying on!= 0.0.Also applies to: 1676-1679
newton/tests/test_import_mjcf.py (1)
1669-1741: New MJCF ref/springref tests give good coverage; consider minor style cleanupThe added tests validate that:
refinitializesjoint_qcorrectly for both hinge (degrees→radians) and slide (linear) joints.springrefis parsed intomodel.mujoco.dof_springrefas expected for a hinge DOF.This is exactly the coverage needed for the new attribute plumbing. As a minor style nit, you could use the already‑imported
SolverMuJoCo.register_custom_attributes(builder)here for consistency with the rest of the file instead ofnewton.solvers.SolverMuJoCo....newton/_src/utils/import_mjcf.py (2)
618-620: Angular DOF tracking viadof_is_angularis consistent; consider broader joint-type coverageUsing
current_dof_indexas the key into bothdof_custom_attributes[...]anddof_is_angularkeeps the bookkeeping aligned, so later degree→radian conversion for per‑DOF attributes is safe for the hinge cases you care about today. The logic is also consistent with howrangeis handled (is_angular and use_degrees).If you ever add MJCF joint types beyond
"hinge"that should also be treated as angular for DOF‑level attributes (e.g., a future"ball"path or other aliases), it’d be good to centralize/extend theis_angularpredicate rather than hard‑coding the string comparison here, sodof_ref/similar attributes stay coherent across all angular DOFs.Also applies to: 651-652
787-799: dof_ref → joint_q initialization is reasonable; make assumptions about coord/DOF layout explicitUsing the returned
new_joint_idxandbuilder.joint_q_start[new_joint_idx]to seedjoint_qfrom"mujoco:dof_ref"is a clean way to plumb initial positions, and the degree→radian conversion guarded bydof_is_angular.get(dof_idx, False)matches thecompiler.anglehandling.The correctness here depends on two implicit assumptions:
- For joints that can carry
mujoco:dof_ref, the per‑joint coordinate count equals the DOF count (i.e.,qlayout is 1‑to‑1 with DOFs for D6/revolute/prismatic, and free/ball are excluded from this path).- The DOF ordering in
builder.add_joint(...)matches thecurrent_dof_indexorder you used to populatedof_custom_attributes/dof_is_angular.Both are true for the existing D6/hinge/slide import logic, but it’d be helpful to call this out in a short comment or docstring so future changes to joint coordinate conventions (e.g., if free/ball start supporting
dof_ref) don’t silently break this mapping.Also applies to: 800-810
newton/tests/test_import_usd.py (2)
2434-2434: Remove unusednoqadirective.The
# noqa: PLC0415directive is not needed here since the PLC0415 rule is not enabled in this project's configuration.Suggested fix
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
2523-2523: Remove unusednoqadirective.The
# noqa: PLC0415directive is not needed here since the PLC0415 rule is not enabled in this project's configuration.Suggested fix
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.pynewton/tests/test_mujoco_solver.py
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 701
File: CONTRIBUTING.md:7-7
Timestamp: 2025-09-02T13:59:02.072Z
Learning: The newton-physics/newton-governance repository's CONTRIBUTING.md file contains both "Legal Requirements" (line 73, anchor #legal-requirements) and "Project Members" (line 108, anchor #project-members) sections that can be properly referenced from other repositories' CONTRIBUTING.md files.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
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.
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.
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.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/tests/test_import_mjcf.pynewton/_src/utils/import_mjcf.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_usd.pynewton/tests/test_mujoco_solver.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/utils/import_mjcf.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_usd.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/_src/utils/import_mjcf.pynewton/_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_mjcf.pynewton/_src/utils/import_usd.pynewton/tests/test_mujoco_solver.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_mjcf.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/_src/utils/import_mjcf.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/_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, 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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/tests/test_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 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_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/tests/test_import_usd.py
🧬 Code graph analysis (3)
newton/_src/utils/import_mjcf.py (1)
newton/_src/sim/builder.py (1)
add_joint(2017-2163)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(649-694)ModelBuilder(70-5994)CustomAttribute(327-417)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-72)ModelAttributeAssignment(31-46)
newton/tests/test_import_usd.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(81-2591)register_custom_attributes(155-348)
🪛 GitHub Actions: Pull Request
newton/tests/test_import_usd.py
[error] 2567-2567: test_springref_attribute_parsed failed. revolute_joint_idx is None: No revolute joint found for parsing mjc:springref attribute.
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2434-2434: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2523-2523: 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 Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (3)
newton/_src/utils/import_usd.py (1)
829-839: Consistent dof_ref initialization and precedence look goodUsing
mujoco:dof_refonly wheninitial_position is Noneand the value is non‑zero, with degrees→radians conversion for revolute joints and linear use for prismatic, cleanly extends the existing initialization path without changing prior behavior. The key name matches thenamespace:nameconvention for thedof_refcustom attribute, so this should interoperate correctly with the MJCF path and the solver.newton/_src/solvers/mujoco/solver_mujoco.py (1)
301-324: dof_springref/dof_ref custom attributes are wired consistently across builder and solverDefining
dof_springref/dof_refasJOINT_DOFattributes in themujoconamespace withusd_attribute_name="mjc:springref"/"mjc:ref"andmjcf_attribute_name="springref"/"ref", then retrieving them viaget_custom_attribute, lines up with how custom attributes are keyed (namespace:name) and used on joints. This should give a clean per‑DOF scalar in both MJCF and USD paths and matches the expected per‑DOF mapping into MuJoCo joints.Also applies to: 1168-1169
newton/tests/test_mujoco_solver.py (1)
3930-3940: MJCF hinge +springreftest wiring matches importer/solver semanticsChanging the three joints in the inline MJCF to
type="hinge"brings the test fixture in line with both MuJoCo’s schema and the importer’sis_angularlogic, so those DOFs are now correctly treated as angular/revolute in Newton.The added assertions on:
model.mujoco.dof_springrefstaying in degrees[30, 0, 0], andsolver.mj_model.qpos_springbeing[np.deg2rad(30), 0, 0]nicely exercise the new spring‑reference plumbing from MJCF → Newton model → underlying MuJoCo
MjModel. This should catch both unit‑conversion and DOF‑indexing regressions.Also applies to: 3953-3956
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/tests/test_import_mjcf.pynewton/tests/test_import_usd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_import_mjcf.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
📚 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_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/tests/test_import_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/tests/test_import_usd.py
🧬 Code graph analysis (1)
newton/tests/test_import_usd.py (2)
newton/tests/test_import_mjcf.py (2)
test_ref_attribute_parsed(1672-1706)test_springref_attribute_parsed(1716-1740)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(81-2591)register_custom_attributes(155-348)
🪛 GitHub Actions: Pull Request
newton/tests/test_import_usd.py
[error] 2593-2593: Test 'springref' attribute parsing failed. No revolute joint with springref found (test_springref_attribute_parsed).
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2434-2434: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2523-2523: 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). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (1)
newton/tests/test_import_usd.py (1)
2523-2523: Remove unused noqa directive.The
noqa: PLC0415directive is unused and should be removed.🔎 Proposed fix
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd⛔ Skipped due to learnings
Learnt from: daniela-hase Repo: newton-physics/newton PR: 1044 File: newton/_src/sensors/tiled_camera_sensor.py:224-256 Timestamp: 2025-11-07T01:42:36.906Z Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.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.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.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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/tests/test_import_usd.py (2)
2434-2434: Remove unused noqa directive.The
# noqa: PLC0415comment is unnecessary as indicated by static analysis.🔎 Proposed fix
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
2523-2523: Remove unused noqa directive.The
# noqa: PLC0415comment is unnecessary as indicated by static analysis.🔎 Proposed fix
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/utils/import_mjcf.pynewton/tests/test_import_usd.py
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/utils/import_mjcf.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/utils/import_mjcf.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/_src/utils/import_mjcf.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.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_mjcf.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_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/tests/test_import_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/tests/test_import_usd.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (2)
newton/_src/utils/import_mjcf.py (1)
newton/_src/sim/builder.py (2)
add_joint(2017-2163)key(410-412)
newton/tests/test_import_usd.py (3)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(81-2591)register_custom_attributes(155-348)newton/_src/sim/ik/ik_solver.py (1)
joint_q(362-363)newton/_src/sim/joints.py (1)
JointType(20-44)
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2434-2434: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2523-2523: 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). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (6)
newton/_src/utils/import_mjcf.py (4)
618-619: LGTM: Angular DOF tracking initialization.The
dof_is_angulardict is properly initialized to track which DOFs require degree-to-radian conversion when settingjoint_qfromdof_refvalues.
651-652: LGTM: Angular DOF tracking logic.The logic correctly identifies angular DOFs by checking both
is_angular(true for hinge joints) anduse_degrees(from compiler settings). This ensures degree-to-radian conversion is only applied when appropriate.
787-798: LGTM: Capture joint index for downstream use.Storing the joint index from
add_jointenables the subsequent logic to initializejoint_qfromdof_refvalues. The change is straightforward and maintains existing functionality.
800-810: LGTM: Initialize joint_q from dof_ref with correct unit conversion.The implementation correctly:
- Retrieves
dof_refvalues from custom attributes- Converts degrees to radians for angular DOFs using the
dof_is_angulartracking- Sets
joint_qat the correct coordinate indices- Only processes non-zero values
The DOF index mapping between
dof_ref_valueskeys and coordinate indices is consistent with howdof_custom_attributesis built earlier in the function.newton/tests/test_import_usd.py (2)
2428-2511: LGTM: Comprehensive test for mjc:ref attribute parsing.The test correctly verifies that:
- The
mjc:refattribute from USD is parsed into themujoco.dof_refcustom attributejoint_qis initialized with the correct values- Degree-to-radian conversion is applied for revolute joints (45.0° → ~0.785 rad)
- Linear values remain unchanged for prismatic joints (0.5 → 0.5)
The USD structure with an articulation chain properly exercises the import path.
2513-2592: LGTM: Comprehensive test for mjc:springref attribute parsing.The test correctly verifies that:
- The
mjc:springrefattribute from USD is parsed into themujoco.dof_springrefcustom attribute- The springref value (30.0) is correctly stored for the revolute joint
The test robustly searches for a revolute joint with non-zero springref rather than assuming joint ordering, which makes it resilient to changes in the import implementation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
newton/tests/test_import_mjcf.py (1)
1669-1757: Ref/springref tests look solid; minor indexing nitThe end-to-end checks for
dof_ref/dof_springref→joint_q→qpos0/qpos_springare well targeted and match the intended semantics for hinge (degrees) vs slide (linear units). One small clarity improvement:qpos0/qpos_springare conceptuallyq-indexed, so usingq_start[...]rather thanqd_start[...]for those assertions would better reflect the coordinate vs DOF distinction (even though they coincide for hinge/slide today).newton/_src/solvers/mujoco/solver_mujoco.py (1)
301-324: New dof_springref/dof_ref custom attributes are consistent with existing MuJoCo schemaDefining
dof_springrefanddof_refasJOINT_DOF/MODELattributes under themujoconamespace with properusd_attribute_name/mjcf_attribute_namematches the existing pattern (solimplimit,solreffriction, etc.) and lines up with howparse_custom_attributesresolves MJCF fields. Consider adding a brief comment near these definitions clarifying that for angular DOFs the stored values are in degrees (mirroring MJCF) while MuJoCo’s compiled model works in radians—this cross-file contract now matters for both MJCF and USD importers.newton/_src/utils/import_mjcf.py (1)
618-620: Joint q initialization from dof_ref is aligned and unit-safeThe combination of
current_dof_index/dof_is_angulartracking and the post‑add_jointinitialization ofbuilder.joint_qfromdof_custom_attributes["mujoco:dof_ref"]gives the expected behaviour:
- Each MJCF hinge/slide DOF gets a local
dof_idx.- Angular DOFs are flagged in
dof_is_angularonly when MJCF is using degrees.- After creating the Newton joint, you write
joint_q[q_start + dof_idx]in radians for angular DOFs and in linear units for slides.This matches the tests (dof_ref stored as degrees, joint_q in radians). The only implicit assumption is that for the DOFs that support
ref(hinge/slide), there is a 1‑to‑1 mapping between DOFs and coordinates for this joint. If you ever extenddof_refto joint types wheren_coords != n_dofs(e.g. ball/free), this block will need revisiting; a short comment noting that assumption neardof_is_angularwould make that intent explicit.Also applies to: 651-653, 787-799, 800-809
newton/tests/test_import_usd.py (2)
2434-2434: Remove unused noqa directive.The
noqa: PLC0415directive is flagged as unused by the linter.🔎 Proposed fix
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
2528-2528: Remove unused noqa directive.The
noqa: PLC0415directive is flagged as unused by the linter.🔎 Proposed fix
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.py
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/tests/test_import_mjcf.pynewton/_src/solvers/mujoco/solver_mujoco.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, 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/solvers/mujoco/solver_mujoco.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/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/tests/test_import_usd.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/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.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/_src/utils/import_mjcf.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_mjcf.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_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/tests/test_import_usd.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (3)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(649-694)ModelBuilder(70-5994)CustomAttribute(327-417)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-72)ModelAttributeAssignment(31-46)
newton/_src/utils/import_mjcf.py (1)
newton/_src/sim/builder.py (2)
add_joint(2017-2163)key(410-412)
newton/tests/test_import_usd.py (2)
newton/tests/test_import_mjcf.py (1)
test_ref_attribute_parsed(1672-1712)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(81-2591)register_custom_attributes(155-348)
🪛 GitHub Actions: Pull Request
newton/tests/test_import_usd.py
[error] 2617-2617: ValueError: Error: error 'inertia must have positive eigenvalues' in fullinertia. Element name '/Articulation/Body0', id 1.
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2434-2434: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2528-2528: 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). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (3)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1168-1170: Plumbing springref/ref per DOF into MuJoCo joints looks correctReading
dof_springref/dof_refviaget_custom_attributeand wiring them intojoint_paramsfor both linear and angular DOFs matches the intended per-DOF mapping (one Newton DOF → one MuJoCo joint, viadof_to_mjc_joint). For angular DOFs, passing the stored degree values straight intospringref/refis consistent with howrangeis already converted (model stores radians; spec receives degrees), and tests aroundqpos_spring/qpos0should catch unit mismatches. No functional issues stand out here.Also applies to: 1590-1594, 1676-1680
newton/tests/test_import_usd.py (2)
2432-2516: LGTM! Well-structured test for mjc:ref parsing.The test correctly verifies that:
- The
mjc:refattribute is parsed into thedof_refcustom attributejoint_qis initialized frommjc:refwith proper unit conversion (degrees→radians for revolute, direct for prismatic)- MuJoCo solver's
qpos0reflects the parsed valuesThe approach of finding joints by type rather than assuming ordering is robust.
2598-2620: Test logic is sound once inertia issue is resolved.The test correctly verifies:
- The
mjc:springrefattribute is parsed into thedof_springrefcustom attribute- MuJoCo solver's
qpos_springreflects the parsed values with proper unit conversion (degrees→radians for revolute, direct for prismatic)The approach of finding joints by type is robust and matches the pattern in
test_ref_attribute_parsed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
newton/tests/test_import_usd.py (1)
2434-2434: Remove unused noqa directive.The
noqa: PLC0415directive is not required since this rule is not enabled in the linting configuration.🔎 Proposed fix
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
🧹 Nitpick comments (1)
newton/tests/test_import_usd.py (1)
2585-2592: Consider specifying the physics:axis attribute for clarity.While USD defaults to Z-axis for revolute joints when not specified, explicitly setting
token physics:axis = "Z"would improve test readability and make the joint configuration more explicit.🔎 Suggested addition
def PhysicsRevoluteJoint "revolute_joint" ( prepend apiSchemas = ["PhysicsDriveAPI:angular"] ) { + token physics:axis = "Z" rel physics:body0 = </Articulation/Body0> rel physics:body1 = </Articulation/Body1> float mjc:springref = 30.0 }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/tests/test_import_mjcf.pynewton/tests/test_import_usd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_import_mjcf.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
📚 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_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/tests/test_import_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/tests/test_import_usd.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/tests/test_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/tests/test_import_usd.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (1)
newton/tests/test_import_usd.py (2)
newton/_src/sim/builder.py (1)
ModelBuilder(70-5994)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(155-348)
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2434-2434: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2528-2528: 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). (3)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (2)
newton/tests/test_import_usd.py (2)
2428-2516: LGTM! Comprehensive test for mjc:ref attribute parsing.The test effectively verifies:
- USD attribute parsing into
model.mujoco.dof_ref- Correct initialization of
joint_qwith unit conversion (degrees→radians for revolute)- Proper propagation to MuJoCo solver's
qpos0The articulation structure and test assertions are well-designed.
2518-2639: LGTM! Comprehensive test for mjc:springref attribute parsing.The test effectively verifies:
- USD attribute parsing into
model.mujoco.dof_springref- Correct propagation to MuJoCo solver's
qpos_springwith unit conversion (degrees→radians for revolute)- Proper articulation structure with collision shapes on all bodies
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/utils/import_mjcf.py (1)
617-652: Guard dof_ref → joint_q initialization for non–1‑to‑1 joints (BALL)Using
dof_idxto write intobuilder.joint_q[q_start + dof_idx]is correct when DOF and coord counts align (hinge/slide/D6), but becomes problematic for joints whose coordinates are not 1‑to‑1 with DOFs:
- For
JointType.BALL,coord_count > dof_count(quaternion), so this block will overwrite the quaternion’s x/y/z components with ref angles (whenconvert_3d_hinge_to_ball_joints=Trueandrefis present), leavingwas whateveradd_jointset. That can yield invalid or surprising initial orientations.- MJCF’s
ref/springrefare only defined for hinge/slide DOFs anyway, so skipping BALL here is more faithful to MuJoCo semantics.Recommend gating the
dof_refapplication to joint types with a 1‑to‑1 DOF↔coord mapping (REVOLUTE/PRISMATIC/D6) so BALL/free joints stay quaternion‑based without per‑DOF refs applied.Proposed fix
- new_joint_idx = builder.add_joint( + new_joint_idx = builder.add_joint( joint_type, parent=parent, child=link, linear_axes=linear_axes, angular_axes=angular_axes, key="_".join(joint_name), parent_xform=parent_xform_for_joint, child_xform=wp.transform(joint_pos, wp.quat_identity()), custom_attributes=joint_custom_attributes | dof_custom_attributes, ) - joint_indices.append(new_joint_idx) - - # Set initial joint positions from dof_ref custom attribute (if registered) - # dof_ref stores values in degrees (for angular DOFs), convert to radians for joint_q - dof_ref_values = dof_custom_attributes.get("mujoco:dof_ref", {}) - if dof_ref_values: - q_start = builder.joint_q_start[new_joint_idx] - for dof_idx, ref_deg in dof_ref_values.items(): - if ref_deg != 0.0: - ref_val = float(ref_deg) - ref_rad = np.deg2rad(ref_val) if dof_is_angular.get(dof_idx, False) else ref_val - builder.joint_q[q_start + dof_idx] = ref_rad + joint_indices.append(new_joint_idx) + + # Set initial joint positions from dof_ref custom attribute (if registered) + # Only apply to joints with 1–1 DOF↔coord mapping (hinge/slide/D6), to avoid + # corrupting quaternion coordinates on BALL/FREE joints. + # dof_ref stores values in degrees for angular DOFs (when use_degrees=True), + # convert to radians for joint_q. + dof_ref_values = dof_custom_attributes.get("mujoco:dof_ref", {}) + if dof_ref_values and joint_type in ( + JointType.REVOLUTE, + JointType.PRISMATIC, + JointType.D6, + ): + q_start = builder.joint_q_start[new_joint_idx] + for dof_idx, ref_deg in dof_ref_values.items(): + if ref_deg != 0.0: + ref_val = float(ref_deg) + ref_rad = np.deg2rad(ref_val) if dof_is_angular.get(dof_idx, False) else ref_val + builder.joint_q[q_start + dof_idx] = ref_radBased on learnings, this keeps behavior consistent with the per‑DOF mapping assumptions used on the MuJoCo side.
Also applies to: 786-808
♻️ Duplicate comments (1)
newton/tests/test_import_usd.py (1)
2434-2434: Remove unused# noqa: PLC0415directives on local pxr importsRuff is flagging these
noqacomments as unused (rulePLC0415isn’t enabled), so they’re just noise. You can safely drop them while keeping the local imports inside the tests.Proposed change
- from pxr import Usd # noqa: PLC0415 + from pxr import UsdApply to both occurrences in
TestUSDRefAttribute.test_ref_attribute_parsedandTestUSDSpringRefAttribute.test_springref_attribute_parsed.Also applies to: 2519-2519
🧹 Nitpick comments (3)
newton/tests/test_import_mjcf.py (1)
1669-1743: New ref / springref tests validate the intended MJCF→Newton→MuJoCo flowThese tests nicely pin down the behavior:
TestMJCFRefAttributechecks both storage inmodel.mujoco.dof_refand initialization ofjoint_q(including degree→radian conversion for the hinge DOF).TestMJCFSpringRefAttributeverifiesspringrefis parsed intomodel.mujoco.dof_springrefat the correct DOF indices.They provide good regression coverage for the new attributes on hinge and slide joints. You might later consider adding a case with multiple DOFs per joint (e.g., merged hinges/D6) or
compiler angle="radian"for extra robustness, but what’s here is already solid.newton/tests/test_import_usd.py (1)
2428-2624: USD ref / springref tests look correct; consider making the USD fixture more conventionalThe new tests correctly:
- Gate on
USD_AVAILABLEand use in‑memory stages.- Verify
dof_ref/dof_springrefat the DOF indices derived fromjoint_qd_start.- Check revolute refs in radians via
joint_q(and linear refs directly), consistent with the MJCF tests and the intended deg→rad behavior for angular DOFs.One thing to consider for future‑proofing: in
TestUSDRefAttributethe fixture uses aPhysicsArticulationRootAPIbody (child1) attached to a kinematic parent via a revolute joint. That pattern has been brittle in past USD import tests. Mirroring the more standardXform "Articulation"+ child bodies structure used elsewhere in this file (and/or dropping the kinematic flag on the parent) would make the test less sensitive to importer edge cases, while keeping the assertions the same.newton/tests/test_mujoco_solver.py (1)
4016-4074: USD ref/springref checks are correct; joint indexing could be made more explicitThe USD fixture and assertions correctly validate:
mjc:condimon the three bodies mapping to[6, 4, 3]on bothmodel.mujoco.condimandgeom_condim.mjc:ref/mjc:springrefon the revolute and prismatic joints drivingqpos0/qpos_springwith the expected deg→rad behavior for the angular DOF and linear values for the prismatic DOF.To make the test less sensitive to changes in joint ordering, you might consider resolving
revolute_q_start/prismatic_q_startviamodel.joint_key.index("<joint_path>")instead of hard‑coding indices 1 and 2, but that’s optional given how controlled this fixture is.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.pynewton/tests/test_mujoco_solver.py
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/utils/import_mjcf.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/utils/import_mjcf.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/_src/utils/import_mjcf.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_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, 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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/tests/test_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/tests/test_mujoco_solver.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/tests/test_mujoco_solver.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_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/tests/test_import_usd.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (3)
newton/_src/utils/import_mjcf.py (1)
newton/_src/sim/builder.py (2)
add_joint(2017-2163)key(410-412)
newton/tests/test_import_mjcf.py (2)
newton/_src/sim/builder.py (2)
ModelBuilder(70-5994)finalize(5363-5890)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(155-348)
newton/tests/test_import_usd.py (2)
newton/tests/test_import_mjcf.py (2)
test_ref_attribute_parsed(1672-1707)test_springref_attribute_parsed(1713-1742)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(81-2591)register_custom_attributes(155-348)
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2434-2434: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2519-2519: 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). (3)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
301-324: dof_ref / dof_springref wiring into MuJoCo joints looks correctThe new custom attributes and their use in the linear/Angular DOF loops are consistent with existing DOF‑level attributes:
dof_springref/dof_refare defined as JOINT_DOF, MODEL‑assigned,wp.float32, with proper MJCF names (springref/ref) and the"mujoco"namespace.- In
_convert_to_mjc, you index them viaai = joint_qd_start[j] + i, matching how stiffness/damping/solimp/solref are already handled and aligning with the per‑DOF MuJoCo joint mapping (dof_to_mjc_joint), as per the learned mapping pattern.- Passing the raw MJCF values through to MuJoCo (
springref/ref) while independently convertingdof_refto radians forjoint_qon import preserves unit consistency: MJCF/solver see degrees, Newton state sees radians.No functional issues spotted here.
Based on learnings, this matches the established DOF→MuJoCo joint mapping strategy.
Also applies to: 1168-1170, 1590-1594, 1676-1680
newton/tests/test_mujoco_solver.py (1)
3930-3958: MJCF ref/springref propagation test looks solidThe added
ref/springrefattributes and assertions correctly exercise the new plumbing:
dof_springrefanddof_refcontain the raw MJCF values[30, 0.25, 0]and[45, 0.5, 0]per DOF.qpos_spring/qpos0are checked in MuJoCo units, with angular refs converted to radians and linear refs left in meters.This gives good end‑to‑end coverage from MJCF → Newton model → MuJoCo model.
Member
adenzler-nvidia
left a comment
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Thanks, some comments. I'm not sure if I understand all the implications of the mujoco docs, and actually I'm not even sure if changing ref is actually something that has an effect for newton. Seems like we would just transform the joint position back-and-forth?
Springref is a different thing because it affects the dynamics though.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @newton/tests/test_import_usd.py:
- Around line 2502-2608: Test authorship: add missing physics:axis to
PhysicsRevoluteJoint, switch joint selection to use model.joint_key, and remove
the unused noqa comment. Update the USD snippet so the PhysicsRevoluteJoint
"revolute_joint" includes token physics:axis = "Z"; in the Python test replace
the "first of type" loops with lookups by model.joint_key (use the joint key
strings produced by builder.add_usd / SolverMuJoCo.register_custom_attributes)
to find revolute and prismatic joint indices, and delete the extraneous "# noqa:
PLC0415" on the pxr import line. Ensure asserts still index springref via
qd_start[joint_index] after these changes.
In @newton/tests/test_mujoco_solver.py:
- Around line 4031-4145: The USD test uses hard-coded joint indices (q_start[1],
q_start[2]) and an unnecessary "# noqa: PLC0415"; change the assertions to look
up joint indices via the model's joint_key mapping (e.g., use
model.joint_key["/Articulation/joint1"] and
model.joint_key["/Articulation/joint2"] to get revolute_q_start and
prismatic_q_start) and remove the unused noqa comment near the pxr import;
update the two assertions to index solver.mj_model.qpos0 using those keys so the
test is robust to joint ordering changes.
🧹 Nitpick comments (1)
newton/tests/test_mujoco_solver.py (1)
3902-4030: Condim coverage looks good; consider tightening URDF validity (optional).
The code/MJCF/USD variants are straightforward and validate bothmodel.mujoco.condimand the MuJoCo-sidegeom_condim. One nit: the URDF snippet includes<joint .../>elements under<link>, which is not standard URDF and could mask parsing differences if the importer gets stricter.Possible follow-up (URDF snippet cleanup)
@@ - <link name="body1"> - <joint type="revolute" axis="0 0 1" /> + <link name="body1"> <collision> <geometry condim="6"> <box size="0.1 0.1 0.1" /> </geometry> </collision> </link> @@ - <link name="body2"> - <joint type="revolute" axis="0 0 1" /> + <link name="body2"> <collision> <geometry condim="4"> <box size="0.1 0.1 0.1" /> </geometry> </collision> </link> @@ - <link name="body3"> - <joint type="revolute" axis="0 0 1" /> + <link name="body3"> <collision> <geometry> <box size="0.1 0.1 0.1" /> </geometry> </collision> </link>
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.pynewton/tests/test_mujoco_solver.py
💤 Files with no reviewable changes (1)
- newton/_src/utils/import_mjcf.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_import_mjcf.py
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
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.
📚 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/tests/test_mujoco_solver.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/tests/test_mujoco_solver.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/tests/test_mujoco_solver.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_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 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_import_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/tests/test_import_usd.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (2)
newton/tests/test_mujoco_solver.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(155-348)
newton/tests/test_import_usd.py (2)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(81-2591)register_custom_attributes(155-348)newton/_src/sim/joints.py (1)
JointType(20-44)
🪛 Ruff (0.14.10)
newton/tests/test_mujoco_solver.py
4087-4087: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
4206-4206: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
newton/tests/test_import_usd.py
2430-2430: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2505-2505: 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-newton-tests / newton-unittests (windows-latest)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
newton/tests/test_mujoco_solver.py (4)
4037-4150: Missing URDF test coverage fordof_refattribute.The
dof_reftests cover code, MJCF, and USD parsing but lack atest_dof_ref_from_urdftest. This is inconsistent with thecondimtests which include URDF coverage. Consider adding a URDF test to maintain parity across import formats.Would you like me to help generate a
test_dof_ref_from_urdftest case following the pattern established bytest_condim_from_urdf?
4093-4093: Remove unusednoqadirective.The static analysis indicates that
PLC0415is not enabled in your ruff configuration, making this directive unnecessary.Suggested fix
- from pxr import Sdf, Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Sdf, Usd, UsdGeom, UsdPhysics
4152-4269: Missing URDF test coverage fordof_springrefattribute.Similar to
dof_ref, thedof_springreftests are missing atest_dof_springref_from_urdftest. For consistent coverage across all import formats, consider adding a URDF test case.Would you like me to help generate a
test_dof_springref_from_urdftest case?
4212-4212: Remove unusednoqadirective.Same as above - the
PLC0415rule is not enabled.Suggested fix
- from pxr import Sdf, Usd, UsdGeom, UsdPhysics # noqa: PLC0415 + from pxr import Sdf, Usd, UsdGeom, UsdPhysics
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/tests/test_import_mjcf.pynewton/tests/test_import_usd.pynewton/tests/test_mujoco_solver.py
💤 Files with no reviewable changes (2)
- newton/tests/test_import_usd.py
- newton/tests/test_import_mjcf.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
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.
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.
📚 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/tests/test_mujoco_solver.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/tests/test_mujoco_solver.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/tests/test_mujoco_solver.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_mujoco_solver.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/tests/test_mujoco_solver.py
🧬 Code graph analysis (1)
newton/tests/test_mujoco_solver.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(155-348)
🪛 Ruff (0.14.10)
newton/tests/test_mujoco_solver.py
4093-4093: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
4212-4212: 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). (2)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (1)
newton/tests/test_mujoco_solver.py (1)
3902-4036: LGTM! Tests renamed and structured consistently.The condim attribute tests are well-organized, covering all four import sources (code, MJCF, URDF, USD). The test logic correctly verifies both the model attribute storage and the propagation to the MuJoCo solver.
Member
|
Thanks. Now the question for me is whether the MJC->Newton conversion for joint position also needs an update that makes sure we can handle joints with ref? Isn't joint_q shifted internally because of the new reference position? Maybe we can do this in a follow-up where we add detailed tests that verify the behavior and help us understand the mechanism better. That would also help us understand if we need to add the feature to newton? What do you think? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @newton/tests/test_import_usd.py:
- Around line 2526-2590: The test test_ref_attribute_parsing uses mjc:ref values
that look like degrees for a revolute joint — MuJoCo expects joint refs in
joint-native units (radians for revolute, meters for prismatic); update the
revolute value to a radian value (e.g., use 0.785398 for ~45°) or explicitly
comment that degrees are intentional, and ensure prismatic values are in meters;
also add an assertion of joint type (using model.joint_key/model.joint_type or
equivalent) before indexing into model.joint_qd_start and model.mujoco.dof_ref
to make failures clearer; apply the same fixes for the corresponding
mjc:springref tests referenced.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.pynewton/tests/test_mujoco_solver.py
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
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.
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.
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/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/tests/test_import_usd.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.pynewton/tests/test_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/tests/test_mujoco_solver.pynewton/tests/test_import_usd.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/tests/test_mujoco_solver.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_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/tests/test_import_usd.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_import_usd.py
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2529-2529: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2594-2594: 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). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (8)
newton/tests/test_mujoco_solver.py (1)
3969-3984: MJCF snippet: switching joint type tohingeis a good test-data correctness fix.newton/tests/test_import_usd.py (1)
2529-2529: Remove unused# noqa: PLC0415to satisfy ruff (RUF100).Proposed diff
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd- from pxr import Usd # noqa: PLC0415 + from pxr import UsdAlso applies to: 2594-2594
⛔ Skipped due to learnings
Learnt from: daniela-hase Repo: newton-physics/newton PR: 1044 File: newton/_src/sensors/tiled_camera_sensor.py:224-256 Timestamp: 2025-11-07T01:42:36.906Z Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.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.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.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.newton/tests/test_import_mjcf.py (2)
1668-1700: LGTM!The test correctly verifies that the
refattribute is parsed for both hinge and slide joints and stored inmodel.mujoco.dof_refat the appropriate DOF indices.
1701-1730: LGTM!The test correctly verifies that the
springrefattribute is parsed for both hinge and slide joints and stored inmodel.mujoco.dof_springref. Good inclusion ofstiffnessattribute to make thespringrefmeaningful.newton/_src/solvers/mujoco/solver_mujoco.py (4)
307-330: LGTM!The custom attribute definitions for
dof_springrefanddof_reffollow the established patterns and are correctly configured with appropriate frequency (JOINT_DOF), assignment (MODEL), and namespace (mujoco).
1167-1168: LGTM!The attribute extraction follows the established pattern using
get_custom_attributehelper.
1591-1596: LGTM!The
springrefandrefattributes are correctly applied to linear DOFs (slide joints). The values are properly indexed by the DOF indexaiand conditionally added only when the attributes exist.
1678-1681: LGTM!The
springrefandrefattributes are correctly applied to angular DOFs (hinge joints), following the same pattern as linear DOFs.Note on units: MuJoCo's MJCF uses degrees for angular
refandspringrefvalues. The current implementation stores and passes through these values as-is, which is correct for the MuJoCo round-trip. However, as noted in the PR comments by adenzler-nvidia, there may be downstream considerations for howrefaffectsjoint_qconversion that could be addressed in a follow-up.
Contributor
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)
783-810:joint_posappears to be applied inconsistently (and possibly twice) after introducingchild_xform_pos = joint_pos.With the new logic at Line 789-Line 809,
child_xformnow includesjoint_pos, but for non-world parentsparent_xform_for_joint(Line 786-Line 787) still incorporates+ joint_posand does not rotate it bybody_ori_for_joints. That’s very likely incorrect when the child body has a non-identity orientation and can also effectively double-count translation.If the intent is “MJCF joint pos is expressed in the child body frame”, then parent joint frame should be
T(parent->child) * T(child->joint):Proposed fix
- else: - parent_xform_for_joint = wp.transform(body_pos_for_joints + joint_pos, body_ori_for_joints) + else: + parent_xform_for_joint = wp.transform(body_pos_for_joints, body_ori_for_joints) * wp.transform( + joint_pos, wp.quat_identity() + )This keeps
child_xform_pos = joint_pos(Line 789) meaningful while making parent/child joint frames consistent.
🤖 Fix all issues with AI agents
In @newton/tests/test_import_usd.py:
- Around line 2526-2591: The test_ref_attribute_parsing function contains an
unused inline noqa on the Usd import and a brittle quaternion equality check;
remove the unused "# noqa: PLC0415" on the "from pxr import Usd" line and change
the quaternion assertion (variables joint_quat and expected) to be
sign-invariant by asserting that either joint_quat ≈ expected or joint_quat ≈
-expected (e.g., compare min(norm(joint_quat-expected),
norm(joint_quat+expected)) against the tolerance) so the test passes if the
quaternion flips sign.
- Around line 2592-2688: Remove the unused ruff disable on the pxr import in
test_springref_attribute_parsing: locate the line "from pxr import Usd # noqa:
PLC0415" inside the test_springref_attribute_parsing function and delete the
trailing "# noqa: PLC0415" comment so the line is simply "from pxr import Usd".
Run the linter to confirm RUF100 is resolved.
🧹 Nitpick comments (1)
newton/_src/utils/import_usd.py (1)
563-578: UseAxis.to_vec3()to simplify axis vector creation.The local
axis_vectorsdict can be replaced withaxis_enum.to_vec3()sinceAxishas a built-into_vec3()method:Simplify axis vector creation
- axis_enum = usd_axis_to_axis[joint_desc.axis] - axis_vectors = {Axis.X: wp.vec3(1, 0, 0), Axis.Y: wp.vec3(0, 1, 0), Axis.Z: wp.vec3(0, 0, 1)} - axis_vec = axis_vectors[axis_enum] + axis_vec = usd_axis_to_axis[joint_desc.axis].to_vec3()Note: The
mjc:refbaking intochild_xformand its preservation in thedof_refcustom attribute is intentional—the testtest_ref_attribute_parsingvalidates that both the frame transform and the custom attribute capture the reference offset. The quaternion composition order (child_tf.q * ref_quat) is correct for baking the offset.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_import_mjcf.py
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
📚 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.pynewton/_src/utils/import_mjcf.pynewton/tests/test_import_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/utils/import_usd.pynewton/tests/test_import_usd.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/utils/import_usd.pynewton/_src/utils/import_mjcf.pynewton/tests/test_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.pynewton/_src/utils/import_mjcf.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/utils/import_mjcf.pynewton/tests/test_import_usd.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/_src/utils/import_mjcf.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/_src/utils/import_mjcf.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_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/tests/test_import_usd.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (3)
newton/_src/utils/import_usd.py (3)
newton/_src/core/types.py (1)
Axis(69-166)newton/_src/sim/builder.py (1)
key(435-437)newton/_src/utils/import_utils.py (1)
transform(139-140)
newton/_src/utils/import_mjcf.py (2)
newton/_src/sim/joints.py (1)
JointType(20-47)newton/_src/utils/import_utils.py (1)
transform(139-140)
newton/tests/test_import_usd.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(161-354)
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2529-2529: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2595-2595: 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). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (1)
newton/_src/utils/import_mjcf.py (1)
650-654: No actionable issues found withrefunit handling.The code correctly parses the MJCF
refattribute (line 651), converts it to radians when bothis_angular and use_degreesare true (lines 652-653), and bakes it into the child transform before passing tobuilder.add_joint()(lines 791-797). Therefvalue is not parsed as a custom attribute or re-applied downstream—it is handled explicitly and consistently. There is no evidence of double-application or unit mismatch between parsing and downstream MuJoCo solver usage.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @newton/tests/test_import_usd.py:
- Around line 2593-2689: The test test_springref_attribute_parsing should
explicitly declare the joint axis for the revolute joint and remove the
unnecessary noqa on the pxr import: add a token physics:axis = "Z" (or
appropriate axis) to the PhysicsRevoluteJoint "revolute_joint" declaration so
the axis is not relying on USD schema defaults, and delete the trailing "# noqa:
PLC0415" from the from pxr import Usd line.
🧹 Nitpick comments (2)
newton/tests/test_import_mjcf.py (1)
1668-1715: Ref parsing + FK baking assertion looks solid; consider also asserting slide ref translation.
Right now only hinge ref’s rotational bake is validated; adding a quick position assertion forchild2would strengthen coverage of prismatic/slide baking.newton/_src/utils/import_mjcf.py (1)
782-810: Harden ref baking: guard zero axis + avoid confusingdof_reflocal naming.
This prevents potential NaNs if an asset hasrefbut no valid axis, and makes it clearer this is just for baking.Proposed diff
if joint_type in (JointType.REVOLUTE, JointType.PRISMATIC): # Parse ref for baking into child_xform - dof_ref = parse_float(joint_attrib, "ref", 0.0) + ref_value = parse_float(joint_attrib, "ref", 0.0) if joint_type == JointType.REVOLUTE: if use_degrees: - dof_ref = np.deg2rad(dof_ref) - if dof_ref != 0.0: - axis = wp.normalize(angular_axes[0].axis) - child_xform_rot = wp.quat_from_axis_angle(axis, float(dof_ref)) + ref_value = np.deg2rad(ref_value) + if ref_value != 0.0: + axis = angular_axes[0].axis + if wp.length(axis) > 0.0: + axis = wp.normalize(axis) + child_xform_rot = wp.quat_from_axis_angle(axis, float(ref_value)) elif joint_type == JointType.PRISMATIC: - if dof_ref != 0.0: - axis = wp.normalize(linear_axes[0].axis) - child_xform_pos = joint_pos + float(dof_ref) * axis + if ref_value != 0.0: + axis = linear_axes[0].axis + if wp.length(axis) > 0.0: + axis = wp.normalize(axis) + child_xform_pos = joint_pos + float(ref_value) * axis
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.py
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
📚 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_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 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_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/tests/test_import_usd.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/tests/test_import_usd.pynewton/_src/utils/import_mjcf.py
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
newton/tests/test_import_usd.pynewton/_src/utils/import_mjcf.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_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/tests/test_import_usd.pynewton/_src/utils/import_mjcf.py
📚 Learning: 2025-12-12T16:25:54.685Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/core/math.py:248-255
Timestamp: 2025-12-12T16:25:54.685Z
Learning: In newton/_src/solvers/kamino/core/math.py, the quat_conj function's current implementation (negating w instead of x,y,z) is a known issue that will be revised in a future PR per vastsoun's decision, not to be flagged in current Kamino reviews.
Applied to files:
newton/tests/test_import_usd.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/utils/import_mjcf.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/_src/utils/import_mjcf.py
🧬 Code graph analysis (1)
newton/_src/utils/import_mjcf.py (1)
newton/_src/sim/joints.py (1)
JointType(20-47)
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2529-2529: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2596-2596: 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). (4)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (2)
newton/tests/test_import_mjcf.py (1)
1716-1746: Springref parsing assertions look good.
Simple, direct checks viaqd_startare the right level here.newton/tests/test_import_usd.py (1)
2526-2592: Fix Ruff RUF100: remove unused# noqa: PLC0415on local pxr import.
Ruff reports the directive as unused; keeping it will fail lint.Proposed diff
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd⛔ Skipped due to learnings
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.Learnt from: daniela-hase Repo: newton-physics/newton PR: 1044 File: newton/_src/sensors/tiled_camera_sensor.py:224-256 Timestamp: 2025-11-07T01:42:36.906Z Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.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.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.
Contributor
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 (1)
newton/_src/utils/import_mjcf.py (1)
782-811: Normalizemujoco:dof_refandmujoco:dof_springrefto degrees when importing withangle="radian".When an MJCF file uses
<compiler angle="radian">, therefattribute for angular joints is in radians. The code correctly bakes this radian value into the child transform for FK. However,parse_custom_attributesat line 687 stores the raw radian value in the model's custom attributes, whileSolverMuJoColater assumes all storeddof_refvalues are in degrees and appliesdeg2radconversion. This causes incorrect behavior when exporting to MuJoCo format.Add normalization after line 687 to convert angular custom attributes from radians to degrees before storing them in
dof_custom_attributeswhenuse_degrees == False:dof_attr = parse_custom_attributes(joint_attrib, builder_custom_attr_dof, parsing_mode="mjcf") # Normalize MuJoCo-facing angular attributes to degrees so SolverMuJoCo can treat them consistently. # MJCF can be configured to use radians via <compiler angle="radian">. if not use_degrees and is_angular: if "mujoco:dof_ref" in dof_attr: dof_attr["mujoco:dof_ref"] = float(np.rad2deg(dof_attr["mujoco:dof_ref"])) if "mujoco:dof_springref" in dof_attr: dof_attr["mujoco:dof_springref"] = float(np.rad2deg(dof_attr["mujoco:dof_springref"]))
🤖 Fix all issues with AI agents
In @newton/_src/solvers/mujoco/solver_mujoco.py:
- Around line 307-330: The dof_ref and dof_springref attributes are registered
correctly but their values are not converted from radians to degrees when
emitting MjSpec for hinge joints; update the code path that writes these
ModelBuilder.CustomAttribute values to MjSpec (the writer that processes dof_ref
and dof_springref) to perform np.rad2deg(...) (or equivalent) for angular/hinge
DOFs only, leaving prismatic/translational DOFs unchanged, so MJCF receives ref
and springref in degrees consistent with range conversion.
- Around line 1442-1462: The solver assumes joint_ref/dof_ref are degrees but
the MJCF importer may store them in radians; add an mjcf_value_transformer for
dof_ref (and similarly for dof_springref) in the MJCF import mapping so that
when the MJCF compiler has angle="radian" the transformer converts radians to
degrees at parse time; ensure the importer writes normalized degree values into
joint_ref (the array read by solver_mujoco.py where joint_ref, joint_qd_start
and JointType.REVOLUTE are used) so the existing undo logic (ref_rad = ref_value
* pi/180) remains correct.
In @newton/tests/test_mujoco_solver.py:
- Around line 4077-4140: The test test_ref_fk_matches_mujoco unconditionally
imports mujoco which hard-fails when the optional dependency is missing; wrap
the import in a pytest.importorskip("mujoco") or a try/except that calls
pytest.skip("requires mujoco") so the test is skipped if mujoco is not
installed, and then tighten the numeric tolerances by changing the position
assert atol to 1e-5 and the quaternion distance threshold from 0.01 to 1e-5
(update the assertions in test_ref_fk_matches_mujoco and the import statement at
the top of that test).
🧹 Nitpick comments (4)
newton/tests/test_import_usd.py (2)
2526-2582: Remove unusednoqaand consider asserting the baked transform (not justdof_ref).
Ruff flags the# noqa: PLC0415as unused on the local import. Also, if the importer now bakesrefinto joint frames for single-DOF joints, an additional assertion onmodel.joint_X_c(or FK result) would better lock in behavior.
2583-2679: Remove unusednoqa; consider settingphysics:axisfor the revolute joint explicitly.
This keeps the test resilient if USD defaults change, and addresses Ruff’s unused# noqa: PLC0415warning.newton/_src/solvers/mujoco/solver_mujoco.py (1)
1610-1613: Propagation is fine; consider only emittingref/springrefwhen non-zero.Right now
joint_params["ref"]/["springref"]are set whenever the arrays exist, even for zeros. It’s probably harmless, but skipping zeros keeps MuJoCo defaults/“unset” semantics intact and reduces spec noise.Also applies to: 1696-1700
newton/tests/test_import_mjcf.py (1)
1668-1731: Add tests for<compiler angle="radian">and non-1.0scaleto prevent regressions.The current tests only validate the default
angle="degree"MJCF path. The code has a latent bug:
- Import bakes
refintochild_xformand stores it as a custom attribute (in degrees for REVOLUTE whenuse_degrees=True)- Export assumes
dof_refvalues are always in degrees (seeref_rad = float(ref_value) * np.pi / 180.0insolver_mujoco.py)- If
<compiler angle="radian">is used, REVOLUTErefwould be incorrectly double-converted, and PRISMATICrefwould not be converted at all, breaking round-trip exportAdd:
- A hinge
ref/springreftest with<compiler angle="radian">to confirm stored values and export units match- A prismatic
reftest with non-1.0 importscale(if scaling should affect translational joint quantities) to catch scale-related regressions
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.pynewton/tests/test_mujoco_solver.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/utils/import_usd.py
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
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.
📚 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_import_usd.pynewton/_src/solvers/mujoco/solver_mujoco.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_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/tests/test_import_usd.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/tests/test_import_usd.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
newton/tests/test_import_usd.pynewton/_src/utils/import_mjcf.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_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/tests/test_import_usd.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-12-12T16:25:54.685Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/core/math.py:248-255
Timestamp: 2025-12-12T16:25:54.685Z
Learning: In newton/_src/solvers/kamino/core/math.py, the quat_conj function's current implementation (negating w instead of x,y,z) is a known issue that will be revised in a future PR per vastsoun's decision, not to be flagged in current Kamino reviews.
Applied to files:
newton/tests/test_import_usd.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/solver_mujoco.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/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_mujoco_solver.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/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.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/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.py
📚 Learning: 2025-12-01T16:21:36.581Z
Learnt from: jvonmuralt
Repo: newton-physics/newton PR: 1160
File: newton/_src/sim/articulation.py:0-0
Timestamp: 2025-12-01T16:21:36.581Z
Learning: In Newton's Featherstone solver, during forward kinematics for FREE/DISTANCE joints, the computed spatial velocity v_wc follows Featherstone's spatial twist convention where the linear component (spatial_top) represents the velocity at the world origin (0,0,0), not at the joint anchor or COM. To convert to State.body_qd (which stores COM velocity), use v_com = v_origin + ω × x_com, where x_com is the absolute world position of the COM.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/_src/utils/import_mjcf.pynewton/tests/test_mujoco_solver.py
🧬 Code graph analysis (3)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-72)ModelAttributeAssignment(31-46)
newton/tests/test_import_mjcf.py (2)
newton/_src/sim/builder.py (1)
ModelBuilder(70-6449)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(82-2647)register_custom_attributes(161-354)
newton/tests/test_mujoco_solver.py (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(161-354)newton/_src/solvers/solver.py (1)
register_custom_attributes(307-314)
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2529-2529: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2586-2586: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
newton/tests/test_mujoco_solver.py
4079-4079: 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). (4)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (2)
newton/tests/test_mujoco_solver.py (1)
3974-3983: MJCF joint type change tohingelooks correct for MuJoCo XML.
This aligns with MuJoCo’s joint taxonomy and should make the fixture less ambiguous.newton/_src/solvers/mujoco/solver_mujoco.py (1)
1167-1168: LGTM: cleanly threaded custom attrs into conversion path.
joint_springref/joint_refare pulled via the existing helper and stay optional when the namespace isn’t present.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/_src/solvers/mujoco/kernels.py (1)
1128-1204:update_joint_transforms_kernel: good “ref un-baking” direction; please de-magicjtypeand verifywp.radians+dof_refunits.
- Consider defining local constants for
mjJNT_*(0/2/3) to avoid magic numbers.- Please verify
warp-langsupportswp.radiansin your pinned version; otherwise useref_rad = ref_value * (wp.pi / 180.0).- Please confirm the unit contract matches the import/parsing path: hinge
refin degrees, sliderefin linear units.Suggested readability + robustness tweak
def update_joint_transforms_kernel( @@ ): + MJJNT_FREE = 0 + MJJNT_SLIDE = 2 + MJJNT_HINGE = 3 @@ - if jtype == 0: # mjJNT_FREE + if jtype == MJJNT_FREE: return @@ - if jtype == 3: # mjJNT_HINGE (revolute) + if jtype == MJJNT_HINGE: # Remove ref rotation (dof_ref is in degrees, convert to radians) - ref_rad = wp.radians(ref_value) + ref_rad = ref_value * (wp.pi / 180.0) ref_quat_inv = wp.quat_from_axis_angle(axis, -ref_rad) mjc_child_xform = wp.transform(child_xform.p, child_xform.q * ref_quat_inv) - elif jtype == 2: # mjJNT_SLIDE (prismatic) + elif jtype == MJJNT_SLIDE: # Remove ref translation mjc_child_xform = wp.transform(child_xform.p - ref_value * axis, child_xform.q)Based on learnings, using
mjc_jnt_to_newton_dofhere is the right mapping direction for per-DOF values.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
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.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-11-28T11:12:40.805Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1146
File: newton/examples/basic/example_basic_joints.py:206-213
Timestamp: 2025-11-28T11:12:40.805Z
Learning: In Newton's standard solvers (XPBD, SemiImplicit, SolverMuJoCo), spatial vectors in State.body_qd use the ordering (linear, angular): wp.spatial_top(qd) returns linear velocity and wp.spatial_bottom(qd) returns angular velocity. This is the opposite of typical Modern Robotics spatial twist notation where (angular, linear) is used.
Applied to files:
newton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-12-01T16:21:36.581Z
Learnt from: jvonmuralt
Repo: newton-physics/newton PR: 1160
File: newton/_src/sim/articulation.py:0-0
Timestamp: 2025-12-01T16:21:36.581Z
Learning: In Newton's Featherstone solver, during forward kinematics for FREE/DISTANCE joints, the computed spatial velocity v_wc follows Featherstone's spatial twist convention where the linear component (spatial_top) represents the velocity at the world origin (0,0,0), not at the joint anchor or COM. To convert to State.body_qd (which stores COM velocity), use v_com = v_origin + ω × x_com, where x_com is the absolute world position of the COM.
Applied to files:
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/kernels.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/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.py
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
newton/tests/test_mujoco_solver.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/tests/test_mujoco_solver.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/tests/test_mujoco_solver.py
🧬 Code graph analysis (2)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(677-722)ModelBuilder(70-6449)CustomAttribute(352-442)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-72)ModelAttributeAssignment(31-46)
newton/tests/test_mujoco_solver.py (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(161-354)newton/_src/sim/articulation.py (1)
eval_fk(414-471)
🪛 Ruff (0.14.10)
newton/tests/test_mujoco_solver.py
4079-4079: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
4098-4098: 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). (4)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (7)
newton/tests/test_mujoco_solver.py (1)
3970-3987: MJCF snippet: switchingjoint typeto"hinge"is the right semantic for MJCF parsing.newton/_src/solvers/mujoco/solver_mujoco.py (6)
307-330: LGTM!The new
dof_springrefanddof_refcustom attribute declarations follow the established pattern for MuJoCo-specific attributes. TheJOINT_DOFfrequency is correct for per-DOF properties, and the naming conventions (mjc:prefix for USD, base names for MJCF) are consistent with other attributes in this file. Based on learnings, each Newton DOF maps to a distinct MuJoCo joint, so per-DOF attributes are appropriate here.
1167-1168: LGTM!The attribute retrieval follows the established pattern used for other custom attributes in this function. The
joint_prefix naming is consistent with similar retrievals above (e.g.,joint_stiffnessfordof_passive_stiffness).
1609-1614: LGTM!The propagation of
springrefandrefto linear DOF joint parameters follows the established pattern for other optional parameters and uses the correct DOF indexai.
1696-1700: LGTM!The propagation of
springrefandrefto angular DOF joint parameters mirrors the linear DOF handling and uses correct indexing.
2324-2342: Code change is complete and correct.The kernel signature in
kernels.pyhas been properly updated to accept thenewton_dof_refparameter at line 1138, matching the parameter passed in the kernel launch at line 2342. The dof_ref attribute is correctly retrieved and passed through, ensuring consistency with the initial conversion logic.
1443-1462: No action required. D6 joints withrefattributes are correctly handled.The ref handling distinguishes between single-DOF and multi-DOF joints by design: only REVOLUTE and PRISMATIC joints have ref baked into
child_xformduring import and subsequently undone in the solver (lines 1451-1462). D6 joints do not undergo this bake/undo pattern because ref is never baked into theirchild_xformduring import. Instead, ref is applied directly to each individual MuJoCo joint when D6 DOFs are decomposed—see lines 1613 (linear DOFs) and 1699 (angular DOFs) wherejoint_params["ref"] = joint_ref[ai]is set for each DOF's corresponding MuJoCo joint. This approach is correct and consistent with the DOF-to-MuJoCo-joint mapping strategy.Likely an incorrect or invalid review comment.
Contributor
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/solvers/mujoco/kernels.py (1)
1128-1204: Fix prismatic ref "unbaking" to rotate axis into body frame.The
newton_joint_axisis stored in joint local frame (per builder.py), but when subtracting the ref offset fromchild_xform.p(which is in body coordinates), the axis must be rotated into the body frame first. This is only correct by accident whenchild_xform.qis identity.Proposed fix
if newton_dof_ref and newton_dof >= 0: ref_value = newton_dof_ref[newton_dof] if ref_value != 0.0: axis = newton_joint_axis[newton_dof] if jtype == 3: # mjJNT_HINGE (revolute) # Remove ref rotation (dof_ref is in degrees, convert to radians) ref_rad = wp.radians(ref_value) ref_quat_inv = wp.quat_from_axis_angle(axis, -ref_rad) mjc_child_xform = wp.transform(child_xform.p, child_xform.q * ref_quat_inv) elif jtype == 2: # mjJNT_SLIDE (prismatic) # Remove ref translation - mjc_child_xform = wp.transform(child_xform.p - ref_value * axis, child_xform.q) + axis_body = wp.quat_rotate(child_xform.q, axis) + mjc_child_xform = wp.transform(child_xform.p - ref_value * axis_body, child_xform.q)
🤖 Fix all issues with AI agents
In @newton/tests/test_mujoco_solver.py:
- Around line 4077-4140: Wrap the optional mujoco_warp import and
SolverMuJoCo(model) creation in try/except blocks that call self.skipTest(...)
on ImportError (matching existing patterns), and replace bare Python asserts
with test assertions: use np.testing.assert_allclose for position checks and
self.assertLess(quat_dist, 0.01, msg) for quaternion checks; refer to symbols
test_ref_fk_matches_mujoco, mujoco_warp, SolverMuJoCo,
np.testing.assert_allclose, and self.assertLess when making these changes.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
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.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-11-28T11:12:40.805Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1146
File: newton/examples/basic/example_basic_joints.py:206-213
Timestamp: 2025-11-28T11:12:40.805Z
Learning: In Newton's standard solvers (XPBD, SemiImplicit, SolverMuJoCo), spatial vectors in State.body_qd use the ordering (linear, angular): wp.spatial_top(qd) returns linear velocity and wp.spatial_bottom(qd) returns angular velocity. This is the opposite of typical Modern Robotics spatial twist notation where (angular, linear) is used.
Applied to files:
newton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-12-01T16:21:36.581Z
Learnt from: jvonmuralt
Repo: newton-physics/newton PR: 1160
File: newton/_src/sim/articulation.py:0-0
Timestamp: 2025-12-01T16:21:36.581Z
Learning: In Newton's Featherstone solver, during forward kinematics for FREE/DISTANCE joints, the computed spatial velocity v_wc follows Featherstone's spatial twist convention where the linear component (spatial_top) represents the velocity at the world origin (0,0,0), not at the joint anchor or COM. To convert to State.body_qd (which stores COM velocity), use v_com = v_origin + ω × x_com, where x_com is the absolute world position of the COM.
Applied to files:
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/kernels.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.py
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
newton/tests/test_mujoco_solver.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/tests/test_mujoco_solver.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/tests/test_mujoco_solver.py
🧬 Code graph analysis (1)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(677-722)ModelBuilder(70-6449)CustomAttribute(352-442)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-72)ModelAttributeAssignment(31-46)
🪛 Ruff (0.14.10)
newton/tests/test_mujoco_solver.py
4079-4079: 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). (3)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (6)
newton/tests/test_mujoco_solver.py (1)
3974-3983: MJCF joint type correction tohingeis appropriate.This matches MuJoCo’s MJCF joint type naming and should make the test input less ambiguous.
newton/_src/solvers/mujoco/solver_mujoco.py (5)
307-330: LGTM! New custom attributes follow established patterns.The
dof_springrefanddof_refattributes are correctly defined withJOINT_DOFfrequency, appropriate defaults, and consistent naming conventions matching other per-DOF attributes in this file.
1167-1168: LGTM!Attribute retrieval follows the established pattern used for other custom attributes.
1609-1614: LGTM!The
springrefandrefparameters are correctly passed to linear joint parameters using the appropriate DOF index.
1696-1700: LGTM!The
springrefandrefparameters are correctly passed to angular joint parameters. The code assumes MuJoCo expects degrees for angular ref values, which aligns with MuJoCo's convention.
2324-2342: Correctly passesdof_refto the joint transform update kernel.The retrieval pattern is consistent with other mujoco attributes in the file, and passing
dof_refto the kernel enables proper ref offset removal during dynamic joint property updates.
Member
Author
I used a flag to rely on mjc's FK when ref is in use. This solves the issue (I think), but let me know if I'm missing something or if there are reasons why we should not use mjc's FK. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/tests/test_mujoco_solver.py (1)
4077-4144: Well-structured test for ref-enabled FK validation.The test correctly validates that when
refis used on joints, Newton's body state matches MuJoCo's forward kinematics results. The quaternion comparison properly handles sign ambiguity.A couple of minor observations:
Static analysis note (line 4083): The
# noqa: PLC0415is flagged as unused since that rule isn't enabled. However, this pattern is consistent with other local imports in the file (lines 3236, 4047, 4150), so keeping it for consistency is acceptable.Style consideration: Using bare
assertstatements instead ofself.assert*methods works but provides less informative failure messages in unittest. Consider usingself.assertTrue(...)for consistency with unittest idioms. This is optional since both patterns exist elsewhere in the file.Optional: Use unittest assertion methods for consistency
- assert solver._has_ref, "Solver should detect that ref is used" + self.assertTrue(solver._has_ref, "Solver should detect that ref is used") ... - assert np.allclose(newton_pos, mj_pos, atol=0.01), ( - f"Position mismatch for {body_name}: Newton={newton_pos}, MuJoCo={mj_pos}" - ) + np.testing.assert_allclose( + newton_pos, mj_pos, atol=0.01, + err_msg=f"Position mismatch for {body_name}: Newton={newton_pos}, MuJoCo={mj_pos}" + ) ... - assert quat_dist < 0.01, f"Quaternion mismatch for {body_name}: Newton={newton_quat}, MuJoCo={mj_quat}" + self.assertLess( + quat_dist, 0.01, + msg=f"Quaternion mismatch for {body_name}: Newton={newton_quat}, MuJoCo={mj_quat}" + )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_mujoco_solver.py
💤 Files with no reviewable changes (1)
- newton/_src/utils/import_mjcf.py
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
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.
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.
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.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.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/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-10-30T07:28:13.112Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 984
File: newton/_src/geometry/broad_phase_nxn.py:318-336
Timestamp: 2025-10-30T07:28:13.112Z
Learning: In Newton's BroadPhaseAllPairs class (newton/_src/geometry/broad_phase_nxn.py), device management is the caller's responsibility. The library intentionally does not perform automatic device transfers for precomputed arrays in the launch method, even if there's a device mismatch. Users must ensure that the device specified in __init__ matches the device used in launch.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_mujoco_solver.py
📚 Learning: 2025-12-12T15:23:30.014Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_boxes_fourbar.py:149-163
Timestamp: 2025-12-12T15:23:30.014Z
Learning: In Kamino simulation examples under newton/_src/solvers/kamino/examples/sim/, the pattern where logging=True and use_cuda_graph=True creates a mismatch (SimulationLogger constructed but settings.compute_metrics and logger.log() calls guarded by `if not self.use_cuda_graph`) should not be flagged. These examples will be refactored in a future commit, and this pattern is intentionally deferred.
Applied to files:
newton/tests/test_mujoco_solver.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/tests/test_mujoco_solver.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/tests/test_mujoco_solver.py
🪛 Ruff (0.14.10)
newton/tests/test_mujoco_solver.py
4083-4083: 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 / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (8)
newton/tests/test_mujoco_solver.py (1)
3974-3983: LGTM - Correct MJCF joint type terminology.The change from
type="revolute"totype="hinge"is correct. MJCF uses "hinge" for single-axis rotational joints, while "revolute" is URDF terminology.newton/_src/solvers/mujoco/solver_mujoco.py (7)
307-330: LGTM!The custom attribute registration for
dof_springrefanddof_reffollows the established pattern and is consistent with other DOF-level attributes likedof_passive_stiffnessanddof_passive_damping.
505-509: LGTM!The
_has_refdetection logic correctly identifies when ref is actively used (non-zero values) and appropriately gates the FK evaluation path.
521-544: LGTM!The conditional FK evaluation correctly handles the
refattribute. Whenrefis used, MuJoCo internally shifts joint positions viaqpos0, so using MuJoCo's FK (eval_fk=False) ensures body transforms are consistent with the reference position. Both CPU and Warp paths are updated consistently.
1176-1177: LGTM!The attribute extraction follows the established pattern for DOF-level attributes.
1600-1605: LGTM!The
springrefandrefparameters are correctly propagated to linear DOF joints using the axis indexai, following the established pattern for optional joint parameters.
2315-2315: No functional change.This appears to be a whitespace-only modification.
1687-1691: No changes needed. The code correctly handlesspringreffor angular DOFs. Unlikerangewhich Newton stores internally in radians (requiring conversion vianp.rad2deg), Newton storesspringrefin degrees. Since MuJoCo's compiler angle setting defaults to "degree", writingspringrefdirectly without conversion is correct.
Member
|
I can't follow why this would affect FK. After Newton FK, we are still in newton land so joint_q is still uncorrected. We only have to add ref to joint_q in update_mjc_data and subtract it in update_newton_state? Because the actual joint_q is always constructed from parent/child transforms? That being said, I'm not sure why we are even adding ref at this point because it's seems like a pure algebraic conversion that doesn't really give us anything in newton land, at least if we don't have the corresponding concept ourselves. Of course the springref is different in that regard. |
Member
Author
Yes, adding and substraction would be an equivalent solution, just that needs to be done every step. I agree that it is not a useful for newton attribute, but we still need to support it to enable use of existing mjcf assets etc? Maybe it would be better to account for ref during parsing the asset and always build mujoco model with default 0 ref? |
Member
|
ref 0 is already the default, right? So you basically mean changing things such that newtons joint_q == 0 is at ref? I need to look at the code, that would mean that we need to setup the newton model with nonzero joint positions. I hope that's possible. And then we need to be careful when setting up the MuJoCo model. I still think I don't have a good enough understanding of this, maybe we need to go through a few examples to guide the design? |
adenzler-nvidia
approved these changes
Jan 12, 2026
Member
adenzler-nvidia
left a comment
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Thanks! Let's use this and see how far we get.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
dof_refanddof_springrefjoint attributes, enabling users to set reference positions and spring reference values for individual joints. The attribute ref is now parsed and used as a custom attribute, FK relies on mujoco. If problems will be detected we can switch to an appoach: adjusting parsing so that we "normalize" a given mjcf to ref = 0. This would imply adjusting the child body transforms and then we would not need this custom attribute at all and can just rely on the default value.Tests
✏️ Tip: You can customize this high-level summary in your review settings.