newton/tests/test_import_usd.py (4)

355-359: Avoid magic quaternion; compute from stated rotations or clarify comment

The comment states “Rotate 45° around Z, then 30° around Y,” but the quaternion is hardcoded. To prevent drift between comment and reality, either compute q from those rotations in code or update the comment to reflect that q is intentionally hardcoded. Computing it also improves readability.

Example direction (no hard API assumptions): derive R from your intended rotations (Rz then Ry), then set q from R and use that q for both USD authoring and expected inertia. Alternatively, keep q but reword the comment.

---

380-383: Strengthen the test with symmetry/PD/inverse checks

Given a valid rigid body inertia, the tensor should be symmetric and positive definite, and its inverse in the builder should match np.linalg.inv(I_body_expected). Add a few lightweight assertions to prevent regressions.

Apply this diff after the existing assert_allclose:

         np.testing.assert_allclose(
             inertia_parsed.reshape(3, 3), I_body_expected, rtol=1e-5, atol=1e-8
         )
+
+        # Invariants: symmetry and positive definiteness
+        M = inertia_parsed.reshape(3, 3)
+        np.testing.assert_allclose(M, M.T, rtol=1e-7, atol=1e-9)
+        self.assertTrue(np.all(np.linalg.eigvalsh(M) > 0.0))
+
+        # Inverse inertia matches expectation
+        inv_parsed = np.array(builder.body_inv_inertia[0]).reshape(3, 3)
+        np.testing.assert_allclose(inv_parsed, np.linalg.inv(I_body_expected), rtol=1e-5, atol=1e-8)

---

363-371: Assert invariance to invert_rotations for inertia parsing

The fix removes quaternion inversion in parse_quat for principal axes. Add a simple cross-check that inertia is identical with invert_rotations=True to prevent regressions that reintroduce coupling.

Apply this diff (append near the end of the test):

         np.testing.assert_allclose(
             inertia_parsed.reshape(3, 3), I_body_expected, rtol=1e-5, atol=1e-8
         )
+
+        # Inertia parsing should be independent of invert_rotations
+        builder2 = newton.ModelBuilder()
+        parse_usd(stage, builder2, invert_rotations=True)
+        inertia_parsed2 = np.array(builder2.body_inertia[0]).reshape(3, 3)
+        np.testing.assert_allclose(inertia_parsed2, I_body_expected, rtol=1e-5, atol=1e-8)

Also applies to: 380-383

---

340-340: Remove trailing whitespace on blank lines (Ruff W293)

Blank lines contain stray whitespace, which fails the linter. Please strip them (ruff --fix will handle this).

Also applies to: 346-346, 349-349, 353-353, 359-359, 366-366, 371-371, 374-374, 379-379

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

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

newton/tests/test_import_usd.py (3)

338-340: Set meters-per-unit for consistency.

Other USD tests in this module explicitly set meters-per-unit (Line 256 in this file). While it doesn’t affect diagonal inertia parsing here, adding it keeps tests consistent and avoids future surprises if scaling is introduced for inertia.

Proposed change:

 UsdGeom.SetStageUpAxis(stage, UsdGeom.Tokens.z)
+UsdGeom.SetStageMetersPerUnit(stage, 1.0)

---

354-362: Avoid hard-coded quaternion; derive it programmatically.

Using a magic quaternion makes intent and correctness harder to audit. Construct it from axis-angle rotations as documented in the comments to prevent drift/brittleness and improve readability.

Suggested approach (replace the hard-coded quaternion):

• Build qz(45°) and qy(30°) via wp.quat_from_axis_angle
• Compose as q = qy * qz to realize “rotate 45° around Z, then 30° around Y”

If you’d like, I can send an exact patch consistent with the wp API you’re using (operator or function name for quaternion multiplication).

---

372-382: Add inverse-inertia verification for stronger coverage.

You compute and check I_body. It’s cheap to also verify that the imported inverse inertia matches the inverse of I_body (when non-singular), catching mismatches in either computation or storage order.

Example follow-up:

I_body_inv_expected = np.linalg.inv(I_body_expected)
np.testing.assert_allclose(
    np.array(builder.body_inv_inertia[0]).reshape(3, 3),
    I_body_inv_expected,
    rtol=1e-5,
    atol=1e-8,
)

newton/tests/test_mujoco_solver.py (2)

1223-1224: Avoid writing pendulum.xml to the repo root; use a temp file.

Tests should avoid side effects on the working directory. Use tempfile to create a writable, auto-cleaned path.

Example:

import tempfile
with tempfile.TemporaryDirectory() as d:
    xml_path = os.path.join(d, "pendulum.xml")
    solver = MuJoCoSolver(self.model, save_to_mjcf=xml_path)
    # ... run steps ...

---

1157-1157: Trim trailing whitespace on blank lines (Ruff W293).

