Make the support for ellipsoid shapes more complete#1132
Conversation
📝 WalkthroughWalkthroughAdds first-class ellipsoid support across the engine: inertia computation, raycasting, bounding radius, mesh generation, ModelBuilder API, viewer rendering, collision-path inclusion, examples, and extensive narrow-phase tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant RayDispatch as ray_intersect_geom
participant Ellipsoid as ray_intersect_ellipsoid
participant Transform as TransformOps
participant QuadSolve as QuadraticSolve
Caller->>RayDispatch: ray_intersect_geom(..., GeoType.ELLIPSOID, geom_to_world, size)
RayDispatch->>Ellipsoid: ray_intersect_ellipsoid(geom_to_world, origin, dir, semi_axes)
Ellipsoid->>Transform: inverse-transform ray into ellipsoid local space
Transform-->>Ellipsoid: ray_local
Ellipsoid->>Ellipsoid: scale by inverse semi-axes → unit-sphere ray
Ellipsoid->>QuadSolve: build & solve quadratic for t
QuadSolve-->>Ellipsoid: t_nearest or no-hit
Ellipsoid-->>Caller: return t or -1.0
sequenceDiagram
autonumber
participant User
participant Builder as ModelBuilder
participant AddEll as add_shape_ellipsoid
participant AddShape as add_shape (core)
User->>Builder: add_shape_ellipsoid(body, xform, a,b,c, cfg, as_site)
Builder->>AddEll: assemble scale=(a,b,c), adjust cfg if as_site
AddEll->>AddShape: add_shape(..., GeoType.ELLIPSOID, scale, cfg)
AddShape-->>User: return shape index
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-08-12T17:51:37.474ZApplied to files:
📚 Learning: 2025-08-12T17:58:16.929ZApplied to files:
📚 Learning: 2025-08-12T05:17:34.423ZApplied to files:
🧬 Code graph analysis (2)newton/_src/geometry/raycast.py (1)
newton/_src/sim/builder.py (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/examples/basic/example_basic_shapes.py (1)
147-207: Fix the failing “ellipsoid at rest pose” test by relaxing the position toleranceThe CI failure (
Test 'ellipsoid at rest pose' failed for the following bodies: [ellipsoid]) indicates that:self.ellipsoid_pos[2] = 0.25 ellipsoid_q = wp.transform(self.ellipsoid_pos, wp.quat_identity()) lambda q, qd: newton.utils.vec_allclose(q, ellipsoid_q, atol=2e-4)is too strict for the new ellipsoid primitive: the final simulated center height is apparently not within
2e-4of exactlyc = 0.25, even though the solver is likely behaving correctly within its numerical tolerances.Two reasonable options:
- Loosen the atol to match other “resting” shapes with more complex contact (e.g. the box uses
atol=0.1):- lambda q, qd: newton.utils.vec_allclose(q, ellipsoid_q, atol=2e-4), + lambda q, qd: newton.utils.vec_allclose(q, ellipsoid_q, atol=1e-3),or even
1e-2if needed.
- Assert a bounded z-range instead of exact equality, similar to the bunny/cylinder custom checks, e.g.:
- newton.examples.test_body_state( - self.model, - self.state_0, - "ellipsoid at rest pose", - lambda q, qd: newton.utils.vec_allclose(q, ellipsoid_q, atol=2e-4), - [1], - ) + newton.examples.test_body_state( + self.model, + self.state_0, + "ellipsoid at rest pose", + lambda q, _qd: abs(q[0] - ellipsoid_q[0]) < 0.01 + and abs(q[1] - ellipsoid_q[1]) < 0.01 + and abs(q[2] - ellipsoid_q[2]) < 1e-3, + [1], + )Either approach should resolve the current pipeline error while still catching real regressions in the ellipsoid’s rest height.
Separately, Ruff’s
ARG005warnings about unused lambda argumentqdcan be addressed by renaming to_qd(here and in the other lambdas) without changing behavior:- lambda q, qd: newton.utils.vec_allclose(q, sphere_q, atol=2e-4), + lambda q, _qd: newton.utils.vec_allclose(q, sphere_q, atol=2e-4),(and similarly for the ellipsoid and bunny checks).
🧹 Nitpick comments (3)
newton/_src/utils/mesh.py (1)
86-169: Ellipsoid mesh generation is mathematically sound; validation could be tightenedThe
create_ellipsoid_meshimplementation correctly:
- Uses a sphere parametric grid (θ, φ) and scales to an ellipsoid via
(rx, ry, rz).- Computes normals from the gradient of
(x/rx)^2 + (y/ry)^2 + (z/rz)^2 = 1and normalizes them.- Reuses the same indexing/winding pattern as
create_sphere_mesh, includingreverse_winding.Two optional improvements:
- Consider validating that
rx,ry,rzare strictly positive (or at least> 0) and raising a clearValueErrorif not, rather than silently producing degenerate geometry.- There is some duplication with
create_sphere_mesh; extracting a small shared helper for the latitude/longitude loop could reduce maintenance overhead if you ever tweak tessellation conventions.newton/_src/geometry/inertia.py (1)
526-534: ELLIPSOID branch incompute_shape_inertiamatches existing solid/hollow conventionsThe new
GeoType.ELLIPSOIDcase follows the same pattern as sphere/box/capsule/cylinder/cone:
- Solid: delegates directly to
compute_ellipsoid_inertia(density, a, b, c).- Hollow: calls
compute_ellipsoid_inertiawith reduced semi-axes and subtracts inner mass/inertia from the solid shell.One caveat (shared with the existing primitives): there is no guard ensuring
a - thickness,b - thickness,c - thicknessremain positive. If a caller provides a too-largethickness, the inner ellipsoid becomes degenerate or inverted, yielding nonsensical inertia. If you expect user-controlled thickness here, consider clamping or asserting positivity, but that’s orthogonal to this PR and follows a pre-existing pattern.newton/tests/test_narrow_phase.py (1)
120-145: Ellipsoid distance helper is appropriate for test‑level surface checks
distance_point_to_ellipsoid:
- Transforms points into ellipsoid-local coordinates using
ellipsoid_rot.T.- Scales to the unit sphere via division by
(a, b, c).- Computes
(‖scaled_point‖ − 1) * avg_scaleas an approximate distance.Given it’s explicitly documented as approximate and used only in test assertions with a relatively loose tolerance, this is sufficient and keeps the tests simple. If you ever want stronger guarantees, you could assert that all semi-axes are positive before dividing, but that’s optional for internal test utilities.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
newton/_src/geometry/inertia.py(2 hunks)newton/_src/geometry/raycast.py(3 hunks)newton/_src/geometry/utils.py(1 hunks)newton/_src/sim/builder.py(1 hunks)newton/_src/utils/mesh.py(1 hunks)newton/_src/viewer/viewer.py(4 hunks)newton/examples/basic/example_basic_shapes.py(4 hunks)newton/tests/test_narrow_phase.py(3 hunks)newton/utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 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.
Applied to files:
newton/examples/basic/example_basic_shapes.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/examples/basic/example_basic_shapes.pynewton/_src/sim/builder.pynewton/_src/geometry/inertia.pynewton/utils.pynewton/tests/test_narrow_phase.pynewton/_src/viewer/viewer.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is properly re-exported from the spatial module via "from .spatial import (..., transform_twist, ...)" and included in __all__, making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/utils.py
📚 Learning: 2025-08-14T17:38:36.106Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/__init__.py:25-29
Timestamp: 2025-08-14T17:38:36.106Z
Learning: The Newton project prefers incremental __all__ building using __all__ += [...] pattern to group exports with their related imports, rather than a single consolidated __all__ at the end of the file.
Applied to files:
newton/utils.py
📚 Learning: 2025-08-12T17:58:16.929Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/tests/test_ik.py:25-26
Timestamp: 2025-08-12T17:58:16.929Z
Learning: In the Newton physics engine codebase, it's acceptable for tests to import and use private `_src` internal modules and functions when needed for testing purposes, even though this breaks the public API boundary. This is an intentional project decision for test code.
Applied to files:
newton/utils.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_narrow_phase.py
🧬 Code graph analysis (8)
newton/examples/basic/example_basic_shapes.py (3)
newton/_src/sim/builder.py (3)
add_body(1664-1736)key(387-389)add_shape_ellipsoid(3148-3219)newton/examples/__init__.py (1)
test_body_state(38-113)newton/_src/utils/__init__.py (1)
vec_allclose(202-209)
newton/_src/geometry/raycast.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-67)
newton/_src/geometry/utils.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-67)
newton/_src/sim/builder.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-67)
newton/_src/geometry/inertia.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-67)
newton/utils.py (1)
newton/_src/utils/mesh.py (1)
create_ellipsoid_mesh(86-169)
newton/tests/test_narrow_phase.py (2)
newton/_src/sim/builder.py (1)
dot(4230-4231)newton/_src/geometry/types.py (2)
GeoType(25-67)normals(243-244)
newton/_src/viewer/viewer.py (2)
newton/_src/utils/mesh.py (1)
create_ellipsoid_mesh(86-169)newton/_src/geometry/types.py (5)
GeoType(25-67)vertices(225-226)vertices(229-231)indices(234-235)indices(238-240)
🪛 GitHub Actions: Pull Request
newton/examples/basic/example_basic_shapes.py
[error] 223-223: Test 'ellipsoid at rest pose' failed for the following bodies: [ellipsoid].
🪛 Ruff (0.14.5)
newton/examples/basic/example_basic_shapes.py
163-163: Unused lambda argument: qd
(ARG005)
172-172: Unused lambda argument: qd
(ARG005)
205-205: Unused lambda argument: qd
(ARG005)
newton/_src/geometry/inertia.py
164-164: Ambiguous variable name: I
(E741)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (12)
newton/_src/geometry/utils.py (1)
89-100: Ellipsoid bounding radius logic looks correct and consistentUsing
max(scale[0], scale[1], scale[2])forGeoType.ELLIPSOIDmatches the intended bounding-sphere radius (largest semi-axis) and is consistent with howgeom_collision_radiusis computed for ellipsoids intest_narrow_phase._create_geometry_arrays. No issues here.newton/utils.py (1)
32-50: Public export ofcreate_ellipsoid_meshis consistent with existing utils APIImporting
create_ellipsoid_meshfrom._src.utils.meshand adding it to__all__follows the existing pattern for mesh helpers (create_*_mesh) and the incremental__all__style used in this project.newton/examples/basic/example_basic_shapes.py (1)
58-64: Ellipsoid body setup is consistent with other primitivesAdding the
ellipsoidbody viaadd_bodyandadd_shape_ellipsoidmirrors the existing patterns for sphere/capsule/cylinder/box, with semi-axes(a=0.6, b=0.4, c=0.25)aligned to the local axes. This looks correct and integrates cleanly with the builder API.newton/_src/geometry/raycast.py (1)
551-578: ELLIPSOID dispatch inray_intersect_geomis correctly wiredThe new
GeoType.ELLIPSOIDbranch:elif geomtype == GeoType.ELLIPSOID: t_hit = ray_intersect_ellipsoid(geom_to_world, ray_origin, ray_direction, size)matches the convention used elsewhere:
sizealready carries the three semi-axes(a, b, c)for ellipsoids.- The rest of the dispatcher behavior for other primitives is unchanged.
Once the zero-length-direction guard is added in
ray_intersect_ellipsoid, this integration looks good.newton/_src/viewer/viewer.py (4)
21-31: Viewer import ofcreate_ellipsoid_meshaligns with other primitivesAdding
create_ellipsoid_meshto the mesh helper imports keeps all primitive mesh generators coming from the same module (newton.utils) and is consistent with the rest of the file’s usage.
219-234: Always passingshapes.scalestolog_instancesis the right callChanging:
self.log_instances(..., shapes.world_xforms, shapes.scales, ..., hidden=...)to always pass
shapes.scales(even whenmodel_changedisFalse) ensures downstream viewers consistently receive scale information for transform matrix computation. This avoids subtle bugs where instance scales might be ignored after the first frame.
458-495: Ellipsoid path inlog_geois consistent with other primitivesThe new
GeoType.ELLIPSOIDbranch:
- Interprets
geo_scaleas(rx, ry, rz)semi-axes.- Sensibly falls back to a sphere-like ellipsoid when fewer than 3 components are provided (
ry/rzdefault torx).- Uses
create_ellipsoid_mesh(rx, ry, rz)to generate vertices/indices, which matches the mesh helper’s contract.Overall, this mirrors the existing plane/sphere/capsule/cylinder/cone/box handling and integrates cleanly with the log_mesh pipeline.
673-683: AddingELLIPSOIDto the geometry cache key map completes viewer supportIncluding:
newton.GeoType.ELLIPSOID: "ellipsoid",in the
_populate_geometrybase_namemap ensures ellipsoid meshes participate in the same caching scheme as other primitives and receive stable, descriptive paths like/geometry/ellipsoid_…. No issues here.newton/_src/geometry/inertia.py (1)
137-167: Ellipsoid inertia helper is correct and matches standard rigid‑body formulas
compute_ellipsoid_inertiacorrectly:
Computes mass as
density * (4/3 * π * a * b * c).Uses the classic solid-ellipsoid inertia about its center of mass:
Ixx = (1/5) m (b² + c²)Iyy = (1/5) m (a² + c²)Izz = (1/5) m (a² + b²)Returns COM at the origin, consistent with the other centered primitives.
No issues with correctness here.
newton/tests/test_narrow_phase.py (2)
234-246: Ellipsoid collision radius in tests matches runtime broad‑phase radiusFor
GeoType.ELLIPSOIDyou set:geom_collision_radius[i] = max(scale_array[0], scale_array[1], scale_array[2])which is exactly the bounding-sphere radius (largest semi-axis) and aligns with
compute_shape_radius’s ellipsoid branch ingeometry/utils.py. This consistency between tests and runtime broad-phase logic is good; no issues here.
1374-1644: Ellipsoid narrow‑phase test coverage is comprehensive and aligned with collision conventionsThe new tests:
- Cover ellipsoid–ellipsoid, ellipsoid–sphere, ellipsoid–box, ellipsoid–plane, and ellipsoid–capsule interactions, including separated and penetrating cases as well as rotated configurations.
- Verify:
- Normals are unit-length (
‖n‖ ≈ 1).- Normal direction is from shape A to shape B (using
check_normal_direction) where ordering is well-defined.- Sphere-equivalent behavior when
a = b = c(test_ellipsoid_sphere_equivalent), with penetration matching the sphere–sphere expectation to O(1e-1).This suite should give strong coverage for the new ellipsoid support in the narrow phase without being over-constraining on exact penetration magnitudes.
newton/_src/sim/builder.py (1)
3148-3219: Ellipsoid shape helper is consistent with existing shape APIs
add_shape_ellipsoidmirrors the patterns of the other shape helpers (add_shape_sphere,add_shape_box, etc.): samecfg/as_sitehandling, usesGeoType.ELLIPSOIDwithscale = wp.vec3(a, b, c), delegates inertia and radius toadd_shape, and the docstring/example clearly describe the semi-axes semantics. No issues from a correctness or API-consistency standpoint.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/examples/basic/example_basic_shapes.py (1)
147-208: Ellipsoid rest‑pose test looks correct; consider silencing unusedqdlambda argsThe new ellipsoid rest‑pose check is consistent with the geometry:
- For
a=b=0.5, c=0.25, a flat ellipsoid resting on the ground should have its center atz=0.25, matchingself.ellipsoid_pos[2] = 0.25.- The new body index
[1]for the ellipsoid and the shifted indices for capsule[2], cylinder[3], box[4], and bunny[5]all align with theadd_bodyorder in__init__.Static analysis (Ruff ARG005) is now reporting unused lambda argument
qdin several of these tests. It’s harmless functionally, but if you want to quiet the warnings, you can mark the argument as intentionally unused:newton.examples.test_body_state( self.model, self.state_0, "sphere at rest pose", - lambda q, qd: newton.utils.vec_allclose(q, sphere_q, atol=2e-4), + lambda q, _qd: newton.utils.vec_allclose(q, sphere_q, atol=2e-4), [0], ) @@ newton.examples.test_body_state( self.model, self.state_0, "ellipsoid at rest pose", - lambda q, qd: newton.utils.vec_allclose(q, ellipsoid_q, atol=2e-2), + lambda q, _qd: newton.utils.vec_allclose(q, ellipsoid_q, atol=2e-2), [1], ) @@ newton.examples.test_body_state( self.model, self.state_0, "capsule at rest pose", - lambda q, qd: newton.utils.vec_allclose(q, capsule_q, atol=2e-4), + lambda q, _qd: newton.utils.vec_allclose(q, capsule_q, atol=2e-4), [2], ) @@ newton.examples.test_body_state( self.model, self.state_0, "bunny at rest pose", - lambda q, qd: q[2] > 0.01 and abs(q[0]) < 0.1 and abs(q[1] - 4.0) < 0.1, + lambda q, _qd: q[2] > 0.01 and abs(q[0]) < 0.1 and abs(q[1] - 4.0) < 0.1, [5], )This keeps the two-argument signature needed by
test_body_statewhile satisfying Ruff.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/geometry/collision_core.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(1 hunks)newton/examples/basic/example_basic_shapes.py(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/_src/geometry/collision_core.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/geometry/collision_core.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/collision_core.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/collision_core.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 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.
Applied to files:
newton/examples/basic/example_basic_shapes.py
🧬 Code graph analysis (3)
newton/_src/geometry/collision_core.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-67)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-67)
newton/examples/basic/example_basic_shapes.py (4)
newton/_src/sim/builder.py (3)
add_body(1664-1736)key(387-389)add_shape_ellipsoid(3148-3219)newton/_src/utils/import_utils.py (1)
transform(139-140)newton/examples/__init__.py (1)
test_body_state(38-113)newton/_src/utils/__init__.py (1)
vec_allclose(202-209)
🪛 Ruff (0.14.5)
newton/examples/basic/example_basic_shapes.py
164-164: Unused lambda argument: qd
(ARG005)
173-173: Unused lambda argument: qd
(ARG005)
206-206: Unused lambda argument: qd
(ARG005)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (3)
newton/_src/geometry/collision_core.py (1)
377-380: LGTM! Multi-contact support for ellipsoids is properly implemented.Verification confirms that the
support_mapfunction insupport_function.py(lines 238-244) correctly handlesGeoType.ELLIPSOID. The implementation properly extracts semi-axes from the scale parameter and applies the correct mathematical formula for ellipsoid support mapping.The change in
collision_core.py(lines 377-380) to extend multi-contact support to ellipsoids is well-founded and appropriately distinguishes ellipsoids from spheres by excluding them from sphere-specific Minkowski treatments.newton/_src/solvers/mujoco/solver_mujoco.py (1)
995-1004: Ellipsoid → MuJoCo geom mapping looks correct; verify MuJoCo version supportAdding
GeoType.ELLIPSOID: mujoco.mjtGeom.mjGEOM_ELLIPSOIDis consistent with the existing mapping and should let ellipsoid shapes flow through_convert_to_mjcandadd_geomswithout further changes in this file. Ellipsoid sites will still degrade to spheres viasupported_site_types, which matches the comment about MuJoCo’s site restrictions.Please just confirm that the minimum MuJoCo version you support defines
mujoco.mjtGeom.mjGEOM_ELLIPSOID; older versions would hit anAttributeErrorat import/compile time.newton/examples/basic/example_basic_shapes.py (1)
58-64: Ellipsoid body setup and parameters are consistent with the new APIThe ellipsoid body creation and
add_shape_ellipsoidcall look correct: the semi-axes(a=0.5, b=0.5, c=0.25)match the “flat disk” comment, and using the body transform (with default identity shape xform) is consistent with the builder API.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/geometry/collision_core.py (1)
346-354: <!-- [scratchpad_start]
[task verification_1/1 gather_context_3/3 COMPLETE][observations]
Code facts from executed scripts:
- Minkowski preprocessing (lines 347-353): SPHERE and CAPSULE only
- Multi-contact flag (lines 378-381): SPHERE and ELLIPSOID only (CAPSULE missing)
- Post-processing (lines 220-227): SPHERE and CAPSULE only
- Docstring (line 197): "Minkowski objects (spheres/capsules)" — confirms capsules are Minkowski objects
- Ellipsoids in codebase: have support functions, inertia, collision primitives, but NOT Minkowski preprocessing or post-processing in collision_core.py
- Git log: commit 691576b "Add per shape contact margin (#1101)" is most recent; other relevant: 1f115ae "Improve performance..."
[analysis]
CONFIRMED: The review comment correctly identifies a genuine inconsistency.Facts:
- Spheres: preprocessed ✓ + post-processed ✓ + in multi-contact flag ✓ — CONSISTENT
- Capsules: preprocessed ✓ + post-processed ✓ + in multi-contact flag ✗ — INCONSISTENT (missing from flag)
- Ellipsoids: preprocessed ✗ + post-processed ✗ + in multi-contact flag ✓ — INCONSISTENT (in flag but not handled)
The multi-contact flag either:
a) Identifies "rounded Minkowski objects" (as docstring suggests) → should include CAPSULE, exclude ELLIPSOID
b) Has new semantics → needs documentationEither way, current state is wrong. Capsules as Minkowski objects should be in the flag for consistency with their preprocessing. Ellipsoids should not be in the flag unless they also receive Minkowski preprocessing and post-processing.
[pending]
None. Evidence is sufficient to confirm the review.[done]
Verification complete. Review comment is substantively correct about the inconsistency. The suggested diff to add CAPSULE is appropriate. ELLIPSOID inclusion may also be questionable without corresponding preprocessing, but CAPSULE omission is the clearer regression.[scratchpad_start] -->
Multi-contact flag is inconsistent with Minkowski preprocessing – capsules missing, ellipsoids unhandled
The review comment is correct. The code exhibits a genuine inconsistency:
- Lines 347–353 (Minkowski preprocessing): Treats SPHERE and CAPSULE (saves original scale as
radius_eff_*, setsscale[0]tosmall_radius)- Lines 220–227 (Post-processing): Also handles SPHERE and CAPSULE explicitly; docstring (line 197) states "Minkowski objects (spheres/capsules)"
- Lines 378–381 (Multi-contact flag): Only checks SPHERE and ELLIPSOID – capsules are excluded
Since capsules are preprocessed as Minkowski objects and later post-processed with
radius_eff_a/b, excluding them from the multi-contact flag breaks the symmetry. This is a regression: capsules need the multi-contact path for proper manifold generation, just like spheres.Additionally, ellipsoids are included in the flag but receive no Minkowski preprocessing or post-processing in this code path, making their presence in the condition unclear and potentially inconsistent.
The suggested fix to restore capsules to the condition is sound:
- type_a == int(GeoType.SPHERE) - or type_b == int(GeoType.SPHERE) - or type_a == int(GeoType.ELLIPSOID) - or type_b == int(GeoType.ELLIPSOID), + ( + type_a == int(GeoType.SPHERE) + or type_b == int(GeoType.SPHERE) + or type_a == int(GeoType.CAPSULE) + or type_b == int(GeoType.CAPSULE) + or type_a == int(GeoType.ELLIPSOID) + or type_b == int(GeoType.ELLIPSOID) + ),Clarifying the semantics (whether this flag identifies "Minkowski objects" or a broader category) with a brief comment would also help prevent similar issues in the future.
🧹 Nitpick comments (1)
newton/examples/basic/example_basic_shapes.py (1)
147-208: Rest-pose tests and body indices are consistent; ARG005 onqdis benign but can be silenced
- The updated indices
[0]sphere,[1]ellipsoid,[2]capsule,[3]cylinder,[4]box,[5]bunny match the body creation order.- Expected z-heights (0.5, 0.25, 1.0, 0.6, 0.25) correctly correspond to each shape’s half-extent along z, so the ellipsoid rest-pose check is physically consistent.
- A looser atol for the ellipsoid (
2e-2) is reasonable given more complex contact behavior.Ruff’s ARG005 warnings about unused
qdin the lambdas (e.g., Lines 164, 173, 206) are not functionally problematic;test_body_stateexpects a(q, qd)signature and these tests simply ignoreqd. If you want to silence the warnings without changing behavior, you can rename the parameter to underscore in these lambdas, for example:- lambda q, qd: newton.utils.vec_allclose(q, ellipsoid_q, atol=2e-2), + lambda q, _qd: newton.utils.vec_allclose(q, ellipsoid_q, atol=2e-2),(and similarly for the other tests that don’t use
qd).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/geometry/collision_core.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(1 hunks)newton/examples/basic/example_basic_shapes.py(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 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.
Applied to files:
newton/examples/basic/example_basic_shapes.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/_src/geometry/collision_core.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/collision_core.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/geometry/collision_core.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/collision_core.py
🧬 Code graph analysis (3)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-67)
newton/examples/basic/example_basic_shapes.py (3)
newton/_src/sim/builder.py (2)
add_body(1664-1736)key(387-389)newton/examples/__init__.py (1)
test_body_state(38-113)newton/_src/utils/__init__.py (1)
vec_allclose(202-209)
newton/_src/geometry/collision_core.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-67)
🪛 Ruff (0.14.5)
newton/examples/basic/example_basic_shapes.py
164-164: Unused lambda argument: qd
(ARG005)
173-173: Unused lambda argument: qd
(ARG005)
206-206: Unused lambda argument: qd
(ARG005)
🔇 Additional comments (2)
newton/examples/basic/example_basic_shapes.py (1)
53-64: Ellipsoid setup and parameters look consistent with intended rest poseThe new ellipsoid body is wired correctly:
- Position is offset in Y like the other shapes and dropped from the same
drop_z.- Parameters
a=0.5, b=0.5, c=0.25match the “flat disk” comment; with ground at z=0 this implies a stable rest COM at z=0.25, which aligns with the test below.No issues here.
newton/_src/solvers/mujoco/solver_mujoco.py (1)
995-1004: MuJoCo geom mapping for ellipsoids is consistent; confirm target MuJoCo versionAdding
GeoType.ELLIPSOID: mujoco.mjtGeom.mjGEOM_ELLIPSOID,to
geom_type_mappingkeeps the MuJoCo export path aligned with the newGeoType.ELLIPSOIDand matches how other primitives are handled. This should make ellipsoid shapes round-trip cleanly into MuJoCo geoms.Just make sure the MuJoCo version you target exposes
mjGEOM_ELLIPSOID; if you still support older versions lacking this enum, you may need a guarded fallback or version check.
eric-heiden
left a comment
There was a problem hiding this comment.
Thanks, LGTM! Just a small comment, and consider the CodeRabbit comment as well.
# Conflicts: # newton/_src/viewer/viewer.py
Description
add_shape_ellipsoid(body, a, b, c, ...)method in ModelBuilderNewton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.