Add passive stiffness and damping support for joints in MuJoCo solver#1082
Conversation
📝 WalkthroughWalkthroughAdds per-joint passive stiffness and per-DOF passive damping support end-to-end: MJCF/USD import and actuator parsing read gains/custom attributes; SolverMuJoCo registers and propagates them into expanded world fields; the Warp kernel conditionally writes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Import as MJCF/USD Import
participant Parser as parse_actuators
participant CA as CustomAttrs (mujoco:stiffness/damping)
participant Solver as SolverMuJoCo
participant Kernel as update_joint_dof_properties_kernel
participant MJ as MuJoCo Model
Import->>Parser: parse actuator blocks -> per-DOF target_ke/target_kd
Import->>CA: read/register joint stiffness & damping attributes
CA->>Solver: attach joint_stiffness / joint_damping to joint params
Solver->>Kernel: pass joint_stiffness, joint_damping, dof fields, solimp/solref, limits
note right of Kernel `#DDFFDD`: kernel conditionally writes per-DOF dof_damping and per-joint jnt_stiffness
Kernel->>MJ: write jnt_stiffness, dof_damping, armature, friction, solimp/solref, limits into MuJoCo model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (11)📓 Common learnings📚 Learning: 2025-09-22T21:03:39.624ZApplied to files:
📚 Learning: 2025-11-24T08:05:21.390ZApplied to files:
📚 Learning: 2025-11-07T01:42:36.906ZApplied to files:
📚 Learning: 2025-08-12T18:07:17.267ZApplied to files:
📚 Learning: 2025-09-22T21:03:18.367ZApplied to files:
📚 Learning: 2025-08-18T15:56:26.587ZApplied to files:
📚 Learning: 2025-08-19T12:27:30.629ZApplied to files:
📚 Learning: 2025-08-19T12:27:30.629ZApplied to files:
📚 Learning: 2025-09-22T21:08:31.901ZApplied to files:
📚 Learning: 2025-07-21T19:11:04.077ZApplied to files:
🧬 Code graph analysis (3)newton/tests/test_import_usd.py (1)
newton/tests/test_import_mjcf.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
🪛 Ruff (0.14.6)newton/tests/test_import_usd.py1466-1466: Unused Remove unused (RUF100) 1596-1596: Unused Remove unused (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)
🔇 Additional comments (6)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/utils/import_mjcf.py (1)
549-550: Clarify the relationship between default joint stiffness/damping and passive stiffness/damping.The change from parsing MJCF
stiffness/dampingattributes to usingdefault_joint_stiffness/default_joint_dampingmay be confusing. Based on the AI summary and related changes insolver_mujoco.py, it appears:
- MJCF
stiffness/dampingattributes are now handled as passive joint properties (stored in custom MuJoCo attributes)target_ke/target_kdare actuator gains (for active control)However, the comment and variable names don't clearly distinguish these concepts. Consider:
- Adding a comment explaining that target_ke/target_kd are actuator gains, not passive properties
- Verifying that the MJCF
stiffness/dampingattributes are still being parsed into the custom MuJoCo attributes (via themjcf_attribute_namemechanism inregister_custom_attributes)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/solvers/mujoco/kernels.py(2 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(8 hunks)newton/_src/utils/import_mjcf.py(2 hunks)newton/tests/test_import_mjcf.py(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 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: 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: 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: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-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-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-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.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_import_mjcf.pynewton/_src/solvers/mujoco/solver_mujoco.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_import_mjcf.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (2)
newton/tests/test_import_mjcf.py (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(152-192)newton/_src/sim/builder.py (2)
ModelBuilder(70-5101)add_mjcf(1131-1215)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
newton/_src/solvers/mujoco/kernels.py (2)
update_joint_stiffness_kernel(932-945)update_dof_properties_kernel(906-928)newton/_src/sim/builder.py (2)
add_custom_attribute(584-629)CustomAttribute(268-358)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-70)ModelAttributeAssignment(31-46)
⏰ 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 (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (9)
newton/_src/utils/import_mjcf.py (1)
950-1002: LGTM! Actuator parsing is well-structured.The new
parse_actuatorsfunction correctly:
- Handles position actuators by setting both
target_ke(kp) andtarget_kd(kv)- Handles velocity actuators by setting
target_kd(kv)- Validates joint names and provides helpful warnings
- Applies gains to all DOFs of a joint
The integration into the main parse flow is appropriate.
newton/tests/test_import_mjcf.py (1)
704-846: Comprehensive test validates the passive stiffness/damping feature.The test correctly:
- Registers custom MuJoCo attributes before parsing MJCF
- Validates that passive stiffness/damping are parsed into
model.mujoco.stiffnessandmodel.mujoco.damping- Verifies actuator gains (target_ke/target_kd) are set correctly from MJCF
<actuator>section- Checks MuJoCo model internals (dof_damping, jnt_stiffness, actuator gains/biases)
- Tests runtime updates via
notify_model_changedThe test thoroughly covers the integration points between Newton, MJCF parsing, and MuJoCo solver.
newton/_src/solvers/mujoco/kernels.py (2)
905-929: LGTM! DOF properties kernel correctly extended for passive damping.The kernel now:
- Accepts
joint_dampingas an additional input- Writes to
dof_dampingoutput array- Maintains consistent indexing with existing armature and friction updates
- Has an updated docstring
The implementation is straightforward and follows the existing pattern.
931-946: LGTM! Joint stiffness kernel correctly maps per-joint stiffness to MuJoCo DOFs.The kernel:
- Uses
joint_qd_startto locate the first DOF of each joint- Maps to MuJoCo joint index via
joint_mjc_dof_start- Only updates when a valid MuJoCo joint index exists (>= 0 check)
- Handles per-world replication correctly
The implementation is consistent with the existing kernel patterns in this file.
newton/_src/solvers/mujoco/solver_mujoco.py (5)
169-192: LGTM! Custom attributes properly registered for passive stiffness and damping.The custom attributes:
- Use correct frequency (JOINT_DOF)
- Have appropriate defaults (0.0 for both)
- Leverage MuJoCo USD schema naming ("mjc:*")
- Map to MJCF attribute names for parsing
This enables the MJCF importer to automatically populate these attributes via the
mjcf_attribute_namemechanism.
969-970: LGTM! Stiffness and damping attributes correctly extracted.The extraction uses the helper function
get_custom_attributewhich safely handles cases where the attributes may not exist.
1239-1242: LGTM! Passive stiffness and damping correctly set during joint creation.The code:
- Checks if the attributes exist before setting them
- Applies them to both linear and angular DOFs
- Uses the correct array indexing (per-DOF)
Also applies to: 1303-1306
1558-1558: LGTM! Stiffness and damping fields correctly added to expansion list.The fields
jnt_stiffnessanddof_dampingare properly included in the list of fields to expand when replicating across multiple worlds.Also applies to: 1563-1563
1675-1742: LGTM! Joint DOF properties update correctly handles passive stiffness and damping.The implementation:
- Updates actuator properties (force ranges, gains) as before
- Extracts stiffness/damping from custom attributes with safe fallbacks
- Launches
update_dof_properties_kernelwith damping input- Launches new
update_joint_stiffness_kernelto populatejnt_stiffnessThe logic is well-structured and handles missing attributes gracefully by falling back to zero-filled arrays.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/utils/import_mjcf.py(3 hunks)newton/tests/test_import_mjcf.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/utils/import_mjcf.py
🧰 Additional context used
🧠 Learnings (3)
📓 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: 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, 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_import_mjcf.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_import_mjcf.py
🧬 Code graph analysis (1)
newton/tests/test_import_mjcf.py (3)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
register_custom_attributes(152-192)notify_model_changed(396-406)newton/_src/sim/builder.py (2)
ModelBuilder(70-5101)finalize(4609-4997)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-56)
⏰ 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 (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/solver_mujoco.py(8 hunks)newton/tests/test_import_mjcf.py(2 hunks)newton/tests/test_import_usd.py(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 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: 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, 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_import_mjcf.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_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.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_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/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/tests/test_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/tests/test_import_mjcf.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_import_mjcf.py
🧬 Code graph analysis (3)
newton/tests/test_import_usd.py (3)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
register_custom_attributes(152-194)notify_model_changed(398-408)newton/_src/sim/builder.py (1)
ModelBuilder(70-5101)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-56)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
newton/_src/solvers/mujoco/kernels.py (2)
update_joint_stiffness_kernel(932-945)update_dof_properties_kernel(906-928)newton/_src/sim/builder.py (3)
add_custom_attribute(584-629)ModelBuilder(70-5101)CustomAttribute(268-358)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-70)ModelAttributeAssignment(31-46)
newton/tests/test_import_mjcf.py (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(152-194)newton/_src/sim/builder.py (2)
ModelBuilder(70-5101)add_mjcf(1131-1215)
⏰ 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 (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (4)
newton/tests/test_import_usd.py (1)
1271-1280: Correct indexing ofjnt_stiffnessusing MuJoCo joint index.This test correctly uses
mjc_joint_idx(line 1279) to indexjnt_stiffness, which is the proper MuJoCo joint index obtained fromjoint_mjc_dof_start. This is the correct and robust approach for accessing MuJoCo's per-joint arrays.newton/_src/solvers/mujoco/solver_mujoco.py (2)
169-194: Custom attribute registration for passive stiffness and damping looks good.The registration correctly uses
JOINT_DOFfrequency, sets appropriate defaults (0.0), and provides both USD and MJCF attribute name mappings. Theusd_value_transformerlambda ensures scalar values are converted to lists as expected by the USD parser.
1704-1744: Safe fallback handling and correct kernel integration.The code properly handles missing custom attributes by creating zero arrays as fallbacks (lines 1704-1713), ensuring the solver works even when stiffness/damping are not specified. The kernel launches correctly pass
joint_dampingtoupdate_dof_properties_kerneland launch the newupdate_joint_stiffness_kernelwith appropriate inputs includingjoint_qd_startandjoint_mjc_dof_startfor proper index mapping.newton/tests/test_import_mjcf.py (1)
863-867: Same indexing inconsistency in the dynamic update verification.Line 867 also uses
joint_idxdirectly to indexjnt_stiffness, but should use the MuJoCo joint index for consistency and correctness.Apply this diff:
+ mjc_joint_idx = joint_mjc_dof_start[joint1_idx] updated_dof_damping = mjw_model.dof_damping.numpy()[0, mjc_dof_idx] self.assertAlmostEqual(updated_dof_damping, 1.5, places=4) - self.assertAlmostEqual(mjw_model.jnt_stiffness.numpy()[0, joint1_idx], 0.15, places=4) + self.assertAlmostEqual(mjw_model.jnt_stiffness.numpy()[0, mjc_joint_idx], 0.15, places=4)⛔ Skipped due to learnings
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.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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/tests/test_import_mjcf.py (1)
1018-1020: Indexingjnt_stiffnesswithjoint_idxworks for this test but may be fragile.The test uses
joint_idx(Newton joint index) to indexmjw_model.jnt_stiffness, while the USD test at line 1623 usesmjc_joint_idx. For single-DOF revolute joints withseparate_worlds=False, these indices coincide, so the test passes. However, usingmjc_joint_idxwould be more consistent with the USD test and more robust if the test evolves to include multi-DOF joints.- self.assertAlmostEqual( - mjw_model.jnt_stiffness.numpy()[0, joint_idx], expected["jnt_stiffness"], places=4 - ) + self.assertAlmostEqual( + mjw_model.jnt_stiffness.numpy()[0, mjc_dof_idx], expected["jnt_stiffness"], places=4 + )newton/_src/solvers/mujoco/solver_mujoco.py (1)
191-212: Consider adding MJCF attribute names for import support.The new
stiffnessanddampingcustom attributes are registered withusd_attribute_namebut lackmjcf_attribute_name, unlikelimit_margin(line 177) which hasmjcf_attribute_name="margin". If importing these properties from MJCF files is intended, consider adding:builder.add_custom_attribute( ModelBuilder.CustomAttribute( name="stiffness", frequency=ModelAttributeFrequency.JOINT_DOF, assignment=ModelAttributeAssignment.MODEL, dtype=wp.float32, default=0.0, namespace="mujoco", - usd_attribute_name="mjc:stiffness" + usd_attribute_name="mjc:stiffness", + mjcf_attribute_name="stiffness" ) ) builder.add_custom_attribute( ModelBuilder.CustomAttribute( name="damping", frequency=ModelAttributeFrequency.JOINT_DOF, assignment=ModelAttributeAssignment.MODEL, dtype=wp.float32, default=0.0, namespace="mujoco", - usd_attribute_name="mjc:damping" + usd_attribute_name="mjc:damping", + mjcf_attribute_name="damping" ) )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/solvers/mujoco/kernels.py(5 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(9 hunks)newton/_src/utils/import_mjcf.py(3 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 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.
📚 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/kernels.pynewton/tests/test_import_mjcf.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/solvers/mujoco/kernels.pynewton/tests/test_import_mjcf.pynewton/_src/solvers/mujoco/solver_mujoco.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/solvers/mujoco/kernels.pynewton/tests/test_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/kernels.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/solvers/mujoco/kernels.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/tests/test_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
🧬 Code graph analysis (1)
newton/tests/test_import_mjcf.py (3)
newton/_src/sim/builder.py (2)
ModelBuilder(70-5401)copy(231-232)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(151-212)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-56)
⏰ 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 (14)
newton/_src/utils/import_mjcf.py (2)
117-118: LGTM: Variable renaming fromdefault_joint_stiffness/dampingtodefault_joint_target_ke/kd.The naming change clarifies these are active control gains (target stiffness/damping for actuators) rather than passive joint properties, which aligns with the new distinction between passive joint stiffness/damping and actuator-driven target gains.
1059-1111: LGTM:parse_actuatorsfunction correctly extracts actuator gains.The implementation properly:
- Parses position actuators (kp → target_ke, kv → target_kd)
- Parses velocity actuators (kv → target_kd, overwriting position actuator's kv when both exist)
- Applies gains per-DOF for multi-DOF joints
- Warns when actuators reference unknown joints
The processing order (position before velocity) means velocity actuator's
kvtakes precedence fortarget_kd, which appears intentional per the test expectations.newton/tests/test_import_mjcf.py (2)
909-973: Comprehensive test coverage for stiffness/damping parsing and propagation.The test thoroughly validates:
- Parsing of
stiffnessanddampingMJCF joint attributes- Actuator
kp/kvextraction intotarget_ke/target_kd- Correct handling of joints with only velocity actuators (joint4)
- Interaction between position and velocity actuators on the same joint (joint1)
1055-1072: LGTM: Runtime property mutation and propagation test.The test correctly validates that updating
model.mujoco.stiffnessandmodel.mujoco.dampingfollowed bynotify_model_changed(JOINT_DOF_PROPERTIES)propagates the changes to the MuJoCo solver's internal arrays.newton/_src/solvers/mujoco/kernels.py (3)
918-931: LGTM: Extended kernel signature for stiffness/damping support.The new inputs (
joint_stiffness,joint_damping) and outputs (jnt_stiffness,dof_damping) are properly typed and positioned. The conditional writes usingif joint_stiffness:andif joint_damping:ensure backward compatibility when these arrays are not provided.
966-981: LGTM: Per-DOF damping and per-joint stiffness updates for linear axes.The implementation correctly:
- Writes damping per-DOF to
dof_damping[worldid, mjc_dof_index]- Writes stiffness per-joint to
jnt_stiffness[worldid, mjc_joint_index]This follows the Newton-MuJoCo mapping where each DOF maps to a distinct MuJoCo joint.
1000-1014: LGTM: Per-DOF damping and per-joint stiffness updates for angular axes.The angular DOF handling mirrors the linear DOF logic, maintaining consistent behavior across axis types.
newton/tests/test_import_usd.py (3)
1425-1586: Comprehensive USD test for joint stiffness/damping parsing.The test properly validates:
- Parsing
mjc:stiffnessandmjc:dampingUSD attributes- USD drive gains (
drive:angular:physics:stiffness/damping) scaled bymath.degrees(1.0)- Correct handling of joints with drives vs without (Joint3 has no drive)
- Integration with
SolverMuJoCocustom attributes
1615-1652: LGTM: MuJoCo solver integration verification.The test correctly uses
mjc_joint_idx(derived fromjoint_mjc_dof_start) to indexjnt_stiffness, which is more robust than using the Newton joint index directly. This is the preferred pattern for accessing MuJoCo per-joint arrays.
1654-1672: LGTM: Runtime property mutation test matches MJCF test pattern.The test verifies that updating stiffness/damping in the Newton model and notifying the solver correctly propagates the changes to MuJoCo arrays.
newton/_src/solvers/mujoco/solver_mujoco.py (4)
989-990: LGTM!The attribute retrieval follows the established pattern and correctly handles the case where attributes may not be present.
1331-1334: LGTM!The stiffness and damping values are correctly propagated to MuJoCo joint parameters, with proper null checks and per-DOF indexing. The implementation is consistent across both linear and angular DOFs.
Also applies to: 1404-1407
1793-1858: LGTM!The function correctly retrieves stiffness and damping from the model's custom attributes, passes them to the update kernel, and writes the results to the appropriate MuJoCo model fields. The docstring accurately reflects the new functionality.
1676-1676: Manual verification required — repository access unavailable.Unfortunately, I cannot execute the verification scripts due to repository access issues. The original review comment appropriately requests verification of the index mapping between Newton DOFs and MuJoCo joints for the
jnt_stiffnessfield expansion, based on the established learning that each Newton DOF maps to a distinct MuJoCo joint via thedof_to_mjc_jointarray.The concern is valid: the kernel must correctly use this mapping to write stiffness values to the appropriate MuJoCo joint indices without overwrites for multi-DOF joints. Manual code inspection of
update_joint_dof_properties_kernelis needed to confirm the implementation follows this pattern correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
newton/tests/test_import_mjcf.py (1)
981-1021: Use MuJoCo joint index (not Newton joint index) when assertingjnt_stiffnessafter updatesIn the first half of this test you correctly derive
mjc_joint_idx = joint_mjc_dof_start[joint_idx]and use that to indexmjw_model.jnt_stiffness. After mutatingmodel.mujoco.stiffness/dampingand callingnotify_model_changed, the final assertion switches to indexingjnt_stiffnesswith the Newton joint index (joint1_idx):mjc_dof_idx = joint_mjc_dof_start[joint1_idx] updated_dof_damping = mjw_model.dof_damping.numpy()[0, mjc_dof_idx] self.assertAlmostEqual(updated_dof_damping, 1.5, places=4) self.assertAlmostEqual(mjw_model.jnt_stiffness.numpy()[0, joint1_idx], 0.15, places=4)Given the established mapping where each Newton DOF maps to a MuJoCo joint via
dof_to_mjc_jointandjoint_mjc_dof_start,mjw_model.jnt_stiffnessshould always be indexed with the MuJoCo joint index (mjc_joint_idx), not the Newton joint index. This is what you already do earlier in the test and what the USD test uses as well; the current code will be fragile for multi‑DOF or more complex joint layouts.Please align the final assertion with the earlier pattern:
- mjc_dof_idx = joint_mjc_dof_start[joint1_idx] - - updated_dof_damping = mjw_model.dof_damping.numpy()[0, mjc_dof_idx] - self.assertAlmostEqual(updated_dof_damping, 1.5, places=4) - self.assertAlmostEqual(mjw_model.jnt_stiffness.numpy()[0, joint1_idx], 0.15, places=4) + mjc_joint_idx = joint_mjc_dof_start[joint1_idx] + mjc_dof_idx = mjc_joint_idx + + updated_dof_damping = mjw_model.dof_damping.numpy()[0, mjc_dof_idx] + self.assertAlmostEqual(updated_dof_damping, 1.5, places=4) + self.assertAlmostEqual(mjw_model.jnt_stiffness.numpy()[0, mjc_joint_idx], 0.15, places=4)Also applies to: 1069-1073
🧹 Nitpick comments (1)
newton/tests/test_import_usd.py (1)
1425-1672: USD stiffness/damping test exercises the full pipeline correctlyThis test does a good job validating parsing of
mjc:stiffness/mjc:damping, mapping intomodel.mujoco.*andjoint_target_ke/kd, MuJoCodof_damping/jnt_stiffness, actuator gain/bias parameters, and thenotify_model_changed(SolverNotifyFlags.JOINT_DOF_PROPERTIES)path. Indexing viajoint_mjc_dof_startis consistent with the solver’s DOF→joint mapping.As a small cleanup, you could drop the inner
import mathandfrom newton.solvers import SolverMuJoCohere since both are already imported at the top of the file.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/solvers/mujoco/kernels.py(5 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(9 hunks)newton/_src/utils/import_mjcf.py(3 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/utils/import_mjcf.py
🧰 Additional context used
🧠 Learnings (9)
📓 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.
📚 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/tests/test_import_mjcf.pynewton/_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/kernels.pynewton/tests/test_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/solvers/mujoco/kernels.pynewton/tests/test_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/kernels.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/solvers/mujoco/kernels.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/tests/test_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
🧬 Code graph analysis (2)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(622-667)ModelBuilder(70-5401)CustomAttribute(304-394)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-70)ModelAttributeAssignment(31-46)
newton/tests/test_import_usd.py (3)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
register_custom_attributes(151-212)notify_model_changed(417-427)newton/_src/solvers/solver.py (2)
register_custom_attributes(307-314)notify_model_changed(267-294)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-56)
⏰ 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 (2)
newton/_src/solvers/mujoco/kernels.py (1)
918-929: Passive stiffness/damping propagation in joint DOF kernel looks consistentThe new
joint_stiffness/joint_dampinginputs and correspondingjnt_stiffness/dof_dampingwrites follow the existing solimplimit/limit_margin pattern and respect thedof_to_mjc_jointmapping and world indexing. I don’t see correctness issues in the added logic.Also applies to: 966-983, 1000-1015
newton/_src/solvers/mujoco/solver_mujoco.py (1)
191-212: Stiffness/damping integration through SolverMuJoCo looks coherentThe new mujoco custom attributes (
stiffness,damping), their retrieval in_convert_to_mjc, use inadd_geomsjoint parameter construction, inclusion inexpand_model_fields, and propagation viaupdate_joint_dof_propertiesintomjw_model.jnt_stiffness/dof_dampingare all consistent with the existing solimplimit/limit_margin plumbing and the DOF→MuJoCo joint mapping (dof_to_mjc_joint/joint_mjc_dof_start). I don’t see correctness issues in these additions.Also applies to: 978-991, 1331-1334, 1404-1407, 1660-1682, 1794-1858
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/tests/test_import_usd.py (2)
1560-1575: Optional: Renamedeg_to_rad_scaleto reflect actual meaning
deg_to_rad_scale = math.degrees(1.0)is a radians→degrees factor, but the variable name suggests degrees→radians. To avoid confusion for future readers, consider renaming it, e.g.:- deg_to_rad_scale = math.degrees(1.0) + rad_to_deg_scale = math.degrees(1.0) ... - "target_ke": 10000.0 * deg_to_rad_scale, + "target_ke": 10000.0 * rad_to_deg_scale,(and similarly for other uses).
1591-1651: Optional: Strengthen actuator expectations for joints without drivesThe MuJoCo-side checks are thorough for joints with actuators, and the mapping via
mjc_axis_to_actuator[dof_idx]aligns with the per-DOF mapping used in the MuJoCo integration (per retrieved learnings). For thehas_actuator=Falsecase (Joint3), the test currently only asserts that any accidentally created actuators have zero gain:else: if pos_actuator_idx != -1: ... if vel_actuator_idx != -1: ...If the intended contract is “no actuators at all when there is no drive,” you could tighten this to assert
pos_actuator_idx == -1andvel_actuator_idx == -1, which would catch unexpected actuator creation instead of just non-zero gains.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_import_usd.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 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 (3)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
register_custom_attributes(151-212)notify_model_changed(417-427)newton/_src/solvers/solver.py (2)
register_custom_attributes(307-314)notify_model_changed(267-294)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-56)
🪛 Ruff (0.14.6)
newton/tests/test_import_usd.py
1428-1428: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
1558-1558: 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 (3)
newton/tests/test_import_usd.py (3)
1425-1545: End-to-end USD → Newton → MuJoCo coverage for stiffness/damping looks solidThe in-memory USD fixture with three joints (passive+driven, driven-only, passive-only) plus the call to
SolverMuJoCo.register_custom_attributes(builder)beforeadd_usd(stage)gives good coverage of parsing, model storage, and later solver use. No issues with the overall structure here.
1652-1670: Good coverage ofnotify_model_changedfor JOINT_DOF_PROPERTIESThe final block that mutates
model.mujoco.stiffness/damping, callssolver.notify_model_changed(SolverNotifyFlags.JOINT_DOF_PROPERTIES), and then re-readsmjw_model.dof_dampingandjnt_stiffnessis a nice regression guard that the JOINT_DOF_PROPERTIES path correctly refreshes MuJoCo-side buffers. No changes needed here.
1428-1428: Clean up unused# noqaand redundantmathimport (RUF100)Ruff flags both of these as unused
noqadirectives:
- Line 1428:
from pxr import Usd # noqa: PLC0415- Line 1558:
import math # noqa: PLC0415Also,
mathis already imported at the top of the file (line 16), so the innerimport mathis redundant.You can simplify and fix the lint warnings with something like:
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd ... - import math # noqa: PLC0415 - - deg_to_rad_scale = math.degrees(1.0) + deg_to_rad_scale = math.degrees(1.0)Also applies to: 1558-1561
⛔ 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.
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 (1)
newton/tests/test_import_usd.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
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-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_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 (4)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
register_custom_attributes(151-212)notify_model_changed(417-427)newton/_src/solvers/solver.py (2)
register_custom_attributes(307-314)notify_model_changed(267-294)newton/_src/solvers/implicit_mpm/solver_implicit_mpm.py (1)
notify_model_changed(1809-1811)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-56)
🪛 Ruff (0.14.6)
newton/tests/test_import_usd.py
1428-1428: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
1558-1558: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (1)
newton/tests/test_import_usd.py (1)
1585-1671: Excellent test coverage for MuJoCo integration.The test thoroughly verifies:
- Parsing of stiffness/damping from USD to Newton model
- Propagation to MuJoCo solver (jnt_stiffness, dof_damping)
- Actuator parameter mapping (gainprm, biasprm)
- Runtime mutation via
notify_model_changedThe test exercises all code paths and validates the complete feature end-to-end.
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Looking good in general, just a few questions/suggestions.
Did you check whether the reference positions are correct? I guess this can be a follow-up, but we should check qpos0. Can you make a ticket for this?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/tests/test_import_usd.py (1)
1558-1560: Remove redundantmathimport.The
mathmodule is already imported at module level (line 16), making this local import unnecessary. Thenoqa: PLC0415directive is also flagged as unused by static analysis.- import math # noqa: PLC0415 - - angular_gain_unit_scale = math.degrees(1.0) + angular_gain_unit_scale = math.degrees(1.0)newton/tests/test_mujoco_solver.py (1)
930-1036: Multi-world passive stiffness/damping test is correct; consider simplifying DOF-damping indexingThe overall structure of
test_dof_passive_stiffness_damping_multiworldlooks solid:
- Registering
dof_passive_stiffness/dof_passive_dampingon the template builder, then usingbuilder.replicateand per-world patterns onmodel.mujoco.dof_passive_*, is consistent with how other MuJoCo custom attributes are tested.- The mapping logic in
assert_passive_values—usingjoint_qd_start/joint_dof_dimfor Newton DOFs anddof_to_mjc_jointfor MuJoCo joint indices—matches the pattern intest_jnt_solimp_conversion_and_updatesand correctly respects the per-DOF → per-joint mapping. This should reliably catch any indexing regressions in the new stiffness/damping paths. Based on learnings, this is aligned with the intendeddof_to_mjc_jointusage.One optional simplification to reduce coupling to solver internals:
For the
dof_dampingcheck, you could avoidjoint_mjc_dof_startand the joint loop entirely and mirror the style used intest_joint_attributes_registration_and_updates:dofs_per_world = model.joint_dof_count // model.num_worlds dof_damping = solver.mjw_model.dof_damping.numpy() for world_idx in range(model.num_worlds): world_dof_offset = world_idx * dofs_per_world for dof_idx in range(dofs_per_world): global_dof_idx = world_dof_offset + dof_idx self.assertAlmostEqual( dof_damping[world_idx, dof_idx], expected_damping[global_dof_idx], places=6, msg=f"dof_damping mismatch for world={world_idx}, dof={dof_idx}", )That keeps the test aligned with existing joint-DOF property tests and makes it a bit less sensitive to potential future changes in
joint_mjc_dof_start/selection semantics, while still validating the same behavior.If you prefer to keep the current joint-based mapping, it's also correct as written.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/solvers/mujoco/solver_mujoco.py(9 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 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/tests/test_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/tests/test_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/tests/test_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/tests/test_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-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-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
🧬 Code graph analysis (3)
newton/tests/test_import_mjcf.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(77-2019)register_custom_attributes(151-214)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(622-667)ModelBuilder(70-5401)CustomAttribute(304-394)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-70)ModelAttributeAssignment(31-46)
newton/tests/test_mujoco_solver.py (3)
newton/_src/sim/builder.py (2)
ModelBuilder(70-5401)joint_count(928-932)newton/_src/solvers/mujoco/solver_mujoco.py (2)
register_custom_attributes(151-214)notify_model_changed(419-429)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-56)
🪛 Ruff (0.14.6)
newton/tests/test_import_usd.py
1428-1428: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
1558-1558: 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 (8)
newton/tests/test_import_mjcf.py (1)
909-969: Well-structured test for MJCF stiffness/damping import.The test comprehensively validates:
- Parsing of
stiffnessanddampingjoint attributes from MJCF- Correct mapping to
model.mujoco.dof_passive_stiffnessandmodel.mujoco.dof_passive_damping- Actuator gain parsing (
target_ke/target_kd) including combined position+velocity actuatorsThe indexing via
joint_qd_start[joint_idx]is correct for per-DOF attributes. Note that this is a single-world test; per past review feedback, consider adding a separate test intest_mujoco_solver.pyto verify multi-world propagation and updates.newton/tests/test_import_usd.py (1)
1425-1584: Comprehensive USD stiffness/damping import test.The test validates:
- Parsing of
mjc:stiffnessandmjc:dampingUSD attributes- Drive stiffness/damping parsing with angular gain unit scaling (
math.degrees(1.0))- Correct propagation to model attributes for all three joint configurations
The test structure mirrors the MJCF test appropriately, ensuring consistent coverage across import paths.
newton/_src/solvers/mujoco/solver_mujoco.py (6)
191-214: Custom attributes for passive stiffness/damping are well-defined.The attributes are correctly registered with:
frequency=ModelAttributeFrequency.JOINT_DOFfor per-DOF storage- Appropriate USD (
mjc:stiffness/mjc:damping) and MJCF (stiffness/damping) attribute names- Default value of
0.0matching MuJoCo semanticsThe naming
dof_passive_stiffnessanddof_passive_dampingclearly indicates these are per-DOF passive joint properties, addressing the past review concern about naming clarity.
991-992: Correct retrieval of passive stiffness/damping attributes.Follows the established pattern for retrieving MuJoCo custom attributes, consistent with
joint_dof_limit_marginandjoint_solimp_limitretrieval above.
1333-1336: Correct propagation of stiffness/damping to linear DOF joint parameters.The per-DOF indexing via
ai = qd_start + icorrectly maps to the DOF-level custom attributes.
1406-1409: Consistent stiffness/damping propagation for angular DOFs.Mirrors the linear DOF handling, maintaining symmetry in the codebase.
1678-1683: Multi-world expansion includes stiffness/damping fields.Adding
jnt_stiffnessanddof_dampingtomodel_fields_to_expandensures these properties are correctly replicated across worlds, consistent with other joint/DOF properties in the list.
1795-1860: Kernel integration for stiffness/damping property updates — verification pending.The changes correctly:
- Retrieve
dof_passive_stiffnessanddof_passive_dampingfrom model attributes- Pass them as inputs to
update_joint_dof_properties_kernel- Map outputs to
mjw_model.jnt_stiffness(per-joint) andmjw_model.dof_damping(per-DOF)Based on learnings, the
dof_to_mjc_jointmapping ensures each Newton DOF correctly maps to its corresponding MuJoCo joint forjnt_stiffnesswrites. The pattern is consistent with other DOF properties (solimplimit, limit_margin).Verify the kernel signature in
newton/_src/solvers/mujoco/kernels.pyincludesjoint_stiffnessandjoint_dampingin the inputs and outputsjnt_stiffnessanddof_dampingto confirm the function call alignment.
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Looks good now, thank you very much!
adenzler-nvidia
left a comment
There was a problem hiding this comment.
approving again after merge from main.
…newton-physics#1082) Co-authored-by: adenzler-nvidia <116633880+adenzler-nvidia@users.noreply.github.com>
…newton-physics#1082) Co-authored-by: adenzler-nvidia <116633880+adenzler-nvidia@users.noreply.github.com>
…newton-physics#1082) Co-authored-by: adenzler-nvidia <116633880+adenzler-nvidia@users.noreply.github.com>
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
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.