The linter flags multiple instances of whitespace-only lines. Remove trailing spaces to satisfy W293.

Also applies to: 1163-1163, 1168-1168, 1175-1175, 1179-1179, 1189-1189, 1198-1198, 1211-1211, 1214-1214, 1219-1219, 1228-1228, 1232-1232, 1235-1235, 1239-1239, 1244-1244, 1248-1248, 1255-1255, 1258-1258, 1265-1265

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

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

newton/_src/utils/import_usd.py (1)

180-184: Normalize quaternion in parse_quat to avoid non-orthonormal rotations

Removing the global inversion here is the right direction for correct inertias. However, the quaternion returned is no longer normalized (unlike from_gfquat()), which risks non-orthonormal rotation matrices downstream (e.g., inertia rotation via quat_to_matrix). Normalize before returning.

Apply this diff:

-        quat = wp.quat(*val.imaginary, val.real)
+        # construct and normalize (matches from_gfquat() behavior without inversion)
+        quat = from_gfquat(val)

newton/_src/solvers/mujoco/solver_mujoco.py (3)

1987-1990: Body offset derivation looks correct; clarify the comment

The composition tf = X_pj * inv(X_cj) is the right relative transform to co-locate parent/child joint frames when q=0. The comment “this assumes that the joint position is 0” can be clearer as “assumes all joint coordinates q are zero.”

Apply this diff:

-            # this assumes that the joint position is 0
+            # assumes all joint coordinates (q) are zero
             tf = wp.transform(*joint_parent_xform[ji])
             tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[ji]))

---

1991-1993: Normalize child joint rotation before using it to rotate DOF axes

joint_child_xform should be unit, but normalizing defensively avoids subtle drift in axis definitions.

Apply this diff:

-            joint_pos = wp.vec3(*joint_child_xform[ji, :3])
-            joint_rot = wp.quat(*joint_child_xform[ji, 3:])
+            joint_pos = wp.vec3(*joint_child_xform[ji, :3])
+            joint_rot = wp.normalize(wp.quat(*joint_child_xform[ji, 3:]))

---

2036-2065: Request verification: add a regression test for rotated child joint frames

Given the axis-frame mapping change, please add a minimal test that asserts correct behavior for:

• Prismatic joint with child frame rotated 90° about Z (e.g., axis X in joint frame results in axis Y in body frame).
• Revolute joint with a non-identity child rotation.
• A D6 joint with mixed linear and angular axes.

I can draft a small USD or direct ModelBuilder-based test that builds a parent-child pair with a rotated child joint frame, then compiles to MuJoCo and verifies the mjcf joint axes match the expected body-frame axes. Want me to add that?

Also applies to: 2098-2127

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

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

newton/_src/solvers/mujoco/solver_mujoco.py (1)

1902-1974: Fix double mesh scaling and avoid in-place mutation of shared size array

• For meshes, vertices are multiplied by size, and then geom_params["size"] = size is also set. MuJoCo will apply the size scaling again at compile time, causing the mesh to be scaled twice.
• size = shape_size[shape] followed by size[size == 0.0] = nonzero mutates the row inside the shared shape_size array (numpy view). This has side effects for subsequent uses.

Apply this diff:

                 tf = wp.transform(*shape_transform[shape])
                 if stype == GeoType.MESH:
                     mesh_src = model.shape_source[shape]
                     # use mesh-specific maxhullvert or fall back to the default
                     maxhullvert = getattr(mesh_src, "maxhullvert", mesh_maxhullvert)
-                    # apply scaling
-                    size = shape_size[shape]
-                    vertices = mesh_src.vertices * size
+                    # do NOT pre-scale vertices; let MuJoCo apply geom size scaling
+                    vertices = mesh_src.vertices
                     spec.add_mesh(
                         name=name,
                         uservert=vertices.flatten(),
                         userface=mesh_src.indices.flatten(),
                         maxhullvert=maxhullvert,
                     )
                     geom_params["meshname"] = name
-                geom_params["pos"] = tf.p
-                geom_params["quat"] = quat_to_mjc(tf.q)
-                size = shape_size[shape]
+                geom_params["pos"] = tf.p
+                geom_params["quat"] = quat_to_mjc(tf.q)
+                size = shape_size[shape].copy()
                 if np.any(size > 0.0):
                     # duplicate nonzero entries at places where size is 0
                     nonzero = size[size > 0.0][0]
                     size[size == 0.0] = nonzero
                     geom_params["size"] = size

This preserves intended scaling via geom “size” and eliminates unintended global mutations.

newton/_src/utils/import_usd.py (1)

180-184: Normalize parsed quaternions in parse_quat to avoid non-unit inputs

