-
Notifications
You must be signed in to change notification settings - Fork 460
Fix multi-world joint indexing in warp-to-mj coord conversion #2332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jsw7460
wants to merge
5
commits into
newton-physics:main
Choose a base branch
from
jsw7460:sangwooshin/fix-warp-coords-kernel-indexing
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+143
−7
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
15dc705
Fix multi-world joint indexing in warp-to-mj coord conversion
jsw7460 a436463
Add CHANGELOG entry for multi-world coordinate conversion fix
jsw7460 085cdb8
Run multi-world CoM test on all devices
jsw7460 66f2b01
Simplify multi-world test: address reviewer feedback
jsw7460 4bee403
Move CHANGELOG entry to Unreleased section
jsw7460 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 The Newton Developers | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Test that per-world body property changes produce correct physics. | ||
|
|
||
| Regression test for a bug in convert_warp_coords_to_mj_kernel where | ||
| joint_type and joint_child were indexed with the per-world joint index | ||
| instead of the global index, causing incorrect CoM references for | ||
| worlds with worldid > 0. | ||
| """ | ||
|
|
||
| import unittest | ||
|
|
||
| import numpy as np | ||
| import warp as wp | ||
|
|
||
| import newton | ||
| from newton.solvers import SolverMuJoCo | ||
| from newton.tests.unittest_utils import add_function_test, get_test_devices | ||
|
|
||
|
|
||
| def _build_model_with_per_world_com(device, world_base_coms): | ||
| """Build a multi-world model where each world has a free-floating | ||
| body with its own base CoM. | ||
| """ | ||
| scene = newton.ModelBuilder() | ||
|
|
||
| for base_com in world_base_coms: | ||
| template = newton.ModelBuilder() | ||
| base = template.add_link( | ||
| mass=5.0, | ||
| com=base_com, | ||
| inertia=wp.mat33(np.eye(3) * 0.1), | ||
| ) | ||
| j_free = template.add_joint_free(parent=-1, child=base) | ||
| template.add_articulation([j_free]) | ||
|
|
||
| scene.begin_world() | ||
| scene.add_builder(template) | ||
| scene.end_world() | ||
|
|
||
| return scene.finalize(device=device) | ||
|
|
||
|
|
||
| def _run_sim(model, num_steps: int = 100, sim_dt: float = 1e-3): | ||
| """Step simulation and return final joint_q as numpy array.""" | ||
| solver = SolverMuJoCo(model) | ||
|
|
||
| state_0 = model.state() | ||
| state_1 = model.state() | ||
| control = model.control() | ||
|
|
||
| # Set initial height and angular velocity for all worlds. | ||
| # Non-zero angular velocity is critical: the bug in | ||
| # convert_warp_coords_to_mj_kernel affects the velocity | ||
| # conversion v_origin = v_com - w x com. With w=0 the cross | ||
| # product vanishes and the wrong com is harmless. | ||
| nw = model.world_count | ||
| q_per_world = model.joint_coord_count // nw | ||
| qd_per_world = model.joint_dof_count // nw | ||
| joint_q = state_0.joint_q.numpy().reshape(nw, q_per_world) | ||
| joint_q[:, 2] = 1.0 # z = 1m for all worlds | ||
| state_0.joint_q.assign(joint_q.flatten()) | ||
| state_1.joint_q.assign(joint_q.flatten()) | ||
| # Set initial angular velocity (world frame) on the free joint | ||
| joint_qd = state_0.joint_qd.numpy().reshape(nw, qd_per_world) | ||
| joint_qd[:, 3] = 1.0 # wx = 1 rad/s | ||
| joint_qd[:, 4] = 0.5 # wy = 0.5 rad/s | ||
| state_0.joint_qd.assign(joint_qd.flatten()) | ||
| state_1.joint_qd.assign(joint_qd.flatten()) | ||
| newton.eval_fk(model, state_0.joint_q, state_0.joint_qd, state_0) | ||
| newton.eval_fk(model, state_1.joint_q, state_1.joint_qd, state_1) | ||
|
|
||
| for _ in range(num_steps): | ||
| state_0.clear_forces() | ||
| solver.step(state_0, state_1, control, None, sim_dt) | ||
| state_0, state_1 = state_1, state_0 | ||
|
|
||
| return state_0.joint_q.numpy() | ||
|
|
||
|
|
||
| def test_perworld_com_produces_consistent_physics(test, device): | ||
| """Multi-world with different per-world base CoM should match | ||
| independent single-world runs with the same CoM.""" | ||
| com_a = wp.vec3(0.0, 0.0, 0.0) | ||
| com_b = wp.vec3(0.05, 0.0, -0.02) | ||
|
|
||
| # --- Reference: single-world runs --- | ||
| ref_q = {} | ||
| for com_val, label in [(com_a, "A"), (com_b, "B")]: | ||
| model = _build_model_with_per_world_com(device, [com_val]) | ||
| ref_q[label] = _run_sim(model) | ||
|
|
||
| # --- Multi-world run --- | ||
| model = _build_model_with_per_world_com(device, [com_a, com_b]) | ||
| multi_q = _run_sim(model) | ||
|
|
||
| q_per_world = model.joint_coord_count // model.world_count | ||
| multi_q = multi_q.reshape(model.world_count, q_per_world) | ||
|
|
||
| np.testing.assert_allclose( | ||
| multi_q[0], | ||
| ref_q["A"], | ||
| atol=1e-4, | ||
| err_msg="World 0 (com_A) diverges from single-world reference", | ||
| ) | ||
| np.testing.assert_allclose( | ||
| multi_q[1], | ||
| ref_q["B"], | ||
| atol=1e-4, | ||
| err_msg="World 1 (com_B) diverges from single-world reference", | ||
| ) | ||
|
|
||
|
|
||
| class TestMultiworldBodyProperties(unittest.TestCase): | ||
| """Verify that multi-world simulations with per-world body mass/CoM | ||
| produce physically consistent results across all worlds.""" | ||
|
|
||
| pass | ||
|
|
||
|
|
||
| devices = get_test_devices() | ||
| for device in devices: | ||
| add_function_test( | ||
| TestMultiworldBodyProperties, | ||
| f"test_perworld_com_produces_consistent_physics_{device}", | ||
| test_perworld_com_produces_consistent_physics, | ||
| devices=[device], | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions about the test scope — I wonder if it can be tightened.
The bug causes world-N to silently read world-0's per-world-replicated data. So with per-world-differing body properties and identical initial joint state, buggy code should produce identical trajectories across worlds while correct code produces divergent ones. Given that:
Do the single-world reference runs add anything beyond "world 0 and world 1 diverge"? It seems like a single multi-world run with a
not np.allclose(multi_q[0], multi_q[1])assertion would catch the same regression at 1/3 the cost.The comment "Using multiple joints/bodies is essential because the indexing bug only manifests when joint_child maps to different bodies across the flat array" — is this actually true? For any
worldid > 0, buggy indexing reads slotjntidinstead ofjoints_per_world * worldid + jntid, so the bug should fire with 1 joint per world as long as body properties differ across worlds. Am I missing something about why the child revolute joint is load-bearing here?The
body_com.numpy().reshape(...).assign(...)patching post-finalize()(with theBODY_INERTIAL_PROPERTIESnotify) — is there a reason to do it this way rather than building each world with its target CoM directly viaadd_builder? The post-hoc mutation seems to be a consequence of usingreplicate, which forces identical worlds.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adenzler-nvidia
Thanks for the review.
After implementing the
assertFalse(allclose(multi_q[0], multi_q[1]))check, I found it doesn't actually detect the bug. Wanted to walk through why in case I'm missing something.Root cause.
notify_model_changed(BODY_INERTIAL_PROPERTIES)syncsmodel.body_cominto the per-worldmjw_model.body_iposarray (see_update_model_inertial_propertieslaunchingupdate_body_mass_ipos_kernel). So MuJoCo's internal dynamics always use the correct per-world CoM, independent of the conversion kernel.The kernel's role is the Newton <-> MuJoCo velocity convention transform
v_origin = v_com - ω × rotate(rot, body_com[child]). With the bug, it makesv_originidentical across worlds (when initialv_com/ω/rotare set identically in the test), but MuJoCo then simulates each world with its own per-worldbody_ipos. Identical initialv_origin+ differentbody_ipos=> different dynamics. Trajectories diverge in the buggy case too.The options I'm considering is either
joint_qd → warp_to_mj → mj_to_warpshould be identity; with the bug it drifts byω × rotate(delta_com)per application. It has no stepping, and directly targets the kernel matth, but calls internal conversion methods rather than the public solver surface.Any preference?
For your second and third points, I agree. Will modify it after we decide (1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right — (1) is the right call, and my original (1) suggestion doesn't work. I missed that
notify_model_changed(BODY_INERTIAL_PROPERTIES)syncs per-worldbody_iposthrough a separately (and correctly) indexed kernel, so MuJoCo steps with the right per-world inertial state regardless of the conversion bug. Trajectories diverge either way, just to the wrong places in the buggy case. Apologies for the noise.Between your two options, I'd go with (1). It tests the externally-observable behavior through the public solver surface, which is what we actually want to guard against regressing. (2) is tighter but tests an internal invariant between two kernels — a symmetric break in both would pass.
Happy to proceed with (1) as-is. Could you also resolve the merge conflicts on this branch when you get a chance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased onto latest main and simplified the test setup per points (2) and (3):
Keeping the reference-based comparison as agreed in option (1).