Fix joint transform for world-attached joints to use MJCF xform argument#1171
Merged
Merged
Conversation
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
…-xform-handling Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Contributor
📝 WalkthroughWalkthroughThe changes modify how fixed-base joints are positioned when importing MJCF and URDF models with transforms applied. When converting floating joints to fixed-base (parent == -1), the code now computes a specialized parent_xform by combining the world_xform with the joint's local position to ensure the overall import transform is respected. Tests validate this behavior across both formats. Changes
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/tests/test_import_mjcf.py (1)
1091-1099: Consider extracting quaternion comparison to a helper function.The quaternion comparison logic (checking both
qand-q) is duplicated across this test andtest_import_urdf.py. Consider extracting this to a shared utility inunittest_utils.py:def assert_quaternion_equal(actual, expected, rtol=1e-5, atol=1e-5, msg=None): """Assert quaternions are equal, accounting for q == -q equivalence.""" actual = np.asarray(actual) expected = np.asarray(expected) if not (np.allclose(actual, expected, rtol=rtol, atol=atol) or np.allclose(actual, -expected, rtol=rtol, atol=atol)): raise AssertionError(msg or f"Quaternions not equal: {actual} vs {expected}")
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/utils/import_mjcf.py(3 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_urdf.py(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common 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: 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-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/utils/import_mjcf.pynewton/tests/test_import_urdf.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/utils/import_mjcf.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
newton/tests/test_import_mjcf.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/tests/test_import_urdf.py
🧬 Code graph analysis (2)
newton/_src/utils/import_mjcf.py (2)
newton/_src/sim/builder.py (2)
add_joint_fixed(2331-2375)key(391-393)newton/_src/utils/import_utils.py (1)
transform(139-140)
newton/tests/test_import_mjcf.py (4)
newton/_src/utils/import_utils.py (1)
transform(139-140)newton/_src/sim/builder.py (2)
ModelBuilder(70-5751)add_mjcf(1302-1392)newton/_src/sim/joints.py (1)
JointType(20-44)newton/_src/sim/model.py (1)
state(517-554)
⏰ 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 (5)
newton/tests/test_import_urdf.py (2)
208-213: LGTM: Default up_axis handling improves test consistency.The default
up_axis="Y"provides a sensible default for URDF tests while still allowing explicit override via kwargs.
470-557: Well-structured test for xform handling with fixed base.The test comprehensively validates:
- Joint count and type (FIXED)
- Joint parent transform matches the expected xform
- Body state after
eval_fkmatches expected transform- Quaternion sign equivalence is correctly handled
The explicit
up_axis="Z"to avoid axis transformation is a good practice documented inline.newton/_src/utils/import_mjcf.py (2)
729-729: Correct fix: Usingworld_xformfor fixed base joint.This change ensures the fixed base joint correctly respects the import
xformargument by using the pre-composedworld_xforminstead of the locally-computed_xform.
746-760: Correct implementation for world-attached joint transforms.The conditional logic correctly handles both cases:
- World-attached (parent == -1): Uses
world_xform * transform(joint_pos, identity)which respects the importxformargument- Non-world parents: Preserves the original body-relative positioning
This is consistent with the fixed_base handling at line 729.
newton/tests/test_import_mjcf.py (1)
1040-1123: Comprehensive test for MJCF xform handling with floating=False.The test validates:
- A freejoint body is converted to FIXED joint when
floating=False- The joint's
parent_xformcorrectly combines the importxformwith the body's local transform- Body state after
eval_fkmatches the expected composed transformThe use of
wp.quat_rpyforlocal_quatprovides a more complex rotation scenario compared to the URDF test's simpler axis-angle rotation.
3 tasks
eric-heiden
added a commit
to eric-heiden/newton
that referenced
this pull request
Jan 28, 2026
…ent (newton-physics#1171) Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
4 tasks
mmacklin
pushed a commit
to mmacklin/newton
that referenced
this pull request
Apr 7, 2026
…ent (newton-physics#1171) Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.