Removing the inversion is correct given the PR’s goal. However, unlike from_gfquat, parse_quat now returns potentially non-unit quaternions. Downstream (e.g., wp.quat_to_matrix) assumes unit quats; silently accepting non-unit inputs can yield skewed rotation matrices.

Normalize before returning.

Apply this diff:

-        quat = wp.quat(*val.imaginary, val.real)
+        quat = wp.normalize(wp.quat(*val.imaginary, val.real))
         l = wp.length(quat)
         if np.isfinite(l) and l > 0.0:
             return quat

newton/tests/test_import_usd.py (1)

324-374: LGTM; consider normalizing q and asserting symmetry as an extra guard

The test correctly validates principal-axes inertia being rotated into body frame. Minor enhancements:

• Normalize q after construction (defensive; avoids subtle drift if values change).
• Optionally assert inertia_parsed is symmetric to catch regressions.

Minimal tweak:

-        q = wp.quat(0.1830127, 0.1830127, 0.6830127, 0.6830127)
+        q = wp.normalize(wp.quat(0.1830127, 0.1830127, 0.6830127, 0.6830127))

Optionally (after computing inertia_parsed):

self.assertTrue(np.allclose(inertia_parsed.reshape(3, 3), inertia_parsed.reshape(3, 3).T, atol=1e-7))

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

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

newton/_src/solvers/mujoco/solver_mujoco.py (1)

1987-1993: Index with the global joint id (selected_joints[ji]), not the local loop index

As previously noted, joint arrays are global; ji indexes into the selected set. Use j = selected_joints[ji] to fetch from joint arrays. This prevents mismatches when selected_joints is filtered/reordered.

Apply this diff:

-            # this assumes that the joint position is 0
-            tf = wp.transform(*joint_parent_xform[ji])
-            tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[ji]))
-
-            joint_pos = wp.vec3(*joint_child_xform[ji, :3])
-            joint_rot = wp.quat(*joint_child_xform[ji, 3:])
+            # this assumes that the joint position is 0
+            j = selected_joints[ji]
+            tf = wp.transform(*joint_parent_xform[j])
+            tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[j]))
+
+            joint_pos = wp.vec3(*joint_child_xform[j, :3])
+            joint_rot = wp.quat(*joint_child_xform[j, 3:])

Note: The same substitution may be needed for j_type = joint_type[ji], qd_start = joint_qd_start[ji], and lin_axis_count, ang_axis_count = joint_dof_dim[ji] later in this block.

newton/_src/solvers/mujoco/solver_mujoco.py (1)

1935-1940: Avoid mutating shared shape_size views in-place

size = shape_size[shape] likely returns a view; mutating it in-place can leak back into shape_size. Copy before modification.

Apply this diff:

-                size = shape_size[shape]
+                # avoid in-place mutation of model.shape_scale's backing array
+                size = np.array(shape_size[shape], copy=True)
                 if np.any(size > 0.0):
                     # duplicate nonzero entries at places where size is 0
                     nonzero = size[size > 0.0][0]
                     size[size == 0.0] = nonzero
                     geom_params["size"] = size

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

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

newton/_src/solvers/mujoco/solver_mujoco.py (1)

1902-1975: Good refactor: on-the-fly geom placement; watch out for in-place size mutation

Nice simplification removing the transform cache and computing geoms directly per body. One caveat: size is taken as a view from shape_size[shape] and then mutated (Line 1939), which mutates the model’s shape_scale in-place. Copy before editing to avoid side effects.

Apply this diff inside add_geoms:

-                size = shape_size[shape]
+                # copy to avoid mutating model.shape_scale in-place
+                size = np.array(shape_size[shape], copy=True)
                 if np.any(size > 0.0):
                     # duplicate nonzero entries at places where size is 0
                     nonzero = size[size > 0.0][0]
                     size[size == 0.0] = nonzero
                     geom_params["size"] = size

newton/_src/solvers/mujoco/solver_mujoco.py (1)

1987-1993: Indexing joint arrays with local ji risks misalignment; prefer global joint index j = selected_joints[ji]

This block indexes joint_* arrays with ji, which is the index into the depth-first traversal list, not the model’s global joint arrays. This can select wrong rows when selected_joints is a filtered/reordered subset. This was raised earlier and tracked in issue #562; reiterating here since this code still uses ji.

Non-blocking for this PR, but here’s a minimal patch for correctness and future-proofing:

-            tf = wp.transform(*joint_parent_xform[ji])
-            tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[ji]))
+            j = selected_joints[ji]
+            tf = wp.transform(*joint_parent_xform[j])
+            tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[j]))

-            joint_pos = wp.vec3(*joint_child_xform[ji, :3])
-            joint_rot = wp.quat(*joint_child_xform[ji, 3:])
+            joint_pos = wp.vec3(*joint_child_xform[j, :3])
+            joint_rot = wp.quat(*joint_child_xform[j, 3:])

