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

386-405: Edge-case note: fromto collinearity can yield a zero rotation axis

When start→end aligns with the reference axis, cross(...) becomes zero and normalization can NaN the quaternion. Not introduced here, but this block is susceptible. Add a small epsilon guard and pick an orthogonal fallback axis when |cross| ≈ 0.

-                    axis = wp.normalize(wp.cross(axis, wp.vec3(0.0, 1.0, 0.0)))
+                    cross_axis = wp.cross(axis, wp.vec3(0.0, 1.0, 0.0))
+                    if wp.length(cross_axis) < 1e-6:
+                        cross_axis = wp.vec3(0.0, 0.0, 1.0)  # fallback orthogonal
+                    axis = wp.normalize(cross_axis)

newton/tests/test_import_urdf.py (2)

16-16: Prefer public API over private internals in tests

Import GeoType via the public surface to avoid tight coupling to internals.

-from newton._src.geometry.types import GeoType
+from newton import GeoType

---

251-306: Good coverage; tweak assertions to avoid order-coupling and fix misleading comment

Visuals are added before colliders in parse_urdf, so “First shape … (collision)” is misleading. Also, indexing makes the test brittle across importer reorderings. Count by type instead.

-            # First shape should be cylinder (collision)
-            self.assertEqual(shape_types[0], GeoType.CYLINDER)
-
-            # Second shape should be cylinder (visual)
-            self.assertEqual(shape_types[1], GeoType.CYLINDER)
-
-            # Third shape should be capsule
-            self.assertEqual(shape_types[2], GeoType.CAPSULE)
+            # We expect exactly two cylinders (visual + collision) and one capsule.
+            from collections import Counter
+            counts = Counter(shape_types)
+            self.assertEqual(counts[GeoType.CYLINDER], 2)
+            self.assertEqual(counts[GeoType.CAPSULE], 1)

newton/tests/test_import_mjcf.py (2)

25-25: Prefer public API import for GeoType

Use newton.GeoType to decouple tests from internal module layout.

-from newton._src.geometry.types import GeoType
+from newton import GeoType

---

570-609: Solid test; simplify and de-ambiguate the fromto case

MJCF ignores size[1] when fromto is present; keeping it in the XML can confuse readers. Also, assert type counts to avoid depending on insertion order.

-            <geom type="cylinder" size="0.3 0.8" fromto="0 0 0 1 0 0" />
+            <geom type="cylinder" size="0.3 0.0" fromto="0 0 0 1 0 0" />

-            # Check shape types
-            shape_types = list(builder.shape_type)
-            # First two shapes should be cylinders
-            self.assertEqual(shape_types[0], GeoType.CYLINDER)
-            self.assertEqual(shape_types[1], GeoType.CYLINDER)
-            # Third shape should be capsule
-            self.assertEqual(shape_types[2], GeoType.CAPSULE)
-            # Fourth shape should be box
-            self.assertEqual(shape_types[3], GeoType.BOX)
+            # Check shape type counts (order-independent)
+            from collections import Counter
+            counts = Counter(builder.shape_type)
+            self.assertEqual(counts[GeoType.CYLINDER], 2)
+            self.assertEqual(counts[GeoType.CAPSULE], 1)
+            self.assertEqual(counts[GeoType.BOX], 1)

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: eric-heiden
PR: newton-physics/newton#584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.

Learnt from: eric-heiden
PR: newton-physics/newton#584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.