SolverMuJoCo: expose joint limit margin as custom solver attribute#1107
Conversation
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
📝 WalkthroughWalkthroughAdds a per-DOF joint limit margin custom attribute propagated through the MuJoCo solver pipeline and auto-expands scalar JOINT_DOF/JOINT_COORD inputs into single-element lists for single-DOF/single-coordinate joints; updates validation, kernels, solver wiring, tests, and docs accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Builder
participant Solver as SolverMuJoCo
participant Kernel as WarpKernel
participant Model as MuJoCoWarpModel
User->>Builder: add_joint(..., joint_dof_limit_margin=0.01 or [0.01,...])
Builder->>Solver: finalize model / register attributes
Solver->>Solver: register_custom_attributes(limit_margin)
User->>Solver: finalize model
Solver->>Solver: read limit_margin from custom attrs
Solver->>Solver: include margin in joint_params during add_geoms
User->>Solver: update solver state
Solver->>Kernel: update_joint_dof_properties(limit_margin_array,...)
Kernel->>Kernel: loop DOFs -> jnt_margin[world, joint] = limit_margin[dof_idx]
Kernel->>Model: output jnt_margin populated
Model-->>User: per-joint margin visible via MuJoCo Warp model
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
|
Thanks, LGTM! We haven’t considered custom attributes for joint dofs so far in the USD importer so this needs to be added here. |
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
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/tests/test_mujoco_solver.py (1)
2392-2392: Optional: Remove unused noqa directive.The static analyzer reports that
noqa: PLC0415is unused because the rule is not enabled. You can safely remove it:- 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 (5)
newton/_src/sim/builder.py(2 hunks)newton/_src/solvers/mujoco/kernels.py(3 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(7 hunks)newton/tests/test_custom_attributes.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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: 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.
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: 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-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/sim/builder.pynewton/_src/solvers/mujoco/kernels.pynewton/tests/test_custom_attributes.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/sim/builder.pynewton/_src/solvers/mujoco/kernels.pynewton/tests/test_custom_attributes.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.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/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-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/tests/test_custom_attributes.py
🧬 Code graph analysis (3)
newton/tests/test_mujoco_solver.py (2)
newton/_src/sim/builder.py (2)
ModelBuilder(70-5313)finalize(4821-5209)newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(151-190)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/sim/builder.py (3)
add_custom_attribute(617-662)ModelBuilder(70-5313)CustomAttribute(301-391)
newton/tests/test_custom_attributes.py (2)
newton/_src/sim/builder.py (1)
add_joint_d6(2244-2295)newton/_src/core/types.py (1)
Axis(63-160)
🪛 Ruff (0.14.5)
newton/_src/sim/builder.py
852-854: Avoid specifying long messages outside the exception class
(TRY003)
newton/tests/test_mujoco_solver.py
2392-2392: 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_custom_attributes.py (1)
515-524: LGTM! Test correctly validates multi-DOF joint requirements.The test appropriately verifies that a D6 joint with 3 DOFs (2 linear + 1 angular) raises TypeError when a scalar value is provided for a JOINT_COORD frequency attribute instead of the required list of 3 values.
newton/_src/solvers/mujoco/kernels.py (2)
916-916: LGTM! Kernel signature additions follow existing patterns.The new
limit_margininput parameter andjnt_marginoutput parameter are consistent with the kernel's existing parameter structure: per-DOF input array and per-joint 2D output array.Also applies to: 923-923
970-972: Verify multi-DOF joint margin behavior and add test coverage.The concern is valid: multi-DOF joints (D6 with 3+ DOFs, FREE with 6 DOFs) will have their per-DOF
limit_marginvalues written to the samemjc_joint_index, with the last DOF's value overwriting previous ones. This mirrors thesolimppattern exactly (both are per-joint properties in MuJoCo, lines 967–968 and 991–992).However, unlike
solrefwhich has a guard condition,limit_marginis unconditionally updated. The codebase does use multi-DOF joints (FREE, D6, BALL joints appear throughout), and test_custom_attributes.py demonstrates per-DOF custom attribute assignment for D6 joints.Findings:
- Multi-DOF joint usage is widespread in tests
- Current
limit_margintest coverage only includes single-DOF revolute joints- The last-wins behavior for multi-DOF joints is undocumented
- Design intent unclear: should all DOFs in a joint share the same margin, or should validation enforce it?
Recommendations:
- Add test coverage for multi-DOF joints with
limit_margin(e.g., D6 or FREE joint)- Add documentation clarifying that per-joint margins are used in MuJoCo, regardless of DOF count
- Consider adding validation to ensure all DOFs in a multi-DOF joint have consistent margin values, or document the last-wins behavior explicitly
newton/_src/sim/builder.py (2)
799-815: LGTM! Clean implementation of scalar value support for single-DOF joints.The sanitization logic correctly handles the ergonomic case where users provide a scalar value for single-DOF joints instead of requiring a list. The defensive check ensures wrapping only occurs when appropriate (
not isinstance(value_sanitized, (list, tuple)) and dof_count == 1), and the subsequent validation usingvalue_sanitizedmaintains correctness.
846-863: LGTM! Consistent implementation for single-coordinate joints.The JOINT_COORD handling mirrors the JOINT_DOF pattern, providing the same ergonomic improvement for single-coordinate joints. The implementation is correct and maintains proper validation of list lengths while allowing scalar inputs where appropriate.
newton/_src/solvers/mujoco/solver_mujoco.py (1)
168-179: LGTM! Comprehensive limit_margin attribute implementation.The new
limit_margincustom attribute is well-integrated:
- Properly registered with JOINT_DOF frequency and correct defaults
- Full propagation path: registration → model export → joint creation → multi-world expansion → runtime updates
- Per-DOF indexing is correct for both linear and angular DOFs
- Multi-world replication handled via
expand_model_fields- Runtime updates supported through JOINT_DOF_PROPERTIES notification
The implementation follows existing patterns (e.g., solimplimit) and maintains consistency across all code paths.
Also applies to: 963-963, 1285-1287, 1354-1356, 1620-1620, 1765-1765, 1780-1780, 1788-1788
newton/tests/test_mujoco_solver.py (2)
2332-2451: Excellent test coverage for limit_margin attribute.The three import path tests comprehensively validate:
- Code-based attribute setting via
custom_attributes- MJCF import using
marginattribute- USD import using
mjc:marginattribute- Default value handling (0.0 when not specified)
All tests correctly verify propagation to both
model.mujoco.limit_marginandsolver.mjw_model.jnt_margin.
2452-2483: Runtime update test validates the complete lifecycle.This test confirms that
limit_margincan be modified at runtime vianotify_model_changed(JOINT_DOF_PROPERTIES), completing the test coverage for all modification paths.
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
|
yes the USD side is still a bit unknown. I had to refine the per-dof processing a bit already in #1047 , and this change includes one more refinement. The main change is that you can pass a sparse dict that maps from dof -> value, that's needed because some values might not be set for all dofs, and we need to maintain the mapping. Things work like this but the USD side definitely needs more testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
docs/concepts/custom_attributes.rst (1)
223-234: Clarify JOINT_DOF/JOINT_COORD dict key constraintsThe new list/dict/scalar description matches the builder’s behavior, but dict key requirements (must be
intindices in[0, dof_count/coord_count), otherwise aTypeError/ValueErroris raised) aren’t stated here. Adding a short note about the key type and valid range would make the authoring rules fully explicit and aligned with_process_joint_custom_attributes.newton/_src/sim/builder.py (4)
736-747: Update joint custom-attribute docstring to match new scalar handlingThe
_process_joint_custom_attributesdocstring still talks about “a single Warp vector/matrix value” for single‑DOF JOINT_DOF attributes, but the implementation (and docs) now support generic scalars for both single‑DOF and single‑coordinate attributes (JOINT_DOF and JOINT_COORD), with list/dict formats for multi‑DOF/coord joints.Consider updating the bullet list here to something like:
- For DOF and COORD attributes, values can be: - - A list with length matching the joint's DOF/coordinate count (all DOFs get values) - - A dict mapping DOF/coord indices to values (only specified indices get values, rest use defaults) - - For single-DOF joints with JOINT_DOF frequency: a single Warp vector/matrix value + For DOF and COORD attributes, values can be: + - A list with length matching the joint's DOF/coordinate count (all DOFs/coords get values) + - A dict mapping DOF/coord indices to values (only specified indices get values, rest use defaults) + - A scalar value for single-DOF / single-coordinate joints, which is automatically expanded to a listto keep the documentation in sync with the behavior and the high-level docs.
770-816: Make multi‑DOF JOINT_DOF scalar misuse fail with a clear errorThe new
value_sanitizedhandling correctly supports:
- dicts for sparse per‑DOF values,
- lists/tuples for full per‑DOF values, and
- arbitrary scalars for single‑DOF joints (
dof_count == 1, wrapped into a one‑element list).For multi‑DOF joints, though, if a user accidentally passes a non‑sequence scalar (e.g.
150.0or a Warp vector)len(value_sanitized)will raise a rawTypeErrorinstead of the targetedValueErroryou construct below.To make error reporting more predictable and user‑friendly without changing valid behavior, you could guard this case explicitly, for example:
- else: - # List format or single value for single-DOF joints - value_sanitized = value - if not isinstance(value_sanitized, (list, tuple)) and dof_count == 1: - value_sanitized = [value_sanitized] - - actual = len(value_sanitized) - if actual != dof_count: - raise ValueError(f"JOINT_DOF '{attr_key}': got {actual}, expected {dof_count}") + else: + # List format or single value for single-DOF joints + value_sanitized = value + if not isinstance(value_sanitized, (list, tuple)) and dof_count == 1: + value_sanitized = [value_sanitized] + + try: + actual = len(value_sanitized) + except TypeError: + raise ValueError( + f"JOINT_DOF attribute '{attr_key}' expects a list or dict for " + f"multi-DOF joints (got {type(value).__name__} with {dof_count} DOFs)" + ) + + if actual != dof_count: + raise ValueError(f"JOINT_DOF '{attr_key}': got {actual}, expected {dof_count}")This keeps all valid inputs working as before, but turns common misuses into consistent, descriptive
ValueErrors instead of leaking a genericTypeError.
846-858: Mirror JOINT_DOF robustness and simplify JOINT_COORD error messagingThe JOINT_COORD branch mirrors the JOINT_DOF behavior and the scalar wrapping for
coord_count == 1is correct. For consistency and clearer failures:
- Apply the same “explicit scalar misuse” handling as in the JOINT_DOF suggestion so that scalar inputs on multi‑coordinate joints trigger a clear
ValueErrorrather than a rawTypeErrorfromlen().- Optionally align the error message style with the JOINT_DOF case and the Ruff TRY003 hint by reusing a precomputed
actualand shortening the message, e.g.:- if len(value_sanitized) != coord_count: - raise ValueError( - f"JOINT_COORD attribute '{attr_key}' has {len(value_sanitized)} values but joint has {coord_count} coordinates" - ) + try: + actual = len(value_sanitized) + except TypeError: + raise ValueError( + f"JOINT_COORD attribute '{attr_key}' expects a list or dict for " + f"multi-coordinate joints (got {type(value).__name__} with {coord_count} coordinates)" + ) + + if actual != coord_count: + raise ValueError( + f"JOINT_COORD '{attr_key}': got {actual}, expected {coord_count}" + )This keeps behavior for valid inputs unchanged while making the JOINT_COORD path’s diagnostics and style match JOINT_DOF and satisfying the static-analysis hint.
1763-1764: Jointcustom_attributesdocstring matches behavior; consider tiny clarificationThe updated
add_jointparameter doc correctly documents list/dict/scalar formats for JOINT_DOF/JOINT_COORD and a single value for JOINT frequency, in line with_process_joint_custom_attributes.If you want to be extra explicit, you could add that scalar values are only accepted when the joint has exactly one DOF/coordinate, and that dict keys must be integer indices in range, but this is optional polish.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/concepts/custom_attributes.rst(2 hunks)newton/_src/sim/builder.py(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
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-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/sim/builder.py
📚 Learning: 2025-09-22T21:03:18.367Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/_src/sim/builder.pydocs/concepts/custom_attributes.rst
📚 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/sim/builder.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/sim/builder.py
🪛 Ruff (0.14.5)
newton/_src/sim/builder.py
852-854: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (1)
docs/concepts/custom_attributes.rst (1)
279-303: Scalar and dict JOINT_DOF/JOINT_COORD examples align with implementationThese examples correctly exercise the three supported formats (list, scalar for 1‑DOF/coord, and sparse dict indices) and match how
_process_joint_custom_attributesmaps values to DOFs/coords. This should make the new behavior clear for users.
eric-heiden
left a comment
There was a problem hiding this comment.
Thanks, I think this looks good! Using a FloatArray for these per-dof values in USD seems to work well for now. We will do more testing once we have assets with the MJC schema applied.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/tests/test_mujoco_solver.py (1)
2466-2617: Consider adding test coverage for multi-DOF joints.All four
limit_margintests use single-DOF revolute joints. Consider adding test coverage for multi-DOF joints (e.g., D6 joints with both linear and angular DOFs) to verify margin handling when a joint has multiple DOFs. This would help clarify the expected behavior documented in the kernel review comment.Example test skeleton:
def test_limit_margin_multi_dof_joint(self): """Test limit_margin with D6 joint (multiple DOFs).""" builder = newton.ModelBuilder() newton.solvers.SolverMuJoCo.register_custom_attributes(builder) b0 = builder.add_body() builder.add_joint_free(-1, b0) b1 = builder.add_body() # D6 joint with 6 DOFs - verify margin handling builder.add_joint_d6(b0, b1, lin_axes=[(1,0,0), (0,1,0), (0,0,1)], ang_axes=[(1,0,0), (0,1,0), (0,0,1)], custom_attributes={"mujoco:limit_margin": [0.01]*6}) model = builder.finalize() solver = SolverMuJoCo(model, separate_worlds=False) # Verify all DOFs share the same margin value in jnt_margin
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/kernels.py(3 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(7 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/solvers/mujoco/solver_mujoco.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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: 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.
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: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.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.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
🪛 Ruff (0.14.5)
newton/tests/test_mujoco_solver.py
2526-2526: 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 (4)
newton/tests/test_mujoco_solver.py (4)
2466-2490: LGTM!The test correctly verifies that
limit_margincustom attributes set via code are properly propagated to both the Newton model (model.mujoco.limit_margin) and the MuJoCo solver (solver.mjw_model.jnt_margin). The test covers both explicit margin values and the default (0.0) for joints without the attribute.
2492-2521: LGTM!The test correctly verifies that
marginattributes from MJCF are imported and propagated through the Newton model to the MuJoCo solver. The MJCF format is correctly structured with themarginattribute on<joint>elements.
2523-2584: LGTM!The test correctly verifies USD import of the
mjc:margincustom attribute. The test properly guards with@unittest.skipUnless(USD_AVAILABLE)and creates valid USD joint prims with the margin attribute.Note: The static analysis hint about an unused
noqadirective on line 2526 is a false positive - thenoqa: PLC0415is correctly suppressing the "import-outside-toplevel" warning.
2586-2617: LGTM!The test correctly verifies that
limit_marginvalues can be updated at runtime vianotify_model_changed(SolverNotifyFlags.JOINT_DOF_PROPERTIES)and that the changes properly propagate to the MuJoCo solver'sjnt_marginarray.
…ewton-physics#1107) Signed-off-by: Alain Denzler <adenzler@nvidia.com>
…ewton-physics#1107) Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Allows simulating soft joint limit. The margin is the minimum distance at which a joint limit is detected/added to the constraint solver.
Before your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.