-            j_type = joint_type[ji]
-            qd_start = joint_qd_start[ji]
+            j_type = joint_type[j]
+            qd_start = joint_qd_start[j]

Follow-up: adjust any later joint_dof_dim[ji] uses in this block to joint_dof_dim[j] as well.

Also applies to: 2016-2019

newton/_src/solvers/mujoco/solver_mujoco.py (3)

2039-2063: Axis mapping: using forward rotation is correct here (joint-local → child body frame)

Using wp.quat_rotate(joint_rot, ...) to map joint axes into the child body frame matches the internal frame conventions and avoids the regressions reported when applying the inverse. Consider adding a short comment to prevent future reversions.

Optional comment for clarity:

-                    axis = wp.quat_rotate(joint_rot, wp.vec3(*joint_axis[ai]))
+                    # Map axis from joint-local frame to child body frame:
+                    # joint_rot encodes joint→child rotation, so use forward rotation.
+                    axis = wp.quat_rotate(joint_rot, wp.vec3(*joint_axis[ai]))

---

2100-2124: Angular axes: same correct forward mapping; degree conversion for limits looks right

• Using wp.quat_rotate for angular axes is consistent with the linear DOF handling and the intended frame mapping.
• Converting angular limits to degrees for MuJoCo via np.rad2deg is correct.

Optional comment mirroring the linear case for future maintainers:

-                    axis = wp.quat_rotate(joint_rot, wp.vec3(*joint_axis[ai]))
+                    # Map axis from joint-local frame to child body frame
+                    axis = wp.quat_rotate(joint_rot, wp.vec3(*joint_axis[ai]))

---

2036-2053: Tests: add a case asserting axis orientation with non-identity X_c to guard mapping

Given past confusion around inverse vs forward mapping, add a unit test that constructs a joint with non-identity child rotation and verifies the MuJoCo joint axis aligns with the intended child-frame axis.

I can draft a small test that builds a two-body model with a rotated child joint frame and asserts the compiled mj_model’s jnt_axis matches wp.quat_rotate(joint_rot, joint_axis) in the child frame. Want me to open a follow-up PR with that test?

Also applies to: 2096-2114

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

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: adenzler-nvidia
PR: newton-physics/newton#552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.540Z
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: adenzler-nvidia
PR: newton-physics/newton#552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.540Z
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: adenzler-nvidia
PR: newton-physics/newton#552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.540Z
Learning: When a user reports that a suggested code change breaks their simulation, acknowledge their empirical evidence and avoid being insistent about theoretical analysis without practical verification. Ask for clarification about the intended behavior rather than doubling down on the original suggestion.

newton/_src/solvers/mujoco/solver_mujoco.py (1)

1987-1993: Use global joint index (selected_joints[ji]) for joint-frame data, not local ji

The arrays joint_parent_xform, joint_child_xform, joint_type, joint_qd_start, joint_dof_dim, joint_key are global. Indexing them with ji (local index into selected joints) is incorrect whenever selected_joints is not a contiguous 0..N slice.

Apply this minimal diff in-place to establish the correct global index and use it for xforms/poses:

             # this assumes that the joint position is 0
-            tf = wp.transform(*joint_parent_xform[ji])
-            tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[ji]))
-
-            joint_pos = wp.vec3(*joint_child_xform[ji, :3])
-            joint_rot = wp.quat(*joint_child_xform[ji, 3:])
+            j = selected_joints[ji]
+            tf = wp.transform(*joint_parent_xform[j])
+            tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[j]))
+
+            joint_pos = wp.vec3(*joint_child_xform[j, :3])
+            joint_rot = wp.quat(*joint_child_xform[j, 3:])

Outside this range, update subsequent uses to j instead of ji within this body block:

• j_type = joint_type[j]
• qd_start = joint_qd_start[j]
• lin_axis_count, ang_axis_count = joint_dof_dim[j]
• name = model.joint_key[j]

Note: Tracked in issue #562; OK to land separately if you prefer, but current code will mis-index in filtered/reordered joint selections. Do you want me to open a focused follow-up PR?

newton/_src/solvers/mujoco/solver_mujoco.py (1)

1946-1958: Collision mask may overflow color groups beyond 32

contype uses a 32-bit mask. If color >= 32, contype/conaffinity remain unset (defaults), which may lead to unexpected collisions. Consider clamping or falling back to a safe mask.

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

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: adenzler-nvidia
PR: newton-physics/newton#552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.540Z
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: adenzler-nvidia
PR: newton-physics/newton#552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.540Z
Learning: When a user reports that a suggested code change breaks their simulation, acknowledge their empirical evidence and avoid being insistent about theoretical analysis without practical verification. Ask for clarification about the intended behavior rather than doubling down on the original suggestion.