newton/utils/import_usd.py (1)

518-527: Swap also the joint frames when swapping parent/child IDs to keep anchors consistent

You correctly swap IDs when body1 is missing, but parent_tf/child_tf remain tied to localPose0/localPose1 of the original roles. This misassigns anchor frames (parent frame should come from the world side when world becomes parent). Fix by swapping the frames too and guarding against the degenerate case where both bodies resolve to world.

Apply this diff:

-        child_id = path_body_map.get(child_path, -1)
-        # If child_id is -1, swap parent and child
-        if child_id == -1:
-            parent_id, child_id = child_id, parent_id
-            if verbose:
-                print(f"Joint {joint_path} connects {parent_path} to world")
-        parent_tf = wp.transform(joint_desc.localPose0Position, from_gfquat(joint_desc.localPose0Orientation))
-        if incoming_xform is not None:
-            parent_tf = wp.mul(incoming_xform, parent_tf)
-        child_tf = wp.transform(joint_desc.localPose1Position, from_gfquat(joint_desc.localPose1Orientation))
+        child_id = path_body_map.get(child_path, -1)
+
+        # If body1 is missing (world), make world the parent and body0 the child.
+        swapped = False
+        if child_id == -1 and getattr(joint_desc, "body1", None) in (None, Sdf.Path.emptyPath):
+            if parent_id == -1:
+                if verbose:
+                    print(f"Skipping joint {joint_path}: both bodies resolve to world (-1)")
+                return
+            parent_id, child_id = -1, parent_id
+            swapped = True
+            if verbose:
+                print(f"Joint {joint_path} connects {parent_path} to world")
+
+        # Compute joint frames; if swapped, also swap the frames so they stay consistent
+        # with the parent/child bodies.
+        tf0 = wp.transform(joint_desc.localPose0Position, from_gfquat(joint_desc.localPose0Orientation))
+        tf1 = wp.transform(joint_desc.localPose1Position, from_gfquat(joint_desc.localPose1Orientation))
+        parent_tf = tf1 if swapped else tf0
+        child_tf = tf0 if swapped else tf1
+        if incoming_xform is not None:
+            parent_tf = wp.mul(incoming_xform, parent_tf)

newton/tests/assets/revolute_articulation.usda (1)

1-40: Consider trimming render/camera metadata to keep the test asset minimal

The asset contains extensive render settings, camera presets, and kit-specific metadata unrelated to the importer logic under test. A smaller, hand-authored USDA focused on physics prims (Articulation, two RigidBodies, the two Joints, PhysicsScene) will be easier to maintain and review.

If helpful, I can provide a pared-down USDA with the same semantics (world Z-up, metersPerUnit=1) that only includes the necessary physics prims.

newton/tests/test_import_usd.py (1)

101-147: Make assertions robust to joint ordering by resolving indices via joint keys

The test assumes a specific joint order ([FREE, REVOLUTE, FIXED]); that can change with topology sorting or future importer tweaks. Resolve indices from builder.joint_key to avoid flakiness.

Apply this diff:

-        # Verify joint types
-        self.assertEqual(builder.joint_type[0], newton.JOINT_FREE)
-        self.assertEqual(builder.joint_type[1], newton.JOINT_REVOLUTE)
-        self.assertEqual(builder.joint_type[2], newton.JOINT_FIXED)
+        # Verify joint types by key to avoid relying on order
+        idx_by_key = {k: i for i, k in enumerate(builder.joint_key)}
+        self.assertIn(newton.JOINT_FREE, builder.joint_type)  # root free joint present
+        self.assertEqual(
+            builder.joint_type[idx_by_key["/Articulation/Arm/RevoluteJoint"]],
+            newton.JOINT_REVOLUTE,
+        )
+        self.assertEqual(
+            builder.joint_type[idx_by_key["/Articulation/CenterPivot/FixedJoint"]],
+            newton.JOINT_FIXED,
+        )
 
-        # The key test: verify the FixedJoint connects CenterPivot to world
-        # because body1 was missing in the USD file
-        fixed_joint_idx = 2
+        # The key test: verify the FixedJoint connects CenterPivot to world
+        # because body1 was missing in the USD file
+        fixed_joint_idx = idx_by_key["/Articulation/CenterPivot/FixedJoint"]
         self.assertEqual(builder.joint_parent[fixed_joint_idx], -1)  # Parent is world (-1)
         # Child should be CenterPivot (which was body0 in the USD)
         center_pivot_idx = builder.body_key.index("/Articulation/CenterPivot")
         self.assertEqual(builder.joint_child[fixed_joint_idx], center_pivot_idx)

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

