Fix: Support X-up to Z-up rotation #906
Conversation
remove test files to verify correct implementation of conversiion Support X-up to Z-up rotation
Dimios45
temporarily deployed
to
external-pr-approval
GitHub Actions
Inactive
Dimios45
temporarily deployed
to
external-pr-approval
GitHub Actions
Inactive
📝 WalkthroughWalkthroughCentralizes up-axis conversion (X/Y/Z) into new helpers, integrates them into MuJoCo kernels and solver update paths, applies converted gravity to model initialization and kernels, updates tests to use the helpers and adds X-up test variants, and documents up-axis support and automatic conversion. Changes
Sequence DiagramsequenceDiagram
participant User as User Code / Importer
participant Loader as Asset Importer / MuJoCo Loader
participant Converter as Up-Axis Utilities
participant Solver as MuJoCo Solver (CPU/GPU kernels)
participant Model as Runtime Model State
User->>Loader: Load asset or initialize model (specify up_axis)
Loader->>Converter: convert_up_axis_pos/quat(body transforms, up_axis)
Converter-->>Loader: transformed transforms (Z-up internal)
Loader->>Converter: convert_up_axis_pos(gravity, up_axis)
Converter-->>Loader: converted gravity (Z-up)
Loader->>Solver: init solver with converted data (transforms, gravity)
Solver->>Solver: launch convert_warp_coords_to_mj / convert_mj_coords_to_warp kernels (use helpers)
Solver->>Model: update_geom_properties_kernel(..., up_axis, shape_body, converted gravity)
Model-->>User: Simulation runs with consistent internal Z-up representation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (15)📚 Learning: 2025-08-19T12:27:30.629ZApplied to files:
📚 Learning: 2025-08-18T15:56:26.587ZApplied to files:
📚 Learning: 2025-08-19T12:27:30.629ZApplied to files:
📚 Learning: 2025-11-24T08:05:21.390ZApplied to files:
📚 Learning: 2025-09-22T21:03:39.624ZApplied to files:
📚 Learning: 2025-12-12T17:45:19.205ZApplied to files:
📚 Learning: 2025-10-10T10:47:41.082ZApplied to files:
📚 Learning: 2025-09-09T12:59:49.887ZApplied to files:
📚 Learning: 2025-09-25T16:14:22.033ZApplied to files:
📚 Learning: 2025-12-12T16:25:51.464ZApplied to files:
📚 Learning: 2025-11-28T11:12:40.805ZApplied to files:
📚 Learning: 2025-12-13T17:26:32.396ZApplied to files:
📚 Learning: 2025-09-26T05:58:21.013ZApplied to files:
📚 Learning: 2025-09-22T21:08:31.901ZApplied to files:
📚 Learning: 2025-12-01T17:00:28.889ZApplied to files:
🧬 Code graph analysis (1)newton/_src/solvers/mujoco/solver_mujoco.py (1)
🔇 Additional comments (3)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/tests/test_mujoco_solver.py (1)
813-825: Gate quaternion rotation in tests to static geoms
Tests intest_mujoco_solver.py(lines 813–825) unconditionally apply up-axis quaternion rotation, butupdate_geom_properties_kernelonly converts orientation whenshape_body == -1. Wrap theexpected_quatrotation inside the sameif shape_body == -1guard used for position. Apply the same change intest_geom_property_update(lines 1028–1037).
🧹 Nitpick comments (2)
docs/migration.rst (1)
152-163: Clarify rotation sign and convention for X-up/Y-up conversionsThe text says “90-degree rotation”; code uses −90° (right‑hand rule). Specify the sign and convention to avoid ambiguity, e.g., “−90° about Y for X‑up, −90° about X for Y‑up (right‑hand rule)”. Also note that position conversion applies to static geoms while dynamic geoms are handled via body/joint frames in the solver.
newton/_src/solvers/mujoco/solver_mujoco.py (1)
377-391: Unused kernel parameter: up_axisconvert_mj_coords_to_warp_kernel, convert_warp_coords_to_mj_kernel, and apply_mjc_body_f_kernel accept up_axis but don’t use it. Consider removing to reduce cognitive load.
Also applies to: 454-466, 587-596
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/migration.rst(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(5 hunks)newton/tests/test_mujoco_solver.py(4 hunks)newton/tests/test_up_axis.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
PR: newton-physics/newton#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.py
🧬 Code graph analysis (1)
newton/tests/test_up_axis.py (3)
newton/tests/unittest_utils.py (1)
add_function_test(312-331)newton/tests/test_control_force.py (1)
TestControlForce(31-32)newton/_src/core/types.py (1)
Axis(64-161)
🔇 Additional comments (6)
newton/tests/test_up_axis.py (1)
86-93: Add X-up gravity test — looks goodNew variant integrates cleanly into the existing matrix and uses Axis.X consistently.
newton/tests/test_mujoco_solver.py (1)
322-328: Position conversion branches align with X-up/Y-up semanticsGating COM position conversion by up_axis and applying the expected permutations looks correct.
Please ensure the model.up_axis values are 0/1/2 consistently across backends (Axis.X/Y/Z) to avoid enum/int mismatches.
Also applies to: 358-364
newton/_src/solvers/mujoco/solver_mujoco.py (4)
86-104: Helper convert_up_axis_pos — OKMapping matches tests and docs: X‑up→Z‑up (-z, y, x), Y‑up→Z‑up (x, -z, y), Z‑up passthrough.
106-126: Helper convert_up_axis_quat — OKLeft-multiplying by −90° about Y/X aligns with right‑hand rule and test expectations.
867-867: Good refactor: centralize COM conversion via helperReplacing bespoke logic with convert_up_axis_pos improves consistency.
1123-1125: Kernel signature extension necessary and correctPassing up_axis and shape_body enables per-geom handling; call site updated accordingly.
eric-heiden
left a comment
eric-heiden
left a comment
There was a problem hiding this comment.
Thanks for your contribution @Dimios45! I've added some comments that would be great to address.
…ion tests for X-up and Y-up
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1178-1189: Critical: Position conversion should only apply to static geoms.The position conversion at line 1180 is applied to all geoms, but based on past review comments and test expectations, it should only apply to static geoms (where
shape_body == -1), just like the quaternion conversion. Applying position conversion to dynamic geoms will misplace them relative to their parent bodies.Apply this fix to gate position conversion by static-ness:
pos = tf.p quat = tf.q - # Apply up-axis conversion for position - converted_pos = convert_up_axis_pos(pos, up_axis) - # Apply up-axis conversion for orientation only for static geoms (shape_body == -1) shape_is_static = shape_body[shape_idx] == -1 - converted_quat = quat + converted_pos = pos + converted_quat = quat if shape_is_static: + converted_pos = convert_up_axis_pos(pos, up_axis) converted_quat = convert_up_axis_quat(quat, up_axis) geom_pos[worldid, geom_idx] = converted_pos geom_quat[worldid, geom_idx] = wp.quatf(converted_quat.w, converted_quat.x, converted_quat.y, converted_quat.z)
🧹 Nitpick comments (2)
newton/tests/test_rigid_contact.py (2)
23-23: Consider removing unused import if not needed for debugging.The import of
convert_up_axis_quatdoesn't appear to be used in the test code. If it's intended for debugging purposes, consider adding a comment explaining its purpose.
225-247: Consider addingstrict=Trueto zip for better error detection.While the current usage is safe (both arrays come from the model and should have matching lengths), adding
strict=Truemakes the intent explicit and catches any unexpected mismatches.As per static analysis hint, apply this change:
- for i, (shape_body, shape_transform_arr) in enumerate(zip(shape_bodies, shape_transforms)): + for i, (shape_body, shape_transform_arr) in enumerate(zip(shape_bodies, shape_transforms, strict=True)):
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/solvers/mujoco/solver_mujoco.py(5 hunks)newton/tests/test_rigid_contact.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
PR: newton-physics/newton#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_rigid_contact.pynewton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (1)
newton/tests/test_rigid_contact.py (3)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
convert_up_axis_quat(114-130)newton/_src/core/types.py (1)
Axis(64-161)newton/_src/sim/builder.py (2)
add_ground_plane(2506-2526)finalize(4064-4422)
🪛 Ruff (0.13.3)
newton/tests/test_rigid_contact.py
225-225: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🔇 Additional comments (7)
newton/tests/test_rigid_contact.py (3)
191-217: LGTM! Well-structured test with proper up-axis handling.The test correctly positions the sphere based on the up-axis and validates the ground plane orientation. The logic for initial positioning is clear and correct for each axis.
272-287: LGTM! Final position validation is correct.The test correctly validates that the sphere settles at the expected position (radius above ground) for each up-axis orientation.
312-334: LGTM! Test registration follows existing pattern.The test registration correctly adds variants for X-up and Y-up across all solvers and devices, mirroring the pattern used for the original
test_shapes_on_plane.newton/_src/solvers/mujoco/solver_mujoco.py (4)
86-103: LGTM! Position conversion logic is mathematically correct.The coordinate transformations correctly implement the rotations needed to convert from X-up and Y-up to Z-up coordinate systems.
106-110: LGTM! Precomputed rotation constants improve performance.The rotation quaternions are correctly defined and will be reused efficiently in the quaternion conversion function.
113-130: LGTM! Quaternion conversion correctly applies precomputed rotations.The function correctly multiplies the input quaternion by the appropriate conversion quaternion based on the up-axis value.
2791-2818: LGTM! Kernel invocation correctly passes up-axis and shape_body parameters.The updated kernel call properly threads through the
up_axisandshape_bodyarrays needed for the conversion logic, though the conversion implementation in the kernel needs fixing (see separate comment).
|
pre-commit.ci autofix |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
newton/tests/test_rigid_contact.py (3)
200-206: Simplify initial position setup.The initial position can be computed directly from the up_axis using the
to_vec3()method available on theAxisenum, eliminating the verbose if-elif-else chain and the unnecessary zero-initialization.Apply this diff:
- initial_pos = wp.vec3(0.0, 0.0, 0.0) - if up_axis == newton.Axis.X: - initial_pos = wp.vec3(1.0, 0.0, 0.0) # Position above ground in X direction - elif up_axis == newton.Axis.Y: - initial_pos = wp.vec3(0.0, 1.0, 0.0) # Position above ground in Y direction - else: # Z-axis (default) - initial_pos = wp.vec3(0.0, 0.0, 1.0) # Position above ground in Z direction + # Position sphere 1 unit above ground along the up-axis + initial_pos = up_axis.to_vec3()
231-237: Simplify expected normal computation.Similar to the initial position, the expected normal can be computed directly from the up_axis, eliminating redundant code and the unnecessary zero-initialization.
Apply this diff:
- expected_normal = wp.vec3(0.0, 0.0, 0.0) - if up_axis == newton.Axis.X: - expected_normal = wp.vec3(1.0, 0.0, 0.0) - elif up_axis == newton.Axis.Y: - expected_normal = wp.vec3(0.0, 1.0, 0.0) - else: # Z-axis - expected_normal = wp.vec3(0.0, 0.0, 1.0) + expected_normal = up_axis.to_vec3()
278-286: Consider simplifying expected position computation.The expected position logic can also be simplified using the up_axis vector, similar to the previous suggestions.
- if up_axis == newton.Axis.X: - # In X-up, gravity pulls in -X direction, so sphere should end up at around X=0.1 (radius above ground at X=0) - expected_pos = wp.vec3(0.1, 0.0, 0.0) - elif up_axis == newton.Axis.Y: - # In Y-up, gravity pulls in -Y direction, so sphere should end up at around Y=0.1 (radius above ground at Y=0) - expected_pos = wp.vec3(0.0, 0.1, 0.0) - else: # Z-axis - # In Z-up, gravity pulls in -Z direction, so sphere should end up at around Z=0.1 (radius above ground at Z=0) - expected_pos = wp.vec3(0.0, 0.0, 0.1) + # Sphere settles at one radius (0.1) above ground in the up direction + expected_pos = up_axis.to_vec3() * 0.1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_rigid_contact.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
newton/tests/test_rigid_contact.py (2)
newton/_src/core/types.py (1)
Axis(64-161)newton/_src/sim/builder.py (2)
add_ground_plane(2506-2526)finalize(4064-4422)
🔇 Additional comments (3)
newton/tests/test_rigid_contact.py (3)
288-288: Review the higher position tolerance.This test uses a tolerance of
2e-1(0.2 units) for position, which is double the1e-1tolerance used intest_shapes_on_plane(line 185). Since the sphere radius is 0.1, a tolerance of 0.2 means allowing 200% error in the expected settling position. Verify whether this increased tolerance is necessary for the up-axis scenarios or if it indicates a potential issue with the up-axis conversion logic.
315-337: LGTM! Test registration properly covers X-up and Y-up.The test registration correctly creates separate test cases for X-up and Y-up across all device/solver combinations, mirroring the filtering logic from the original
test_shapes_on_plane. The Z-up case is already covered by the existingtest_shapes_on_planetest (which uses the default up-axis).
224-224:strict=Falseonzip()is supported
pyproject.tomlclassifiers specify Python 3.10+, so this syntax is safe.
|
any more changes to be made? |
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Failure
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
newton/tests/test_rigid_contact.py (1)
200-200: Update the comment to reflect all solver types.The comment mentions "Euler integrators" but the test runs across all solver types (Featherstone, MuJoCo, XPBD, SemiImplicit). Update the comment for accuracy.
🧹 Nitpick comments (1)
newton/tests/test_rigid_contact.py (1)
237-243: Consider using the Axis enum helper method.The expected normal can be computed more concisely using the
to_vec3()helper method available on theAxisenum (seenewton/_src/core/types.py).Apply this diff to simplify the code:
- expected_normal = wp.vec3(0.0, 0.0, 0.0) - if up_axis == newton.Axis.X: - expected_normal = wp.vec3(1.0, 0.0, 0.0) - elif up_axis == newton.Axis.Y: - expected_normal = wp.vec3(0.0, 1.0, 0.0) - else: # Z-axis - expected_normal = wp.vec3(0.0, 0.0, 1.0) + expected_normal = up_axis.to_vec3()The same simplification can be applied to lines 205-211 and 284-292.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_rigid_contact.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
PR: newton-physics/newton#899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_rigid_contact.py
🧬 Code graph analysis (1)
newton/tests/test_rigid_contact.py (3)
newton/_src/core/types.py (1)
Axis(63-160)newton/_src/sim/builder.py (3)
ModelBuilder(67-4482)add_ground_plane(2512-2532)finalize(4070-4428)newton/tests/unittest_utils.py (2)
assert_np_equal(240-246)add_function_test(312-331)
🔇 Additional comments (4)
newton/tests/test_rigid_contact.py (4)
213-221: Good fix!The missing articulation issue from the previous review has been properly addressed. The articulation is now added before the body (line 213), following the correct pattern.
273-278: Good fix!The iteration count has been increased to 250 (from 50 in the previous version), matching the original
test_shapes_on_planetest. This should provide sufficient time for convergence and address the reported test failure.
294-294: Verify whether such a large tolerance is necessary.The tolerance of
2e-1(0.2 units) is twice the sphere's radius (0.1 units) and larger than the tolerance used in the original test (1e-1at line 190). While this may be necessary to account for solver-specific behavior or numerical differences with up-axis transformations, verify that this tolerance isn't masking actual position errors.
321-343: LGTM!The test registration correctly creates test cases for both X-up and Y-up configurations across all supported device and solver combinations, following the same pattern as the original test registration.
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Failure
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
… newton/_src/solvers/mujoco/solver_mujoco.py
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
|
pre-commit.ci autofix |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1128-1203: Critical: Missing up-axis conversion for static geoms in kernel.The
up_axisandshape_bodyparameters were added to the kernel signature but are never used. This causes static geoms (e.g., ground planes) to not be rotated according to the up-axis convention, resulting in the failingtest_shapes_on_plane_x_uptest mentioned in the PR comments.Apply this diff to add conversion for static geoms only:
# store position and orientation - geom_pos[worldid, geom_idx] = tf.p - geom_quat[worldid, geom_idx] = wp.quat(tf.q.w, tf.q.x, tf.q.y, tf.q.z) + + # Apply up-axis conversion for static geoms only + # Dynamic geoms are attached to bodies which handle conversion separately + shape_is_static = shape_body[shape_idx] == -1 + + converted_pos = tf.p + converted_quat = tf.q + if shape_is_static: + converted_pos = convert_up_axis_pos(tf.p, up_axis) + converted_quat = convert_up_axis_quat(tf.q, up_axis) + + geom_pos[worldid, geom_idx] = converted_pos + geom_quat[worldid, geom_idx] = wp.quat(converted_quat.w, converted_quat.x, converted_quat.y, converted_quat.z)
🧹 Nitpick comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
2870-2893: Gravity conversion works correctly, but comment is misleading.The implementation correctly converts gravity for both CPU and GPU paths. However, the comment on line 2880 suggests the kernel performs the conversion, when it actually just copies the pre-converted value from the Python wrapper.
Consider updating the comment for clarity:
else: if hasattr(self, "mjw_data"): - # For GPU, we need to update the kernel that handles gravity to perform the conversion - # Create a temporary array with the converted gravity + # For GPU path, pass converted gravity through a kernel to update MjWarp model temp_gravity = wp.array([converted_gravity], dtype=wp.vec3, device=self.model.device)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/solvers/mujoco/solver_mujoco.py(6 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 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: 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-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.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-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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-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/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-22T21:03:18.367Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:59:49.887Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/tests/test_implicit_mpm.py:82-82
Timestamp: 2025-09-09T12:59:49.887Z
Learning: In granular heap simulations, when particles fall onto a plane, they spread outward in all horizontal directions due to impact and granular flow, not just settle vertically. Test assertions should verify spreading in all axes, not just along the gravity/up-axis.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🪛 Ruff (0.14.3)
newton/_src/solvers/mujoco/solver_mujoco.py
1144-1144: Unused function argument: up_axis
(ARG001)
1145-1145: Unused function argument: shape_body
(ARG001)
🔇 Additional comments (2)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
87-131: LGTM! Coordinate conversion functions implemented correctly.The position and quaternion conversion functions correctly handle X-up, Y-up, and Z-up coordinate systems. The rotation quaternions match the position mappings as verified in previous reviews.
1957-1959: LGTM! Gravity conversion correctly applied during model creation.The gravity vector is properly converted from Newton's coordinate system to MuJoCo's Z-up convention when creating the MuJoCo spec.
eric-heiden
temporarily deployed
to
external-pr-approval
GitHub Actions
Inactive
Resolved conflicts in: - newton/_src/solvers/mujoco/solver_mujoco.py - newton/tests/test_mujoco_solver.py Changes: - Kept coordinate conversion helper functions (convert_up_axis_pos, convert_up_axis_quat) - Updated to use new update_geom_properties_kernel from kernels.py - Added both vec5 and coordinate conversion imports to test file
Root cause: - convert_mj_coords_to_warp_kernel and convert_warp_coords_to_mj_kernel received up_axis parameter but didn't use it for FREE joints - Position and quaternion were copied directly without coordinate transformation - This caused incorrect behavior for X-up and Y-up coordinate systems Solution: - Added convert_up_axis_pos/quat and their inverses to kernels.py - Updated convert_mj_coords_to_warp_kernel to use inverse transforms (Z-up MuJoCo → target up_axis Newton) - Updated convert_warp_coords_to_mj_kernel to use forward transforms (up_axis Newton → Z-up MuJoCo) - Applied transformations to both position and quaternion for FREE joints - Applied transformations to linear and angular velocities with proper rotation frame handling This ensures: - X-up systems: (x,y,z) ↔ (-z,y,x) position, -90° Y-axis quaternion - Y-up systems: (x,y,z) ↔ (x,-z,y) position, +90° X-axis quaternion - Z-up systems: no conversion (identity transform)
Dimios45
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
|
pre-commit.ci autofix |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/solvers/mujoco/kernels.py (2)
956-961: Missing X-up support for body COM position conversion.This kernel only handles Y-up (
up_axis == 1) but not X-up (up_axis == 0). The PR objective is to support X-up → Z-up conversion, but this falls through to theelsebranch which applies no conversion.Apply this diff to add X-up support:
# update COM position - if up_axis == 1: + if up_axis == 0: # X-up to Z-up: (x, y, z) -> (-z, y, x) + body_ipos[world, mjc_body] = wp.vec3f( + -body_com[newton_body][2], body_com[newton_body][1], body_com[newton_body][0] + ) + elif up_axis == 1: # Y-up to Z-up: (x, y, z) -> (x, -z, y) body_ipos[world, mjc_body] = wp.vec3f( body_com[newton_body][0], -body_com[newton_body][2], body_com[newton_body][1] ) - else: + else: # Z-up: no conversion needed body_ipos[world, mjc_body] = body_com[newton_body]Alternatively, consider using the
convert_up_axis_poshelper for consistency:# update COM position - if up_axis == 1: - body_ipos[world, mjc_body] = wp.vec3f( - body_com[newton_body][0], -body_com[newton_body][2], body_com[newton_body][1] - ) - else: - body_ipos[world, mjc_body] = body_com[newton_body] + converted_com = convert_up_axis_pos(body_com[newton_body], up_axis) + body_ipos[world, mjc_body] = wp.vec3f(converted_com[0], converted_com[1], converted_com[2])
1296-1324: Kernel signature doesn't include expected up_axis and shape_body parameters.The call site in
solver_mujoco.py(lines 2297-2301) attempts to passup_axisandshape_bodyparameters, but this kernel signature doesn't include them. This confirms the argument mismatch flagged in the other file.If up-axis conversion for static geom positions/orientations is needed (as suggested by past review comments about static-only position conversion), this kernel needs to be updated to:
- Accept
up_axis: intandshape_body: wp.array(dtype=int)parameters- Apply
convert_up_axis_posandconvert_up_axis_quatonly for static geoms (shape_body[shape_idx] < 0)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/migration.rst(1 hunks)newton/_src/solvers/mujoco/kernels.py(3 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(4 hunks)newton/tests/test_mujoco_solver.py(5 hunks)newton/tests/test_up_axis.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/migration.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/tests/test_mujoco_solver.py
- newton/tests/test_up_axis.py
🧰 Additional context used
🧠 Learnings (18)
📓 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: 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.
📚 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-12-01T17:00:28.889Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: newton/_src/solvers/mujoco/kernels.py:1268-1268
Timestamp: 2025-12-01T17:00:28.889Z
Learning: MuJoCo Warp spatial vectors (e.g., cacc, cvel) use the convention (angular, linear), which is opposite to Newton's (linear, angular) convention. When converting from MuJoCo Warp to Newton format, spatial_top() on MuJoCo Warp data extracts the angular component, and spatial_bottom() extracts the linear component.
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-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/solvers/mujoco/kernels.py
📚 Learning: 2025-11-28T11:12:40.805Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1146
File: newton/examples/basic/example_basic_joints.py:206-213
Timestamp: 2025-11-28T11:12:40.805Z
Learning: In Newton's standard solvers (XPBD, SemiImplicit, SolverMuJoCo), spatial vectors in State.body_qd use the ordering (linear, angular): wp.spatial_top(qd) returns linear velocity and wp.spatial_bottom(qd) returns angular velocity. This is the opposite of typical Modern Robotics spatial twist notation where (angular, linear) is used.
Applied to files:
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-01T16:21:36.581Z
Learnt from: jvonmuralt
Repo: newton-physics/newton PR: 1160
File: newton/_src/sim/articulation.py:0-0
Timestamp: 2025-12-01T16:21:36.581Z
Learning: In Newton's Featherstone solver, during forward kinematics for FREE/DISTANCE joints, the computed spatial velocity v_wc follows Featherstone's spatial twist convention where the linear component (spatial_top) represents the velocity at the world origin (0,0,0), not at the joint anchor or COM. To convert to State.body_qd (which stores COM velocity), use v_com = v_origin + ω × x_com, where x_com is the absolute world position of the COM.
Applied to files:
newton/_src/solvers/mujoco/kernels.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-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-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-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-12-12T17:45:19.205Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/geometry/primitives.py:0-0
Timestamp: 2025-12-12T17:45:19.205Z
Learning: In newton/_src/solvers/kamino/geometry/primitives.py, the narrow-phase kernel `_primitive_narrowphase` does not need to handle symmetric shape-pair orderings (e.g., both SPHERE-BOX and BOX-SPHERE) because `kamino.core.builder.ModelBuilder` and `kamino.geometry.primitive.broadphase` guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:59:49.887Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/tests/test_implicit_mpm.py:82-82
Timestamp: 2025-09-09T12:59:49.887Z
Learning: In granular heap simulations, when particles fall onto a plane, they spread outward in all horizontal directions due to impact and granular flow, not just settle vertically. Test assertions should verify spreading in all axes, not just along the gravity/up-axis.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-12T16:25:51.464Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/core/math.py:248-255
Timestamp: 2025-12-12T16:25:51.464Z
Learning: In newton/_src/solvers/kamino/core/math.py, the quat_conj function's current implementation (negating w instead of x,y,z) is a known issue that will be revised in a future PR per vastsoun's decision, not to be flagged in current Kamino reviews.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (2)
newton/_src/solvers/mujoco/kernels.py (1)
newton/_src/sim/ik/ik_solver.py (1)
joint_q(362-363)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/solvers/mujoco/kernels.py (2)
convert_up_axis_pos(65-81)update_model_properties_kernel(1286-1293)
🔇 Additional comments (5)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
1046-1048: LGTM!Gravity conversion using
convert_up_axis_posis correctly applied during model initialization. The conversion from Newton's up-axis to MuJoCo's Z-up convention is properly handled.
2319-2340: LGTM!The gravity conversion in
update_model_propertiesis correctly implemented for both CPU and GPU paths. The temporary array creation for the GPU path is acceptable given this method is called infrequently (only on model property changes vianotify_model_changed).newton/_src/solvers/mujoco/kernels.py (3)
64-149: LGTM! Well-documented up-axis conversion helpers.The position and quaternion conversion functions are mathematically correct:
- X-up to Z-up: -90° rotation about Y matches
(x,y,z) → (-z,y,x)- Y-up to Z-up: +90° rotation about X matches
(x,y,z) → (x,-z,y)- Inverse functions correctly reverse the transformations
The precomputed quaternion constants at module level avoid repeated computation.
432-477: LGTM! Correct FREE joint coordinate conversion from MuJoCo to Newton.The angular velocity conversion correctly handles the frame transformation chain:
- Read body-frame angular velocity from MuJoCo
- Transform to world frame (MuJoCo Z-up world)
- Convert world frame from Z-up to Newton's up_axis
- Transform back to body frame (Newton body frame)
This properly accounts for the fact that the body orientation differs between coordinate systems.
525-570: LGTM! Symmetric conversion from Newton to MuJoCo.The reverse conversion correctly mirrors the MuJoCo-to-Newton path, maintaining consistency.
| self.model.rigid_contact_torsional_friction, | ||
| self.model.rigid_contact_rolling_friction, | ||
| self.contact_stiffness_time_const, | ||
| self.model.up_axis, # Add up_axis parameter | ||
| self.model.shape_body, # Add shape_body parameter |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the kernels.py file
fd -name "kernels.py" --type f | head -20
# Look for update_geom_properties_kernel
rg -n "update_geom_properties_kernel" --type pyRepository: newton-physics/newton
Length of output: 700
🏁 Script executed:
#!/bin/bash
echo "=== Kernel signature in kernels.py ==="
sed -n '1297,1350p' newton/_src/solvers/mujoco/kernels.py
echo ""
echo "=== Actual kernel call in solver_mujoco.py ==="
sed -n '2280,2310p' newton/_src/solvers/mujoco/solver_mujoco.py
echo ""
echo "=== Search for contact_stiffness_time_const definition ==="
rg "contact_stiffness_time_const" newton/_src/solvers/mujoco/solver_mujoco.py -B 2 -A 2Repository: newton-physics/newton
Length of output: 3947
🏁 Script executed:
#!/bin/bash
# Verify that contact_stiffness_time_const is never defined elsewhere in the file
echo "=== Checking if contact_stiffness_time_const is defined anywhere ==="
rg "contact_stiffness_time_const\s*=" newton/_src/solvers/mujoco/solver_mujoco.py
# Check the class definition to see what attributes exist
echo ""
echo "=== Looking for attribute definitions around contact parameters ==="
rg "self\.contact_|self\.model\." newton/_src/solvers/mujoco/solver_mujoco.py | grep -i "friction\|stiffness\|contact" | head -20Repository: newton-physics/newton
Length of output: 688
Critical: Kernel call arguments don't match kernel signature and use undefined attribute.
Multiple issues with this kernel call:
-
self.contact_stiffness_time_constis not defined anywhere in the class - this will raiseAttributeErrorat runtime. -
The parameters passed don't align with the kernel signature in
kernels.py. The kernel expects 17 input parameters:- Parameters 1-14: shape collision/material properties (correct in call)
- Parameter 15:
shape_geom_solimp: wp.array(dtype=vec5) - Parameter 16:
shape_geom_solmix: wp.array(dtype=float) - Parameter 17:
shape_geom_gap: wp.array(dtype=float)
But the call passes 19 parameters with incorrect types/values at positions 15-17:
self.contact_stiffness_time_const(undefined scalar)self.model.up_axis(int, not array of vec5)self.model.shape_body(array of int, not float)
Either the kernel signature needs to be updated to include up_axis and shape_body parameters, or these arguments should be removed and replaced with the correct arrays (shape_geom_solimp, shape_geom_solmix, shape_geom_gap).
🤖 Prompt for AI Agents
In newton/_src/solvers/mujoco/solver_mujoco.py around lines 2297 to 2301, the
kernel call is passing an undefined attribute self.contact_stiffness_time_const
and the wrong arguments/types for the last three kernel parameters; replace the
undefined scalar and the misplaced self.model.up_axis and self.model.shape_body
arguments with the three arrays expected by the kernel (shape_geom_solimp:
wp.array(dtype=vec5), shape_geom_solmix: wp.array(dtype=float), shape_geom_gap:
wp.array(dtype=float)) so the call supplies the exact 17 parameters in the
correct order and types, or alternatively update the kernel signature in
kernels.py to explicitly accept up_axis and shape_body (and remove the undefined
attribute) if those values are truly required — ensure types and ordering match
between caller and kernel.
All coordinate conversion functions (convert_up_axis_pos, convert_up_axis_quat, etc.) are now imported from kernels.py instead of being duplicated. This eliminates ~186 lines of duplicate code while maintaining all functionality.
Dimios45
force-pushed
the
fix-issue-229
branch
from
fb0243d to
c8d4434
Compare
4 participants
Description
Adds support for
X-up → Z-upconversion alongside existingY-up → Z-up.Introduces a shared
convert_up_axis()helper for quaternion and position transforms.Newton Migration Guide
The migration guide in
docs/migration.rstis up-to dateNecessary tests have been added and new examples are tested (see
newton/tests/test_examples.py)Documentation is up-to-date
Code passes formatting and linting checks with
pre-commit run -aSummary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.