SDF Hydroelastic Collisions#1248
Conversation
commit 58eda20 Merge: 6e830d8 51f102b Author: nvtw <110816143+nvtw@users.noreply.github.com> Date: Tue Dec 9 08:37:10 2025 +0100 Merge branch 'main' into dev/tw/contact_reduction_experiment_no_sdf_2 commit 6e830d8 Author: Lennart Roestel <lroestel@nvidia.com> Date: Mon Dec 8 16:15:35 2025 +0100 make mujoco work for example_sdf commit caa2e2a Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 15:10:36 2025 +0100 Improve the default beta values commit 862fd5b Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 14:49:55 2025 +0100 Fix the unit tests commit b1e8a68 Merge: 28295ab affafaa Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 13:13:50 2025 +0100 Merge branch 'main' into dev/tw/contact_reduction_experiment_no_sdf_2 commit 28295ab Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 13:13:11 2025 +0100 Slightly optimize the intermediate data representation of mpr and simplex_solver commit 427afe6 Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 13:12:44 2025 +0100 Add more unit tests commit ab4dba5 Merge: 6f441cf e4078ce Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 12:29:32 2025 +0100 Merge branch 'main' into dev/tw/contact_reduction_experiment_no_sdf_2 commit 6f441cf Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 11:14:26 2025 +0100 Ran ruff commit b1afec1 Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 11:12:38 2025 +0100 Implement comment from last MR commit 2a56de8 Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 11:00:19 2025 +0100 Improve the sdf example commit 687d06e Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 10:15:37 2025 +0100 Centralize the configuration of sdfs in the ShapeConfig commit ac7a784 Author: twidmer <twidmer@nvidia.com> Date: Mon Dec 8 09:21:19 2025 +0100 Add possibility to use bvh queries in sdf contacts when no precomputed sdf is available
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
newton/_src/viewer/viewer.py (1)
134-168: Remove unusednoqadirective.The static analysis tool correctly identifies that the
noqadirective forPLC0415is unused. Additionally, the lazy evaluation pattern withFalseas a sentinel value is a clean approach.🔎 Proposed fix
- from ..geometry.sdf_utils import compute_isomesh # noqa: PLC0415 + from ..geometry.sdf_utils import compute_isomeshnewton/_src/sim/builder.py (1)
5967-6072:shape_sdf_shape2blocksis zero-length when SDF/hydro is disabled; safer to keep a per-shape sentinel mapping.When neither
has_sdf_meshesnorhas_hydroelastic_shapesis true,finalize()still allocates oneSDFDataper shape, but initializes:m.shape_sdf_block_coords = wp.array([], dtype=wp.vec3us) m.shape_sdf_shape2blocks = wp.array([], dtype=wp.vec2i)Conceptually,
shape_sdf_shape2blocksis “per-shape” metadata (mapping each shape to a[start, end)range intoshape_sdf_block_coords). If any downstream code ever indexesshape_sdf_shape2blocks[shape_idx]unconditionally—even in non‑hydro/SDF configurations—this becomes an immediate out‑of‑bounds access.Given that you already create per‑shape empty
SDFDataentries so narrow‑phase can safely touchshape_sdf_data[shape_idx], it would be more robust to mirror that pattern here and fill a per‑shape sentinel, e.g.:m.shape_sdf_block_coords = wp.array([], dtype=wp.vec3us) m.shape_sdf_shape2blocks = wp.array([[0, 0]] * len(self.shape_type), dtype=wp.vec2i)This keeps the metadata arrays shape‑consistent and makes accidental indexing safe even when SDF/hydro is globally disabled.
#!/bin/bash # Check all uses of shape_sdf_shape2blocks to ensure they guard against the "no-SDF" case. set -euo pipefail # Show all call sites with a bit of context rg -n "shape_sdf_shape2blocks" newton -C3 || true echo echo "If any kernel / function indexes model.shape_sdf_shape2blocks[shape_idx]" echo "without first checking that SDF/hydro is enabled (or that the array is non-empty)," echo "that path will currently be unsafe when no SDFs are built."
🧹 Nitpick comments (2)
newton/_src/viewer/viewer.py (1)
413-473: Consider making depth range configurable.The hardcoded depth range values (
min_depth=0.0,max_depth=0.0005) at lines 459-460 may not be suitable for all simulation scales. Consider exposing these as optional parameters or class attributes.🔎 Suggested approach
- def log_hydro_contact_surface(self, contact_surface_data, penetrating_only: bool = True): + def log_hydro_contact_surface( + self, + contact_surface_data, + penetrating_only: bool = True, + min_depth: float = 0.0, + max_depth: float = 0.0005, + ): ... inputs=[ vertices, depths, shape_pairs, self.model.shape_world, self.world_offsets, num_contacts, - 0.0, - 0.0005, + min_depth, + max_depth, penetrating_only, ],newton/_src/sim/builder.py (1)
197-210: Hydroelastic ShapeConfig + wiring look consistent; consider tightening the docstring around planes/heightfields.
- The new
is_hydroelastic/k_hydrofields and propagation (shape_material_k_hydroin the builder, copied viamore_builder_attrs, and exposed on the Model) are wired through correctly; solvers can reliably see per‑shape hydro stiffness.add_shape()’s logic that clearsShapeFlags.HYDROELASTICforGeoType.PLANE/GeoType.HFIELDmatches the doc intent of silently falling back for non‑volumetric shapes and avoids creating impossible hydro contacts for those types.One minor clarity nit: the note on
is_hydroelasticsays:“This flag will be set to False for planes and heightfields in :meth:
ModelBuilder.add_shape.”In practice,
add_shape()only clears the shape flag bit it pushes intoself.shape_flags; it does not mutatecfg.is_hydroelasticon the config instance. So:
- Reusing the same
ShapeConfigfor a mesh after adding a plane will still treat that mesh as hydroelastic (arguably desirable).- But reading back
cfg.is_hydroelasticafter callingadd_shape_planewill still showTrue, which doesn’t quite match the wording.If you want the config object to reflect the per‑shape behavior, you could also clear
cfg.is_hydroelasticin the PLANE/HFIELD branch. Otherwise, I’d slightly rephrase the note to say that the HYDROELASTIC shape flag is cleared for planes/heightfields inModelBuilder.add_shape().Also applies to: 244-269, 528-540, 1806-1807, 3514-3535
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/sim/builder.py(10 hunks)newton/_src/viewer/viewer.py(11 hunks)newton/_src/viewer/viewer_gl.py(4 hunks)newton/examples/robot/example_robot_panda_hydro.py(1 hunks)
🧰 Additional context used
🧠 Learnings (19)
📚 Learning: 2025-08-27T19:05:44.697Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 535
File: newton/tests/test_examples.py:320-414
Timestamp: 2025-08-27T19:05:44.697Z
Learning: In newton/examples/__init__.py, the robot policy example is registered with the key "robot_policy" (not "robot.example_robot_policy"), so tests should reference it as name="robot_policy".
Applied to files:
newton/examples/robot/example_robot_panda_hydro.py
📚 Learning: 2025-12-12T15:23:30.014Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_boxes_fourbar.py:149-163
Timestamp: 2025-12-12T15:23:30.014Z
Learning: In Kamino simulation examples under newton/_src/solvers/kamino/examples/sim/, the pattern where logging=True and use_cuda_graph=True creates a mismatch (SimulationLogger constructed but settings.compute_metrics and logger.log() calls guarded by `if not self.use_cuda_graph`) should not be flagged. These examples will be refactored in a future commit, and this pattern is intentionally deferred.
Applied to files:
newton/examples/robot/example_robot_panda_hydro.pynewton/_src/sim/builder.py
📚 Learning: 2025-10-29T14:55:48.171Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 940
File: newton/examples/mpm/example_mpm_granular.py:92-101
Timestamp: 2025-10-29T14:55:48.171Z
Learning: In newton/examples/mpm/example_mpm_granular.py, the capture() method intentionally advances the simulation state once during CUDA graph capture. This is expected behavior because CUDA graph capture records exact memory addresses, so the state arrays (state_0, state_1) cannot be reset or reallocated after capture without invalidating the graph.
Applied to files:
newton/examples/robot/example_robot_panda_hydro.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/examples/robot/example_robot_panda_hydro.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/examples/robot/example_robot_panda_hydro.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/examples/robot/example_robot_panda_hydro.py
📚 Learning: 2025-12-06T19:10:24.360Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_box_on_plane.py:322-371
Timestamp: 2025-12-06T19:10:24.360Z
Learning: In Newton Kamino examples under newton/_src/solvers/kamino/examples/sim, CLI arguments parsed via argparse (including flags like --test, --device, --headless, etc.) are passed as a complete args object to newton.examples.run(example, args). The run function internally consumes these flags to control execution behavior, so flags may appear "unused" in the example script itself even though they are actively used by the framework.
Applied to files:
newton/examples/robot/example_robot_panda_hydro.py
📚 Learning: 2025-08-25T21:41:45.795Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/tests/test_examples.py:329-333
Timestamp: 2025-08-25T21:41:45.795Z
Learning: Newton examples use a centralized CLI argument parsing system in newton/examples/__init__.py. The create_parser() function defines common arguments like --device, --viewer, --output-path, and --num-frames, while init(parser) creates viewers based on parsed arguments. Individual example scripts don't need to define these flags themselves - they inherit them from the centralized system via the example_map and runpy execution.
Applied to files:
newton/examples/robot/example_robot_panda_hydro.py
📚 Learning: 2025-12-12T08:45:43.428Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:43.428Z
Learning: In Newtown (Newton) example code, specifically files under newton/examples, computing contacts once per frame and reusing them across all substeps is an intentional design choice, not a bug. Reviewers should verify that contacts are computed before the substep loop and reused for every substep within the same frame. This pattern reduces redundant work and preserves frame-consistency; do not flag as a regression unless the behavior is changed for correctness or performance reasons.
Applied to files:
newton/examples/robot/example_robot_panda_hydro.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/viewer/viewer.pynewton/_src/sim/builder.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/_src/viewer/viewer.pynewton/_src/sim/builder.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/viewer/viewer.pynewton/_src/sim/builder.py
📚 Learning: 2025-12-12T17:45:26.847Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/geometry/primitives.py:0-0
Timestamp: 2025-12-12T17:45:26.847Z
Learning: In newton/_src/solvers/kamino/geometry/primitives.py, the narrow-phase kernel `_primitive_narrowphase` does not need to handle symmetric shape-pair orderings (e.g., both SPHERE-BOX and BOX-SPHERE) because `kamino.core.builder.ModelBuilder` and `kamino.geometry.primitive.broadphase` guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order.
Applied to files:
newton/_src/viewer/viewer.pynewton/_src/sim/builder.py
📚 Learning: 2025-12-12T17:41:15.097Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/_src/sim/builder.py:5556-5587
Timestamp: 2025-12-12T17:41:15.097Z
Learning: newton._src.geometry.sdf_utils.compute_sdf accepts max_resolution=None (e.g., when sdf_target_voxel_size is provided), so passing None from ModelBuilder.finalize is valid and should not be flagged.
Applied to files:
newton/_src/viewer/viewer.pynewton/_src/sim/builder.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/viewer/viewer.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/_src/sim/builder.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 the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-13T17:26:39.791Z
Learnt from: lenroe-nv
Repo: newton-physics/newton PR: 1248
File: newton/_src/geometry/contact_data.py:47-49
Timestamp: 2025-12-13T17:26:39.791Z
Learning: In ContactData (newton/_src/geometry/contact_data.py), the fields contact_stiffness, contact_damping, and contact_friction_scale use 0.0 as the "unset" sentinel value. This is intentional: Warp struct fields initialize to 0.0 by default, and solver-side code interprets 0.0 as "use default/unscaled" rather than "disable." This design avoids explicit initialization in contact generation kernels.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-12T08:46:29.875Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:189-191
Timestamp: 2025-12-12T08:46:29.875Z
Learning: In Newton, SDF data structures are shared across replicated world instances (e.g., when using ModelBuilder.replicate()). Therefore, the memory cost of SDF generation scales with sdf_max_resolution but does not scale with the number of worlds—100 worlds with sdf_max_resolution=512 uses the same SDF memory as 1 world with sdf_max_resolution=512.
Applied to files:
newton/_src/sim/builder.py
🧬 Code graph analysis (3)
newton/_src/viewer/viewer_gl.py (3)
newton/_src/viewer/picking.py (2)
is_picking(112-118)_apply_picking_force(86-110)newton/_src/sim/model.py (1)
state(536-573)newton/examples/diffsim/example_diffsim_drone.py (1)
state(447-448)
newton/_src/viewer/viewer.py (3)
newton/_src/viewer/kernels.py (1)
compute_hydro_contact_surface_lines(451-524)newton/_src/geometry/sdf_utils.py (1)
compute_isomesh(448-507)newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)
newton/_src/sim/builder.py (3)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)newton/_src/geometry/types.py (1)
GeoType(25-67)newton/_src/geometry/sdf_utils.py (3)
SDFData(27-51)compute_sdf(239-445)create_empty_sdf_data(60-76)
🪛 Ruff (0.14.8)
newton/examples/robot/example_robot_panda_hydro.py
363-363: Unused function argument: qd
(ARG001)
newton/_src/viewer/viewer.py
164-164: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
newton/_src/sim/builder.py
5900-5900: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
5948-5951: Avoid specifying long messages outside the exception class
(TRY003)
5954-5958: Avoid specifying long messages outside the exception class
(TRY003)
5961-5965: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (20)
newton/_src/viewer/viewer_gl.py (4)
430-430: LGTM — Picking guard is correctly applied.The additional
picking_enabledcheck ensures that picking-related rendering is only performed when the feature is enabled. This aligns with the new flag introduced in the base class.
498-499: LGTM — Force application respects the picking toggle.The picking force is now conditionally applied based on the
picking_enabledflag, correctly gating the behavior as intended.
761-761: LGTM — Right-click picking is gated correctly.The mouse press handler now respects the
picking_enabledflag before initiating a pick operation.
802-802: LGTM — Drag picking respects the enabled flag.Consistent with the other changes, right-button drag interactions are now conditional on
picking_enabled.newton/_src/viewer/viewer.py (9)
36-36: LGTM — Import addition for hydroelastic visualization.The new import for
compute_hydro_contact_surface_linesaligns with the hydroelastic surface rendering feature.
80-86: LGTM — New display options and lazy caches.The new
show_hydro_contact_surfaceandpicking_enabledflags provide good runtime control. The lazy allocation pattern for hydroelastic line buffers avoids unnecessary GPU memory usage.
95-101: LGTM — SDF isomesh caching infrastructure.Using
volume.idas the cache key is an effective deduplication strategy when multiple shapes share the same SDF volume.
299-322: LGTM — Lazy SDF isomesh rendering integration.The lazy population pattern (
_sdf_isomesh_populatedflag) is correctly implemented, avoiding GPU allocation untilshow_collisionis enabled. Thesdf_isomesh_just_populatedflag ensures appearance data is sent on the first render.
780-799: LGTM — Visibility logic now correctly separates collision and visual flags.The updated
_should_show_shapeproperly handles shapes that can be both collision and visual, respecting both toggle states independently.
912-926: LGTM — Flag manipulation for dual-purpose shapes with SDF.The logic correctly identifies shapes that are both visual and collision with SDF volumes, and removes the
COLLIDE_SHAPESflag so the visual mesh is rendered separately from the SDF isomesh. This prevents duplicate rendering.
940-944: LGTM — Collision shape coloring uses shape index.Using
_shape_color_map(s)with the shape index ensures distinct colors per collision shape, improving visual differentiation.
993-1080: LGTM — Lazy SDF isomesh population implementation.The method correctly:
- Only processes collision shapes with SDF volumes
- Handles the
scale_bakedflag appropriately- Uses geometry hashing for batching
- Applies distinct collision colors
1258-1276: LGTM — Distinct collision color palette.The
_collision_color_mapprovides desaturated, cooler tones that contrast well with the brighter visual shape colors, aiding visual distinction between collision and visual geometry.newton/examples/robot/example_robot_panda_hydro.py (7)
27-63: LGTM! Clean utilities and imports.The imports, enum, and utility functions are well-structured. The
broadcast_ik_solution_kernelefficiently replicates a single IK solution across multiple parallel worlds, which is a nice optimization for batched simulation.
65-216: LGTM! Robust model construction with smart optimizations.The model building is thorough and well-structured:
- Selective hydroelastic collision on finger bodies only (lines 99-108) reduces computational overhead while maintaining contact quality where needed.
- Convex hull approximation for non-finger shapes (lines 111-113) is an effective performance optimization.
- USD asset loading for pads and cup follows proper patterns with transforms and scales.
- Scene-specific handling for pen and cube objects is clean.
217-272: LGTM! Comprehensive collision and solver setup.The collision pipeline and solver configuration are well-thought-out:
- SDFHydroelasticConfig properly links
output_contact_surfaceto the isosurface toggle (line 227).- MuJoCo solver parameters are extensively tuned for the manipulation task (lines 238-251).
- IK is solved on a single world (
model_single) then broadcast to all parallel worlds, which is efficient (lines 261-263).
274-307: LGTM! Efficient IK control pipeline.The waypoint interpolation and IK solving is well-implemented:
- Smoothly interpolates position, rotation, and gripper state (lines 278-296).
- Efficiently broadcasts a single IK solution to all parallel worlds (lines 297-302), minimizing computation.
- Proper CUDA graph usage when available (lines 289-292).
314-348: LGTM! Efficient simulation and rendering.The simulation loop is well-optimized:
- CUDA graph capture properly guards with device check (line 316).
- Computing contacts every
collide_substeps(line 326) and reusing them across substeps is an intentional performance optimization, as per project patterns.- Rendering logic cleanly handles state, contacts, and optional hydroelastic contact surfaces.
Based on learnings, the contact reuse pattern is by design.
355-373: Test verification looks good.The
test_finalmethod properly validates that the object lands in the cup. The closurein_cuphas an unusedqdparameter (line 363), but this is likely required by thetest_body_stateAPI signature and is not a real issue.
375-467: LGTM! Complete IK setup and clean entry point.The IK configuration and waypoint planning are well-designed:
- IK objectives include position, rotation, and joint limits (lines 381-398).
- Waypoint sequence logically defines the pick-and-place task (lines 419-441).
- Scene-specific adjustments (grasping offset, place offset) handle pen vs. cube differences.
- Command-line interface follows Newton examples conventions with scene type and num-worlds arguments.
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/sim/collide_unified.py (1)
86-177: Critical bug: Array access without None checks in write_contactLines 168-176 attempt to access
.shape[0]on array fields that may be None:
- Lines 168-170:
writer_data.contact_keyandwriter_data.contact_pair_keyare assignedNonewhen contact matching arrays are not enabled- Lines 173-176:
writer_data.out_stiffness/out_damping/out_frictionare assignedNonewhenper_contact_shape_properties=FalseAccessing
.shape[0]on None will raiseAttributeError. Both checks need to guard against None before accessing shape attributes:# Lines 168-170 should be: if writer_data.contact_key is not None and writer_data.contact_pair_key is not None and writer_data.contact_key.shape[0] > 0 and writer_data.contact_pair_key.shape[0] > 0: # Lines 173-176 should be: if writer_data.out_stiffness is not None and writer_data.out_stiffness.shape[0] > 0:The
output_indexparameter implementation is correct, but these None-safety checks must be fixed before the function can safely run with disabled optional features.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/sim/collide_unified.py(14 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
📚 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/sim/collide_unified.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 the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/sim/collide_unified.py
📚 Learning: 2025-12-12T08:45:51.908Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:51.908Z
Learning: In Newton examples (e.g., example_sdf.py), computing contacts once per frame and reusing them across multiple substeps is an intentional design choice, not a bug. The contacts are computed before the substep loop and intentionally kept constant throughout all substeps within that frame.
Applied to files:
newton/_src/sim/collide_unified.py
⏰ 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). (3)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (6)
newton/_src/sim/collide_unified.py (6)
29-29: LGTM!The import of hydroelastic components is correctly placed and necessary for the integration.
65-69: LGTM!The per-contact property arrays are properly documented with clear semantics for empty arrays and zero-values.
308-308: LGTM!The
sdf_hydroelasticparameter is properly typed, documented, and wired through toNarrowPhase. Reading back the value at line 416 is correct sinceNarrowPhasemay modify or validate it.Also applies to: 347-348, 414-416
453-453: LGTM!The
sdf_hydroelastic_configparameter and initialization logic are correct. The comment at line 498 clarifies thatSDFHydroelastic._from_modelreturnsNoneif no hydroelastic pairs exist, which is properly handled by downstream None checks.Also applies to: 475-475, 497-520
544-544: LGTM!The per-contact properties wiring is correct:
- Line 544 properly uses
narrow_phase.sdf_hydroelastic(notself.sdf_hydroelastic) as the authoritative source- Lines 656-658 populate
writer_datawith contact property arrays (empty if disabled, checked at lines 173-176)- Line 671 correctly passes
shape_flagsto the narrow phaseAlso applies to: 656-658, 671-671
678-686: LGTM!The
get_hydro_contact_surfaceaccessor is a clean delegation pattern that provides visualization access to hydroelastic contact surface data when configured.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
newton/_src/geometry/sdf_hydroelastic.py (1)
1456-1464: Remove unused parametersnum_normal_binsandmargin_contact_area.These parameters are passed to the factory function but never used within the three kernels it creates.
num_normal_binsis only used in the calling scope to size arrays, andmargin_contact_areais used inget_decode_contacts_kernelbut not here.newton/_src/sim/builder.py (1)
5903-6075: SDF/hydro finalize path is well‑structured; consider safer default forshape_sdf_shape2blockswhen SDF is disabledThe new SDF/hydro section in
finalize()looks thoughtfully put together:
has_sdf_meshes/has_hydroelastic_shapesdetection and the per‑shape validation loop correctly enforce “hydroelastic ⇒ SDF params set” and CUDA‑only execution.needs_sdfcleanly captures “mesh + SDF config” or “hydro + colliding shape”, and thebake_scale = is_hydroelasticswitch gives a reasonable split between mesh‑SDF and hydro uses.- The
sdf_cachekeyed on(hash(shape_src), shape_type, thickness, margin, narrow band, resolution, baked scale)plus sharedsdf_block_coords/sdf_shape2blocksranges is a nice optimization and appears index‑consistent.One robustness concern remains from earlier review: in the
elsebranch when neither mesh SDF nor hydroelastics are active,m.shape_sdf_shape2blocksis created as an empty array even though downstream code may conceptually treat it as “per‑shape metadata” similar toshape_sdf_data. If any narrow‑phase or hydro code ever indexesshape_sdf_shape2blocks[shape_idx]without first checking that SDF is enabled, this would be an out‑of‑bounds access.Given the low cost, you could defensively give every shape a
[0, 0]sentinel range in the disabled case:Suggested sentinel initialization for `shape_sdf_shape2blocks`
else: # SDF mesh-mesh collision and hydroelastics not enabled or no colliding meshes/shapes # Still need one SDFData per shape (all empty) so narrow phase can safely access shape_sdf_data[shape_idx] empty_sdf_data = create_empty_sdf_data() m.shape_sdf_data = wp.array([empty_sdf_data] * len(self.shape_type), dtype=SDFData, device=device) m.shape_sdf_volume = [None] * len(self.shape_type) m.shape_sdf_coarse_volume = [None] * len(self.shape_type) m.shape_sdf_block_coords = wp.array([], dtype=wp.vec3us) - m.shape_sdf_shape2blocks = wp.array([], dtype=wp.vec2i) + m.shape_sdf_shape2blocks = wp.array( + [[0, 0]] * len(self.shape_type), + dtype=wp.vec2i, + )This keeps the “no SDF blocks” semantics (empty
shape_sdf_block_coords) but makesshape_sdf_shape2blocks[shape_idx]safe for all shapes.To double‑check that no current kernel relies on
shape_sdf_shape2blocksbeing empty when SDF is disabled (and that there are no unconditional index reads), you can run:#!/bin/bash # Inspect all uses of shape_sdf_shape2blocks for potential unconditional indexing rg -n "shape_sdf_shape2blocks" newton -S -C3
🧹 Nitpick comments (2)
newton/_src/geometry/sdf_hydroelastic.py (2)
314-314: Minor: Remove unused noqa directive.The
# noqa: PLC0415comment is flagged as unused by static analysis. The import is valid as-is.🔎 Suggested fix
- from ..sim.builder import ShapeFlags # noqa: PLC0415 + from ..sim.builder import ShapeFlags
1367-1367: Minor: Remove unnecessaryint()cast.The literal
0is already an integer; the explicitint()call is redundant.🔎 Suggested fix
- local_idx = int(0) + local_idx = 0
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/geometry/sdf_hydroelastic.py(1 hunks)newton/_src/sim/builder.py(11 hunks)
🧰 Additional context used
🧠 Learnings (21)
📚 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/sdf_hydroelastic.pynewton/_src/sim/builder.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/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-10-30T07:28:13.112Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 984
File: newton/_src/geometry/broad_phase_nxn.py:318-336
Timestamp: 2025-10-30T07:28:13.112Z
Learning: In Newton's BroadPhaseAllPairs class (newton/_src/geometry/broad_phase_nxn.py), device management is the caller's responsibility. The library intentionally does not perform automatic device transfers for precomputed arrays in the launch method, even if there's a device mismatch. Users must ensure that the device specified in __init__ matches the device used in launch.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.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 the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-12-13T17:26:39.791Z
Learnt from: lenroe-nv
Repo: newton-physics/newton PR: 1248
File: newton/_src/geometry/contact_data.py:47-49
Timestamp: 2025-12-13T17:26:39.791Z
Learning: In ContactData (newton/_src/geometry/contact_data.py), the fields contact_stiffness, contact_damping, and contact_friction_scale use 0.0 as the "unset" sentinel value. This is intentional: Warp struct fields initialize to 0.0 by default, and solver-side code interprets 0.0 as "use default/unscaled" rather than "disable." This design avoids explicit initialization in contact generation kernels.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-09-26T05:56:07.255Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/kernels.py:220-227
Timestamp: 2025-09-26T05:56:07.255Z
Learning: In the Style3D collision system, static_diags values used in edge-edge collision stiffness blending won't result in zero denominators in practice, despite inactive particles having static_A_diags = 0.0, due to system-level constraints preventing such edge cases in collision detection.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-10-06T18:14:18.582Z
Learnt from: daedalus5
Repo: newton-physics/newton PR: 868
File: newton/examples/diffsim/example_diffsim_bear.py:82-96
Timestamp: 2025-10-06T18:14:18.582Z
Learning: Warp's tile operations (`wp.tile_store()` and `wp.tile_load()`) have built-in boundary checking. Out-of-bounds writes are automatically skipped and out-of-bounds reads are zero-filled, so tiled kernels are safe even when the array size is not a multiple of the tile size.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-12-12T17:45:26.847Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/geometry/primitives.py:0-0
Timestamp: 2025-12-12T17:45:26.847Z
Learning: In newton/_src/solvers/kamino/geometry/primitives.py, the narrow-phase kernel `_primitive_narrowphase` does not need to handle symmetric shape-pair orderings (e.g., both SPHERE-BOX and BOX-SPHERE) because `kamino.core.builder.ModelBuilder` and `kamino.geometry.primitive.broadphase` guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-09-09T10:33:36.573Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/rasterized_collisions.py:133-138
Timestamp: 2025-09-09T10:33:36.573Z
Learning: In newton's implicit MPM rasterized collisions system, collider_density function doesn't need zero volume guards because collider_id being registered to a node implies non-zero volume through the collider_volumes integrand's atomic accumulation logic.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-12-12T08:45:51.908Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:51.908Z
Learning: In Newton examples (e.g., example_sdf.py), computing contacts once per frame and reusing them across multiple substeps is an intentional design choice, not a bug. The contacts are computed before the substep loop and intentionally kept constant throughout all substeps within that frame.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-09-18T07:05:56.836Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:969-986
Timestamp: 2025-09-18T07:05:56.836Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, transposed_strain_mat parameter cannot be None - the type signature was corrected to reflect this guarantee, eliminating the need for None checks when accessing transposed_strain_mat.offsets.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.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/sdf_hydroelastic.py
📚 Learning: 2025-12-12T17:41:15.097Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/_src/sim/builder.py:5556-5587
Timestamp: 2025-12-12T17:41:15.097Z
Learning: newton._src.geometry.sdf_utils.compute_sdf accepts max_resolution=None (e.g., when sdf_target_voxel_size is provided), so passing None from ModelBuilder.finalize is valid and should not be flagged.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.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/_src/sim/builder.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-12T15:23:30.014Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_boxes_fourbar.py:149-163
Timestamp: 2025-12-12T15:23:30.014Z
Learning: In Kamino simulation examples under newton/_src/solvers/kamino/examples/sim/, the pattern where logging=True and use_cuda_graph=True creates a mismatch (SimulationLogger constructed but settings.compute_metrics and logger.log() calls guarded by `if not self.use_cuda_graph`) should not be flagged. These examples will be refactored in a future commit, and this pattern is intentionally deferred.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-12T08:46:29.875Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:189-191
Timestamp: 2025-12-12T08:46:29.875Z
Learning: In Newton, SDF data structures are shared across replicated world instances (e.g., when using ModelBuilder.replicate()). Therefore, the memory cost of SDF generation scales with sdf_max_resolution but does not scale with the number of worlds—100 worlds with sdf_max_resolution=512 uses the same SDF memory as 1 world with sdf_max_resolution=512.
Applied to files:
newton/_src/sim/builder.py
🧬 Code graph analysis (2)
newton/_src/geometry/sdf_hydroelastic.py (7)
newton/_src/geometry/collision_core.py (2)
build_pair_key2(43-52)sat_box_intersection(1076-1115)newton/_src/geometry/contact_data.py (1)
ContactData(26-66)newton/_src/geometry/contact_reduction.py (2)
get_slot(180-226)get_spatial_direction_2d(166-176)newton/_src/geometry/sdf_contact.py (1)
sample_sdf_extrapolated(169-223)newton/_src/geometry/sdf_mc.py (1)
get_mc_tables(34-86)newton/_src/geometry/sdf_utils.py (1)
SDFData(27-51)newton/_src/geometry/utils.py (1)
scan_with_total(840-856)
newton/_src/sim/builder.py (3)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)newton/_src/geometry/types.py (1)
GeoType(25-67)newton/_src/geometry/sdf_utils.py (2)
SDFData(27-51)compute_sdf(239-445)
🪛 Ruff (0.14.8)
newton/_src/geometry/sdf_hydroelastic.py
314-314: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
345-345: Avoid specifying long messages outside the exception class
(TRY003)
976-976: Redefinition of unused sdf_diff_sdf from line 942
(F811)
1367-1367: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1458-1458: Unused function argument: num_normal_bins
(ARG001)
1463-1463: Unused function argument: margin_contact_area
(ARG001)
newton/_src/sim/builder.py
220-222: Avoid specifying long messages outside the exception class
(TRY003)
5904-5904: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
5952-5955: Avoid specifying long messages outside the exception class
(TRY003)
5958-5962: Avoid specifying long messages outside the exception class
(TRY003)
5965-5969: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (9)
newton/_src/geometry/sdf_hydroelastic.py (6)
40-42: Good: Constants extracted to module level.Moving threshold constants (
MIN_FRICTION,EPS_LARGE,EPS_SMALL) to module scope improves readability and makes them easier to tune if needed.
71-104: Well-documented configuration class.The
SDFHydroelasticConfigdataclass provides clear docstrings for each parameter, making it easy for users to understand and tune the hydroelastic behavior.
165-167: LGTM - Device management is correctly implemented.The
deviceparameter is properly handled with a fallback towp.get_device(), and all buffer allocations occur withinwp.ScopedDevice(device)context, ensuring consistent device placement.Also applies to: 189-244
941-1005: Warp function overloading is intentional - static analysis false positive.Ruff flags
sdf_diff_sdfat line 976 as a redefinition (F811), but this is a false positive. Warp's@wp.funcdecorator supports function overloading based on argument types:
- Lines 941-972: Signature with
x_id: wp.int32, y_id: wp.int32, z_id: wp.int32for voxel-index lookup- Lines 975-1004: Signature with
pos_a_local: wp.vec3for interpolated samplingWarp dispatches to the correct implementation at compile time based on call-site argument types. This is a valid and intentional pattern.
1100-1109: LGTM - Inner-loop bounds check prevents out-of-bounds writes.The condition at line 1104 correctly guards against buffer overflow by checking
write_idx >= max_num_iso_subblocksbefore each write operation within the loop.
1871-1930: Good: Comprehensive buffer overflow verification.The
verify_collision_stepkernel provides actionable warnings for each pipeline stage, helping users identify and resolve buffer sizing issues by recommending specific multiplier parameters to adjust.newton/_src/sim/builder.py (3)
197-210: Hydroelastic ShapeConfig fields and validation look solid
is_hydroelastic/k_hydrodefaults, the SDF requirement in__post_init__, and the HYDROELASTIC flag wiring inflags/flags.setterare coherent and align with the documented semantics (hydro requires SDF, flags stay in sync with config). I don’t see functional issues here.Also applies to: 219-222, 248-249, 256-273
532-533:k_hydropropagation through builder/add_builder/finalize is consistentInitializing
self.shape_material_k_hydro, copying it inmore_builder_attrsduringadd_builder, and exporting it asm.shape_material_k_hydroinfinalize()ensures per‑shape hydro stiffness survives replication and model construction. This looks correctly wired and non‑invasive for non‑hydro shapes.Also applies to: 1810-1811, 5894-5896
3518-3524: Hydro flag masking for planes/heightfields is correctMasking out
ShapeFlags.HYDROELASTICforGeoType.PLANEandGeoType.HFIELDwhile still appendingk_hydrogives the desired “fallback to standard collisions” behavior for non‑volumetric shapes without disturbing shared configs. The rest ofadd_shapecontinues to respect collision flags and inertia as before.Also applies to: 3538-3547
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
newton/_src/sim/builder.py (1)
5904-6077: SDF/hydro finalize path: caching looks good; consider safer default forshape_sdf_shape2blockswhen SDF is disabled.The new SDF/hydro block is well structured:
has_sdf_meshesandhas_hydroelastic_shapesaccurately gate GPU‑only paths.- Per‑shape hydro validation (missing SDF params) gives users clear error messages before
compute_sdf.- The SDF cache key (including
shape_scaleonly whenbake_scaleis True) prevents accidental sharing between hydro and non‑hydro shapes and between differently scaled shapes.sdf_block_coords+sdf_shape2blocks[start,end)is a nice compact encoding of tile ranges.One defensive improvement:
In the
elsebranch (has_sdf_meshesandhas_hydroelastic_shapesare both False), you currently set:m.shape_sdf_block_coords = wp.array([], dtype=wp.vec3us) m.shape_sdf_shape2blocks = wp.array([], dtype=wp.vec2i)whereas in the main path, every shape gets a
[start, end]entry insdf_shape2blocks, with non‑SDF shapes using[0, 0]as a sentinel. If any narrow‑phase/hydro code ever indexesshape_sdf_shape2blocks[shape_idx]without first checking the globalhas_sdf_meshes/has_hydroelastic_shapescondition, the empty array becomes an out‑of‑bounds hazard.To make the API uniformly safe, you could mirror the per‑shape sentinel behavior here:
from ..geometry.sdf_utils import create_empty_sdf_data empty_sdf_data = create_empty_sdf_data() m.shape_sdf_data = wp.array([empty_sdf_data] * len(self.shape_type), dtype=SDFData, device=device) m.shape_sdf_volume = [None] * len(self.shape_type) m.shape_sdf_coarse_volume = [None] * len(self.shape_type) m.shape_sdf_block_coords = wp.array([], dtype=wp.vec3us) m.shape_sdf_shape2blocks = wp.array([[0, 0]] * len(self.shape_type), dtype=wp.vec2i)That way,
shape_sdf_shape2blocks[shape_idx]is always defined, and[0, 0]consistently represents “no tiles”.This was already pointed out in a prior review; the behavior is still the same in this revision, so treating it as a defensive clean‑up rather than a confirmed bug seems appropriate.
🧹 Nitpick comments (3)
newton/_src/geometry/sdf_hydroelastic.py (3)
314-314: Remove unusednoqadirective.The
# noqa: PLC0415comment is unnecessary since PLC0415 (import not at top level) is not enabled in the linter configuration.🔎 Proposed fix
- from ..sim.builder import ShapeFlags # noqa: PLC0415 + from ..sim.builder import ShapeFlags
1367-1367: Remove unnecessaryint()cast.The explicit
int(0)cast is redundant since0is already an integer literal.🔎 Proposed fix
- local_idx = int(0) + local_idx = 0
1456-1465: Remove unused parameters fromget_binning_kernels.The static analysis correctly identifies that
num_normal_bins(line 1458) andmargin_contact_area(line 1463) are not used within this factory function or the kernels it creates. These parameters add confusion to the function signature. This was flagged in a previous review comment.🔎 Proposed fix
Update the function signature:
def get_binning_kernels( n_bin_dirs: int, - num_normal_bins: int, num_betas: int, sticky_contacts: float = 1e-6, normal_matching: bool = True, moment_matching: bool = True, - margin_contact_area: float = 1e-4, writer_func: Any = None, ):And update the call site in
__init__(around line 283):self.compute_bin_scores, self.assign_contacts_to_bins, self.generate_contacts_from_bins = ( get_binning_kernels( self.bin_directions, - num_normal_bins, self.num_betas, self.config.sticky_contacts, self.config.normal_matching, self.config.moment_matching, - self.config.margin_contact_area, writer_func, ) )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/geometry/sdf_hydroelastic.py(1 hunks)newton/_src/sim/builder.py(11 hunks)
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
📚 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/sdf_hydroelastic.pynewton/_src/sim/builder.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/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-10-30T07:28:13.112Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 984
File: newton/_src/geometry/broad_phase_nxn.py:318-336
Timestamp: 2025-10-30T07:28:13.112Z
Learning: In Newton's BroadPhaseAllPairs class (newton/_src/geometry/broad_phase_nxn.py), device management is the caller's responsibility. The library intentionally does not perform automatic device transfers for precomputed arrays in the launch method, even if there's a device mismatch. Users must ensure that the device specified in __init__ matches the device used in launch.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.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 the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-12-13T17:26:39.791Z
Learnt from: lenroe-nv
Repo: newton-physics/newton PR: 1248
File: newton/_src/geometry/contact_data.py:47-49
Timestamp: 2025-12-13T17:26:39.791Z
Learning: In ContactData (newton/_src/geometry/contact_data.py), the fields contact_stiffness, contact_damping, and contact_friction_scale use 0.0 as the "unset" sentinel value. This is intentional: Warp struct fields initialize to 0.0 by default, and solver-side code interprets 0.0 as "use default/unscaled" rather than "disable." This design avoids explicit initialization in contact generation kernels.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-09-26T05:56:07.255Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/kernels.py:220-227
Timestamp: 2025-09-26T05:56:07.255Z
Learning: In the Style3D collision system, static_diags values used in edge-edge collision stiffness blending won't result in zero denominators in practice, despite inactive particles having static_A_diags = 0.0, due to system-level constraints preventing such edge cases in collision detection.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-10-06T18:14:18.582Z
Learnt from: daedalus5
Repo: newton-physics/newton PR: 868
File: newton/examples/diffsim/example_diffsim_bear.py:82-96
Timestamp: 2025-10-06T18:14:18.582Z
Learning: Warp's tile operations (`wp.tile_store()` and `wp.tile_load()`) have built-in boundary checking. Out-of-bounds writes are automatically skipped and out-of-bounds reads are zero-filled, so tiled kernels are safe even when the array size is not a multiple of the tile size.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-12-12T17:45:26.847Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/geometry/primitives.py:0-0
Timestamp: 2025-12-12T17:45:26.847Z
Learning: In newton/_src/solvers/kamino/geometry/primitives.py, the narrow-phase kernel `_primitive_narrowphase` does not need to handle symmetric shape-pair orderings (e.g., both SPHERE-BOX and BOX-SPHERE) because `kamino.core.builder.ModelBuilder` and `kamino.geometry.primitive.broadphase` guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-09-09T10:33:36.573Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/rasterized_collisions.py:133-138
Timestamp: 2025-09-09T10:33:36.573Z
Learning: In newton's implicit MPM rasterized collisions system, collider_density function doesn't need zero volume guards because collider_id being registered to a node implies non-zero volume through the collider_volumes integrand's atomic accumulation logic.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-12-12T08:45:51.908Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:51.908Z
Learning: In Newton examples (e.g., example_sdf.py), computing contacts once per frame and reusing them across multiple substeps is an intentional design choice, not a bug. The contacts are computed before the substep loop and intentionally kept constant throughout all substeps within that frame.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.py
📚 Learning: 2025-09-18T07:05:56.836Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:969-986
Timestamp: 2025-09-18T07:05:56.836Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, transposed_strain_mat parameter cannot be None - the type signature was corrected to reflect this guarantee, eliminating the need for None checks when accessing transposed_strain_mat.offsets.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.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/sdf_hydroelastic.py
📚 Learning: 2025-12-12T17:41:15.097Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/_src/sim/builder.py:5556-5587
Timestamp: 2025-12-12T17:41:15.097Z
Learning: newton._src.geometry.sdf_utils.compute_sdf accepts max_resolution=None (e.g., when sdf_target_voxel_size is provided), so passing None from ModelBuilder.finalize is valid and should not be flagged.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/geometry/sdf_hydroelastic.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/_src/sim/builder.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-12T15:23:30.014Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_boxes_fourbar.py:149-163
Timestamp: 2025-12-12T15:23:30.014Z
Learning: In Kamino simulation examples under newton/_src/solvers/kamino/examples/sim/, the pattern where logging=True and use_cuda_graph=True creates a mismatch (SimulationLogger constructed but settings.compute_metrics and logger.log() calls guarded by `if not self.use_cuda_graph`) should not be flagged. These examples will be refactored in a future commit, and this pattern is intentionally deferred.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-12T08:46:29.875Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:189-191
Timestamp: 2025-12-12T08:46:29.875Z
Learning: In Newton, SDF data structures are shared across replicated world instances (e.g., when using ModelBuilder.replicate()). Therefore, the memory cost of SDF generation scales with sdf_max_resolution but does not scale with the number of worlds—100 worlds with sdf_max_resolution=512 uses the same SDF memory as 1 world with sdf_max_resolution=512.
Applied to files:
newton/_src/sim/builder.py
🧬 Code graph analysis (2)
newton/_src/geometry/sdf_hydroelastic.py (10)
newton/_src/geometry/collision_core.py (2)
build_pair_key2(43-52)sat_box_intersection(1076-1115)newton/_src/geometry/contact_data.py (1)
ContactData(26-66)newton/_src/geometry/contact_reduction.py (2)
get_slot(180-226)get_spatial_direction_2d(166-176)newton/_src/geometry/sdf_contact.py (1)
sample_sdf_extrapolated(169-223)newton/_src/geometry/sdf_mc.py (1)
get_mc_tables(34-86)newton/_src/geometry/sdf_utils.py (1)
SDFData(27-51)newton/_src/geometry/utils.py (1)
scan_with_total(840-856)newton/_src/sim/contacts.py (1)
device(110-114)newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)newton/_src/sim/collide_unified.py (1)
get_hydro_contact_surface(678-686)
newton/_src/sim/builder.py (3)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)newton/_src/geometry/types.py (1)
GeoType(25-67)newton/_src/geometry/sdf_utils.py (3)
SDFData(27-51)compute_sdf(239-445)create_empty_sdf_data(60-76)
🪛 Ruff (0.14.8)
newton/_src/geometry/sdf_hydroelastic.py
314-314: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
345-345: Avoid specifying long messages outside the exception class
(TRY003)
976-976: Redefinition of unused sdf_diff_sdf from line 942
(F811)
1367-1367: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1458-1458: Unused function argument: num_normal_bins
(ARG001)
1463-1463: Unused function argument: margin_contact_area
(ARG001)
newton/_src/sim/builder.py
215-218: Avoid specifying long messages outside the exception class
(TRY003)
220-222: Avoid specifying long messages outside the exception class
(TRY003)
5905-5905: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
5953-5956: Avoid specifying long messages outside the exception class
(TRY003)
5959-5963: Avoid specifying long messages outside the exception class
(TRY003)
5966-5970: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (8)
newton/_src/geometry/sdf_hydroelastic.py (7)
40-43: LGTM! Constants are now defined at module level.The threshold constants (
MIN_FRICTION,EPS_LARGE,EPS_SMALL) are appropriately extracted to named constants at the top of the module, addressing the earlier review feedback about exposing tunable parameters.
50-104: LGTM! Well-structured configuration and data classes.Both
HydroelasticContactSurfaceDataandSDFHydroelasticConfigare properly documented with clear field descriptions. The configuration defaults are reasonable and the docstrings explain the purpose of each field.
148-298: LGTM! Buffer initialization with proper device handling.The constructor correctly handles device allocation (addressing the past review concern), initializes all necessary buffers for the collision pipeline stages, and properly configures the kernel factories based on the reduction settings.
451-646: LGTM! Pipeline stages are well-structured.The broadphase, iso-voxel refinement, and contact generation stages follow a consistent count-scan-scatter pattern. The octree-like hierarchical refinement efficiently prunes the search space.
976-1005: Warp function overloading is intentional.The static analysis warning about redefinition of
sdf_diff_sdfat line 976 is a false positive. Warp's@wp.funcdecorator supports function overloading based on parameter types. The two versions handle different input types (integer voxel coordinates vs. local position vec3), which is a common pattern in Warp codebases.
1795-1798: Verify overflow handling behavior is intentional.The current overflow handling reserves slots atomically then skips writing if capacity is exceeded, leaving
contact_countpotentially inflated. This is consistent withdecode_contacts_kerneland relies onverify_collision_stepto warn users. Since past review confirmed this as addressed, this appears to be an intentional design choice to keep the code simple rather than adding rollback complexity.
1871-1930: LGTM! Comprehensive buffer overflow verification.The
verify_collision_stepkernel provides excellent runtime safety by checking all buffer stages and printing actionable warnings with specific configuration parameters to adjust. This helps users diagnose and fix capacity issues.newton/_src/sim/builder.py (1)
532-532: Wiring ofk_hydrothrough builder/add_builder/finalize looks correct.
self.shape_material_k_hydrois initialized, populated fromcfg.k_hydroinadd_shape(), copied inadd_builder()viamore_builder_attrs, and exported asm.shape_material_k_hydroinfinalize(). That gives solvers a clean per‑shape hydro stiffness channel and preserves it across world replication and sub‑builders.One thing to double‑check in the downstream contact/solver code: ensure that
shape_material_k_hydrois only interpreted when the shape pair is actually hydroelastic (e.g. gated onShapeFlags.HYDROELASTICor a dedicated per‑shape flag), so non‑hydro shapes carrying the default1e10don’t inadvertently affect contact stiffness in non‑hydro paths.Also applies to: 1810-1811, 3539-3539, 5895-5897
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/concepts/collisions.rst (1)
393-423: Hydroelastic section documentation should include parameter guidance and viewer feature reference.The new Hydroelastic Contacts section explains enabling the flag, the both-shapes requirement, and
k_hydrotuning well. The suggested enhancements remain valuable:
sdf_max_resolutionguidance: Currently documented only as a code comment. Add brief explanation of its impact on grid resolution and suggest typical ranges (e.g., 32–64 for most use cases).reduce_contactsbehavior: Document that this defaults toTrueand reduces contact points for mesh-mesh and mesh-plane collisions via shared memory contact reduction, improving performance and stability.- Visualization features: Reference the
show_hydro_contact_surfaceviewer toggle to help readers discover the visualization capability for debugging contact surfaces.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/concepts/collisions.rst(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
📚 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:
docs/concepts/collisions.rst
📚 Learning: 2025-12-13T17:26:39.791Z
Learnt from: lenroe-nv
Repo: newton-physics/newton PR: 1248
File: newton/_src/geometry/contact_data.py:47-49
Timestamp: 2025-12-13T17:26:39.791Z
Learning: In ContactData (newton/_src/geometry/contact_data.py), the fields contact_stiffness, contact_damping, and contact_friction_scale use 0.0 as the "unset" sentinel value. This is intentional: Warp struct fields initialize to 0.0 by default, and solver-side code interprets 0.0 as "use default/unscaled" rather than "disable." This design avoids explicit initialization in contact generation kernels.
Applied to files:
docs/concepts/collisions.rst
⏰ 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). (4)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (1)
docs/concepts/collisions.rst (1)
384-386: Material properties table addition looks good.The
shape_material_k_hydroentry is well-integrated and correctly identifies SemiImplicit, Featherstone, and MuJoCo as supported solvers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
newton/_src/sim/builder.py (1)
3482-3545: Order ofcfg.validate()vs. PLANE/HFIELD HYDROELASTIC clearingCurrently
cfg.validate()runs before you clearShapeFlags.HYDROELASTICforGeoType.PLANE/HFIELD. That means a config like:cfg = ModelBuilder.ShapeConfig(is_hydroelastic=True) builder.add_ground_plane(cfg=cfg)will raise
"Hydroelastic shapes require an SDF..."even though the note onis_hydroelasticsays the flag is automatically disabled for planes/heightfields inadd_shape().If the intended UX is “users can reuse a global
ShapeConfig(is_hydroelastic=True, …)with planes/heightfields and have them silently downgraded”, consider either:
- letting
validate()accept an optionalgeo_typeand skipping the hydro/SDF requirement for unsupported types, or- moving the validation call after you clear
HYDROELASTICfor planes/heightfields and, if you want config to reflect the downgrade, also settingcfg.is_hydroelastic = Falsein that branch.Otherwise, you may want to adjust the docstring on
is_hydroelasticto clarify that even for planes/heightfields, setting it to True still requires SDF parameters.
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
6053-6061: Consider safer defaults forshape_sdf_shape2blockswhen SDF/hydro are disabledIn the
elsebranch (no SDF meshes and no hydroelastic shapes),m.shape_sdf_shape2blocksis created as an empty array whileshape_sdf_datais still sized per shape. If any downstream code ever indexesshape_sdf_shape2blocks[shape_idx]unconditionally (even in a non‑SDF path), this would be an immediate out‑of‑bounds.A more defensive default would be to allocate a per‑shape array of
[0, 0]sentinels:Proposed defensive initialization
empty_sdf_data = create_empty_sdf_data() - m.shape_sdf_data = wp.array([empty_sdf_data] * len(self.shape_type), dtype=SDFData, device=device) - m.shape_sdf_volume = [None] * len(self.shape_type) - m.shape_sdf_coarse_volume = [None] * len(self.shape_type) - m.shape_sdf_block_coords = wp.array([], dtype=wp.vec3us) - m.shape_sdf_shape2blocks = wp.array([], dtype=wp.vec2i) + shape_count = len(self.shape_type) + m.shape_sdf_data = wp.array([empty_sdf_data] * shape_count, dtype=SDFData, device=device) + m.shape_sdf_volume = [None] * shape_count + m.shape_sdf_coarse_volume = [None] * shape_count + m.shape_sdf_block_coords = wp.array([], dtype=wp.vec3us) + m.shape_sdf_shape2blocks = wp.array([[0, 0]] * shape_count, dtype=wp.vec2i)This keeps the “no SDF tiles” semantics while guaranteeing that
shape_sdf_shape2blocks[shape_idx]is always defined. If you know all call sites already gate on SDF/hydro being enabled before indexing, this is purely a robustness improvement.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/sim/builder.py(11 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 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/sim/builder.py
📚 Learning: 2025-12-12T17:41:15.097Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/_src/sim/builder.py:5556-5587
Timestamp: 2025-12-12T17:41:15.097Z
Learning: newton._src.geometry.sdf_utils.compute_sdf accepts max_resolution=None (e.g., when sdf_target_voxel_size is provided), so passing None from ModelBuilder.finalize is valid and should not be flagged.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-12T17:45:26.847Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/geometry/primitives.py:0-0
Timestamp: 2025-12-12T17:45:26.847Z
Learning: In newton/_src/solvers/kamino/geometry/primitives.py, the narrow-phase kernel `_primitive_narrowphase` does not need to handle symmetric shape-pair orderings (e.g., both SPHERE-BOX and BOX-SPHERE) because `kamino.core.builder.ModelBuilder` and `kamino.geometry.primitive.broadphase` guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order.
Applied to files:
newton/_src/sim/builder.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/_src/sim/builder.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/sim/builder.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 the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-12T15:23:30.014Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_boxes_fourbar.py:149-163
Timestamp: 2025-12-12T15:23:30.014Z
Learning: In Kamino simulation examples under newton/_src/solvers/kamino/examples/sim/, the pattern where logging=True and use_cuda_graph=True creates a mismatch (SimulationLogger constructed but settings.compute_metrics and logger.log() calls guarded by `if not self.use_cuda_graph`) should not be flagged. These examples will be refactored in a future commit, and this pattern is intentionally deferred.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-12-13T17:26:39.791Z
Learnt from: lenroe-nv
Repo: newton-physics/newton PR: 1248
File: newton/_src/geometry/contact_data.py:47-49
Timestamp: 2025-12-13T17:26:39.791Z
Learning: In ContactData (newton/_src/geometry/contact_data.py), the fields contact_stiffness, contact_damping, and contact_friction_scale use 0.0 as the "unset" sentinel value. This is intentional: Warp struct fields initialize to 0.0 by default, and solver-side code interprets 0.0 as "use default/unscaled" rather than "disable." This design avoids explicit initialization in contact generation kernels.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/builder.py
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (3)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)newton/_src/geometry/types.py (1)
GeoType(25-67)newton/_src/geometry/sdf_utils.py (3)
SDFData(27-51)compute_sdf(239-445)create_empty_sdf_data(60-76)
🪛 Ruff (0.14.8)
newton/_src/sim/builder.py
215-218: Avoid specifying long messages outside the exception class
(TRY003)
225-227: Avoid specifying long messages outside the exception class
(TRY003)
5910-5910: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
5943-5947: Avoid specifying long messages outside the exception class
(TRY003)
5950-5954: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (6)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-24.04-arm)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (macos-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- 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 (5)
newton/_src/sim/builder.py (5)
197-227: Hydroelastic config and validation semantics look consistent overallThe added
is_hydroelastic,k_hydro, andvalidate()checks are coherent with the finalize-time SDF logic: hydroelastic shapes must request an SDF (viasdf_max_resolutionorsdf_target_voxel_size), and the divisibility-by‑8 constraint matches the SDF tiling assumptions. The units/usage notes onk_hydroalso align with how stiffness is consumed downstream.
245-253: Flags ↔ config coupling for HYDROELASTIC is correctly wiredIncluding
ShapeFlags.HYDROELASTICinflagsand syncing it via the setter ensures that round‑tripping through flags preservesis_hydroelastic, and that helpers likeapproximate_meshes(which copy flags) correctly propagate hydroelastic state. No issues here.Also applies to: 260-262
537-538: Per‑shapek_hydrostorage and builder merging are consistentInitializing
self.shape_material_k_hydroand threading it throughmore_builder_attrskeeps the per‑shape arrays aligned underadd_builder. Sinceadd_shapealways appendscfg.k_hydro, lengths will stay in sync withshape_type.Also applies to: 1815-1816
5900-5902: Model wiring forshape_material_k_hydrois correctExporting
self.shape_material_k_hydrotom.shape_material_k_hydrowithrequires_gradmatches how other per‑shape material arrays are handled, so solvers can consumek_hydroper contact.
5909-6033: SDF/hydro generation and caching path looks solidThe combined SDF/hydro path (
has_sdf_meshes or has_hydroelastic_shapes) with:
- GPU guard rails for SDF meshes and hydroelastic shapes,
needs_sdfcovering both mesh SDF requests and hydroelastic shapes,- cache key that only bakes
shape_scalein whenis_hydroelastic(so unbaked mesh SDFs can be shared across scaled instances),- and consistent reuse of
sdf_data_list[idx]/sdf_volumes[idx]/sdf_shape2blocks[idx]is logically consistent and should avoid redundant SDF builds while keeping hydro cases scale‑baked. I don’t see functional issues in this block.
448f424
Co-authored-by: twidmer <twidmer@nvidia.com> Co-authored-by: nvtw <110816143+nvtw@users.noreply.github.com>
Co-authored-by: twidmer <twidmer@nvidia.com> Co-authored-by: nvtw <110816143+nvtw@users.noreply.github.com>
Description
This PR adds SDF Hydroelastics, a new approach to collision detection and resolution based on grid-sampled SDFs. The implementation allows for fast computation of contact isosurfaces, which can be used for hydroelastic contact generation.
This PR also implements a discrete approximation of hydroelastic contact surfaces for efficient integration into existing solvers.
Collision Pipeline for SDF-SDF Collisions
Integration into Unified Collision Pipeline
SDFHydroelasticas an optional component ofCollisionPipelineUnifiedNarrowPhaseand passed toSDFHydroelasticfor processing.New Configuration Options
SDFHydroelasticConfigdataclass for specifying SDF collision optionsk_hydroparameter toShapeConfigto control area-dependent contact stiffnessis_hydroelasticflag toShapeConfigto enable SDF-based hydroelastic collisions on that shapePer-Contact Properties
rigid_contact_stiffness,rigid_contact_damping, andrigid_contact_frictionscaling to contact buffer (enabled when hydroelastics are active)Solver Integration
solrefparameters in contact conversion kernel (experimental)SDF Builder Extensions
Viewer Enhancements
Show CollisionViewer toggle now displays isomeshes of underlying SDFsshow_hydro_contact_surfaceoption in viewer for visualizing contact isosurface wireframesExample
Newton 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
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.