Add support for speculative contacts#2366
Draft
nvtw wants to merge 16 commits intonewton-physics:mainfrom
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces speculative contact detection to prevent object tunneling through thin geometry. It adds Changes
Sequence DiagramsequenceDiagram
participant User
participant CollisionPipeline
participant BroadPhase
participant ShapeVelCompute
participant NarrowPhase
participant Kernel as Kernel (Speculative)
User->>CollisionPipeline: collide(state, contacts, dt=...)
alt Speculative Config Enabled
CollisionPipeline->>ShapeVelCompute: Compute per-shape linear velocity<br/>and angular speed bounds
ShapeVelCompute-->>CollisionPipeline: shape_lin_vel, shape_ang_speed_bound
CollisionPipeline->>BroadPhase: Use speculative AABB kernel<br/>(expands bounds by velocity×dt)
BroadPhase-->>CollisionPipeline: Expanded AABB pairs
else Speculative Config Disabled
CollisionPipeline->>BroadPhase: Use standard AABB kernel
BroadPhase-->>CollisionPipeline: Standard AABB pairs
end
CollisionPipeline->>NarrowPhase: launch_custom_write(...,<br/>shape_lin_vel, speculative_dt,...)
alt Speculative in Narrow Phase
NarrowPhase->>Kernel: Process contacts with<br/>velocity-extended gap
Kernel->>Kernel: gap_sum += min(rel_speed × speculative_dt,<br/>max_speculative_extension)
Kernel->>Kernel: Evaluate contact acceptance<br/>using extended gap
Kernel-->>NarrowPhase: Accepted contacts
else Non-Speculative
NarrowPhase->>Kernel: Process contacts<br/>with standard gap
Kernel-->>NarrowPhase: Accepted contacts
end
NarrowPhase-->>CollisionPipeline: Contact manifolds
CollisionPipeline-->>User: Contacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
newton/tests/test_speculative_contacts.py (1)
138-165: Please add a linear GJK regression alongside the angular one.The suite covers linear motion only on sphere-based paths and covers boxes only with angular motion. A simple approaching/diverging box-box case would exercise the remaining non-primitive speculative acceptance branch and would catch regressions in the directed
write_contact()path much earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_speculative_contacts.py` around lines 138 - 165, Add a second test inside (or next to) test_speculative_angular_velocity that performs a linear GJK regression for box-box speculative contacts: reuse the newton.ModelBuilder setup (builder, builder.rigid_gap = 0.0), create two box bodies (similar to body_a and body_b) positioned apart on the x-axis, set their body_qd linear velocity components so they are approaching each other (e.g., nonzero vx on body_a or opposite vx on body_b) while keeping angular rates zero, finalize the model and state, create the same newton.SpeculativeContactConfig and newton.CollisionPipeline (broad_phase="nxn", speculative_config=config), call pipeline.collide(state, contacts) and assert contacts.rigid_contact_count.numpy()[0] > 0 to validate linear speculative acceptance; name the test to indicate linear GJK regression (e.g., test_speculative_linear_velocity) so it exercises the non-primitive speculative acceptance branch and directed write_contact path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/narrow_phase.py`:
- Around line 242-245: The current speculative widening uses raw relative speed
(vel_rel / |v_rel| via rel_speed) to increase gap_sum (using vel_rel, rel_speed,
gap_sum, speculative_dt, max_speculative_extension), which can accept
diverging/tangential pairs; change this so the rel_speed-based widening is used
only for candidate generation and not as the final acceptance metric: keep the
existing conservative addition to gap_sum for early pruning in the speculative
branch (both where vel_rel/rel_speed is used and in the duplicate primitive path
around the same logic), but ensure the final contact_passes_gap_check uses the
approach-speed projected onto the resolved contact normal (compute approach =
dot(vel_rel, contact_normal) + angular bounds) and use that projection for the
final gap acceptance instead of |v_rel|.
In `@newton/_src/sim/collide.py`:
- Around line 181-205: The speculative AABB kernel compute_shape_aabbs never
applies the configured max_speculative_extension, so add a kernel parameter
(e.g. max_speculative_extension: float) to compute_shape_aabbs (and the other
similar kernel variants referenced around the other occurrences) and clamp the
computed speculative expansion to that value before adding it to
aabb_lower/aabb_upper; locate where shape_lin_vel/shape_ang_speed_bound and
speculative_dt are used to compute the linear/angular expansions and insert a
min(extension, max_speculative_extension) (or equivalent) there so the
broad-phase expansion is bounded by the user-configured cap.
- Around line 31-47: Ensure speculative parameters are validated and reject
invalid values before any kernel/AABB math: add checks that
SpeculativeContactConfig.max_speculative_extension and
SpeculativeContactConfig.collision_update_dt are finite and non-negative (and >0
for dt) when creating/using the config, and in CollisionPipeline.collide(dt=...)
validate the per-call dt similarly; if any value is negative, zero, or
non-finite, raise a clear exception (or disable speculative extension
explicitly) so the invalid values never flow into AABB/gap computations or
kernel launches.
- Around line 123-136: The approach-speed projection is reversed: compute
approach speed so approaching motion yields a positive value and only then
extend the speculative gap. Change the v_approach calculation to use the negated
linear projection (e.g., use wp.dot(vel_a - vel_b, contact_normal_a_to_b) or
negate wp.dot(vel_b - vel_a, contact_normal_a_to_b)) while keeping the angular
bounds, then compute spec_gap = wp.min(wp.max(v_approach *
writer_data.speculative_dt, 0.0), writer_data.max_speculative_extension) and add
it to contact_gap; update the v_approach variable and usage accordingly
(symbols: writer_data.speculative_dt, vel_a, vel_b, contact_normal_a_to_b,
v_approach, spec_gap, writer_data.max_speculative_extension, contact_gap).
---
Nitpick comments:
In `@newton/tests/test_speculative_contacts.py`:
- Around line 138-165: Add a second test inside (or next to)
test_speculative_angular_velocity that performs a linear GJK regression for
box-box speculative contacts: reuse the newton.ModelBuilder setup (builder,
builder.rigid_gap = 0.0), create two box bodies (similar to body_a and body_b)
positioned apart on the x-axis, set their body_qd linear velocity components so
they are approaching each other (e.g., nonzero vx on body_a or opposite vx on
body_b) while keeping angular rates zero, finalize the model and state, create
the same newton.SpeculativeContactConfig and newton.CollisionPipeline
(broad_phase="nxn", speculative_config=config), call pipeline.collide(state,
contacts) and assert contacts.rigid_contact_count.numpy()[0] > 0 to validate
linear speculative acceptance; name the test to indicate linear GJK regression
(e.g., test_speculative_linear_velocity) so it exercises the non-primitive
speculative acceptance branch and directed write_contact path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 789f46d6-07bb-4f8e-a93c-f470a41c5965
📒 Files selected for processing (6)
newton/__init__.pynewton/_src/geometry/narrow_phase.pynewton/_src/sim/__init__.pynewton/_src/sim/collide.pynewton/_src/sim/model.pynewton/tests/test_speculative_contacts.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/collisions.rst`:
- Around line 1120-1122: The docs sentence claiming speculative widening uses a
"directed approach speed along the contact normal" is incorrect; update the
description to reflect that the narrow-phase widening uses the sum of relative
linear speed magnitude and angular-speed bounds (then clamped by
max_speculative_extension) rather than a normal-projected approach term —
mention the actual computation (relative linear speed magnitude + angular-speed
bounds, clamped by max_speculative_extension) and remove or correct the phrase
"directed approach speed along the contact normal" so readers get accurate
tuning guidance referencing max_speculative_extension and the angular-speed
bounds logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 065ae639-d5f5-4033-9bbc-7c6b670c599e
📒 Files selected for processing (3)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/sim/collide.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/sim/collide.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/_src/geometry/narrow_phase.py (1)
1768-1772: Consider caching the empty speculative arrays.These empty arrays are allocated on every
launch_custom_writecall. SinceNarrowPhasealready caches similar sentinel buffers (e.g.,_empty_edge_indices,_empty_edge_range), the speculative fallback arrays could be cached in__init__to avoid repeated allocations.♻️ Suggested caching in __init__
# In __init__, after allocating other sentinel buffers: + # Sentinel arrays for speculative fallback (when not provided) + self._empty_lin_vel = wp.empty(0, dtype=wp.vec3, device=device) + self._empty_ang_speed = wp.empty(0, dtype=wp.float32, device=device)# In launch_custom_write: - _empty_vec3 = wp.empty(0, dtype=wp.vec3, device=device) - _empty_float = wp.empty(0, dtype=wp.float32, device=device) - _slv = shape_lin_vel if shape_lin_vel is not None else _empty_vec3 - _sasb = shape_ang_speed_bound if shape_ang_speed_bound is not None else _empty_float + _slv = shape_lin_vel if shape_lin_vel is not None else self._empty_lin_vel + _sasb = shape_ang_speed_bound if shape_ang_speed_bound is not None else self._empty_ang_speed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/narrow_phase.py` around lines 1768 - 1772, Create and cache the empty speculative arrays in NarrowPhase.__init__ (e.g., self._empty_spec_vec3 = wp.empty(0, dtype=wp.vec3, device=self.device) and self._empty_spec_float = wp.empty(0, dtype=wp.float32, device=self.device) using the same device as the instance), then update launch_custom_write to use those cached sentinels instead of allocating new ones: replace local allocations of _empty_vec3/_empty_float with the cached attributes and keep the same fallback logic for _slv (shape_lin_vel) and _sasb (shape_ang_speed_bound). Ensure the cached arrays match dtype and device so existing code that references _slv/_sasb continues to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@newton/_src/geometry/narrow_phase.py`:
- Around line 1768-1772: Create and cache the empty speculative arrays in
NarrowPhase.__init__ (e.g., self._empty_spec_vec3 = wp.empty(0, dtype=wp.vec3,
device=self.device) and self._empty_spec_float = wp.empty(0, dtype=wp.float32,
device=self.device) using the same device as the instance), then update
launch_custom_write to use those cached sentinels instead of allocating new
ones: replace local allocations of _empty_vec3/_empty_float with the cached
attributes and keep the same fallback logic for _slv (shape_lin_vel) and _sasb
(shape_ang_speed_bound). Ensure the cached arrays match dtype and device so
existing code that references _slv/_sasb continues to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3b97f6cd-435e-430f-8a20-26d558595cfb
📒 Files selected for processing (4)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/narrow_phase.pynewton/_src/sim/collide.py
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- docs/concepts/collisions.rst
# Conflicts: # newton/_src/geometry/contact_reduction_global.py # newton/_src/geometry/narrow_phase.py # newton/_src/sim/collide.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
newton/_src/viewer/picking.py (1)
27-39:⚠️ Potential issue | 🟠 MajorReject negative or non-finite
pick_max_accelerationvalues.A negative value currently turns the clamp into a force reversal, because the kernel scales by
max_force / force_magandmax_forcebecomes negative. Validate this once in the constructor before writing it intoPickingState.Proposed fix
def __init__( self, model: newton.Model, pick_stiffness: float = 50.0, pick_damping: float = 5.0, pick_max_acceleration: float = 5.0, world_offsets: wp.array | None = None, ) -> None: @@ Args: model: The model to pick from. pick_stiffness: The stiffness that will be used to compute the force applied to the picked body. pick_damping: The damping that will be used to compute the force applied to the picked body. pick_max_acceleration: Maximum picking acceleration in multiples of g [9.81 m/s^2]. Clamps the picking force to prevent runaway divergence on light objects near stiff contacts. world_offsets: Optional warp array of world offsets (dtype=wp.vec3) for multi-world picking support. """ + if not np.isfinite(pick_max_acceleration) or pick_max_acceleration < 0.0: + raise ValueError("pick_max_acceleration must be a finite, non-negative value") + self.model = model self.pick_stiffness = pick_stiffness self.pick_damping = pick_damping self.world_offsets = world_offsetsAlso applies to: 65-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/viewer/picking.py` around lines 27 - 39, The constructor currently allows negative or non-finite pick_max_acceleration which can invert clamping logic; validate pick_max_acceleration in the initializer before assigning into PickingState (e.g., in the function/method that constructs PickingState or the class __init__) and raise an error (ValueError) for negative or non-finite values (NaN/inf), or clamp to a sensible minimum (>0) per project conventions; ensure the validated value is what gets stored in PickingState and update any related uses that assume it is positive.newton/_src/utils/import_usd.py (1)
1856-1887:⚠️ Potential issue | 🟠 MajorOnly merge revolute/prismatic siblings here.
parse_merged_joints()only supportsRevoluteJointandPrismaticJoint, but this grouping now buckets any non-excluded joint that shares a body pair. If a fixed/spherical/D6/distance joint appears alongside a mergeable sibling, the importer now fails withValueErrorinstead of parsing both joints normally.Proposed fix
- body_pair = (parent_id, child_id) - if body_pair in body_pair_to_representative: - # Another joint between the same bodies — merge into existing group - rep = body_pair_to_representative[body_pair] - merged_joint_groups[rep].append(joint_path) - else: - # First joint for this body pair - body_pair_to_representative[body_pair] = joint_path - merged_joint_groups[joint_path] = [joint_path] - joint_edges.append(body_pair) - joint_names.append(joint_path) + mergeable_types = { + UsdPhysics.ObjectType.RevoluteJoint, + UsdPhysics.ObjectType.PrismaticJoint, + } + body_pair = (parent_id, child_id) + rep = body_pair_to_representative.get(body_pair) if joint_desc.type in mergeable_types else None + if rep is not None: + # Another mergeable joint between the same bodies — fold into existing D6 group + merged_joint_groups[rep].append(joint_path) + else: + # Keep non-mergeable joint types independent + if joint_desc.type in mergeable_types: + body_pair_to_representative[body_pair] = joint_path + merged_joint_groups[joint_path] = [joint_path] + joint_edges.append(body_pair) + joint_names.append(joint_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/utils/import_usd.py` around lines 1856 - 1887, The grouping currently merges any non-excluded joint that shares a body pair which breaks parse_merged_joints() (it only supports RevoluteJoint and PrismaticJoint); update the loop that builds merged_joint_groups/body_pair_to_representative so you only merge siblings when both the existing representative and the current joint are revolute or prismatic. Concretely, when you encounter a body_pair already in body_pair_to_representative, check the types of joint_desc and joint_descriptions[rep] (use isinstance(..., (RevoluteJoint, PrismaticJoint)) or compare their type names) and only append to merged_joint_groups[rep] if both are mergeable; if not mergeable, treat the current joint as its own representative (i.e., create a new merged_joint_groups[joint_path] = [joint_path], add joint_edges/joint_names) instead of merging into the existing group. Ensure parse_merged_joints is left unchanged and referenced symbols are merged_joint_groups, body_pair_to_representative, parse_merged_joints, joint_desc, and joint_descriptions.newton/_src/geometry/narrow_phase.py (1)
1232-1256:⚠️ Potential issue | 🔴 CriticalProject speculative widening onto the plane normal here too.
These branches widen
gap_sumwith|v_rel|, then use that widened threshold for the final plane-distance test. Fast tangential motion, or a plane moving away, will still generate speculative contacts even though the pair is not approaching alongplane_normal.🐛 Proposed fix
- rel_speed = ( - wp.length(vel_rel) + shape_ang_speed_bound[mesh_shape] + shape_ang_speed_bound[plane_shape] - ) - gap_sum = gap_sum + wp.min(rel_speed * speculative_dt, max_speculative_extension) + v_approach = ( + -wp.dot(vel_rel, plane_normal) + + shape_ang_speed_bound[mesh_shape] + + shape_ang_speed_bound[plane_shape] + ) + gap_sum = gap_sum + wp.min( + wp.max(v_approach * speculative_dt, 0.0), + max_speculative_extension, + )Also applies to: 1375-1404
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/narrow_phase.py` around lines 1232 - 1256, The speculative widening currently adds a scalar rel_speed (from vel_rel magnitude and angular bounds) to gap_sum, which can cause false positives from tangential motion; instead compute the relative motion projected onto plane_normal and only widen by the approach component: compute vel_rel = shape_lin_vel[plane_shape] - shape_lin_vel[mesh_shape], compute rel_normal = wp.dot(vel_rel, plane_normal), clamp to positive approach (rel_normal = max(0.0, -rel_normal) if plane_normal points from plane->mesh or use sign consistent with distance sign), add angular contributions projected onto the normal as appropriate, then use rel_normal * speculative_dt (clamped by max_speculative_extension) to increase gap_sum; apply the same projection change in the other occurrence noted (around lines 1375-1404) referencing gap_sum, rel_speed, speculative_dt, max_speculative_extension, plane_normal, vel_rel, and shape_ang_speed_bound to locate and update the logic.newton/tests/test_contact_reduction_global.py (1)
561-569:⚠️ Potential issue | 🟠 MajorInitialize
contact_sort_keyin this test writer data.
ContactWriterDatanow has a requiredcontact_sort_keyarray. This test still leaves that field unset, so the export kernel is marshaling an incomplete Warp struct.💚 Proposed fix
writer_data.contact_penetration = contact_penetration_out writer_data.contact_tangent = contact_tangent_out + writer_data.contact_sort_key = wp.zeros(0, dtype=wp.int64, device=device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_contact_reduction_global.py` around lines 561 - 569, The test fails to populate the new required ContactWriterData.contact_sort_key field, causing the export kernel to marshal an incomplete Warp struct; fix it by creating and assigning a contact_sort_key array matching contact_count_out (e.g., contact_sort_key_out = np.zeros(contact_count_out, dtype=np.int32) or contact_sort_key_out = np.arange(contact_count_out, dtype=np.int32) if order matters) and then set writer_data.contact_sort_key = contact_sort_key_out alongside the other writer_data fields (refer to ContactWriterData and writer_data in the test).newton/_src/solvers/kamino/examples/rl/simulation.py (1)
435-459:⚠️ Potential issue | 🟠 Major
joint_qreset staging is still sized in DOF space.
self._q_janddefault_q_jnow exposenjc, but_reset_q_jis still allocated as(nw, njd)and later fed intojoint_q=during reset. For BALL/FREE joints that truncates coordinate data and makes the newq_jshape unusable for reset paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/kamino/examples/rl/simulation.py` around lines 435 - 459, Reset buffer _reset_q_j is incorrectly sized to DOF count (njd) while joint-space q_j uses coordinate count (njc); allocate and reshape the reset buffer to (nw, njc) so resets (and any use of default_q_j) supply full generalized-coordinate data to joint_q= paths. Update the initialization of _reset_q_j to use njc (matching self._q_j and default_q_j) and keep _reset_dq_j sized to njd; ensure any code that writes _reset_q_j or passes it into joint_q= uses the new (nw, njc) shape.
♻️ Duplicate comments (1)
docs/concepts/collisions.rst (1)
1127-1135:⚠️ Potential issue | 🟠 MajorSpeculative-contact narrow-phase docs still describe the wrong velocity term.
This still says the narrow phase uses a normal-projected relative linear velocity term. The implementation uses relative linear speed magnitude plus angular-speed bounds, then clamps by
max_speculative_extension, so the current wording will mislead users tuningSpeculativeContactConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/concepts/collisions.rst` around lines 1127 - 1135, Update the docs to correct the description of the narrow-phase speculative contact calculation: replace the current phrase that says it uses the "normal-projected relative linear velocity" with an accurate statement that the narrow phase recomputes the contact gap using the magnitude of the relative linear velocity plus per-shape angular-speed bounds (each clamped by SpeculativeContactConfig.max_speculative_extension), matching how State.body_qd is used to compute per-shape linear velocities and angular-speed bounds; ensure the wording clearly notes the sum is clamped by max_speculative_extension before acceptance.
🧹 Nitpick comments (7)
newton/examples/robot/example_robot_panda_hydro.py (1)
434-434: Prefer a coarse placement assertion over removing placement validation entirely.Removing the in-cup check makes
Example.test_final()validate only pickup, not the place phase. Consider adding a relaxed, non-flaky final-position check (e.g., broad XY radius near cup center) so placement regressions are still caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/robot/example_robot_panda_hydro.py` at line 434, The in-cup placement check was removed, leaving Example.test_final() only verifying pickup; restore a relaxed placement assertion by adding a coarse final-position check after the place phase: compute the cup center (e.g., cup_center or cup_pose) and the object's final_pose returned/observed by Example.test_final(), then assert the XY distance from final_pose to cup_center is within a broad radius (e.g., a few centimeters) and optionally allow a larger Z tolerance; implement this as a non-flaky, conservative check (use names like Example.test_final(), cup_center/cup_pose, final_pose) so placement regressions are caught without reintroducing flaky strict in-cup logic.newton/_src/viewer/viewer_gl.py (1)
875-907: Factor shared line/arrow batching logic into one helper
log_lines()andlog_arrows()now duplicate color normalization, capacity growth, and update/hide behavior. A shared helper would reduce drift and future bug risk.Also applies to: 939-966
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/viewer/viewer_gl.py` around lines 875 - 907, log_lines() and log_arrows() duplicate color normalization, capacity growth and update/hide logic; extract the shared behavior into a helper (e.g., _ensure_and_update_lines) that accepts starts, ends, colors, name, hidden and handles: validating types and lengths, converting tuple/list or flat float arrays to a wp.array of wp.vec3, handling zero-length cases, creating or resizing the LinesGL instance (using LinesGL(max_lines, self.device, hidden)) with the same growth policy, calling .update(...) and setting .hidden. Replace the duplicated blocks in both log_lines and log_arrows to call this new helper so both use the identical normalization and capacity logic.newton/_src/solvers/kamino/_src/core/shapes.py (2)
53-68: Consider emitting runtime deprecation warnings.The docstrings mark these functions as deprecated, but no
warnings.warn()is emitted at runtime. This makes the deprecation less discoverable for users who don't read docstrings.🔔 Add runtime deprecation warnings
+import warnings + def is_primitive_geo_type(geo_type: GeoType) -> bool: """Return whether the geo type is a primitive shape. .. deprecated:: Use :attr:`GeoType.is_primitive` instead. """ + warnings.warn( + "is_primitive_geo_type() is deprecated, use GeoType.is_primitive instead", + DeprecationWarning, + stacklevel=2, + ) return geo_type.is_primitive def is_explicit_geo_type(geo_type: GeoType) -> bool: """Return whether the geo type is an explicit shape (mesh, convex, heightfield). .. deprecated:: Use :attr:`GeoType.is_explicit` instead. """ + warnings.warn( + "is_explicit_geo_type() is deprecated, use GeoType.is_explicit instead", + DeprecationWarning, + stacklevel=2, + ) return geo_type.is_explicit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/kamino/_src/core/shapes.py` around lines 53 - 68, The two deprecated helpers is_primitive_geo_type and is_explicit_geo_type should emit runtime deprecation warnings; import the warnings module and call warnings.warn with a clear deprecation message naming the function and the preferred replacement (GeoType.is_primitive / GeoType.is_explicit), use category=DeprecationWarning and stacklevel=2 so callers see the warning, then return the existing geo_type.is_primitive / geo_type.is_explicit value unchanged.
544-580: Consider overriding__hash__to include heightfield identity.
HFieldShapeinherits__hash__fromShapeDescriptor, which hashes(type, params). Sinceparamsis always(1.0, 1.0, 1.0)for all heightfields, different heightfield shapes will produce the same hash. This could cause issues if heightfields are used as dictionary keys or in sets.🔧 Override __hash__ to include heightfield identity
+ `@override` + def __hash__(self) -> int: + """Returns a hash computed using the underlying Heightfield's identity.""" + return hash((self.type, id(self._data))) + `@override` def __repr__(self): return f"HFieldShape(\nname: {self.name},\nuid: {self.uid}\n)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/kamino/_src/core/shapes.py` around lines 544 - 580, HFieldShape currently inherits ShapeDescriptor's __hash__ which only hashes (type, params) making all heightfields collide; add an override of __hash__ on HFieldShape that incorporates the heightfield identity (e.g., use id(self._data) or hash(self._data) combined with the existing type/params) so each distinct Heightfield instance produces a distinct hash; implement the __hash__ method on the HFieldShape class (referencing HFieldShape, __hash__, and self._data or self.data) and ensure it remains consistent with __eq__ semantics.newton/tests/test_import_usd_multi_dof.py (1)
53-63: Docstring slightly overstates test scope.The docstring mentions "simulate without errors" but the test only verifies
finalize()succeeds. Consider updating the docstring to accurately reflect what's tested.📝 Suggested docstring fix
`@unittest.skipUnless`(USD_AVAILABLE, "Requires usd-core") def test_humanoid_mjc_finalize(self): - """The imported humanoid should finalize and simulate without errors.""" + """The imported humanoid should finalize without errors.""" builder = newton.ModelBuilder()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_import_usd_multi_dof.py` around lines 53 - 63, The test docstring for test_humanoid_mjc_finalize overstates its scope; update it to accurately state that the test only verifies builder.finalize() succeeds and the resulting model has the expected body and joint counts (e.g., "The imported humanoid should finalize successfully and have the expected body and joint counts."). Edit the docstring above the test_humanoid_mjc_finalize definition to reflect this, referencing the finalize() call and the assertions for model.body_count and model.joint_count.newton/_src/solvers/kamino/_src/geometry/unified.py (1)
524-530: Consider vectorized numpy operations for shape type detection.The current implementation iterates element-by-element. For large models, numpy's vectorized operations would be more efficient.
♻️ Vectorized alternative
# Detect whether the model contains mesh, convex mesh, or heightfield shapes. # Keep mesh and heightfield flags separate: heightfield-only scenes should not # trigger mesh-only kernel setup (mesh-mesh SDF contacts require CUDA). geom_type_np = self._model.geoms.type.numpy() - _has_meshes = any(int(t) in (GeoType.MESH, GeoType.CONVEX_MESH) for t in geom_type_np) - _has_heightfields = any(int(t) == GeoType.HFIELD for t in geom_type_np) + _has_meshes = bool(np.isin(geom_type_np, [GeoType.MESH, GeoType.CONVEX_MESH]).any()) + _has_heightfields = bool((geom_type_np == GeoType.HFIELD).any()) _has_explicit = _has_meshes or _has_heightfields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/kamino/_src/geometry/unified.py` around lines 524 - 530, The element-wise generator expressions for detecting mesh/heightfield types are inefficient; replace them with vectorized numpy comparisons on geom_type_np (obtained from self._model.geoms.type.numpy()) by comparing the whole array against GeoType values and using numpy.any to set _has_meshes and _has_heightfields (e.g., check equality to GeoType.HFIELD and membership for (GeoType.MESH, GeoType.CONVEX_MESH) via vectorized boolean ops), then compute _has_explicit as before from those booleans.newton/_src/solvers/kamino/_src/core/geometry.py (1)
347-357: Add SI units to docstrings for physical quantities.Per coding guidelines, public API docstrings for physical quantities should include SI units. The
collision_aabb_lower,collision_aabb_upper, andcollision_radiusfields represent spatial measurements.📝 Suggested docstring improvements
collision_aabb_lower: wp.array | None = None - """Per-shape local-space collision AABB lower bounds.""" + """Per-shape local-space collision AABB lower bounds [m].""" collision_aabb_upper: wp.array | None = None - """Per-shape local-space collision AABB upper bounds.""" + """Per-shape local-space collision AABB upper bounds [m].""" voxel_resolution: wp.array | None = None """Per-shape voxel resolution for mesh contact reduction.""" collision_radius: wp.array | None = None - """Per-shape bounding-sphere radius for broadphase AABB computation.""" + """Per-shape bounding-sphere radius for broadphase AABB computation [m]."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/kamino/_src/core/geometry.py` around lines 347 - 357, Update the docstrings for the public fields collision_aabb_lower, collision_aabb_upper, and collision_radius to include SI units: specify that collision_aabb_lower and collision_aabb_upper are per-shape local-space AABB bounds given in meters (m) per component (e.g., [x, y, z] in m) and that collision_radius is the per-shape bounding-sphere radius in meters (m); leave voxel_resolution unchanged. Locate these symbols in geometry.py and append the unit information to their existing docstring lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 98-106: The changelog claims immediate removal of old API
values/names ("instantaneous"/"finite_difference" for collider_velocity_mode and
the collide_plane_cylinder() parameter cylinder_center), but per guidelines we
must instead provide a deprecation shim: update the MPM collider code paths that
parse collider_velocity_mode to continue accepting the old strings
("instantaneous" and "finite_difference"), internally map them to the new modes
("forward"/"backward"), and emit a DeprecationWarning when the old values are
used; likewise, modify collide_plane_cylinder() to accept both cylinder_center
and the new cylinder_pos parameter (map cylinder_center -> cylinder_pos if
present) and emit a DeprecationWarning for the old name, and then change the
CHANGELOG entries to state these are deprecated (with warnings) rather than
removed pending a future breaking release.
In `@newton/_src/geometry/contact_sort.py`:
- Around line 259-278: Add record_tape=False to every wp.launch call inside
ContactSorter so these non-differentiable sorter kernels are not recorded by an
active wp.Tape; specifically update the launches of _backup_simple_kernel and
_gather_simple_kernel (and the other sorter kernel launches referenced in this
file) to pass record_tape=False as an argument to wp.launch (e.g.,
wp.launch(_backup_simple_kernel, ..., device=device, record_tape=False)).
In `@newton/_src/geometry/narrow_phase.py`:
- Around line 1846-1849: The public NarrowPhase.launch(...) signature currently
doesn't accept the new speculative inputs (shape_lin_vel, shape_ang_speed_bound,
speculative_dt, max_speculative_extension) even though launch_custom_write(...)
supports them; update the NarrowPhase.launch method to add those parameters with
the same defaults and types and forward them through to the existing call to
launch_custom_write(...) (preserving existing behavior when None/0.0) so callers
of NarrowPhase.launch can opt into per-shape velocities and speculative tuning;
reference the launch and launch_custom_write functions and the parameter names
shape_lin_vel, shape_ang_speed_bound, speculative_dt, max_speculative_extension
when making the change.
- Around line 153-155: The module name for Warp kernels currently ignores the
speculative flag causing compiled-module cache collisions; update the module key
construction (the line setting _module =
f"narrow_phase_primitive_{writer_func.__name__}") to include the speculative
mode (e.g., f"narrow_phase_primitive_{writer_func.__name__}_{speculative}")
before the `@wp.kernel` decorator so each speculative/non-speculative kernel uses
a distinct module key; apply the same change to the other identical module
declarations in this file (the other _module assignments used to decorate the
corresponding `@wp.kernel` functions).
In `@newton/_src/geometry/sdf_contact.py`:
- Around line 40-45: The code flips the face normal using the local `sign` after
calling wp.mesh_eval_face_normal, which already returns outward-pointing
normals; remove the conditional negation (the `if sign < 0.0: n = -n` branch) so
that the function returns the face normal `n` directly after
wp.mesh_eval_face_normal(mesh_id, face_index); keep the evaluation and return of
`n` as-is in the block that follows wp.mesh_query_point_sign_normal.
In `@newton/_src/sim/collide.py`:
- Around line 869-876: The deterministic branch currently enables deterministic
mode even when hydroelastic contacts exist, but hydro contacts share identical
sort keys causing nondeterminism; update the initialization in the block that
sets self.deterministic to reject/disable deterministic mode when
self.narrow_phase.hydroelastic_sdf is not None (i.e., if per_contact_props is
True) by either raising an error or forcing deterministic=False and logging, and
only allocate self._sort_key_array and create ContactSorter (rigid_contact_max,
per_contact_shape_properties=per_contact_props, device=device) when
hydroelastic_sdf is absent so hydroelastic scenes do not enable deterministic
sorting.
In `@newton/_src/sim/contacts.py`:
- Around line 9-20: The sentinel constant GENERATION_SENTINEL (-1) and the
kernel _increment_contact_generation are inconsistent: update the kernel to
honor the sentinel instead of incrementing it; inside
_increment_contact_generation check if generation[0] == GENERATION_SENTINEL and
leave it unchanged, else perform the existing wrap logic for 2147483647 -> 0 and
normal increment; reference the generation array parameter (generation:
wp.array[wp.int32]) and the GENERATION_SENTINEL and
_increment_contact_generation symbols so callers relying on the sentinel
behavior are correct.
In `@newton/_src/solvers/kamino/examples/rl/example_rl_bipedal.py`:
- Around line 159-166: BALL_POSITIONS is using hard-coded z values assuming flat
ground which causes interpenetration with the heightfield; instead sample the
terrain height at each (x,y) and set z = terrain_height + BALL_RADIUS + 0.01.
Update the code that constructs BALL_POSITIONS (and the similar block at lines
169-183) to call the terrain/heightfield sampling function used elsewhere in
this module (e.g., the heightfield or terrain height query helper) for each
position and compute z from that sample plus BALL_RADIUS + 0.01 so balls spawn
exactly above the terrain.
In `@newton/_src/solvers/kamino/examples/rl/simulation.py`:
- Around line 142-149: _run_newton_collision currently only syncs body_q and
calls self._newton_model.collide with stale motion/timestep; set the Newton
motion state and pass the current timestep before colliding: copy the
motion/velocity from StateKamino (e.g. state_kamino.motion or velocities) into
self._newton_state (the same place convert_body_com_to_origin writes q), then
call collide with the timestep (e.g.
self._newton_model.collide(self._newton_state, self._newton_contacts,
dt=CONFIG_DT) or by setting self._newton_state.dt) so speculative contacts use
the current motion and config.dt rather than the model default. Ensure you use
the correct dt source (state_kamino or the instance config) and the exact
collide signature in the code.
- Around line 249-250: The parameters terrain_fn and scene_callback are
annotated with the builtin predicate "callable" instead of a typing form; update
their annotations to use typing.Callable with the correct signature (they accept
a single newton.ModelBuilder and return None), e.g.
Callable[[newton.ModelBuilder], None] | None, and add "from typing import
Callable" (or include Callable in existing typing imports) so type checkers and
docs correctly recognize these callbacks; update the annotation occurrences for
terrain_fn and scene_callback accordingly.
In `@newton/_src/utils/import_usd.py`:
- Around line 1230-1241: The code currently picks the first joint
(first_desc/first_prim) to represent a merged D6’s metadata, which allows mixing
enabled and disabled siblings when only_load_enabled_joints is False; instead,
before synthesizing a single D6, iterate joint_paths and inspect
joint_descriptions[...] .jointEnabled for all candidates and if you detect a mix
of enabled and disabled states (both True and False), reject the merge (e.g.,
return None or raise) so you don't silently inherit a single enabled flag; apply
the same mixed-state check where similar merging logic occurs (also referenced
by the other merge location using joint_paths/joint_descriptions at the later
block around the 1484-1486 region).
In `@newton/_src/viewer/gl/shaders.py`:
- Around line 742-748: Guard against degenerate segments before calling
normalize: compute dir_ndc = e_ndc - s_ndc and dir_scr = vec2(dir_ndc.x /
safe_asp, dir_ndc.y) as before, then compute float dlen = length(dir_scr) and if
dlen > 1e-6 set right_scr = normalize(dir_scr.yx * vec2(1.0, -1.0)) (or
normalize(vec2(dir_scr.y, -dir_scr.x))); otherwise set right_scr to a safe
fallback (e.g. vec2(0.0, 1.0)) to avoid normalize(vec2(0.0)) producing NaNs;
finally compute right = vec2(right_scr.x * safe_asp, right_scr.y). Apply this
change around the existing safe_asp/dir_scr/right_scr/right code in
log_lines()/the shader fragment.
In `@newton/_src/viewer/viewer_gl.py`:
- Around line 591-592: The code currently pops GL objects with
self.objects.pop(k) and then simply del obj, which only drops the Python
reference and can leak VRAM; change each occurrence (where you do obj =
self.objects.pop(k); del obj) to first call the object's GL teardown API (e.g.,
obj.destroy() or obj.delete() or obj.release_gl() — use the actual method
implemented on the GL object class) inside a try/except to swallow/log any
teardown errors, then remove it from self.objects; ensure you call the object's
explicit cleanup before removing the reference so GL buffers are freed promptly.
In `@newton/_src/viewer/viewer_viser.py`:
- Around line 686-719: The current try/except around the uplot update swallows
exceptions and always calls self._scalar_dirty.clear(), losing pending updates;
modify the logic in the viewer_viser.py update block so that the try/except only
wraps the uplot/gui operations (imports, folder creation, add_uplot/setting
handle.data) and only clear self._scalar_dirty if the plot update succeeded
(i.e., move self._scalar_dirty.clear() into the success path or set a success
flag inside the loop/try and clear when success is True); preserve references to
self._plot_handles, self._plot_folder, and the existing loop over
self._scalar_dirty so failures leave dirty signals intact for retry.
In `@newton/tests/test_menagerie_usd_mujoco.py`:
- Around line 1282-1284: TestMenagerieUSD re-enablement of backfill_model for H1
and UR5e is unsafe because backfill_model_from_native() blindly copies
MODEL_BACKFILL_FIELDS by array positions (e.g., body_pos, body_quat,
body_inertia) and will misassign bodies if USD→MJCF body ordering differs;
either (A) revert backfill_model to False for H1 and UR5e until you add
verification, or (B) modify backfill_model_from_native() to accept and apply an
explicit index map when copying MODEL_BACKFILL_FIELDS (add parameters and use
the map to remap body-indexed arrays), and add a verification routine (or
assert) in the H1/UR5e test classes that confirms USD and native body ordering
before allowing backfill_model=True.
In `@newton/tests/test_mujoco_solver.py`:
- Around line 4012-4015: The test currently wraps SolverMuJoCo(model,
use_mujoco_contacts=False, njmax=200, nconmax=200) in a try/except that calls
self.skipTest on ImportError, which can hide regressions; remove the try/except
and construct SolverMuJoCo directly (no self.skipTest) so ImportError (or other
exceptions) fail the test immediately; locate the construction call to
SolverMuJoCo in the test_mujoco_solver test and delete the surrounding
try/except block.
---
Outside diff comments:
In `@newton/_src/geometry/narrow_phase.py`:
- Around line 1232-1256: The speculative widening currently adds a scalar
rel_speed (from vel_rel magnitude and angular bounds) to gap_sum, which can
cause false positives from tangential motion; instead compute the relative
motion projected onto plane_normal and only widen by the approach component:
compute vel_rel = shape_lin_vel[plane_shape] - shape_lin_vel[mesh_shape],
compute rel_normal = wp.dot(vel_rel, plane_normal), clamp to positive approach
(rel_normal = max(0.0, -rel_normal) if plane_normal points from plane->mesh or
use sign consistent with distance sign), add angular contributions projected
onto the normal as appropriate, then use rel_normal * speculative_dt (clamped by
max_speculative_extension) to increase gap_sum; apply the same projection change
in the other occurrence noted (around lines 1375-1404) referencing gap_sum,
rel_speed, speculative_dt, max_speculative_extension, plane_normal, vel_rel, and
shape_ang_speed_bound to locate and update the logic.
In `@newton/_src/solvers/kamino/examples/rl/simulation.py`:
- Around line 435-459: Reset buffer _reset_q_j is incorrectly sized to DOF count
(njd) while joint-space q_j uses coordinate count (njc); allocate and reshape
the reset buffer to (nw, njc) so resets (and any use of default_q_j) supply full
generalized-coordinate data to joint_q= paths. Update the initialization of
_reset_q_j to use njc (matching self._q_j and default_q_j) and keep _reset_dq_j
sized to njd; ensure any code that writes _reset_q_j or passes it into joint_q=
uses the new (nw, njc) shape.
In `@newton/_src/utils/import_usd.py`:
- Around line 1856-1887: The grouping currently merges any non-excluded joint
that shares a body pair which breaks parse_merged_joints() (it only supports
RevoluteJoint and PrismaticJoint); update the loop that builds
merged_joint_groups/body_pair_to_representative so you only merge siblings when
both the existing representative and the current joint are revolute or
prismatic. Concretely, when you encounter a body_pair already in
body_pair_to_representative, check the types of joint_desc and
joint_descriptions[rep] (use isinstance(..., (RevoluteJoint, PrismaticJoint)) or
compare their type names) and only append to merged_joint_groups[rep] if both
are mergeable; if not mergeable, treat the current joint as its own
representative (i.e., create a new merged_joint_groups[joint_path] =
[joint_path], add joint_edges/joint_names) instead of merging into the existing
group. Ensure parse_merged_joints is left unchanged and referenced symbols are
merged_joint_groups, body_pair_to_representative, parse_merged_joints,
joint_desc, and joint_descriptions.
In `@newton/_src/viewer/picking.py`:
- Around line 27-39: The constructor currently allows negative or non-finite
pick_max_acceleration which can invert clamping logic; validate
pick_max_acceleration in the initializer before assigning into PickingState
(e.g., in the function/method that constructs PickingState or the class
__init__) and raise an error (ValueError) for negative or non-finite values
(NaN/inf), or clamp to a sensible minimum (>0) per project conventions; ensure
the validated value is what gets stored in PickingState and update any related
uses that assume it is positive.
In `@newton/tests/test_contact_reduction_global.py`:
- Around line 561-569: The test fails to populate the new required
ContactWriterData.contact_sort_key field, causing the export kernel to marshal
an incomplete Warp struct; fix it by creating and assigning a contact_sort_key
array matching contact_count_out (e.g., contact_sort_key_out =
np.zeros(contact_count_out, dtype=np.int32) or contact_sort_key_out =
np.arange(contact_count_out, dtype=np.int32) if order matters) and then set
writer_data.contact_sort_key = contact_sort_key_out alongside the other
writer_data fields (refer to ContactWriterData and writer_data in the test).
---
Duplicate comments:
In `@docs/concepts/collisions.rst`:
- Around line 1127-1135: Update the docs to correct the description of the
narrow-phase speculative contact calculation: replace the current phrase that
says it uses the "normal-projected relative linear velocity" with an accurate
statement that the narrow phase recomputes the contact gap using the magnitude
of the relative linear velocity plus per-shape angular-speed bounds (each
clamped by SpeculativeContactConfig.max_speculative_extension), matching how
State.body_qd is used to compute per-shape linear velocities and angular-speed
bounds; ensure the wording clearly notes the sum is clamped by
max_speculative_extension before acceptance.
---
Nitpick comments:
In `@newton/_src/solvers/kamino/_src/core/geometry.py`:
- Around line 347-357: Update the docstrings for the public fields
collision_aabb_lower, collision_aabb_upper, and collision_radius to include SI
units: specify that collision_aabb_lower and collision_aabb_upper are per-shape
local-space AABB bounds given in meters (m) per component (e.g., [x, y, z] in m)
and that collision_radius is the per-shape bounding-sphere radius in meters (m);
leave voxel_resolution unchanged. Locate these symbols in geometry.py and append
the unit information to their existing docstring lines.
In `@newton/_src/solvers/kamino/_src/core/shapes.py`:
- Around line 53-68: The two deprecated helpers is_primitive_geo_type and
is_explicit_geo_type should emit runtime deprecation warnings; import the
warnings module and call warnings.warn with a clear deprecation message naming
the function and the preferred replacement (GeoType.is_primitive /
GeoType.is_explicit), use category=DeprecationWarning and stacklevel=2 so
callers see the warning, then return the existing geo_type.is_primitive /
geo_type.is_explicit value unchanged.
- Around line 544-580: HFieldShape currently inherits ShapeDescriptor's __hash__
which only hashes (type, params) making all heightfields collide; add an
override of __hash__ on HFieldShape that incorporates the heightfield identity
(e.g., use id(self._data) or hash(self._data) combined with the existing
type/params) so each distinct Heightfield instance produces a distinct hash;
implement the __hash__ method on the HFieldShape class (referencing HFieldShape,
__hash__, and self._data or self.data) and ensure it remains consistent with
__eq__ semantics.
In `@newton/_src/solvers/kamino/_src/geometry/unified.py`:
- Around line 524-530: The element-wise generator expressions for detecting
mesh/heightfield types are inefficient; replace them with vectorized numpy
comparisons on geom_type_np (obtained from self._model.geoms.type.numpy()) by
comparing the whole array against GeoType values and using numpy.any to set
_has_meshes and _has_heightfields (e.g., check equality to GeoType.HFIELD and
membership for (GeoType.MESH, GeoType.CONVEX_MESH) via vectorized boolean ops),
then compute _has_explicit as before from those booleans.
In `@newton/_src/viewer/viewer_gl.py`:
- Around line 875-907: log_lines() and log_arrows() duplicate color
normalization, capacity growth and update/hide logic; extract the shared
behavior into a helper (e.g., _ensure_and_update_lines) that accepts starts,
ends, colors, name, hidden and handles: validating types and lengths, converting
tuple/list or flat float arrays to a wp.array of wp.vec3, handling zero-length
cases, creating or resizing the LinesGL instance (using LinesGL(max_lines,
self.device, hidden)) with the same growth policy, calling .update(...) and
setting .hidden. Replace the duplicated blocks in both log_lines and log_arrows
to call this new helper so both use the identical normalization and capacity
logic.
In `@newton/examples/robot/example_robot_panda_hydro.py`:
- Line 434: The in-cup placement check was removed, leaving Example.test_final()
only verifying pickup; restore a relaxed placement assertion by adding a coarse
final-position check after the place phase: compute the cup center (e.g.,
cup_center or cup_pose) and the object's final_pose returned/observed by
Example.test_final(), then assert the XY distance from final_pose to cup_center
is within a broad radius (e.g., a few centimeters) and optionally allow a larger
Z tolerance; implement this as a non-flaky, conservative check (use names like
Example.test_final(), cup_center/cup_pose, final_pose) so placement regressions
are caught without reintroducing flaky strict in-cup logic.
In `@newton/tests/test_import_usd_multi_dof.py`:
- Around line 53-63: The test docstring for test_humanoid_mjc_finalize
overstates its scope; update it to accurately state that the test only verifies
builder.finalize() succeeds and the resulting model has the expected body and
joint counts (e.g., "The imported humanoid should finalize successfully and have
the expected body and joint counts."). Edit the docstring above the
test_humanoid_mjc_finalize definition to reflect this, referencing the
finalize() call and the assertions for model.body_count and model.joint_count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e41f1bf0-b8d7-49f0-865d-1f2ad81c5594
⛔ Files ignored due to path filters (2)
newton/_src/viewer/gl/newton_envmap.jpgis excluded by!**/*.jpguv.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
.github/workflows/aws_gpu_benchmarks.yml.github/workflows/aws_gpu_tests.yml.github/workflows/minimum_deps_tests.yml.github/workflows/mujoco_warp_tests.yml.github/workflows/warp_nightly_tests.ymlCHANGELOG.mddocs/concepts/collisions.rstdocs/guide/release.rstnewton/_src/geometry/collision_convex.pynewton/_src/geometry/collision_core.pynewton/_src/geometry/contact_data.pynewton/_src/geometry/contact_reduction_global.pynewton/_src/geometry/contact_reduction_hydroelastic.pynewton/_src/geometry/contact_sort.pynewton/_src/geometry/multicontact.pynewton/_src/geometry/narrow_phase.pynewton/_src/geometry/raycast.pynewton/_src/geometry/sdf_contact.pynewton/_src/geometry/sdf_hydroelastic.pynewton/_src/geometry/support_function.pynewton/_src/geometry/types.pynewton/_src/sim/builder.pynewton/_src/sim/collide.pynewton/_src/sim/contacts.pynewton/_src/solvers/kamino/_src/core/builder.pynewton/_src/solvers/kamino/_src/core/geometry.pynewton/_src/solvers/kamino/_src/core/model.pynewton/_src/solvers/kamino/_src/core/shapes.pynewton/_src/solvers/kamino/_src/geometry/unified.pynewton/_src/solvers/kamino/examples/rl/example_rl_bipedal.pynewton/_src/solvers/kamino/examples/rl/simulation.pynewton/_src/solvers/kamino/tests/test_geometry_mesh_heightfield.pynewton/_src/solvers/kamino/tests/test_geometry_unified.pynewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/solvers/xpbd/kernels.pynewton/_src/solvers/xpbd/solver_xpbd.pynewton/_src/utils/import_usd.pynewton/_src/viewer/gl/opengl.pynewton/_src/viewer/gl/shaders.pynewton/_src/viewer/kernels.pynewton/_src/viewer/picking.pynewton/_src/viewer/viewer.pynewton/_src/viewer/viewer_gl.pynewton/_src/viewer/viewer_viser.pynewton/examples/__init__.pynewton/examples/basic/example_basic_conveyor.pynewton/examples/cloth/example_cloth_franka.pynewton/examples/robot/example_robot_panda_hydro.pynewton/tests/assets/humanoid_mjc.usdanewton/tests/test_broad_phase.pynewton/tests/test_collision_pipeline.pynewton/tests/test_contact_reduction_global.pynewton/tests/test_hashtable.pynewton/tests/test_hydroelastic.pynewton/tests/test_import_usd.pynewton/tests/test_import_usd_multi_dof.pynewton/tests/test_menagerie_mujoco.pynewton/tests/test_menagerie_usd_mujoco.pynewton/tests/test_mujoco_solver.pynewton/tests/test_narrow_phase.pynewton/tests/test_raycast.pynewton/tests/test_remesh.pynewton/tests/test_sensor_raycast.pynewton/tests/test_solver_xpbd.pynewton/tests/test_speculative_contacts.pynewton/tests/test_viewer_picking.py
💤 Files with no reviewable changes (7)
- newton/tests/test_menagerie_mujoco.py
- newton/tests/test_narrow_phase.py
- newton/tests/test_hashtable.py
- newton/tests/test_viewer_picking.py
- newton/tests/test_broad_phase.py
- newton/tests/test_hydroelastic.py
- newton/tests/test_remesh.py
✅ Files skipped from review due to trivial changes (4)
- newton/_src/solvers/kamino/tests/test_geometry_unified.py
- .github/workflows/aws_gpu_benchmarks.yml
- newton/examples/basic/example_basic_conveyor.py
- newton/_src/sim/builder.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_speculative_contacts.py
# Conflicts: # newton/_src/geometry/contact_reduction_global.py # newton/_src/geometry/narrow_phase.py # newton/_src/sim/collide.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
newton/_src/geometry/sdf_contact.py (1)
1293-1300:⚠️ Potential issue | 🔴 CriticalRemove the sign flip in
_query_mesh_face_normal; always return the outward face normal.The function currently flips the normal when the query point is inside the mesh (when
sign < 0.0), producing inward-pointing normals. Sincewp.mesh_eval_face_normalreturns outward normals by definition, the sign should not be used to flip the direction. This inversion causes speculative contact normals to be reversed, which can make the solver push shapes together instead of apart. Delete lines 43–44:if sign < 0.0: n = -nAlways return the outward normal directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_contact.py` around lines 1293 - 1300, _query_mesh_face_normal currently flips the face normal when the computed sign is negative, causing inward-pointing normals; remove that sign-based inversion so the function always returns the outward face normal produced by wp.mesh_eval_face_normal. Edit _query_mesh_face_normal to stop negating n when sign < 0.0 (delete the if sign < 0.0: n = -n block) and return the original wp.mesh_eval_face_normal result so callers like the speculative branch in sdf_contact.py receive outward normals consistently.newton/_src/geometry/narrow_phase.py (2)
153-155:⚠️ Potential issue | 🟠 MajorInclude speculative mode in Warp module keys to avoid specialization aliasing.
These factories emit different kernels for
speculative=True/False, but both variants currently share the samemodulekey. That can alias Warp’s compiled specialization cache and reuse the wrong variant.♻️ Proposed fix
- _module = f"narrow_phase_primitive_{writer_func.__name__}" + _module = f"narrow_phase_primitive_{writer_func.__name__}_{int(speculative)}" - _module = f"narrow_phase_gjk_mpr_{external_aabb}_{writer_func.__name__}_{_sf}_{_ppc}" + _module = f"narrow_phase_gjk_mpr_{external_aabb}_{writer_func.__name__}_{_sf}_{_ppc}_{int(speculative)}" - _module = f"narrow_phase_mesh_tri_{writer_func.__name__}" + _module = f"narrow_phase_mesh_tri_{writer_func.__name__}_{int(speculative)}" - _module = f"narrow_phase_mesh_plane_{writer_func.__name__}_{reduce_contacts}" + _module = f"narrow_phase_mesh_plane_{writer_func.__name__}_{reduce_contacts}_{int(speculative)}"In NVIDIA Warp (v1.12.x), if two `@wp.kernel` definitions use the same `module` name but differ by `wp.static(...)`-guarded code paths, can the compiled specialization cache alias and reuse the wrong variant?Also applies to: 661-663, 960-963, 1166-1169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/narrow_phase.py` around lines 153 - 155, The Warp kernel module name currently uses _module = f"narrow_phase_primitive_{writer_func.__name__}" for kernels decorated with `@wp.kernel` (see the decorator block referencing _module), which causes both speculative=True and speculative=False variants to share the same module key and risk specialization aliasing; update the module key generation to include the speculative mode (e.g., append the speculative boolean or "speculative"/"no_speculative" to the f-string) so each variant gets a unique module name whenever the kernel uses wp.static-guarded speculative code paths (apply same change to the other occurrences around the decorator blocks at the other noted sites).
1843-1846:⚠️ Potential issue | 🔴 CriticalDon’t silently substitute empty velocity arrays in speculative mode.
When
self.speculative=True, falling back to zero-length_slv/_sasbcan cause out-of-bounds indexing in kernels. This is especially risky becauseNarrowPhase.launch()does not forward speculative arrays today.🐛 Proposed fix
- # Resolve speculative velocity arrays (empty when disabled) - _empty_vec3 = wp.empty(0, dtype=wp.vec3, device=device) - _empty_float = wp.empty(0, dtype=wp.float32, device=device) - _slv = shape_lin_vel if shape_lin_vel is not None else _empty_vec3 - _sasb = shape_ang_speed_bound if shape_ang_speed_bound is not None else _empty_float + # Resolve speculative velocity arrays + if self.speculative: + if shape_lin_vel is None or shape_ang_speed_bound is None: + raise TypeError( + "Speculative narrow phase requires `shape_lin_vel` and " + "`shape_ang_speed_bound`." + ) + _slv = shape_lin_vel + _sasb = shape_ang_speed_bound + else: + _slv = wp.empty(0, dtype=wp.vec3, device=device) + _sasb = wp.empty(0, dtype=wp.float32, device=device)Also applies to: 1880-1885
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/narrow_phase.py` around lines 1843 - 1846, When self.speculative is True we must not silently substitute zero-length shape_lin_vel/_slv or shape_ang_speed_bound/_sasb because kernels will OOB; update NarrowPhase.launch() and callers to forward the speculative arrays (shape_lin_vel and shape_ang_speed_bound) rather than leaving them None, add a validation in the NarrowPhase initializer or in the function that sets _slv/_sasb to raise a clear exception if speculative=True and those arrays are None or have length 0, and guard any kernel-launch code that indexes into _slv/_sasb (search for uses of _slv, _sasb and shape_lin_vel/shape_ang_speed_bound) so it only launches when valid arrays are provided (or allocate correctly-sized arrays when a safe default is acceptable).
🧹 Nitpick comments (1)
newton/_src/geometry/sdf_contact.py (1)
36-36: Remove unnecessaryint()cast.
face_indexis already declared as an integer literal; the explicitint(0)is redundant.♻️ Suggested fix
- face_index = int(0) + face_index = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_contact.py` at line 36, In sdf_contact.py remove the redundant int() cast on the face_index initialization by replacing the explicit int(0) usage with a plain integer literal; locate the assignment to face_index (currently "face_index = int(0)") and change it to a simple "face_index = 0" to eliminate the unnecessary constructor call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 79-81: Remove the duplicated CHANGELOG bullets in the [Unreleased]
section: delete the later occurrences of `TRIANGLE_PRISM` support-function type
and `ViewerGL.log_scalar()` (the duplicate bullets near the end of the section)
and keep the original entries already present earlier in the same section; this
ensures only the first occurrences of `TRIANGLE_PRISM` and
`ViewerGL.log_scalar()` remain in CHANGELOG.md.
---
Duplicate comments:
In `@newton/_src/geometry/narrow_phase.py`:
- Around line 153-155: The Warp kernel module name currently uses _module =
f"narrow_phase_primitive_{writer_func.__name__}" for kernels decorated with
`@wp.kernel` (see the decorator block referencing _module), which causes both
speculative=True and speculative=False variants to share the same module key and
risk specialization aliasing; update the module key generation to include the
speculative mode (e.g., append the speculative boolean or
"speculative"/"no_speculative" to the f-string) so each variant gets a unique
module name whenever the kernel uses wp.static-guarded speculative code paths
(apply same change to the other occurrences around the decorator blocks at the
other noted sites).
- Around line 1843-1846: When self.speculative is True we must not silently
substitute zero-length shape_lin_vel/_slv or shape_ang_speed_bound/_sasb because
kernels will OOB; update NarrowPhase.launch() and callers to forward the
speculative arrays (shape_lin_vel and shape_ang_speed_bound) rather than leaving
them None, add a validation in the NarrowPhase initializer or in the function
that sets _slv/_sasb to raise a clear exception if speculative=True and those
arrays are None or have length 0, and guard any kernel-launch code that indexes
into _slv/_sasb (search for uses of _slv, _sasb and
shape_lin_vel/shape_ang_speed_bound) so it only launches when valid arrays are
provided (or allocate correctly-sized arrays when a safe default is acceptable).
In `@newton/_src/geometry/sdf_contact.py`:
- Around line 1293-1300: _query_mesh_face_normal currently flips the face normal
when the computed sign is negative, causing inward-pointing normals; remove that
sign-based inversion so the function always returns the outward face normal
produced by wp.mesh_eval_face_normal. Edit _query_mesh_face_normal to stop
negating n when sign < 0.0 (delete the if sign < 0.0: n = -n block) and return
the original wp.mesh_eval_face_normal result so callers like the speculative
branch in sdf_contact.py receive outward normals consistently.
---
Nitpick comments:
In `@newton/_src/geometry/sdf_contact.py`:
- Line 36: In sdf_contact.py remove the redundant int() cast on the face_index
initialization by replacing the explicit int(0) usage with a plain integer
literal; locate the assignment to face_index (currently "face_index = int(0)")
and change it to a simple "face_index = 0" to eliminate the unnecessary
constructor call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: df7549df-528e-4acb-af6b-02e08ecc7fa9
📒 Files selected for processing (7)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/contact_reduction_global.pynewton/_src/geometry/narrow_phase.pynewton/_src/geometry/sdf_contact.pynewton/_src/sim/collide.pynewton/tests/test_speculative_contacts.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/concepts/collisions.rst
- newton/_src/sim/collide.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
newton/_src/geometry/narrow_phase.py (1)
1843-1846:⚠️ Potential issue | 🟠 Major
launch()still doesn't expose the speculative inputs.These parameters are now available on
launch_custom_write(), butNarrowPhase.launch()still can't accept or forward them. Callers on the simple API path therefore can't enable the feature even when the instance was constructed withspeculative=True.♻️ Proposed fix
def launch( self, *, @@ contact_count: wp.array[int], contact_tangent: wp.array[wp.vec3] | None = None, device: Devicelike | None = None, + shape_lin_vel: wp.array[wp.vec3] | None = None, + shape_ang_speed_bound: wp.array[wp.float32] | None = None, + speculative_dt: float = 0.0, + max_speculative_extension: float = 0.0, **kwargs: Any, ) -> None: @@ self.launch_custom_write( candidate_pair=candidate_pair, candidate_pair_count=candidate_pair_count, @@ writer_data=writer_data, device=device, + shape_lin_vel=shape_lin_vel, + shape_ang_speed_bound=shape_ang_speed_bound, + speculative_dt=speculative_dt, + max_speculative_extension=max_speculative_extension, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/narrow_phase.py` around lines 1843 - 1846, NarrowPhase.launch() currently doesn't accept the speculative inputs introduced on launch_custom_write(), so update NarrowPhase.launch() signature to include the same optional parameters (shape_lin_vel: wp.array[wp.vec3] | None = None, shape_ang_speed_bound: wp.array[wp.float32] | None = None, speculative_dt: float = 0.0, max_speculative_extension: float = 0.0) and forward them through to NarrowPhase.launch_custom_write(...) when calling it; preserve existing defaults and pass the new args through unmodified so callers on the simple API path can enable speculative behavior when the instance was constructed with speculative=True.
🧹 Nitpick comments (1)
newton/_src/geometry/sdf_contact.py (1)
36-36: Consider removing redundantint()cast.Static analysis (Ruff RUF046) flags
int(0)as redundant since0is already an integer. However, this is a Warp function where explicit typing can sometimes aid code generation—feel free to keep if intentional for clarity.🔧 Optional fix
- face_index = int(0) + face_index = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_contact.py` at line 36, The assignment face_index = int(0) uses a redundant int() around a literal; change it to face_index = 0 (or keep the int() only if you intentionally need an explicit cast for Warp codegen) and ensure any nearby usage or type expectations of the face_index variable remain satisfied; locate the face_index symbol in sdf_contact.py and replace the expression while running tests or lint to confirm no codegen requirement forces the explicit cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/narrow_phase.py`:
- Around line 880-894: The heightfield branch is still calling
heightfield_vs_convex_midphase(...) with only shape_gap, missing the speculative
extension used by the mesh branch; compute the same speculative gap used for
mesh candidate expansion (e.g. speculative_gap = shape_gap +
speculative_extension or the existing shape_speculative variable used elsewhere)
and pass that speculative_gap into heightfield_vs_convex_midphase (when invoking
with shape_a, shape_b, hfd, shape_transform, ..., triangle_pairs,
triangle_pairs_count) so heightfield candidate generation matches the mesh
branch's speculative expansion.
In `@newton/_src/geometry/sdf_contact.py`:
- Around line 1076-1082: The mesh-SDF kernel currently uses an undirected
rel_speed to extend gap_sum (vars: base_gap_sum, gap_sum, vel_rel, rel_speed,
speculative_dt, max_speculative_extension) which allows diverging pairs to
produce false speculative contacts; change the logic so after contact_normal is
computed (the same contact_normal used around the mesh SDF contact handling /
contact_data assignment) you compute a directed approach speed v_approach =
-dot(vel_rel, contact_normal) + shape_ang_speed_bound[pair[0]] +
shape_ang_speed_bound[pair[1]], clamp it with wp.max(v_approach, 0.0), compute
directed_extension = wp.min(v_approach * speculative_dt,
max_speculative_extension), and use base_gap_sum + directed_extension (instead
of the undirected rel_speed extension) when writing gap_sum into contact_data so
diverging pairs are not given speculative extensions.
In `@newton/_src/sim/collide.py`:
- Around line 598-599: In collide(), when callers pass a prebuilt NarrowPhase
(the expert-components branch), validate that speculative_config is not None
matches narrow_phase.speculative and fail early on mismatch: check
narrow_phase.speculative and if speculative_config is None while
narrow_phase.speculative is True (or speculative_config is not None while
narrow_phase.speculative is False) raise a clear ValueError (or similar)
describing the mismatch; perform this check before any pipeline prep or kernel
invocation so we don't silently disable candidate generation or feed empty
velocity arrays into speculative kernels—refer to the collide() parameter
speculative_config and the narrow_phase.speculative attribute to locate where to
add the validation (also apply same check in the other expert-components branch
handling around the alternate collide() call site).
---
Duplicate comments:
In `@newton/_src/geometry/narrow_phase.py`:
- Around line 1843-1846: NarrowPhase.launch() currently doesn't accept the
speculative inputs introduced on launch_custom_write(), so update
NarrowPhase.launch() signature to include the same optional parameters
(shape_lin_vel: wp.array[wp.vec3] | None = None, shape_ang_speed_bound:
wp.array[wp.float32] | None = None, speculative_dt: float = 0.0,
max_speculative_extension: float = 0.0) and forward them through to
NarrowPhase.launch_custom_write(...) when calling it; preserve existing defaults
and pass the new args through unmodified so callers on the simple API path can
enable speculative behavior when the instance was constructed with
speculative=True.
---
Nitpick comments:
In `@newton/_src/geometry/sdf_contact.py`:
- Line 36: The assignment face_index = int(0) uses a redundant int() around a
literal; change it to face_index = 0 (or keep the int() only if you
intentionally need an explicit cast for Warp codegen) and ensure any nearby
usage or type expectations of the face_index variable remain satisfied; locate
the face_index symbol in sdf_contact.py and replace the expression while running
tests or lint to confirm no codegen requirement forces the explicit cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d4008b2b-a8c7-4bcf-a379-4073409b6e15
📒 Files selected for processing (7)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/contact_reduction_global.pynewton/_src/geometry/narrow_phase.pynewton/_src/geometry/sdf_contact.pynewton/_src/sim/collide.pynewton/tests/test_speculative_contacts.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/concepts/collisions.rst
Comment on lines
+880
to
+894
| if type_a == GeoType.HFIELD: | ||
| # Only run on j==0; the j dimension is for tiled BVH queries (mesh only). | ||
| if j != 0: | ||
| continue | ||
| hfd = heightfield_data[shape_heightfield_index[shape_a]] | ||
| heightfield_vs_convex_midphase( | ||
| shape_a, | ||
| shape_b, | ||
| hfd, | ||
| shape_transform, | ||
| shape_collision_radius, | ||
| shape_gap, | ||
| triangle_pairs, | ||
| triangle_pairs_count, | ||
| ) |
Contributor
There was a problem hiding this comment.
Speculative heightfield pairs still use a non-speculative midphase.
The GeoType.HFIELD branch still calls heightfield_vs_convex_midphase(...) with only the baseline shape_gap, while the mesh branch below expands candidate generation with the speculative extension. A fast convex can therefore enter a new heightfield cell between collision updates and never emit the triangle pairs needed for later GJK, so speculative contacts will still miss some heightfield tunneling cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/geometry/narrow_phase.py` around lines 880 - 894, The heightfield
branch is still calling heightfield_vs_convex_midphase(...) with only shape_gap,
missing the speculative extension used by the mesh branch; compute the same
speculative gap used for mesh candidate expansion (e.g. speculative_gap =
shape_gap + speculative_extension or the existing shape_speculative variable
used elsewhere) and pass that speculative_gap into
heightfield_vs_convex_midphase (when invoking with shape_a, shape_b, hfd,
shape_transform, ..., triangle_pairs, triangle_pairs_count) so heightfield
candidate generation matches the mesh branch's speculative expansion.
Comment on lines
+1076
to
+1082
| base_gap_sum = shape_gap[pair[0]] + shape_gap[pair[1]] | ||
| gap_sum = base_gap_sum | ||
|
|
||
| if wp.static(speculative): | ||
| vel_rel = shape_lin_vel[pair[1]] - shape_lin_vel[pair[0]] | ||
| rel_speed = wp.length(vel_rel) + shape_ang_speed_bound[pair[0]] + shape_ang_speed_bound[pair[1]] | ||
| gap_sum = gap_sum + wp.min(rel_speed * speculative_dt, max_speculative_extension) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for directed velocity handling in broadphase or AABB expansion
rg -n -C3 'speculative' newton/_src/geometry/ --type=py | head -80Repository: newton-physics/newton
Length of output: 6325
🏁 Script executed:
rg -n 'test_speculative_diverging_no_contacts' --type=py -B5 -A20Repository: newton-physics/newton
Length of output: 4365
🏁 Script executed:
# Find where speculative pairs are selected/filtered in broadphase
rg -n 'broadphase\|midphase\|pair' newton/_src/geometry/ --type=py | rg -i 'velocity|speculative' -B2 -A2Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Check the complete narrow-phase flow around lines 1290-1293
sed -n '1285,1310p' newton/_src/geometry/sdf_contact.pyRepository: newton-physics/newton
Length of output: 1699
🏁 Script executed:
# Find broadphase and pair generation logic with velocity/speculative handling
rg -n 'narrow_phase_primitive_pairs\|broad_phase\|speculative' newton/_src/geometry/ --type=py | grep -i 'pair\|velocity' | head -40Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Look for where pairs are actually generated/filtered before narrow-phase
rg -n 'def.*pair\|shape_pairs' newton/_src/geometry/narrow_phase.py -B2 -A5 | head -80Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Check if test is using a custom speculative config that might filter
sed -n '77,110p' newton/tests/test_speculative_contacts.pyRepository: newton-physics/newton
Length of output: 1291
🏁 Script executed:
# Find where pairs are generated and how speculative affects pair generation
rg -n 'speculative' newton/_src/geometry/midphase.py --type=py -B3 -A5 | head -100Repository: newton-physics/newton
Length of output: 137
🏁 Script executed:
# Check the primitive narrow-phase kernel definition and parameter passing
sed -n '136,250p' newton/_src/geometry/narrow_phase.pyRepository: newton-physics/newton
Length of output: 4926
🏁 Script executed:
# Look for velocity-based pair filtering in collision pipeline
rg -n 'approach\|relative.*velocity' newton/_src/geometry/ --type=py | head -30Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Search for how the gap_sum is used after speculative extension
sed -n '1076,1150p' newton/_src/geometry/sdf_contact.pyRepository: newton-physics/newton
Length of output: 3734
🏁 Script executed:
# Look at primitive narrow-phase gap_sum usage
rg -n 'gap_sum' newton/_src/geometry/narrow_phase.py -B2 -A2 | head -60Repository: newton-physics/newton
Length of output: 2727
🏁 Script executed:
# Check if there's any dot product or approach velocity logic
rg -n 'dot\|approach' newton/_src/geometry/ --type=py | head -30Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Check the complete directed velocity logic in primitive kernel
sed -n '540,560p' newton/_src/geometry/narrow_phase.pyRepository: newton-physics/newton
Length of output: 1171
🏁 Script executed:
# Search for v_approach definition and calculation
rg -n 'v_approach' newton/_src/geometry/narrow_phase.py -B5 -A3Repository: newton-physics/newton
Length of output: 941
🏁 Script executed:
# Check if there are any mesh-SDF diverging tests
rg -n 'diverging\|diverge' newton/tests/ --type=pyRepository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Check if SDF mesh kernel has any directed velocity recalculation (should be around line 1290-1320)
sed -n '1280,1330p' newton/_src/geometry/sdf_contact.py | grep -C5 'v_approach\|dot\|directed'Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Check the second SDF kernel too (around line 1395-1398)
sed -n '1390,1430p' newton/_src/geometry/sdf_contact.py | grep -C5 'v_approach\|dot\|directed'Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Confirm if test_speculative_diverging_no_contacts actually tests mesh-SDF or only primitives
rg -n '_build_two_sphere_model' newton/tests/test_speculative_contacts.py -A30 | head -50Repository: newton-physics/newton
Length of output: 2018
🏁 Script executed:
# Check if the SDF contact kernel does any additional filtering after gap_sum is extended
sed -n '1076,1320p' newton/_src/geometry/sdf_contact.py | grep -n 'gap_sum\|dist'Repository: newton-physics/newton
Length of output: 1603
🏁 Script executed:
# See if there's conditional logic that prevents contact creation for large gaps
sed -n '1285,1305p' newton/_src/geometry/sdf_contact.pyRepository: newton-physics/newton
Length of output: 1371
🏁 Script executed:
# Search for all uses of gap_sum in sdf_contact after the speculative extension
rg -n 'gap_sum\|base_threshold' newton/_src/geometry/sdf_contact.py | head -40Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Verify the contact creation logic in primitive kernel to confirm directed logic
sed -n '250,260p' newton/_src/geometry/narrow_phase.pyRepository: newton-physics/newton
Length of output: 747
🏁 Script executed:
# Double-check: does the primitive kernel pre-routing use undirected gap_sum?
sed -n '246,280p' newton/_src/geometry/narrow_phase.pyRepository: newton-physics/newton
Length of output: 1988
🏁 Script executed:
# Check if there are any mesh-SDF specific tests for speculative diverging
rg -n 'test_speculative.*mesh\|mesh.*speculative' newton/tests/ --type=pyRepository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Verify SDF kernel never applies directed velocity like the primitive kernel does
rg -n 'directed_gap_sum\|v_approach' newton/_src/geometry/sdf_contact.pyRepository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Double-check the contact_data assignment in SDF kernel
sed -n '1300,1320p' newton/_src/geometry/sdf_contact.pyRepository: newton-physics/newton
Length of output: 1107
🏁 Script executed:
# Check if there's a second SDF kernel variant that might have different logic
sed -n '1395,1430p' newton/_src/geometry/sdf_contact.py | head -50Repository: newton-physics/newton
Length of output: 1716
🏁 Script executed:
# Verify the second SDF kernel also doesn't have directed velocity logic
rg -n 'def.*collision_kernel\|gap_sum' newton/_src/geometry/sdf_contact.py | grep -A2 -B2 '1395\|1398'Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Search for any comment or documentation about why SDF kernel doesn't use directed velocity
rg -n 'undirected\|conservative\|candidate.*routing' newton/_src/geometry/sdf_contact.pyRepository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Verify that primitive kernel actually uses the directed_gap_sum in contact creation
sed -n '555,580p' newton/_src/geometry/narrow_phase.pyRepository: newton-physics/newton
Length of output: 1373
🏁 Script executed:
# Check what collision types the two-sphere model uses (primitive vs mesh)
rg -n 'add_shape_sphere' newton/tests/test_speculative_contacts.py -B2 -A2Repository: newton-physics/newton
Length of output: 895
🏁 Script executed:
# Confirm sphere-sphere collisions use primitive kernel, not SDF
rg -n 'GeoType\|SPHERE' newton/_src/geometry/narrow_phase.py | head -20Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Look for shape type definitions to understand sphere collision routing
rg -n 'class.*GeoType\|GeoType\.' newton/_src/geometry/ --type=py | head -30Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Verify that sphere collisions don't use SDF by checking if they route to GJK/primitives
rg -n 'SPHERE\|sphere\|type_a.*type_b' newton/_src/geometry/narrow_phase.py | grep -i sphere | head -10Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Check how the primitive kernel routes sphere-sphere vs mesh pairs
sed -n '180,220p' newton/_src/geometry/narrow_phase.pyRepository: newton-physics/newton
Length of output: 1600
🏁 Script executed:
# Understand the complete picture: search for where the directed_gap_sum comment appears
rg -n 'undirected speed as a conservative' newton/_src/geometry/ --type=pyRepository: newton-physics/newton
Length of output: 187
🏁 Script executed:
# Verify that mesh-SDF kernels are in fact used for certain collision types (not just spheres)
rg -n 'shape_pairs_mesh\|mesh.*collision\|GeoType.MESH' newton/_src/geometry/narrow_phase.py | head -30Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Confirm that the SDF kernel never recomputes with directed velocity like primitives do
sed -n '1070,1330p' newton/_src/geometry/sdf_contact.py | grep -n 'v_approach\|dot.*normal\|directed'Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Final confirmation: list all gap_sum assignments in SDF kernel
rg -n 'gap_sum =' newton/_src/geometry/sdf_contact.pyRepository: newton-physics/newton
Length of output: 558
🏁 Script executed:
# Check if there's any contact filtering in the writer function that might catch this
rg -n 'contact_passes_gap_check\|def.*writer' newton/_src/geometry/sdf_contact.py | head -20Repository: newton-physics/newton
Length of output: 47
SDF mesh kernel uses undirected speculative extension without directed approach velocity filtering.
The mesh-SDF collision kernel (lines 1076–1082 and 1395–1398) extends gap_sum using undirected relative speed without recomputing with a directed approach velocity component. This asymmetry allows diverging mesh-SDF pairs to report false-positive speculative contacts.
In contrast, the primitive kernel (narrow_phase.py) applies this two-stage logic:
- Initial undirected gap_sum for candidate routing (conservative bound)
- Directed refinement after contact normal is available:
v_approach = -wp.dot(vel_rel, contact_normal) + ...clamped towp.max(v_approach * speculative_dt, 0.0)to exclude diverging objects (lines 540–555)
The existing test test_speculative_diverging_no_contacts only covers primitives (spheres), which benefit from the directed refinement, masking the bug for mesh pairs. SDF kernels should apply the same directed approach velocity check (using contact_normal at lines 1290–1320) before assigning gap_sum to contact_data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/geometry/sdf_contact.py` around lines 1076 - 1082, The mesh-SDF
kernel currently uses an undirected rel_speed to extend gap_sum (vars:
base_gap_sum, gap_sum, vel_rel, rel_speed, speculative_dt,
max_speculative_extension) which allows diverging pairs to produce false
speculative contacts; change the logic so after contact_normal is computed (the
same contact_normal used around the mesh SDF contact handling / contact_data
assignment) you compute a directed approach speed v_approach = -dot(vel_rel,
contact_normal) + shape_ang_speed_bound[pair[0]] +
shape_ang_speed_bound[pair[1]], clamp it with wp.max(v_approach, 0.0), compute
directed_extension = wp.min(v_approach * speculative_dt,
max_speculative_extension), and use base_gap_sum + directed_extension (instead
of the undirected rel_speed extension) when writing gap_sum into contact_data so
diverging pairs are not given speculative extensions.
Comment on lines
+598
to
599
| speculative_config: SpeculativeContactConfig | None = None, | ||
| deterministic: bool = False, |
Contributor
There was a problem hiding this comment.
Reject mismatched speculative settings for expert components.
When callers provide a prebuilt NarrowPhase, speculative_config only toggles the pipeline-side prep. If the supplied narrow_phase was built with speculative=False, speculative candidate generation is silently disabled; if it was built with speculative=True while speculative_config is None, collide() will pass empty velocity arrays into kernels compiled to index them. Please validate speculative_config is not None against narrow_phase.speculative in the expert-components branch and fail fast on mismatches.
♻️ Proposed fix
using_expert_components = broad_phase_instance is not None or narrow_phase is not None
if using_expert_components:
if broad_phase_instance is None or narrow_phase is None:
raise ValueError("Provide both broad_phase and narrow_phase for expert component construction")
if sdf_hydroelastic_config is not None:
raise ValueError("sdf_hydroelastic_config cannot be used when narrow_phase is provided")
+ if getattr(narrow_phase, "speculative", False) != self._speculative_enabled:
+ raise ValueError(
+ "speculative_config must match the supplied NarrowPhase.speculative setting"
+ )
inferred_mode = _infer_broad_phase_mode_from_instance(broad_phase_instance)Also applies to: 658-660
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/sim/collide.py` around lines 598 - 599, In collide(), when
callers pass a prebuilt NarrowPhase (the expert-components branch), validate
that speculative_config is not None matches narrow_phase.speculative and fail
early on mismatch: check narrow_phase.speculative and if speculative_config is
None while narrow_phase.speculative is True (or speculative_config is not None
while narrow_phase.speculative is False) raise a clear ValueError (or similar)
describing the mismatch; perform this check before any pipeline prep or kernel
invocation so we don't silently disable candidate generation or feed empty
velocity arrays into speculative kernels—refer to the collide() parameter
speculative_config and the narrow_phase.speculative attribute to locate where to
add the validation (also apply same check in the other expert-components branch
handling around the alternate collide() call site).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add support for speculative contacts. They are the easiest way to get a decent amount of continuous collision detection for fast moving objects while keeping the complexity low.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
New feature / API change
Summary by CodeRabbit
Release Notes
New Features
SpeculativeContactConfigclass for enabling and controlling speculative behaviordtparameter for per-call collision update timing overrideTRIANGLE_PRISMsupport-function type for heightfield trianglesDocumentation
Tests