Skip to content

Fix mesh geom transform issues, remove +Y up axis handling logic in MuJoCoSolver #326#376

Merged
eric-heiden merged 16 commits into
newton-physics:mainfrom
eric-heiden:mujoco-up-axis
Jul 14, 2025
Merged

Fix mesh geom transform issues, remove +Y up axis handling logic in MuJoCoSolver #326#376
eric-heiden merged 16 commits into
newton-physics:mainfrom
eric-heiden:mujoco-up-axis

Conversation

@eric-heiden

@eric-heiden eric-heiden commented Jul 10, 2025

Copy link
Copy Markdown
Member

Description

Computes transform corrections for shapes in the MuJoCoSolver which account for the joint child transform and the mesh transformation MuJoCo does in its compilation step.

This PR is based on #378 (closes #365):
It standardizes the internal representation of primitive shapes (capsules, cones, cylinders, and planes) to use the +Z axis as their primary axis or normal. This change, which closes #365, aligns Newton's conventions with other simulation environments like MuJoCo and simplifies the conversion process between them.

Key changes include:

  • Collision Kernels (newton/geometry/kernels.py):
    • SDFs and collision functions for capsules, cones, and cylinders now assume the primary axis is along +Z.
    • Planes are now defined as XY planes with a +Z normal.
  • ModelBuilder (newton/sim/builder.py):
    • Shape creation methods (add_shape_capsule, add_shape_cylinder, add_shape_cone) now apply a rotation to align the user-specified axis with the internal +Z axis.
    • The misleading up_axis parameter has been renamed to axis for clarity.
    • add_shape_plane has been updated to correctly handle plane equations and compute robust rotations, including for normals opposite to +Z.
  • Rendering (newton/utils/render.py):
    • A corrective rotation is applied during rendering to transform the internal +Z aligned shapes to the Y-up convention expected by the Warp renderer.
  • MuJoCo Solver (newton/solvers/mujoco/solver_mujoco.py):
    • Removed unnecessary Y-to-Z axis conversion logic, as both Newton and MuJoCo now share the same Z-axis convention for these shapes.
  • Importers (newton/utils/import_*.py):
    • Updated URDF, MJCF, and USD importers to use the new axis parameter correctly.

Newton Migration Guide

Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.

  • The migration guide in docs/migration.rst is up-to date

Before your PR is "Ready for review"

  • All commits are signed-off to indicate that your contribution adheres to the Developer Certificate of Origin requirements
  • I understand that GitHub does not perform any GPU testing of this pull request
  • Necessary tests have been added
  • Documentation is up-to-date
  • Code passes formatting and linting checks with pre-commit run -a

Summary by CodeRabbit

Summary by CodeRabbit

  • Refactor
    • Simplified coordinate and quaternion conversions by standardizing axis handling and removing special-case logic for different up-axis conventions.
    • Streamlined geometry and body processing to follow a single axis convention.
    • Improved naming for internal tracking of body names.
    • Extended rotation correction to include plane geometries.
  • New Features
    • Updated shape orientation defaults to align with the Z-axis as the primary axis for capsules, cylinders, cones, and planes.
    • Adjusted plane shapes to use the XY plane with Z-axis normal by default.
  • Bug Fixes
    • Fixed inconsistencies in collision geometry calculations by aligning all shape signed distance functions and collision handling to a consistent axis convention.
    • Corrected ground plane orientation in rendering and contact handling to use the Z-axis normal.
  • Tests
    • Updated tests to reflect new axis conventions and parameter naming for shape orientations.
    • Added tolerance to collision contact assertions for numerical stability.
  • Style
    • Renamed parameters from up_axis to axis across shape-related methods and shape additions for clarity and consistency.
  • Chores
    • Increased default damping stiffness coefficient in example model configuration.

Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
@coderabbitai

coderabbitai Bot commented Jul 10, 2025

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

📝 Walkthrough

