Fix SolverMuJoCo to follow Newton convention around spatial velocities#1350
Conversation
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>
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughMuJoCo↔Newton conversions and solver flows were made CoM‑aware: conversion kernels now accept per‑body Changes
Sequence Diagram(s)sequenceDiagram
participant N as Newton State
participant K as Conversion Kernel
participant M as MuJoCo Model
Note over N,M: Conversions handle COM vs origin shifts and angular-frame changes
N->>K: send body_q, joint_qd (v_com, ω_world), joint_child, body_com
K->>K: ω_body = R^T * ω_world
K->>K: r_world = R * body_com
K->>K: v_origin = v_com − ω_world × r_world
K->>M: write qvel = [v_origin, ω_body]
M->>K: send body_q, qvel = [v_origin, ω_body], joint_child, body_com
K->>K: ω_world = R * ω_body
K->>K: r_world = R * body_com
K->>K: v_com = v_origin + ω_world × r_world
K->>N: write joint_qd = [v_com, ω_world]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes the SolverMuJoCo to correctly handle spatial velocities when bodies have non-zero center of mass (CoM) offsets. The fix involves converting between MuJoCo's body frame origin velocity convention and Newton's CoM velocity convention.
Changes:
- Adds comprehensive unit tests verifying correct rotation and translation behavior for bodies with non-zero CoM offsets
- Implements velocity conversion kernels that properly transform between body origin and CoM reference points
- Updates documentation to clarify velocity conventions and limitations across different solvers
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
newton/tests/test_body_velocity.py |
New comprehensive test suite covering angular, linear, and combined velocity scenarios for free-floating bodies with CoM offsets |
newton/_src/solvers/mujoco/kernels.py |
Implements bidirectional velocity conversions between MuJoCo (body origin) and Newton (CoM) coordinate systems |
newton/_src/solvers/mujoco/solver_mujoco.py |
Adds required parameters (joint_child, body_com, bodies_per_world) to kernel invocations |
docs/concepts/conventions.rst |
Clarifies MuJoCo velocity conventions, documents the conversion formulas, and adds warning about SolverFeatherstone limitations |
docs/tutorials/00_introduction.ipynb |
Simplifies XPBD description to be more neutral |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/concepts/conventions.rst`:
- Around line 63-73: The Sphinx cross-reference :ref:`MuJoCo conversion` uses a
label with spaces/case and breaks the docs build; replace the inline ref with a
valid label reference (e.g. change :ref:`MuJoCo conversion` to :ref:`MuJoCo
conversion <mujoco-conversion>` or simply :ref:`mujoco-conversion`) and ensure
the target label definition is renamed to a matching safe identifier (e.g.
add/rename the section label to .. _mujoco-conversion:) so the ref resolves;
apply the same fix for the other occurrence around lines 183-207.
In `@newton/tests/test_body_force.py`:
- Around line 256-635: Enable MuJoCo torque-CoM tests by setting the
supports_torque_com flag to True for the "mujoco_cpu" and "mujoco_warp" entries
in the com_solvers dict (or add an expected-failure guard if MuJoCo still fails)
so the torque-CoM tests actually exercise your conversion; and fix the unused
tolerance in test_combined_force_torque (function test_combined_force_torque) by
applying the passed tolerance to the assertions (e.g., use tolerance for the
com_displacement X check and/or the quaternion dot-product threshold) or remove
the tolerance parameter and its call-site arg if you prefer.
- Around line 46-50: compute_com_world_position currently calls
wp.transform(*body_q) which expands the flattened numpy row into positional
arguments instead of constructing a transform from a position vec3 and a
quaternion; fix by slicing body_q into pos = body_q[:3] and quat = body_q[3:7],
build the transform with wp.transform(wp.vec3(*pos), wp.quat(*quat)), then call
wp.transform_point(...) with that transform and wp.vec3(*body_com) and return
the resulting numpy array; update references in compute_com_world_position and
ensure quaternion component order matches the existing [qx,qy,qz,qw] layout.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/concepts/conventions.rstdocs/tutorials/00_introduction.ipynbnewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_body_force.pynewton/tests/test_body_velocity.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
CLI arguments must use
kebab-case(e.g.,--use-cuda-graph, not--use_cuda_graph)
Files:
docs/tutorials/00_introduction.ipynbnewton/_src/solvers/mujoco/solver_mujoco.pydocs/concepts/conventions.rstnewton/tests/test_body_velocity.pynewton/_src/solvers/mujoco/kernels.pynewton/tests/test_body_force.py
newton/_src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
newton/_src/directory is internal library implementation only and must not be imported by user code (examples, documentation, or external users)
Files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/solvers/mujoco/kernels.py
newton/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Any user-facing class/function/object added under
_srcmust be exposed via the public Newton API through re-exports
Files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_body_velocity.pynewton/_src/solvers/mujoco/kernels.pynewton/tests/test_body_force.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use prefix-first naming for classes:ActuatorPD,ActuatorPID(notPDActuator,PIDActuator)
Use prefix-first naming for methods:add_shape_sphere()(notadd_sphere_shape())
Method names must usesnake_case
Prefer nested classes when self-contained: if a helper type or enum is only meaningful inside one parent class and doesn't need a public identity, define it as a nested class instead of a top-level class/module
Follow PEP 8 for Python code
Use Google-style docstrings with clear, concise explanations of what functions do, their parameters, and return values
Files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_body_velocity.pynewton/_src/solvers/mujoco/kernels.pynewton/tests/test_body_force.py
🧠 Learnings (25)
📓 Common learnings
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.
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.
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: jumyungc
Repo: newton-physics/newton PR: 1333
File: newton/_src/sim/builder.py:0-0
Timestamp: 2026-01-13T03:11:57.249Z
Learning: Repo: newton-physics/newton — VBD conventions
- Radians are treated as dimensionless; prefer wording “torque per radian” rather than writing “N·m/rad” in docs.
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: jumyungc
Repo: newton-physics/newton PR: 1333
File: newton/_src/sim/builder.py:0-0
Timestamp: 2026-01-13T03:11:57.249Z
Learning: Repo: newton-physics/newton — VBD damping semantics
- Joint/cable damping in SolverVBD is Rayleigh-style and treated as unitless; same convention applies across VBD constraints.
Learnt from: nvtw
Repo: newton-physics/newton PR: 747
File: newton/_src/viewer/kernels.py:0-0
Timestamp: 2025-09-10T10:09:05.676Z
Learning: In Newton physics picking implementation (newton/_src/viewer/kernels.py), damping is intentionally applied using center of mass velocity (vel_com) rather than velocity at the pick point. This is a deliberate design choice for stability improvements.
📚 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 STYLE3D solver in Newton is a new solver that did not exist in warp.sim, so it does not require any migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 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:
docs/tutorials/00_introduction.ipynbnewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/solvers/mujoco/kernels.py
📚 Learning: 2026-01-13T03:11:57.249Z
Learnt from: jumyungc
Repo: newton-physics/newton PR: 1333
File: newton/_src/sim/builder.py:0-0
Timestamp: 2026-01-13T03:11:57.249Z
Learning: Repo: newton-physics/newton — VBD damping semantics
- Joint/cable damping in SolverVBD is Rayleigh-style and treated as unitless; same convention applies across VBD constraints.
Applied to files:
docs/tutorials/00_introduction.ipynbdocs/concepts/conventions.rst
📚 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/solver_mujoco.pydocs/concepts/conventions.rstnewton/_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, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pydocs/concepts/conventions.rstnewton/_src/solvers/mujoco/kernels.py
📚 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.pydocs/concepts/conventions.rstnewton/_src/solvers/mujoco/kernels.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: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.pynewton/_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/solver_mujoco.pydocs/concepts/conventions.rstnewton/_src/solvers/mujoco/kernels.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.pydocs/concepts/conventions.rst
📚 Learning: 2026-01-13T03:11:40.556Z
Learnt from: jumyungc
Repo: newton-physics/newton PR: 1333
File: newton/_src/sim/builder.py:0-0
Timestamp: 2026-01-13T03:11:40.556Z
Learning: Ensure all VBD damping semantics follow Rayleigh-style damping and are unitless. This convention, demonstrated in SolverVBD, should be applied consistently across all VBD-related constraints in the codebase. In newton/_src/sim/builder.py, validate that any damping terms use the same normalization and are treated as dimensionless; add comments and unit tests to enforce consistency across VBD components.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_body_velocity.pynewton/_src/solvers/mujoco/kernels.pynewton/tests/test_body_force.py
📚 Learning: 2026-01-13T03:11:57.249Z
Learnt from: jumyungc
Repo: newton-physics/newton PR: 1333
File: newton/_src/sim/builder.py:0-0
Timestamp: 2026-01-13T03:11:57.249Z
Learning: Repo: newton-physics/newton — VBD conventions
- Radians are treated as dimensionless; prefer wording “torque per radian” rather than writing “N·m/rad” in docs.
Applied to files:
docs/concepts/conventions.rst
📚 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:
docs/concepts/conventions.rstnewton/_src/solvers/mujoco/kernels.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:
docs/concepts/conventions.rstnewton/_src/solvers/mujoco/kernels.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:
docs/concepts/conventions.rstnewton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-08-25T11:07:47.818Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
Applied to files:
docs/concepts/conventions.rst
📚 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: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
docs/concepts/conventions.rst
📚 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:
docs/concepts/conventions.rstnewton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-11-28T11:12:40.030Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1146
File: newton/examples/basic/example_basic_joints.py:215-222
Timestamp: 2025-11-28T11:12:40.030Z
Learning: In Warp's spatial vector convention used by Newton: spatial_bottom() extracts the angular velocity component (omega) and spatial_top() extracts the linear velocity component (v) from a spatial twist (qd). Similarly for spatial wrenches: spatial_bottom() is torque and spatial_top() is force.
Applied to files:
docs/concepts/conventions.rst
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is re-exported from the spatial module via "from .spatial import transform_twist", making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
docs/concepts/conventions.rst
📚 Learning: 2025-12-25T21:44:17.047Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: docs/concepts/sensors.rst:12-12
Timestamp: 2025-12-25T21:44:17.047Z
Learning: Future API standardization: The sensor.update() method should be standardized to a single-argument form update(state) across all sensor types. The redundant model parameter will be removed from sensors like SensorFrameTransform. In code reviews, check for API changes that move toward a single-argument update, ensure all sensor implementations conform, and update user-facing docs to reflect the change. If reviewing related code, verify that calls using the old two-argument form are refactored and that any dependent serialization or tests are updated accordingly.
Applied to files:
docs/concepts/conventions.rst
📚 Learning: 2026-01-13T07:40:13.635Z
Learnt from: CR
Repo: newton-physics/newton PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T07:40:13.635Z
Learning: Applies to newton/examples/**/*.py : Examples must implement a `test_final()` method that verifies the simulation state is valid after the example has run
Applied to files:
newton/tests/test_body_velocity.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/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
🧬 Code graph analysis (2)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (1)
body_count(1142-1146)newton/_src/sim/state.py (1)
body_count(191-195)
newton/tests/test_body_velocity.py (1)
newton/tests/unittest_utils.py (2)
add_function_test(320-339)get_test_devices(98-132)
🪛 GitHub Actions: Pull Request
docs/concepts/conventions.rst
[error] 63-63: Sphinx: Failed cross-reference (title not found) for 'mujoco conversion' [ref.ref]. Build treated warnings as errors; exit code 1.
🪛 Ruff (0.14.11)
newton/_src/solvers/mujoco/kernels.py
388-388: Unused function argument: bodies_per_world
(ARG001)
483-483: Unused function argument: bodies_per_world
(ARG001)
newton/tests/test_body_force.py
462-462: Unused function argument: tolerance
(ARG001)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (12)
docs/tutorials/00_introduction.ipynb (1)
259-264: Tutorial solver selection text looks fine and stays solver-agnostic enough for an intro.newton/_src/solvers/mujoco/solver_mujoco.py (1)
947-967: Kernel plumbing looks consistent with the new signatures.
Only thing to watch: if kernels are updated to truly usebodies_per_worldfor indexing, this call-site is already correct.Also applies to: 996-1016
newton/_src/solvers/mujoco/kernels.py (1)
376-392:bodies_per_worldis unused parameter and should be removed from kernel signatures.The parameter appears in the function signatures at lines 386, 388 (first kernel) and 483 (second kernel) but is never referenced within the kernel implementations. The
joint_childarray is already globally indexed via_flatten_body_array()in the host code (mujoco_world.py), sobody_com[joint_child[jntid]]correctly accesses the body center-of-mass across multiple worlds without requiring per-world offset calculations in the kernel itself. Remove the unused parameter to eliminate dead code warnings and improve clarity.newton/tests/test_body_velocity.py (9)
16-32: Well-documented module with clear explanations.The docstring effectively explains the test purpose, solver distinctions (generalized vs. maximal coordinates), and tolerance differences. This aligns well with the retrieved learnings about Newton's spatial twist conventions.
34-44: LGTM!The imports are minimal and appropriate. The pattern of an empty
TestBodyVelocityclass with dynamically added tests follows established project patterns per the relevant code snippets fromunittest_utils.py.
47-50: LGTM!The helper correctly transforms the local CoM to world frame. The docstring is brief but sufficient for an internal test utility.
53-141: Well-structured test with correct validation logic.The test correctly validates:
- CoM stays stationary when only angular velocity is applied (rotation about CoM)
- The body actually rotated (quaternion dot product check)
The use of
.copy()on line 109 is appropriate sincebody_q_initialis referenced after simulation for the quaternion comparison.
144-220: LGTM!The test correctly validates linear velocity behavior. The computation of
com_initialat line 194 immediately captures the necessary value before simulation steps modify state, so the missing.copy()on line 192 is acceptable.
223-305: LGTM!The combined velocity test correctly validates that CoM translation is independent of rotation, and properly uses
.copy()on line 271 for the later quaternion comparison.
310-342: Good documentation of solver limitations.The detailed comment explaining Featherstone's known issues (lines 311-316) is helpful for maintainability. The tolerance differences between generalized coordinate solvers (1e-3) and maximal coordinate solvers (1e-4) align with the module docstring's explanation about internal velocity representation.
364-410: Consider including device in test names for cross-device clarity.The test names use format
test_angular_com_stationary_{solver_name}_com{i}_ang{j}but passdevices=[device]toadd_function_test. Per theunittest_utils.pysnippet, whendevicesis a list, the utility appends a device suffix (e.g.,_cpu,_cuda_0) to make test names unique across devices. However, because the outer loop iterates over devices and creates separate tests per device, this should work correctly.The skip logic at lines 366-367 for
mujoco_cpuon CUDA devices is appropriate since the MuJoCo CPU solver requires CPU execution.
413-414: LGTM!Standard test entry point.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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>
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Looks good, only 1 small comment. Thanks for fixing this!
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>
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>
|
tagging @preist-nvidia for a read of the conventions doc. The MuJoCo conversion looks good. |
…therstone) Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
|
In 2bc8c3e I've now updated MuJoCo, XPBD and SemiImplicit to treat joint_f for free joints exactly like body_f (wrench in world frame with the moment referenced at the child body's center of mass). |
|
Thanks @vreutskyy for taking this over. My observations:
|
Signed-off-by: Viktor Reutskyy <vreutskyy@nvidia.com>
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/kernels.py (1)
400-445: Extend CoM velocity conversion to DISTANCE joints in both kernels.
Other kernels in the same file (apply_mjc_qfrc_kernelat line 679, kinematics kernel at line 793) already handleJointType.FREE or JointType.DISTANCEtogether. The conversion functions here must do the same: DISTANCE joints require the same origin↔CoM velocity transformation as FREE joints.Apply to both conversion functions
- if type == JointType.FREE: + if type == JointType.FREE or type == JointType.DISTANCE:Apply this change at:
- Lines 400-445 in
convert_mj_coords_to_warp_kernel- Lines 493-538 in
convert_warp_coords_to_mj_kernel
♻️ Duplicate comments (1)
newton/tests/test_body_force.py (1)
47-57: Unused duplicate helper.
compute_com_world_positionappears unused here and duplicates the helper innewton/tests/test_body_velocity.py. Consider removing to avoid drift.🧹 Suggested cleanup
-def compute_com_world_position(body_q, body_com, body_world, world_offsets=None, body_index: int = 0) -> np.ndarray: - """Compute the center of mass position in world frame.""" - com_world = wp.zeros(body_q.shape[0], dtype=wp.vec3, device=body_q.device) - wp.launch( - kernel=compute_com_positions, - dim=body_q.shape[0], - inputs=[body_q, body_com, body_world, world_offsets], - outputs=[com_world], - device=body_q.device, - ) - return com_world.numpy()[body_index]
| # Two potential reasons for the original test failing: | ||
| # 1. MuJoCo stores velocity at the body origin, not at COM. For freely rotating | ||
| # bodies with COM offset, this causes the COM to naturally drift over time. | ||
| # 2. Our velocity conversion (origin <-> COM) may cause issues for fast-rotating | ||
| # bodies due to orientation mismatch between position and velocity updates. |
There was a problem hiding this comment.
@eric-heiden could you have a look? What do you think about this?
There was a problem hiding this comment.
I also tried all kinds of conversions but it really seems we can't do anything better with qfrc_applied. So I have some work in progress to use xfrc_applied instead for free, distance joints. This is guaranteed to behave like body_f obviously and the unit tests work with sim_dt = 0.01.
This is the explanation I got from Cursor:
MuJoCo represents free-joint generalized forces in a mixed-frame convention in
qfrc_applied. To preserve Newton's
COM-wrench semantics, :class:~newton.solvers.SolverMuJoCoapplies free-joint
:attr:Control.joint_f <newton.Control.joint_f>throughxfrc_applied(world-frame wrench at the COM) and
usesqfrc_appliedonly for non-free joints. This keeps free-jointjoint_fbehavior aligned with
:attr:State.body_f <newton.State.body_f>.We avoid converting free-joint wrenches into
qfrc_applieddirectly becauseqfrc_appliedis generalized-force
space, not a physical wrench. For free joints the 6-DOF basis depends on the currentcdof(subtree COM frame),
and the rotational components are expressed in the body frame. A naive world-to-body rotation is insufficient because
the correct mapping is the Jacobian-transpose operation used internally by MuJoCo (the same path asxfrc_applied).
Routing throughxfrc_appliedensures the wrench is interpreted at the COM in world coordinates and then mapped to
generalized forces consistently with MuJoCo's own dynamics.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@newton/_src/solvers/mujoco/kernels.py`:
- Around line 685-692: The code is rotating the computed angular torque `w` into
the body frame with `wp.quat_rotate_inv(rot, w)` which causes mixed-frame
outputs for FREE/DISTANCE joints; instead leave `w` in world frame. Remove the
final rotation call (the `wp.quat_rotate_inv(rot, w)` use) so after computing
`r_com = wp.quat_rotate(rot, body_com[child])` and `w = w + wp.cross(r_com, v)`
you store `w` directly (world-frame) alongside the linear force `v`, ensuring
`qfrc_applied` supplies both linear and angular components in world coordinates.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
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/_src/solvers/mujoco/kernels.py (1)
386-444: Fix world-specificjoint_childindexing for multi-world models.In both FREE-joint conversion paths,
child = joint_child[jntid]uses the template joint index, which points to world-0 body indices. Forworldid > 0, this can read the wrongbody_com, breaking the COM offset conversion. Use the world-scoped joint index instead.🐛 Proposed fix
- child = joint_child[jntid] + child = joint_child[joints_per_world * worldid + jntid]Apply the same change in both
convert_mj_coords_to_warp_kernelandconvert_warp_coords_to_mj_kernel.Also applies to: 479-537
🤖 Fix all issues with AI agents
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 1407-1422: When external MuJoCo model/data (mjw_model/mjw_data)
are provided and _convert_to_mjc() is skipped, body_free_qd_start can be None
but is passed into apply_mjc_free_joint_f_to_body_f_kernel; add a guard in the
branch that handles control.joint_f to either initialize body_free_qd_start
(build the mapping the same way _convert_to_mjc() would) or raise a clear
exception before launching wp.launch, e.g., check if self.body_free_qd_start is
None and then fail fast with an explicit error message referencing
body_free_qd_start and mjw_model/mjw_data (or call the existing conversion
helper to populate it) so the kernel never receives a None.
newton-physics#1350) Signed-off-by: Eric Heiden <eric-heiden@outlook.com> Signed-off-by: Viktor Reutskyy <vreutskyy@nvidia.com> Co-authored-by: Philipp Reist <preist@nvidia.com> Co-authored-by: Viktor Reutskyy <vreutskyy@nvidia.com> Co-authored-by: Viktor Reutskyy <33062116+vreutskyy@users.noreply.github.com>
Applying linear velocity through
joint_qdto floating bodies with nonzero center of mass resulted in unexpected change in world position of the CoM. We have to convert between MuJoCo's linear velocity in body frame origin and Newton's linear velocity which is defined relative to the CoM.This PR adds a series of unit tests for applying twists to those free-floating bodies and fixes some issues in the documentation section on conventions. Note that SolverFeatherstone currently doesn't follow Newton conventions, which is now called out in the documentation.
Also adds more tests for body forces
body_fapplied to bodies with nonzero CoM.In 2bc8c3e I've now updated MuJoCo, XPBD and SemiImplicit to treat
joint_ffor free joints exactly likebody_f(wrench in world frame with the moment referenced at the child body's center of mass).Summary by CodeRabbit
Documentation
Bug Fixes / Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.