Skip to content

Remove dead up-axis conversion from MuJoCo solver#1707

Open
adenzler-nvidia wants to merge 1 commit intonewton-physics:mainfrom
adenzler-nvidia:adenzler/remove-mujoco-up-axis-conversion
Open

Remove dead up-axis conversion from MuJoCo solver#1707
adenzler-nvidia wants to merge 1 commit intonewton-physics:mainfrom
adenzler-nvidia:adenzler/remove-mujoco-up-axis-conversion

Conversation

@adenzler-nvidia
Copy link
Member

@adenzler-nvidia adenzler-nvidia commented Feb 19, 2026

Summary

  • Removes leftover Y-up→Z-up coordinate swizzle from update_body_mass_ipos_kernel
  • The MuJoCo solver spec is built entirely in Newton's native coordinate system (body transforms, joint frames, gravity all passed through as-is), so converting the COM position to Z-up was incorrect — it put body_ipos in a different coordinate system from everything else
  • Also removes the matching incorrect test expectations and an unused shape_bodies variable

Closes #229 — the scattered pos2mjc/rot_y2z conversions referenced in the issue have all been removed. The remaining up_axis usage across the codebase (importers, geometry SDF kernels, viewer, mesh utils) properly handles all 3 axes (X, Y, Z).

Test plan

  • All 141 MuJoCo solver tests pass (uv run --extra dev -m newton.tests --no-cache-clear -k test_mujoco_solver)
  • CI passes

Summary by CodeRabbit

  • Refactor

    • Streamlined inertial property calculations by simplifying coordinate transformation handling in the MuJoCo solver.
  • Tests

    • Updated test cases to reflect simplified calculation approach.

The MuJoCo solver spec is built entirely in Newton's native coordinate
system — body transforms, joint frames, and gravity are all passed
through without any axis conversion. The up_axis swizzle in
update_body_mass_ipos_kernel was a leftover that incorrectly converted
the COM position to Z-up while everything else remained in Newton
coordinates.

Remove the up_axis parameter from the kernel and the corresponding
test expectations that matched this incorrect behavior.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 19, 2026

No actionable comments were generated in the recent review. 🎉


📝 Walkthrough

Walkthrough

This PR removes the up_axis parameter from the update_body_mass_ipos_kernel function signature and eliminates conditional coordinate reorientation logic. The corresponding kernel caller in SolverMuJoCo no longer passes the up_axis argument, and test expectations are simplified to use untransformed Newton model coordinates directly.

Changes

Cohort / File(s) Summary
MuJoCo up_axis parameter removal
newton/_src/solvers/mujoco/kernels.py, newton/_src/solvers/mujoco/solver_mujoco.py
Removed up_axis parameter from update_body_mass_ipos_kernel signature; eliminated conditional branch reorienting COM coordinates based on up_axis; body_ipos now always set directly from body_com without axis-based transformation.
Test coordinate conversion cleanup
newton/tests/test_mujoco_solver.py
Removed up_axis-based position and orientation conversions; simplified test comparisons to use untransformed Newton model values (newton_pos, newton_com) without Y-up/Z-up reordering adjustments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • PR #779: Directly related—both PRs modify MuJoCo solver inertial/axis handling by removing/upstreaming up_axis-based reorientation in kernels and solver calls.
  • PR #1199: Related—both PRs modify the update_body_mass_ipos_kernel signature and its parameter handling.

Suggested reviewers

  • eric-heiden
  • jvonmuralt
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The PR removes specific up-axis conversion code but does not implement the broader refactoring proposed in #229 to support X-up, Y-up, Z-up consistently or introduce shared functions. Clarify whether this PR fully closes #229 or is a prerequisite step; if partial, consider opening a follow-up issue for the shared function refactoring mentioned in #229.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing up-axis conversion logic from the MuJoCo solver kernel, which is the primary focus of the changeset.
Out of Scope Changes check ✅ Passed All changes are directly related to removing up-axis conversion from the MuJoCo solver, with no unrelated modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@adenzler-nvidia
Copy link
Member Author

I'm not very confident on this one because I don't really remember the full up-axis story. But the code looked inconsistent at least, because we were only rotating ipos.

@codecov
Copy link

codecov bot commented Feb 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When we rotate from Y-up to Z-up we should also support X-up to Z-up

1 participant

Comments