"""

Walkthrough

The changes remove all conditional logic and code paths related to handling different up-axis conventions (specifically up_axis == 1) in the MuJoCo solver's coordinate and quaternion conversions. The code now directly uses the model's axis conventions without swapping axes or applying quaternion rotations, simplifying the conversion and geometry addition logic.

Changes

Files/Paths Change Summary
newton/solvers/mujoco/solver_mujoco.py Removed all up-axis transform logic; simplified coordinate and quaternion conversions; removed pos2mjc and perm_position logic; removed rotation quaternion for Y-up to Z-up; updated geometry and body processing logic; renamed body name tracking variable; removed parameters from kernels.
newton/geometry/kernels.py Changed axis references in shape SDFs, gradients, plane computations, and collision kernels from Y-axis (index 1) to Z-axis (index 2) for consistent coordinate system alignment.
newton/sim/builder.py Changed default plane normal from Y-up to Z-up; renamed up_axis parameter to axis with default Z-axis for capsule, cylinder, and cone shapes; updated shape orientation conventions accordingly.
newton/tests/test_collision.py Added tolerance to numpy array equality assertions in collision test.
newton/tests/test_import_urdf.py Updated expected capsule visual geometry quaternion in URDF inertial parameter test to identity quaternion.
newton/tests/test_mujoco_solver.py Removed conditional quaternion rotations and special static geom handling related to up-axis in MuJoCo solver geometry property tests.
newton/tests/test_rigid_contact.py Renamed keyword argument up_axis to axis in capsule shape addition call in rigid contact test.
newton/tests/test_up_axis.py Renamed keyword argument up_axis to axis in capsule shape addition call.
newton/utils/import_mjcf.py Renamed keyword argument up_axis to axis in capsule and cylinder shape additions during MJCF import.
newton/utils/import_urdf.py Renamed keyword argument up_axis to axis in capsule and cylinder shape additions during URDF import.
newton/utils/import_usd.py Renamed keyword argument up_axis to axis in capsule, cylinder, and cone shape additions; updated plane up-axis convention from Y to Z.
newton/utils/render.py Changed ground plane normal from Y-axis to Z-axis; applied fixed -90° X-axis rotation to capsule, cylinder, and cone shape rendering quaternions.
newton/examples/example_anymal_c_walk.py Increased default damping stiffness coefficient (kd) from 500 to 700 in shape configuration.

Assessment against linked issues

Objective (Issue #) Addressed Explanation
Remove up-axis transform logic for MJWarp Solver (#326)

Assessment against linked issues: Out-of-scope changes

No out-of-scope changes found.
"""


📜 Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc9d27f and f86324d.