newton/utils/import_usd.py (2)

517-524: Add a defensive guard when both bodies resolve to -1; narrow the swap condition

Edge case: if both parent_id and child_id are -1 (e.g., unresolved paths or both missing), attempting to add the joint would still be invalid. Suggest short-circuiting in that case and narrowing the swap to the intended scenario (child is -1, parent is valid).

Consider this diff:

         child_tf = wp.transform(joint_desc.localPose1Position, from_gfquat(joint_desc.localPose1Orientation))
-        # If child_id is -1, swap parent and child
-        if child_id == -1:
+        # Defensive handling: both bodies unresolved -> skip
+        if parent_id == -1 and child_id == -1:
+            if verbose:
+                print(f"Skipping joint {joint_path}: both bodies unresolved (-1)")
+            return
+        # If child_id is -1 but parent is valid, swap so world is the parent
+        if child_id == -1 and parent_id != -1:
             parent_id, child_id = child_id, parent_id
             parent_tf, child_tf = child_tf, parent_tf
             if verbose:
                 print(f"Joint {joint_path} connects {parent_path} to world")

Additionally, please verify revolute/prismatic joints that connect to world via a missing body (fixed asset covers FixedJoint only). Axis conventions may depend on body0/body1 frames in USD; the swap should preserve semantics, but a quick unit test for a world-anchored Revolute/Prismatic would de-risk this.

---

527-528: Remove commented-out debug code

These commented prints add noise. If needed, prefer a verbose-gated print with context or delete them.

Apply:

-        # if parent_id == -1:
-        #     print("joint connected to world")

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

newton/tests/test_import_usd.py (2)

131-136: Make free-joint detection more robust (avoid relying on key names).

Current logic infers the free joint by exclusion of specific path keys, which is brittle if additional joints are ever authored. Prefer detecting by joint type and asserting uniqueness.

Apply this diff:

-        # The free joint typically has a generic key like "joint_1"
-        free_joint_idx = next(
-            i
-            for i, key in enumerate(builder.joint_key)
-            if key not in ["/Articulation/CenterPivot/FixedJoint", "/Articulation/Arm/RevoluteJoint"]
-        )
+        # The free joint is identified by its type
+        self.assertEqual(builder.joint_type.count(newton.JointType.FREE), 1)
+        free_joint_idx = builder.joint_type.index(newton.JointType.FREE)

---

149-152: Relax shape-map count assertion to reduce brittleness.

Assets often accrete visuals/colliders over time. Hard-coding exactly 1 shape path makes this test flaky for unrelated changes. Prefer asserting a minimum.

Apply this diff:

-        self.assertEqual(len(results["path_shape_map"]), 1)
+        self.assertGreaterEqual(len(results["path_shape_map"]), 1)

Would you like me to extend the test to also assert parent/child indices for the revolute joint (CenterPivot → Arm) to lock-in that relationship?

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

518-526: Narrow the swap to truly “omitted body1” (avoid misconnecting filtered/disabled bodies to world).

child_id == -1 can also arise when the referenced child body exists in USD but wasn’t loaded (ignored path, disabled body, etc.). In those cases, silently connecting to world could be surprising. Tighten the condition to only swap when USD explicitly omits body1 (Sdf.Path.emptyPath). Otherwise, skip the joint (or gate on a verbose warning).

Proposed change:

-        # If child_id is -1, swap parent and child
-        if child_id == -1:
+        # If child_id is -1, only swap when USD actually omitted body1.
+        # Otherwise (unresolved due to filtering/disabled), skip to avoid accidental world-connections.
+        if child_id == -1:
+            if joint_desc.body1 != Sdf.Path.emptyPath:
+                if verbose:
+                    print(f"Skipping joint {joint_path}: child '{child_path}' unresolved (possibly filtered/disabled)")
+                return
             if parent_id == -1:
                 if verbose:
                     print(f"Skipping joint {joint_path}: both bodies unresolved")
                 return
             parent_id, child_id = child_id, parent_id
             parent_tf, child_tf = child_tf, parent_tf
             if verbose:
                 print(f"Joint {joint_path} connects {parent_path} to world")

If you want, I can scan the repository for other places where we treat -1 as “world” to ensure this change doesn’t regress any intended “world-parent” connections that come from authored body0 == world.

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.