Fix ball joint conversion for SolverMuJoCo - #2981
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBall-joint MuJoCo↔Newton conversions and actuator mappings were fixed to use per-joint child-frame quaternion conjugation: kernels now accept joint child transforms, solver export/wiring threads per-actuator child-anchor bases, and tests were added for rotated child_xform cases. ChangesMuJoCo ball-joint frame-rotation alignment
Sequence Diagram(s)sequenceDiagram
participant SolverMuJoCo
participant convert_mj_coords_to_warp_kernel
participant apply_mjc_control_kernel
participant apply_mjc_qfrc_kernel
participant convert_qfrc_actuator_from_mj_kernel
participant NewtonState
SolverMuJoCo->>convert_mj_coords_to_warp_kernel: mj state, joint_X_c -> joint_q/joint_qd (conjugated)
SolverMuJoCo->>apply_mjc_control_kernel: actuator targets, mjc_actuator_to_newton_ball_jnt, joint_X_c, joint_q -> mj child-frame targets
apply_mjc_control_kernel->>apply_mjc_qfrc_kernel: joint_q, joint_X_c, qfrc_applied
apply_mjc_qfrc_kernel->>convert_qfrc_actuator_from_mj_kernel: mj_data.qfrc_actuator, joint_X_c
convert_qfrc_actuator_from_mj_kernel->>NewtonState: qfrc_actuator (rotated into Newton DOFs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/_src/solvers/mujoco/kernels.py (1)
226-226: ⚡ Quick winUse inline code literals for private
_srcsymbols.These
:func:references point at private helper/kernel symbols in_src, so they can become unresolved Sphinx targets. Prefer inline code literals here instead of cross-references.Based on learnings, avoid Sphinx cross-references (
:func:) to private helpers that are not re-exported as public API; use inline code literals instead.Also applies to: 2715-2717
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/kernels.py` at line 226, Update the docstring to avoid Sphinx cross-reference to a private helper by replacing the :func:`ball_rotate_mj_to_newton` cross-reference with an inline code literal ``ball_rotate_mj_to_newton`` (and similarly change any other occurrences around lines referenced, e.g., the other two places at 2715-2717) so the docstring reads like: "Inverse of ``ball_rotate_mj_to_newton``; ``r`` is the Newton-side ball joint quaternion." Ensure you only change the docstring text and keep the existing backticks for inline code.newton/tests/test_mujoco_solver.py (1)
7496-7586: ⚡ Quick winCover the kinematic-child BALL readback branch too.
This new regression only exercises dynamic children, but the PR also changes
convert_qfrc_actuator_from_mj_kernelto early-return for kinematic children. Add oneis_kinematic=Truevariant here and assert the Newton-sidemujoco:qfrc_actuatorstays zero so that branch cannot regress untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/tests/test_mujoco_solver.py` around lines 7496 - 7586, Add a subcase that makes the child link kinematic and verifies kinematic readback stays zero: when building the model, add a second child link with is_kinematic=True (use the same call site of builder.add_link that created "child") or set the existing child to is_kinematic=True for that subtest; then run the same sync/update sequence (solver._update_mjc_data and solver._update_newton_state) and assert that state.mujoco.qfrc_actuator[qd_start:qd_start+3] is all zeros for that kinematic case so the early-return in convert_qfrc_actuator_from_mj_kernel is exercised and cannot regress.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/_src/solvers/mujoco/kernels.py`:
- Line 226: Update the docstring to avoid Sphinx cross-reference to a private
helper by replacing the :func:`ball_rotate_mj_to_newton` cross-reference with an
inline code literal ``ball_rotate_mj_to_newton`` (and similarly change any other
occurrences around lines referenced, e.g., the other two places at 2715-2717) so
the docstring reads like: "Inverse of ``ball_rotate_mj_to_newton``; ``r`` is the
Newton-side ball joint quaternion." Ensure you only change the docstring text
and keep the existing backticks for inline code.
In `@newton/tests/test_mujoco_solver.py`:
- Around line 7496-7586: Add a subcase that makes the child link kinematic and
verifies kinematic readback stays zero: when building the model, add a second
child link with is_kinematic=True (use the same call site of builder.add_link
that created "child") or set the existing child to is_kinematic=True for that
subtest; then run the same sync/update sequence (solver._update_mjc_data and
solver._update_newton_state) and assert that
state.mujoco.qfrc_actuator[qd_start:qd_start+3] is all zeros for that kinematic
case so the early-return in convert_qfrc_actuator_from_mj_kernel is exercised
and cannot regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fd361b7c-d1c2-4429-8cbf-66f857ec4bb0
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Comment verbosity Each BALL branch in Spots that read as restatement rather than load-bearing context:
The test docstrings have the same pattern. The non- Ambiguous notation Two of the comments use
The other two blocks already use the |
1d22ed9 to
e957a0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
5218-5225: ⚡ Quick winFail fast on parented free joints instead of warning and continuing.
This configuration is already known to be invalid for MuJoCo, so letting it reach
spec.compile()just moves the error farther from the source joint.Suggested change
- if parent != -1: - warnings.warn( - f"Free joint '{model.joint_label[j]}' has parent body {parent} instead of the world (-1). " - "SolverMuJoCo requires free joints to attach directly to the world; " - "MuJoCo will reject this model at compile time.", - UserWarning, - stacklevel=2, - ) + if parent != -1: + raise ValueError( + f"Free joint '{model.joint_label[j]}' must attach directly to the world (-1); " + f"got parent body {parent}." + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 5218 - 5225, The current code in SolverMuJoCo emits a warning when a free joint (checked where model.joint_label[j] is accessed) has parent != -1, but MuJoCo treats that as invalid and will fail at spec.compile(); change this to fail fast by raising a clear exception (e.g., ValueError or RuntimeError) instead of warnings.warn so the invalid joint configuration is reported immediately (include the joint label and parent in the exception message) in the same location where parent != -1 is checked.
2818-2828: ⚡ Quick winSync
_init_actuators()'sArgs:block with the new signature.The docstring still skips the newly added target-q lookup and child-anchor quaternion parameters, so it no longer matches the callable surface.
As per coding guidelines, "Follow Google-style docstrings. Types in annotations, not docstrings. Use
Args:withname: descriptionformat".Also applies to: 2841-2854
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 2818 - 2828, Update the _init_actuators() docstring Args: section to match the current function signature by adding entries (name: brief description) for mjc_actuator_to_target_q_idx_list, mjc_actuator_to_target_q_axis_idx_list, and mjc_actuator_q_cj_list (child-anchor quaternion tuple), and ensure existing entries reflect the annotated types (mjc_actuator_ctrl_source_list, mjc_actuator_to_newton_idx_list, dof_to_mjc_joint, mjc_joint_names, selected_tendons, mjc_tendon_names, body_name_mapping, site_mapping). Make the changes in both the Args block starting at the _init_actuators() docstring and the duplicate block referenced around lines 2841-2854 so the docstring fully matches the callable surface and follows the "name: description" Google-style format.
358-360: ⚡ Quick winUse explicit public Sphinx targets here.
These references are less robust than the surrounding
~newton...style and are likely to render as unresolved or ambiguous in generated docs. Prefer explicit public paths such as:attr:\~newton.Model.use_coord_layout_targets`,:attr:`~newton.Control.joint_target_q`, and:attr:`~newton.Model.joint_target_q_start``.As per coding guidelines, "Use Sphinx cross-references (
:class:,:meth:) with shortest possible targets. Prefer public API paths; never usenewton._srcin docstring references".Also applies to: 3245-3250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 358 - 360, Update the docstring references to use explicit public Sphinx targets: replace ambiguous references in the JOINT_TARGET paragraph to use :attr:`~newton.Model.use_coord_layout_targets`, :attr:`~newton.Control.joint_target_q`, :attr:`~newton.Control.joint_target_qd` (and the deprecated aliases as :attr:`~newton.Control.joint_target_pos`/:attr:`~newton.Control.joint_target_vel` if needed), and any start/index constants such as :attr:`~newton.Model.joint_target_q_start`; ensure all cross-references use the shortest public API paths (no _src) and mirror the surrounding `~newton...` style so Sphinx resolves them reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 700-730: This change introduces new user-facing attributes
mujoco.solref and mujoco.solref_mode (created via ModelBuilder.CustomAttribute
with name="solref" and name="solref_mode"), so add a dedicated entry under the
[Unreleased] section of CHANGELOG.md recording the new shape-level controls (use
an "Added" or "Changed" heading), mention the attribute names and default
behavior (SOLREF_MODE_MJCF_DEFAULT and the wp.vec2 default), and reference the
USD/MJCF mapping (mjc:solref / solref) so users know how to opt into force-space
scaling.
---
Nitpick comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 5218-5225: The current code in SolverMuJoCo emits a warning when a
free joint (checked where model.joint_label[j] is accessed) has parent != -1,
but MuJoCo treats that as invalid and will fail at spec.compile(); change this
to fail fast by raising a clear exception (e.g., ValueError or RuntimeError)
instead of warnings.warn so the invalid joint configuration is reported
immediately (include the joint label and parent in the exception message) in the
same location where parent != -1 is checked.
- Around line 2818-2828: Update the _init_actuators() docstring Args: section to
match the current function signature by adding entries (name: brief description)
for mjc_actuator_to_target_q_idx_list, mjc_actuator_to_target_q_axis_idx_list,
and mjc_actuator_q_cj_list (child-anchor quaternion tuple), and ensure existing
entries reflect the annotated types (mjc_actuator_ctrl_source_list,
mjc_actuator_to_newton_idx_list, dof_to_mjc_joint, mjc_joint_names,
selected_tendons, mjc_tendon_names, body_name_mapping, site_mapping). Make the
changes in both the Args block starting at the _init_actuators() docstring and
the duplicate block referenced around lines 2841-2854 so the docstring fully
matches the callable surface and follows the "name: description" Google-style
format.
- Around line 358-360: Update the docstring references to use explicit public
Sphinx targets: replace ambiguous references in the JOINT_TARGET paragraph to
use :attr:`~newton.Model.use_coord_layout_targets`,
:attr:`~newton.Control.joint_target_q`, :attr:`~newton.Control.joint_target_qd`
(and the deprecated aliases as
:attr:`~newton.Control.joint_target_pos`/:attr:`~newton.Control.joint_target_vel`
if needed), and any start/index constants such as
:attr:`~newton.Model.joint_target_q_start`; ensure all cross-references use the
shortest public API paths (no _src) and mirror the surrounding `~newton...`
style so Sphinx resolves them reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 67895d15-ebe7-4779-aae4-3a7cf6f28efd
📒 Files selected for processing (5)
CHANGELOG.mdnewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_joint_controllers.pynewton/tests/test_mujoco_solver.py
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- newton/tests/test_mujoco_solver.py
- newton/_src/solvers/mujoco/kernels.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (3)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
5218-5225: ⚡ Quick winFail fast on parented free joints instead of warning and continuing.
This configuration is already known to be invalid for MuJoCo, so letting it reach
spec.compile()just moves the error farther from the source joint.Suggested change
- if parent != -1: - warnings.warn( - f"Free joint '{model.joint_label[j]}' has parent body {parent} instead of the world (-1). " - "SolverMuJoCo requires free joints to attach directly to the world; " - "MuJoCo will reject this model at compile time.", - UserWarning, - stacklevel=2, - ) + if parent != -1: + raise ValueError( + f"Free joint '{model.joint_label[j]}' must attach directly to the world (-1); " + f"got parent body {parent}." + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 5218 - 5225, The current code in SolverMuJoCo emits a warning when a free joint (checked where model.joint_label[j] is accessed) has parent != -1, but MuJoCo treats that as invalid and will fail at spec.compile(); change this to fail fast by raising a clear exception (e.g., ValueError or RuntimeError) instead of warnings.warn so the invalid joint configuration is reported immediately (include the joint label and parent in the exception message) in the same location where parent != -1 is checked.
2818-2828: ⚡ Quick winSync
_init_actuators()'sArgs:block with the new signature.The docstring still skips the newly added target-q lookup and child-anchor quaternion parameters, so it no longer matches the callable surface.
As per coding guidelines, "Follow Google-style docstrings. Types in annotations, not docstrings. Use
Args:withname: descriptionformat".Also applies to: 2841-2854
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 2818 - 2828, Update the _init_actuators() docstring Args: section to match the current function signature by adding entries (name: brief description) for mjc_actuator_to_target_q_idx_list, mjc_actuator_to_target_q_axis_idx_list, and mjc_actuator_q_cj_list (child-anchor quaternion tuple), and ensure existing entries reflect the annotated types (mjc_actuator_ctrl_source_list, mjc_actuator_to_newton_idx_list, dof_to_mjc_joint, mjc_joint_names, selected_tendons, mjc_tendon_names, body_name_mapping, site_mapping). Make the changes in both the Args block starting at the _init_actuators() docstring and the duplicate block referenced around lines 2841-2854 so the docstring fully matches the callable surface and follows the "name: description" Google-style format.
358-360: ⚡ Quick winUse explicit public Sphinx targets here.
These references are less robust than the surrounding
~newton...style and are likely to render as unresolved or ambiguous in generated docs. Prefer explicit public paths such as:attr:\~newton.Model.use_coord_layout_targets`,:attr:`~newton.Control.joint_target_q`, and:attr:`~newton.Model.joint_target_q_start``.As per coding guidelines, "Use Sphinx cross-references (
:class:,:meth:) with shortest possible targets. Prefer public API paths; never usenewton._srcin docstring references".Also applies to: 3245-3250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 358 - 360, Update the docstring references to use explicit public Sphinx targets: replace ambiguous references in the JOINT_TARGET paragraph to use :attr:`~newton.Model.use_coord_layout_targets`, :attr:`~newton.Control.joint_target_q`, :attr:`~newton.Control.joint_target_qd` (and the deprecated aliases as :attr:`~newton.Control.joint_target_pos`/:attr:`~newton.Control.joint_target_vel` if needed), and any start/index constants such as :attr:`~newton.Model.joint_target_q_start`; ensure all cross-references use the shortest public API paths (no _src) and mirror the surrounding `~newton...` style so Sphinx resolves them reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 700-730: This change introduces new user-facing attributes
mujoco.solref and mujoco.solref_mode (created via ModelBuilder.CustomAttribute
with name="solref" and name="solref_mode"), so add a dedicated entry under the
[Unreleased] section of CHANGELOG.md recording the new shape-level controls (use
an "Added" or "Changed" heading), mention the attribute names and default
behavior (SOLREF_MODE_MJCF_DEFAULT and the wp.vec2 default), and reference the
USD/MJCF mapping (mjc:solref / solref) so users know how to opt into force-space
scaling.
---
Nitpick comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 5218-5225: The current code in SolverMuJoCo emits a warning when a
free joint (checked where model.joint_label[j] is accessed) has parent != -1,
but MuJoCo treats that as invalid and will fail at spec.compile(); change this
to fail fast by raising a clear exception (e.g., ValueError or RuntimeError)
instead of warnings.warn so the invalid joint configuration is reported
immediately (include the joint label and parent in the exception message) in the
same location where parent != -1 is checked.
- Around line 2818-2828: Update the _init_actuators() docstring Args: section to
match the current function signature by adding entries (name: brief description)
for mjc_actuator_to_target_q_idx_list, mjc_actuator_to_target_q_axis_idx_list,
and mjc_actuator_q_cj_list (child-anchor quaternion tuple), and ensure existing
entries reflect the annotated types (mjc_actuator_ctrl_source_list,
mjc_actuator_to_newton_idx_list, dof_to_mjc_joint, mjc_joint_names,
selected_tendons, mjc_tendon_names, body_name_mapping, site_mapping). Make the
changes in both the Args block starting at the _init_actuators() docstring and
the duplicate block referenced around lines 2841-2854 so the docstring fully
matches the callable surface and follows the "name: description" Google-style
format.
- Around line 358-360: Update the docstring references to use explicit public
Sphinx targets: replace ambiguous references in the JOINT_TARGET paragraph to
use :attr:`~newton.Model.use_coord_layout_targets`,
:attr:`~newton.Control.joint_target_q`, :attr:`~newton.Control.joint_target_qd`
(and the deprecated aliases as
:attr:`~newton.Control.joint_target_pos`/:attr:`~newton.Control.joint_target_vel`
if needed), and any start/index constants such as
:attr:`~newton.Model.joint_target_q_start`; ensure all cross-references use the
shortest public API paths (no _src) and mirror the surrounding `~newton...`
style so Sphinx resolves them reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 67895d15-ebe7-4779-aae4-3a7cf6f28efd
📒 Files selected for processing (5)
CHANGELOG.mdnewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_joint_controllers.pynewton/tests/test_mujoco_solver.py
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- newton/tests/test_mujoco_solver.py
- newton/_src/solvers/mujoco/kernels.py
🛑 Comments failed to post (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
700-730:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a separate
[Unreleased]changelog entry for the new shape contact attributes.This introduces user-facing
mujoco.solref/mujoco.solref_modebehavior, but the referenced changelog content only covers the ball-joint frame-conversion fix. Please add a dedicatedAddedorChangedentry for these shape-level controls.As per coding guidelines, "
**: Check that any user-facing change includes a corresponding entry in CHANGELOG.md under the [Unreleased] section."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 700 - 730, This change introduces new user-facing attributes mujoco.solref and mujoco.solref_mode (created via ModelBuilder.CustomAttribute with name="solref" and name="solref_mode"), so add a dedicated entry under the [Unreleased] section of CHANGELOG.md recording the new shape-level controls (use an "Added" or "Changed" heading), mention the attribute names and default behavior (SOLREF_MODE_MJCF_DEFAULT and the wp.vec2 default), and reference the USD/MJCF mapping (mjc:solref / solref) so users know how to opt into force-space scaling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 133-140: The code uses 0.0 as a sentinel for "unset" which makes
an explicit Model.joint_damping[dof] = 0.0 get overwritten by
mujoco.dof_passive_damping; change the logic to track authored/unset state
instead of numeric sentinel: ensure the producer of canonical_value returns None
(or expose an authored flag) when the canonical joint damping was not explicitly
set, then replace the check "if canonical_value != 0.0 and not
math.isclose(canonical_value, alias_value...)" with "if canonical_value is not
None and not math.isclose(...)" (or check the authored flag) so explicit zeros
are treated as real values; keep the rest of the flow that initializes
updated_joint_damping and assigns updated_joint_damping[damping_index] =
alias_value only when canonical_value is None (unset), and preserve use of
builder.joint_damping and variables damping_index, alias_value,
updated_joint_damping, canonical_value, Model.mujoco.dof_passive_damping, and
Model.joint_damping to locate where to change this behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bf7def88-cef3-44aa-938b-c2f072a64eff
📒 Files selected for processing (5)
CHANGELOG.mdnewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_joint_controllers.pynewton/tests/test_mujoco_solver.py
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/tests/test_joint_controllers.py
- newton/tests/test_mujoco_solver.py
- newton/_src/solvers/mujoco/kernels.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 133-140: The code uses 0.0 as a sentinel for "unset" which makes
an explicit Model.joint_damping[dof] = 0.0 get overwritten by
mujoco.dof_passive_damping; change the logic to track authored/unset state
instead of numeric sentinel: ensure the producer of canonical_value returns None
(or expose an authored flag) when the canonical joint damping was not explicitly
set, then replace the check "if canonical_value != 0.0 and not
math.isclose(canonical_value, alias_value...)" with "if canonical_value is not
None and not math.isclose(...)" (or check the authored flag) so explicit zeros
are treated as real values; keep the rest of the flow that initializes
updated_joint_damping and assigns updated_joint_damping[damping_index] =
alias_value only when canonical_value is None (unset), and preserve use of
builder.joint_damping and variables damping_index, alias_value,
updated_joint_damping, canonical_value, Model.mujoco.dof_passive_damping, and
Model.joint_damping to locate where to change this behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bf7def88-cef3-44aa-938b-c2f072a64eff
📒 Files selected for processing (5)
CHANGELOG.mdnewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_joint_controllers.pynewton/tests/test_mujoco_solver.py
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/tests/test_joint_controllers.py
- newton/tests/test_mujoco_solver.py
- newton/_src/solvers/mujoco/kernels.py
🛑 Comments failed to post (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
133-140:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't use
0.0as the “canonical unset” sentinel.An explicit
Model.joint_damping[dof] = 0.0currently loses to a conflicting deprecatedmujoco.dof_passive_dampingvalue, because the conflict branch only runs whencanonical_value != 0.0. That silently exports the alias value instead of the user's canonical setting. This needs authored/assigned-state tracking rather than a numeric sentinel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 133 - 140, The code uses 0.0 as a sentinel for "unset" which makes an explicit Model.joint_damping[dof] = 0.0 get overwritten by mujoco.dof_passive_damping; change the logic to track authored/unset state instead of numeric sentinel: ensure the producer of canonical_value returns None (or expose an authored flag) when the canonical joint damping was not explicitly set, then replace the check "if canonical_value != 0.0 and not math.isclose(canonical_value, alias_value...)" with "if canonical_value is not None and not math.isclose(...)" (or check the authored flag) so explicit zeros are treated as real values; keep the rest of the flow that initializes updated_joint_damping and assigns updated_joint_damping[damping_index] = alias_value only when canonical_value is None (unset), and preserve use of builder.joint_damping and variables damping_index, alias_value, updated_joint_damping, canonical_value, Model.mujoco.dof_passive_damping, and Model.joint_damping to locate where to change this behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
4886-4891:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard equality-attribute access behind the existing None-safe path.
Line 4890 and the related ranges directly dereference
model.mujoco...even though this codepath now explicitly supports non-standard model assembly via None-safe equality reads. Those direct accesses can still raiseAttributeErrorbefore the fallback logic is reached.💡 Suggested fix
- selected_constraints = np.arange(model.mujoco.equality_constraint_count, dtype=np.int32) + eq_constraint_count = int(len(eq_constraint_world)) if eq_constraint_world is not None else 0 + selected_constraints = np.arange(eq_constraint_count, dtype=np.int32) ... - eq_constraints_per_world = model.mujoco.equality_constraint_count // model.world_count + eq_constraints_per_world = eq_constraint_count // model.world_count if model.world_count > 0 else 0- if self.model.mujoco.equality_constraint_count == 0: + mujoco_attrs = getattr(self.model, "mujoco", None) + eq_constraint_count = int(getattr(mujoco_attrs, "equality_constraint_count", 0)) + if eq_constraint_count == 0: return- eq_constraint_world = ( - model.mujoco.equality_constraint_world.numpy() - if model.mujoco.equality_constraint_count > 0 - else np.empty(0, dtype=np.int32) - ) + mujoco_attrs = getattr(model, "mujoco", None) + eq_constraint_count = int(getattr(mujoco_attrs, "equality_constraint_count", 0)) + eq_constraint_world_attr = getattr(mujoco_attrs, "equality_constraint_world", None) + eq_constraint_world = ( + eq_constraint_world_attr.numpy() + if eq_constraint_world_attr is not None and eq_constraint_count > 0 + else np.empty(0, dtype=np.int32) + )Also applies to: 6261-6261, 6984-6990, 7598-7599, 7850-7854, 7954-7956
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 4886 - 4891, The code directly dereferences model.mujoco (e.g., when computing selected_constraints and selected_mimic_constraints) which can raise AttributeError; instead obtain a None-safe reference like mj = getattr(model, "mujoco", None) and use getattr(mj, "<attr>", 0) (or appropriate default) for equality_constraint_count and any other mujoco attributes, then build selected_constraints/selected_mimic_constraints from those safe counts; apply the same pattern to the other occurrences referenced (around the symbols producing selected_constraints, selected_mimic_constraints and any direct model.mujoco.* uses at the other listed locations).
🧹 Nitpick comments (2)
newton/_src/solvers/mujoco/kernels.py (1)
668-670: 💤 Low valueClarify rotation-vs-multiplication notation in comment.
The expression
w_newton = (q_cj^{-1} * qpos) * w_mjcould be read as quaternion multiplication, but the operation is vector rotation. Consider using explicit notation likeR(q_cj^{-1} * q_mj) · ω_mjto distinguish quaternion composition from rotating a vector. This aligns with the reviewer feedback requesting R(·) · v notation for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/kernels.py` around lines 668 - 670, Update the explanatory comment around q_cj, qpos, w_newton, and w_mj to make clear the operation is a vector rotation rather than quaternion multiplication: replace the ambiguous expression `w_newton = (q_cj^{-1} * qpos) * w_mj` with an explicit rotation notation such as `w_newton = R(q_cj^{-1} * qpos) · w_mj` (or equivalent R(...)·ω_mj), and optionally clarify that `r = q_cj^{-1} * qpos * q_cj` denotes quaternion similarity while R(...) denotes the rotation matrix/operator applied to the angular velocity vector.newton/_src/solvers/mujoco/solver_mujoco.py (1)
4777-4779: ⚡ Quick winConsolidate repeated inline rationale comments.
The same “shape-stable empty arrays / non-standard pipeline robustness” explanation is repeated in multiple places. Keep one concise canonical comment and trim duplicates to reduce noise.
As per coding guidelines,
**/*.py: “Flag inline code comments ... that repeat the same point in multiple places. Comments should be brief and reserved for non-obvious code.”Also applies to: 6988-6990, 7848-7850
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 4777 - 4779, Consolidate the repeated inline rationale about “shape-stable empty arrays / non-standard pipeline robustness” into a single concise canonical comment next to the None-safe helper call that reads the per-row equality arrays (keep a short note mentioning that finalize() materializes shape-stable empty arrays and the None-safe path preserves robustness for models missing the custom-attribute pipeline). Remove the duplicate explanatory comments at the other occurrences (the similar blocks around the other None-safe helper usages you pointed out) so only the one canonical comment remains; keep any local, non-redundant comments that are specific to the surrounding code logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 4886-4891: The code directly dereferences model.mujoco (e.g., when
computing selected_constraints and selected_mimic_constraints) which can raise
AttributeError; instead obtain a None-safe reference like mj = getattr(model,
"mujoco", None) and use getattr(mj, "<attr>", 0) (or appropriate default) for
equality_constraint_count and any other mujoco attributes, then build
selected_constraints/selected_mimic_constraints from those safe counts; apply
the same pattern to the other occurrences referenced (around the symbols
producing selected_constraints, selected_mimic_constraints and any direct
model.mujoco.* uses at the other listed locations).
---
Nitpick comments:
In `@newton/_src/solvers/mujoco/kernels.py`:
- Around line 668-670: Update the explanatory comment around q_cj, qpos,
w_newton, and w_mj to make clear the operation is a vector rotation rather than
quaternion multiplication: replace the ambiguous expression `w_newton =
(q_cj^{-1} * qpos) * w_mj` with an explicit rotation notation such as `w_newton
= R(q_cj^{-1} * qpos) · w_mj` (or equivalent R(...)·ω_mj), and optionally
clarify that `r = q_cj^{-1} * qpos * q_cj` denotes quaternion similarity while
R(...) denotes the rotation matrix/operator applied to the angular velocity
vector.
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 4777-4779: Consolidate the repeated inline rationale about
“shape-stable empty arrays / non-standard pipeline robustness” into a single
concise canonical comment next to the None-safe helper call that reads the
per-row equality arrays (keep a short note mentioning that finalize()
materializes shape-stable empty arrays and the None-safe path preserves
robustness for models missing the custom-attribute pipeline). Remove the
duplicate explanatory comments at the other occurrences (the similar blocks
around the other None-safe helper usages you pointed out) so only the one
canonical comment remains; keep any local, non-redundant comments that are
specific to the surrounding code logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b2c8e2c6-71de-4eaa-b933-d7cf792176de
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_mujoco_solver.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_mujoco_solver.py
adenzler-nvidia
left a comment
adenzler-nvidia
left a comment
There was a problem hiding this comment.
A few documentation and cleanup notes on the ball-joint conversion path — all non-blocking. The frame-conversion math and the actuator wiring look correct to me; these are just about a redundant debug assert, a stale comment, and two docstring/comment wording fixes. Details inline.
|
Thanks for the review, @adenzler-nvidia! Fixed the remaining items (except for the assert). Reasoning: |
4 participants
Description
Fix
SolverMuJoCoball-joint conversion under non-identitychild_xformrotation. MuJoCo's ballqvel/qfrclive in the post-qposchild body frame, not the rest-body frame, so the four bridge kernels (convert_{mj_coords_to_warp,warp_coords_to_mj}_kernel,apply_mjc_qfrc_kernel,convert_qfrc_actuator_from_mj_kernel) now composejoint_X_c.qwith the current balljoint_q(r = X_cj.q^{-1} * qpos * X_cj.qand duals). Newton'sbody_q/body_qdand applied/actuator torques now round-trip through MuJoCo for arbitrary ball poses.Folded in while touching these kernels:
apply_mjc_qfrc_kernelwas readingjoint_type/joint_dof_dimat the per-world template index instead of the global joint index — silent on homogeneous worlds, wrong on heterogeneous multi-world models.convert_qfrc_actuator_from_mj_kernelBALL to matchapply_mjc_qfrc_kernel.Checklist
CHANGELOG.mdhas been updatedTest plan
New tests in
TestMuJoCoArticulationConversioncovering FK, qvel, applied torque (identity and non-identityjoint_q), and the actuator readback branch. Each fails without the corresponding fix. Full suite: 202 pass / 7 skipped.Bug fix
Steps to reproduce:
SolverMuJoComodel with a ball joint whosechild_xformhas a non-identity rotation.joint_qd(and optionallyjoint_q), step, or calleval_fk.body_qdagainstmujoco.mj_objectVelocity— they diverge for any rotation that does not commute withchild_xform.q.Summary by CodeRabbit
Bug Fixes
Tests