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

175-185: Normalize parsed USD quaternions to ensure a proper rotation; avoid duplication with from_gfquat

The component order is correct (x, y, z, w), but the quaternion is not normalized before use. Downstream we call wp.quat_to_matrix(i_rot) for inertia, which assumes unit quaternions. A non-unit quaternion would skew the rotation. Also, you already have from_gfquat() that performs normalization; reusing it here improves consistency.

Apply this minimal fix to normalize and keep the existing guard:

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

Alternatively, reuse the helper for consistency:

-        quat = wp.quat(*val.imaginary, val.real)
-        l = wp.length(quat)
-        if np.isfinite(l) and l > 0.0:
-            return quat
+        # reuse helper which normalizes
+        quat = from_gfquat(val)
+        l = wp.length(quat)
+        if np.isfinite(l) and l > 0.0:
+            return quat
         return default

newton/tests/test_import_usd.py (1)

349-351: Normalize the test quaternion to avoid subtle drift and align with importer behavior

Normalizing ensures R is a true rotation matrix and makes the test robust if the importer normalizes quaternions (recommended).

-        q = wp.quat(0.1830127, 0.1830127, 0.6830127, 0.6830127)
+        q = wp.normalize(wp.quat(0.1830127, 0.1830127, 0.6830127, 0.6830127))
         R = np.array(wp.quat_to_matrix(q)).reshape(3, 3)

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/utils/import_usd.py (1)

201-209: Action required: clarify invert_rotations policy — it's applied to matrix-derived Xform rotations only (fix required)

Short: parse_xform (matrix path) transposes the 3x3 when invert_rotations=True, while Gf.Quat → wp.quat (from_gfquat) and all quaternion-based paths are not inverted. The current docstring ("inverts any rotations") is therefore inaccurate and should be corrected or the behavior reconciled.

Files/locations to fix or review:

• newton/_src/utils/import_usd.py
  • parse_usd signature / docstring (around lines ~41 and ~65–71): currently says "inverts any rotations defined in the shape transforms." — update to specify matrix-only behavior.
  • parse_xform (lines ~201–209): applies transpose when invert_rotations is True (matrix-derived rotation).
  • from_gfquat (definition around lines ~172–174) and its call sites (around lines 180, 479, 517, 986): quaternion-derived rotations are parsed without inversion.
• tests: newton/tests/test_import_usd.py (calls parse_usd with invert_rotations=True at ~line 203) — ensure tests and expectations match the clarified policy.

Suggested docstring replacement (concise):
Replace
"invert_rotations (bool): If True, inverts any rotations defined in the shape transforms."
with
"invert_rotations (bool): If True, inverts rotations derived from UsdGeom.Xform local transformation matrices only. Quaternion attributes (e.g., principalAxes, localRot, joint localPose orientations and other Gf.Quat sources) are parsed directly from Gf.Quat and are not affected by this flag."

Options for maintainers:

• If intended: update the docstring as above and add an inline comment near parse_xform to avoid future confusion.
• If not intended (you want quaternion paths inverted too): implement optional inversion in from_gfquat or its callers and update tests accordingly.

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

172-174: Correct XYZW mapping from USD Gf.Quat to Warp; consider guarding zero-length/NaN inputs

The mapping (x, y, z, w) and normalization are correct for Warp. To harden this helper against malformed USD data (zero-length or non-finite quats) and to prevent NaNs from propagating to callsites that use from_gfquat directly (parse_body/parse_joint), add a light guard before normalization.

-def from_gfquat(gfquat):
-    return wp.normalize(wp.quat(*gfquat.imaginary, gfquat.real))
+def from_gfquat(gfquat):
+    # USD stores quats as (real=w, imaginary=(x,y,z)); Warp expects (x,y,z,w)
+    x, y, z = gfquat.imaginary
+    w = gfquat.real
+    v = np.array([x, y, z, w], dtype=np.float32)
+    # Guard against non-finite or zero-length quaternions to avoid NaNs
+    if not np.isfinite(v).all() or np.linalg.norm(v) == 0.0:
+        return wp.quat_identity()
+    return wp.normalize(wp.quat(x, y, z, w))

---

175-185: parse_quat now uses from_gfquat; minor simplification opportunity

Since from_gfquat already normalizes (and with the guard above, returns identity on invalid inputs), the extra length check can be dropped for a leaner path. If you keep the check, it’s fine as-is.

-        quat = from_gfquat(val)
-        l = wp.length(quat)
-        if np.isfinite(l) and l > 0.0:
-            return quat
-        return default
+        quat = from_gfquat(val)
+        return quat

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.