📒 Files selected for processing (3)
  • docs/migration.rst (1 hunks)
  • newton/geometry/kernels.py (9 hunks)
  • newton/solvers/mujoco/solver_mujoco.py (12 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@eric-heiden eric-heiden changed the title Remove +Y up axis handling logic #326 Remove +Y up axis handling logic in MuJoCoSolver #326 Jul 10, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (3)
newton/solvers/mujoco/solver_mujoco.py (3)

521-524: Critical: Remove remaining up-axis == 1 handling.

This kernel still contains conditional logic for Y-up axis, which should be removed according to the PR objective.

-    if up_axis == 1:
-        body_ipos[worldid, mjc_idx] = wp.vec3f(body_com[tid][0], -body_com[tid][2], body_com[tid][1])
-    else:
-        body_ipos[worldid, mjc_idx] = body_com[tid]
+    body_ipos[worldid, mjc_idx] = body_com[tid]

742-750: Critical: Remove remaining up-axis == 1 handling in geom properties.

This section still contains Y-up axis conversion logic that should be removed.

-    if up_axis == 1:
-        # MuJoCo uses Z-up, Newton Y-up requires conversion
-        # for static geoms, position conversion is handled by perm_position flag in add_geoms
-        if body_idx == -1:
-            pos = wp.vec3f(pos[0], -pos[2], pos[1])
-        rot_y2z = wp.quat_from_axis_angle(wp.vec3(1.0, 0.0, 0.0), -wp.pi * 0.5)
-        quat = rot_y2z * quat

Also remove the now-obsolete comment about perm_position flag since that parameter was removed.


494-499: Remove all residual +Y up-axis branches in solver_mujoco.py

The following instances still reference up_axis and must be removed or refactored to fully drop +Y up‐axis support:

• Line 494: if up_axis == 1: in convert_body_xforms_to_warp_kernel
• Line 521: if up_axis == 1: in update_body_mass_ipos_kernel
• Line 573: # if up_axis == 1: (commented) in update_geom_properties_kernel — remove dead code
• Line 737: if up_axis == 2 and body_idx == -1: — verify necessity or remove
• Line 742: if up_axis == 1: in update_geom_properties_kernel

Please eliminate these conditionals so that all +Y up-axis handling is consistently removed.

🧹 Nitpick comments (1)
newton/solvers/mujoco/solver_mujoco.py (1)

85-113: LGTM! Simplified coordinate conversion by removing axis swapping.

The removal of conditional axis swapping for positions and velocities is consistent with the PR objective of removing +Y up axis handling.

Note: The up_axis parameter on line 66 is no longer used in the JOINT_FREE case after these changes. Consider removing it or documenting why it's retained for other joint types.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 96164c8 and 4b8d4f9.

📒 Files selected for processing (1)
  • newton/solvers/mujoco/solver_mujoco.py (12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
newton/solvers/mujoco/solver_mujoco.py (1)
newton/sim/builder.py (1)
  • add_body (685-746)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: run-newton-tests / newton-unittests (windows-latest)
  • GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
  • GitHub Check: run-asv-benchmarks
🔇 Additional comments (4)
newton/solvers/mujoco/solver_mujoco.py (4)

163-185: LGTM! Consistent removal of axis transformations.

The direct assignment of positions, quaternions, and velocities without conditional transformations aligns with the PR objective.


1211-1211: LGTM! Simplified gravity assignment.

Direct assignment of gravity without coordinate conversion is correct for removing up-axis handling.


1407-1498: Breaking change: Removed position permutation parameter.

The removal of perm_position parameter from add_geoms is a breaking change that affects how static geometry positions are handled. Ensure this change is documented in the migration guide mentioned in the PR description.


1299-1300: No action required: the –90° rotation is intentional and consistent.

The switch from +π/2 to –π/2 around the X-axis now matches all other rot_y2z uses (up_axis handling, static‐shape alignment, and default cases) and aligns correctly with the accompanying position swaps.

Comment thread newton/solvers/mujoco/solver_mujoco.py Outdated
Comment thread newton/solvers/mujoco/solver_mujoco.py Outdated
eric-heiden and others added 3 commits July 10, 2025 15:28
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Viktor Reutskyy <vreutskyy@nvidia.com>
@adenzler-nvidia

Copy link
Copy Markdown
Member

@eric-heiden I guess this needs a mention here: #378

vreutskyy and others added 4 commits July 11, 2025 13:38
…nes-cylinders-planes

Signed-off-by: Viktor Reutskyy <vreutskyy@nvidia.com>
Signed-off-by: Viktor Reutskyy <vreutskyy@nvidia.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
newton/solvers/mujoco/solver_mujoco.py (1)

729-733: Good extension of rotation correction to planes.

Adding planes to the shape types requiring rotation correction is logical and consistent with the axis standardization effort.

🧹 Nitpick comments (1)
newton/solvers/mujoco/solver_mujoco.py (1)

737-745: Remove debug print statement.

The debug print statement will spam the output and impact performance. Consider removing it or making it conditional on a debug flag.

-    wp.printf(
-        "pos before: %.3f %.3f %.3f   after: %.3f %.3f %.3f\n",
-        geom_pos[worldid, geom_idx][0],
-        geom_pos[worldid, geom_idx][1],
-        geom_pos[worldid, geom_idx][2],
-        pos[0],
-        pos[1],
-        pos[2],
-    )
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b8d4f9 and 6eec55e.

📒 Files selected for processing (1)
  • newton/solvers/mujoco/solver_mujoco.py (14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
  • GitHub Check: run-newton-tests / newton-unittests (windows-latest)
  • GitHub Check: pull-request-docs
  • GitHub Check: run-asv-benchmarks
🔇 Additional comments (14)
newton/solvers/mujoco/solver_mujoco.py (14)

85-86: Good simplification of coordinate conversion.

The removal of conditional up-axis handling streamlines the coordinate conversion logic. This assumes a standardized axis convention across the system.


111-113: Consistent velocity conversion simplification.

The direct assignment of linear velocity components aligns with the position conversion changes and removes the Y-up axis special case.


163-164: Consistent reverse coordinate conversion.

The position conversion logic correctly mirrors the forward conversion changes.


173-176: Streamlined quaternion conversion.

The direct quaternion component assignment removes the conditional axis correction and maintains the wxyz order conversion.


183-185: Completes velocity conversion simplification.

The linear velocity conversion is now consistent with the forward conversion and removes the Y-up axis special case.


1202-1208: Improved function naming and symmetry.

The renaming to quat_to_mjc and addition of quat_from_mjc provides better naming consistency and bidirectional conversion symmetry.


1212-1212: Direct gravity assignment removes up-axis complexity.

The direct assignment of the gravity vector from the model is consistent with removing the up-axis handling logic.


1410-1410: Good simplification of function signature.

Removing the perm_position parameter simplifies the function and is consistent with removing the up-axis handling logic.


1426-1427: Streamlined transform assignment.

The direct transform assignment removes the conditional position permutation logic and is consistent with the axis standardization.


1447-1448: Consistent transform handling.

The transform assignment logic is now consistent throughout the function without conditional axis handling.


1506-1524: Simplified body creation logic.

The removal of conditional axis handling in body creation streamlines the code and maintains consistency with the overall axis standardization effort.


1699-1713: Improved approach for shape transform corrections.

Computing the corrective transforms after model compilation is more robust than predicting them beforehand. This ensures accurate transform corrections based on MuJoCo's actual internal transforms.


1-2007: Well-executed refactoring removing up-axis complexity.

The changes consistently remove conditional up-axis handling throughout the MuJoCo solver, resulting in cleaner and more maintainable code. The standardization on a single axis convention eliminates the need for coordinate permutations and conditional quaternion rotations.

Key improvements:

  • Simplified coordinate conversion kernels
  • Streamlined transform handling
  • Consistent quaternion conversion
  • Robust post-compilation transform corrections

The refactoring aligns well with the PR objective of removing +Y up axis handling logic.


1299-1300: Rotation direction change requires manual verification

The sign of the 90° rotation about the X-axis was flipped (now –π/2 instead of +π/2) in newton/solvers/mujoco/solver_mujoco.py (lines 1299–1300). We couldn’t locate any other uses of this quaternion to confirm the intended axis convention.

Please manually verify that rotating by –90° around X correctly maps the Y axis to Z under the new convention:

  • Check the convert_to_mjc coordinate‐system mapping logic for Y→Z conversion.
  • Add or run a simple unit test: apply rot_y2z to wp.vec3(0,1,0) and ensure the result is (0,0,1).
  • Visually inspect a representative Mujoco model (e.g., a capsule) to confirm correct alignment.

…rs-planes' of github.com:vreutskyy/newton into mujoco-up-axis

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>
@eric-heiden eric-heiden changed the title Remove +Y up axis handling logic in MuJoCoSolver #326 Fix mesh geom transform issues, remove +Y up axis handling logic in MuJoCoSolver #326 Jul 11, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
newton/solvers/mujoco/solver_mujoco.py (1)

66-66: Consider removing unused up_axis parameters.

The up_axis parameter is still passed to several kernels but is no longer used in the logic. Consider removing these unused parameters to clean up the API.

 @wp.kernel
 def convert_mj_coords_to_warp_kernel(
     qpos: wp.array2d(dtype=wp.float32),
     qvel: wp.array2d(dtype=wp.float32),
     joints_per_env: int,
-    up_axis: int,
     joint_type: wp.array(dtype=wp.int32),
     # ... rest of parameters
 ):

Apply similar changes to other kernels that no longer use the up_axis parameter.

Also applies to: 144-144, 234-234, 498-498, 528-528

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6eec55e and 4ac7ae3.

📒 Files selected for processing (12)
  • newton/geometry/kernels.py (9 hunks)
  • newton/sim/builder.py (8 hunks)
  • newton/solvers/mujoco/solver_mujoco.py (12 hunks)
  • newton/tests/test_collision.py (1 hunks)
  • newton/tests/test_import_urdf.py (1 hunks)
  • newton/tests/test_mujoco_solver.py (0 hunks)
  • newton/tests/test_rigid_contact.py (1 hunks)
  • newton/tests/test_up_axis.py (1 hunks)
  • newton/utils/import_mjcf.py (2 hunks)
  • newton/utils/import_urdf.py (2 hunks)
  • newton/utils/import_usd.py (2 hunks)
  • newton/utils/render.py (2 hunks)
💤 Files with no reviewable changes (1)
  • newton/tests/test_mujoco_solver.py
✅ Files skipped from review due to trivial changes (1)
  • newton/tests/test_up_axis.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
newton/tests/test_collision.py (1)
newton/tests/unittest_utils.py (1)
  • assert_np_equal (240-246)
newton/tests/test_rigid_contact.py (1)
newton/core/types.py (1)
  • Axis (64-121)
newton/utils/import_usd.py (3)
newton/sim/builder.py (2)
  • add_shape_capsule (1925-1970)
  • add_shape_cone (2019-2064)
newton/core/types.py (1)
  • Axis (64-121)
newton/core/spatial.py (1)
  • quat_between_axes (237-257)
newton/solvers/mujoco/solver_mujoco.py (1)
newton/sim/builder.py (1)
  • add_body (685-746)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: run-asv-benchmarks
  • GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
  • GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (35)
newton/utils/import_urdf.py (1)

161-161: LGTM - Parameter name standardization

The parameter name changes from up_axis to axis align with the broader standardization effort across the codebase for shape orientation parameters. The functionality remains unchanged.

Also applies to: 172-172

newton/tests/test_rigid_contact.py (1)

118-118: LGTM - Parameter name update

The parameter name change from up_axis to axis is consistent with the API standardization across the codebase. The test logic remains unchanged.

newton/tests/test_collision.py (1)

784-786: LGTM - Added tolerance for numerical precision

The addition of tol=1e-6 to the assert_np_equal calls appropriately handles small numerical differences that may result from the coordinate system convention changes (Y-up to Z-up). The tolerance is appropriately small to maintain test accuracy while accounting for floating-point precision differences.

newton/utils/import_mjcf.py (1)

379-379: LGTM - Parameter name standardization

The parameter name changes from up_axis to axis for both cylinder and capsule geometries align with the codebase-wide standardization effort. The functionality remains unchanged.

Also applies to: 388-388

newton/tests/test_import_urdf.py (1)

198-198: LGTM - Updated expected rotation for new axis conventions

The change from a 90-degree rotation around the X-axis ([0.7071068, 0.0, 0.0, 0.7071068]) to the identity quaternion ([0.0, 0.0, 0.0, 1.0]) correctly reflects the new Z-up coordinate system conventions. This eliminates the special-case rotation that was previously needed for Y-up axis handling.

newton/utils/import_usd.py (2)

870-870: LGTM: Parameter renaming is consistent with the broader refactoring.

The parameter renaming from up_axis to axis in the shape creation methods aligns with the standardization shown in the relevant code snippets from newton/sim/builder.py. This change maintains consistency across the codebase.

Also applies to: 877-877, 883-883, 890-890


918-922: LGTM: Plane axis convention updated correctly.

The plane handling logic correctly implements the switch from +Y to +Z axis convention. The comment clarifies that "Warp uses +Z convention for planes" and the conditional logic appropriately applies rotation when the axis is not Z, using the quat_between_axes function to compute the correct transformation.

newton/utils/render.py (2)

178-178: LGTM: Ground plane normal correctly updated for Z-up convention.

The ground plane normal vector is correctly changed from (0,1,0) to (0,0,1) to align with the Z-up coordinate system convention. This ensures the plane lies in the XY-plane with normal along +Z.


237-240: LGTM: Shape rotation correction properly implements Z-up alignment.

The rotation correction applies a -90 degree rotation around the X-axis for capsule, cylinder, and cone geometries, which correctly aligns these shapes' default axis from +Y to +Z. This is consistent with the coordinate system standardization across the codebase and matches the changes in newton/sim/builder.py where the default axis for these shapes was changed to Z.

newton/solvers/mujoco/solver_mujoco.py (9)

85-86: Clean removal of Y-up axis handling in coordinate conversion.

The direct copying of position and velocity components without conditional axis swapping correctly standardizes the coordinate conversion to a single axis convention.

Also applies to: 111-113


163-164: Consistent coordinate conversion simplification.

The reverse conversion kernel correctly mirrors the simplification in the forward conversion, maintaining consistency in the coordinate transformation pipeline.

Also applies to: 173-176, 183-185


1169-1175: Simplified quaternion conversion functions.

The renamed and simplified quaternion conversion functions correctly remove axis-dependent quaternion rotations, providing straightforward wxyz ↔ xyzw conversion.


1179-1179: Direct gravity assignment simplification.

Setting gravity directly from model.gravity without conditional axis permutation correctly standardizes the gravity handling.


1374-1413: Improved transform handling in geometry addition.

The refactored add_geoms function with the incoming_xform parameter provides a cleaner approach to handling transforms. The direct application of transforms removes the need for conditional axis handling.


1465-1473: Streamlined body and joint transform handling.

The direct usage of joint parent transforms and conditional application of incoming transforms correctly removes axis-specific handling while maintaining proper transform composition.


1477-1482: Clean body name uniqueness handling.

The refactored unique name generation logic is cleaner and more maintainable than the previous approach.


1487-1490: Direct body creation without axis permutation.

The direct usage of transform components and COM values correctly removes conditional axis handling in body creation.


1658-1672: Improved post-compilation transform correction approach.

Computing corrective transforms after model compilation rather than applying corrections during construction is a cleaner architectural approach. This should provide more accurate transform handling.

newton/sim/builder.py (7)

780-816: LGTM: Plane equation and orientation correctly updated to Z-up convention

The changes properly implement the coordinate system shift:

  • Default plane equation changed from Y-up (0,1,0,0) to Z-up (0,0,1,0)
  • Documentation updated to reflect XY plane at Z=0 instead of XZ plane at Y=0
  • Quaternion rotation correctly uses +Z axis as reference instead of +Y

931-958: LGTM: Capsule orientation updated to Z-up convention

The changes properly implement the coordinate system shift:

  • Parameter renamed from up_axis to axis for clarity
  • Default axis changed from Axis.Y to Axis.Z
  • Documentation updated to reflect Z-axis alignment
  • Internal rotation logic correctly uses Z-axis as reference

978-1005: LGTM: Cylinder orientation updated to Z-up convention

The changes are consistent with the capsule method and properly implement the coordinate system shift:

  • Parameter renamed from up_axis to axis for clarity
  • Default axis changed from Axis.Y to Axis.Z
  • Documentation updated to reflect Z-axis alignment
  • Internal rotation logic correctly uses Z-axis as reference

1025-1052: LGTM: Cone orientation updated to Z-up convention

The changes maintain consistency with the capsule and cylinder methods:

  • Parameter renamed from up_axis to axis for clarity
  • Default axis changed from Axis.Y to Axis.Z
  • Documentation updated to reflect Z-axis alignment for base to apex direction
  • Internal rotation logic correctly uses Z-axis as reference

845-851: Ground plane implementation is consistent with Z-up convention

The add_ground_plane method correctly uses self.up_vector which is computed from self.up_axis (defaulting to Axis.Z). This ensures the ground plane is properly oriented according to the new coordinate system convention.


254-406: Coordinate system convention changes are well-implemented

The constructor properly initializes with up_axis: AxisType = Axis.Z and gravity: float = -9.81, which is consistent with the new Z-up convention. The up_vector property correctly computes the 3D vector from the axis setting.

These changes align well with the broader coordinate system standardization effort described in the PR objectives.


931-1052: Ignore changes to up_axis in import utilities and tests

The up_axis parameter in utilities, importers, examples, and tests refers to the scene’s up‐axis (Y vs Z) and is intentionally distinct from shape‐orientation’s axis parameter in the builder methods. All found references to up_axis are correct and should remain unchanged.

Likely an incorrect or invalid review comment.

newton/geometry/kernels.py (10)

237-256: LGTM! Capsule orientation correctly updated to Z-axis.

The capsule SDF and gradient functions have been properly updated to use the Z-axis (index 2) for height calculations instead of the Y-axis (index 1), consistent with the Z-up convention.


259-272: LGTM! Cylinder orientation correctly updated to Z-axis.

The cylinder SDF and gradient functions properly compute radial distance in the XY plane and height along the Z-axis, consistent with the new coordinate convention.


275-288: LGTM! Cone orientation correctly updated to Z-axis.

The cone SDF and gradient functions have been properly updated to orient the cone along the Z-axis with radius variation in the XY plane.


291-297: Plane SDF correctly updated for XY plane orientation.

The function now properly computes the SDF for a plane in the XY plane with Z as the normal direction. The existing comment "SDF for a quad in the xy plane" is now accurate with this change.


300-311: LGTM! Plane projection correctly updated to XY plane.

The function now properly projects points onto the XY plane (Z=0) with appropriate clamping of X and Y coordinates based on width and length parameters.


381-392: LGTM! Plane edge generation correctly updated for XY plane.

The function now properly generates edges for a plane in the XY plane with all Z-coordinates set to 0.


747-747: Plane normals consistently updated throughout collision detection.

All plane normal vectors have been correctly changed from (0,1,0) to (0,0,1), maintaining consistency with the XY plane orientation where Z is the normal direction.

Also applies to: 1206-1206, 1265-1265, 1303-1303, 1398-1398, 1419-1419, 1510-1510


1390-1391: LGTM! Capsule collision points correctly updated to Z-axis orientation.

The capsule endpoint calculations in collision detection have been properly updated to use the Z-axis for height offsets, maintaining consistency with the new capsule orientation.

Also applies to: 1415-1417


1507-1511: LGTM! Plane boundary checks correctly updated for XY orientation.

The boundary verification now properly checks X and Y coordinates against plane width and length, consistent with the XY plane orientation.


237-1511: Action required: Update Y-up references to Z-up across dependent code

The Z-up conversion is correctly implemented in newton/geometry/kernels.py, but the following areas still assume Y-up and must be updated:

• Utility modules

  • newton/utils/render.py (up_axis handling, y_axis = wp.vec3(0.0, 1.0, 0.0))
  • newton/utils/import_usd.py, import_mjcf.py, isaaclab.py (builder.up_axis assignments)

• Simulation builder

  • newton/sim/model.py (self.up_axis = 2)
  • newton/sim/builder.py (propagating up_axis)

• Quaternion/spatial helpers

  • newton/utils/init.py (wp.quat_rotate with Y axis)
  • newton/sim/articulation.py (multiple wp.quat_rotate(q, wp.vec3(0.0, 1.0, 0.0)))
  • newton/core/spatial.py, solvers/featherstone/kernels.py, solvers/euler/kernels.py

• Tests (all up_axis and vec3 Y-up patterns)

  • test_up_axis.py, test_mujoco_solver.py, test_import_urdf.py, test_collision.py, test_joint_controllers.py, test_joint_drive.py, test_implicit_mpm.py, test_import_mjcf.py, test_cloth.py, test_body_force.py, test_kinematics.py, test_model.py, test_gjk.py

• Examples under newton/examples/ (up_axis="Y", wp.vec3(0.0, 1.0, 0.0))

Next steps:

  1. Replace all wp.vec3(0.0, 1.0, 0.0) Y-up vectors with wp.vec3(0.0, 0.0, 1.0) Z-up equivalents.
  2. Update builder default up_axis values and all ModelBuilder(up_axis=…) calls in tests and examples.
  3. Adjust tests to expect Z-up behavior.
  4. Expand the migration guide to list these files and codemod steps.

eric-heiden and others added 3 commits July 11, 2025 10:05
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>

@adenzler-nvidia adenzler-nvidia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Impossible to tell whether we caught everything, but we'll find the bugs as we go.

Very cool to clean up all of this up-axis handling! Thanks!

@adenzler-nvidia

Copy link
Copy Markdown
Member

Does this need a warp.sim migration guide update?

@eric-heiden eric-heiden enabled auto-merge (squash) July 14, 2025 16:26
@eric-heiden eric-heiden merged commit e88773b into newton-physics:main Jul 14, 2025
8 of 9 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Oct 7, 2025
4 tasks
eric-heiden added a commit to eric-heiden/newton that referenced this pull request Jan 28, 2026
…uJoCoSolver newton-physics#326 (newton-physics#376)

Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Viktor Reutskyy <vreutskyy@nvidia.com>
Co-authored-by: Viktor Reutskyy <vreutskyy@nvidia.com>
mmacklin pushed a commit to mmacklin/newton that referenced this pull request Apr 7, 2026
…uJoCoSolver newton-physics#326 (newton-physics#376)

Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Viktor Reutskyy <vreutskyy@nvidia.com>
Co-authored-by: Viktor Reutskyy <vreutskyy@nvidia.com>
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.

Use +Z as default up axis for capsules, cones, cylinders, planes Remove Up-Axis Transform Logic for MJWarp Solver

3 participants