Added collision filtering feature for VBD #1092
Conversation
📝 WalkthroughWalkthroughAdds topology- and distance-based collision filtering: kernels and TriMeshCollisionDetector now accept max/min query radii and optional per-entity CSR-style filter lists; SolverVBD builds and registers per-entity filtering lists; kernels apply fast-fail filtering, min-distance pruning, record bidirectional collision buffers, and signal buffer overflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant SolverVBD as SolverVBD
participant FilterBuilder as Filter Builder
participant Detector as TriMeshCollisionDetector
participant Kernels as Collision Kernels
User->>SolverVBD: initialize(with filter params)
SolverVBD->>FilterBuilder: compute_contact_filtering_list()
FilterBuilder->>FilterBuilder: build_vertex_n_ring_tris_collision_filter()
FilterBuilder->>FilterBuilder: build_edge_n_ring_edge_collision_filter()
FilterBuilder-->>SolverVBD: CSR arrays (lists + offsets)
SolverVBD->>Detector: set_collision_filter_list(CSRs)
Detector->>Detector: store filtering lists & offsets
User->>Detector: vertex_triangle_collision_detection(max_qr,min_qr,ref_pos?)
Detector->>Kernels: vertex_triangle_collision_detection_kernel(...)
note right of Kernels `#DDEEFF`: Fast-fail via per-vertex list\nmin/max radius checks\nmin-distance pruning\nrecord vertex→triangle and triangle→vertex buffers
Kernels-->>Detector: collision results (+ buffers, resize flags)
User->>Detector: edge_edge_collision_detection(max_qr,min_qr,ref_pos?)
Detector->>Kernels: edge_colliding_edges_detection_kernel(...)
note right of Kernels `#DDEEFF`: Edge filtering list checks\nmin/max radius checks\nmin-distance pruning\nrecord edge collisions + resize flags
Kernels-->>Detector: edge collision results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
AnkaChan
requested review from
eric-heiden,
mmacklin and
shi-eric
and removed request for
shi-eric
AnkaChan
force-pushed
the
vbd_collision_filtering
branch
from
9060e08 to
8ae7d27
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
newton/_src/solvers/vbd/tri_mesh_collision.py (2)
102-139: Initialize filter-list attributes in__init__to fix AttributeError and clarify defaults
vertex_triangle_collision_detection()andedge_edge_collision_detection()unconditionally accessself.vertex_triangle_filtering_list,self.vertex_triangle_filtering_list_offsets,self.edge_filtering_list, andself.edge_filtering_list_offsets, but these attributes are never defined in__init__. When aTriMeshCollisionDetectoris used directly (as intest_vertex_triangle_collision/test_edge_edge_collision),set_collision_filter_list()is never called, leading to theAttributeErrorreported in CI.Given that the constructor already accepts
vertex_triangle_filtering_list,vertex_triangle_filtering_list_offsets,edge_filtering_list, andedge_filtering_list_offsets, the simplest fix is to store them onselfand default toNonewhen no filtering is desired. That also resolves the Ruff ARG002 “unused argument” warnings.Consider a minimal change along these lines:
class TriMeshCollisionDetector: def __init__( self, model: Model, record_triangle_contacting_vertices=False, vertex_positions=None, vertex_collision_buffer_pre_alloc=8, vertex_collision_buffer_max_alloc=256, - vertex_triangle_filtering_list=None, - vertex_triangle_filtering_list_offsets=None, + vertex_triangle_filtering_list=None, + vertex_triangle_filtering_list_offsets=None, triangle_collision_buffer_pre_alloc=16, triangle_collision_buffer_max_alloc=256, edge_collision_buffer_pre_alloc=8, edge_collision_buffer_max_alloc=256, - edge_filtering_list=None, - edge_filtering_list_offsets=None, + edge_filtering_list=None, + edge_filtering_list_offsets=None, ... ): self.model = model ... self.collision_detection_block_size = collision_detection_block_size + + # Optional per-vertex / per-edge filtering CSR lists; None means "no filtering". + self.vertex_triangle_filtering_list = vertex_triangle_filtering_list + self.vertex_triangle_filtering_list_offsets = vertex_triangle_filtering_list_offsets + self.edge_filtering_list = edge_filtering_list + self.edge_filtering_list_offsets = edge_filtering_list_offsetsWith this in place, calling code that does not configure filters continues to work (the kernels check the arrays in
if vertex_triangle_filtering_list/if edge_filtering_listguards), and SolverVBD’sset_collision_filter_list()remains responsible for overriding these when topological or external filters are enabled.Also applies to: 345-393
345-364: Triangle min-distance array is not initialized whenrecord_triangle_contacting_verticesisFalse— ISSUE CONFIRMEDThe analysis confirms the review is correct:
init_triangle_collision_data_kernelexplicitly initializestriangle_colliding_vertices_min_dist[tri_index] = query_radius(line 2235 in kernels.py), but is only called whenrecord_triangle_contacting_vertices=True(line 348).
vertex_triangle_collision_detection_kerneluseswp.atomic_min(triangle_colliding_vertices_min_dist, tri_index, dist)(lines 2332, 2482), which is a read-modify-write operation that requires the buffer to be pre-initialized. It is always launched (line 366), regardless of the flag.The buffer is allocated unconditionally (line 206) and read by
compute_particle_conservative_boundkernel (solver_vbd.py:1281), but when the detector is instantiated in SolverVBD (line 2513) without specifyingrecord_triangle_contacting_vertices, it defaults toFalse, leaving the buffer uninitialized.Using
wp.atomic_min()on uninitialized memory causes undefined behavior. The recommended fix (option 1 in the review) is correct: always initialize the buffer independently of the flag.newton/_src/solvers/vbd/solver_vbd.py (1)
2408-2495: Wirerest_shape_contact_exclusion_radiusinto collision detection callsThe parameter is stored at initialization but never passed to
collision_detection_penetration_free. The trimesh collision detector methods support the needed parameters:vertex_triangle_collision_detection(max_query_radius, min_query_radius=0., min_distance_filtering_ref_pos=None)andedge_edge_collision_detection(max_query_radius, min_query_radius=0., min_distance_filtering_ref_pos=None). However, the solver only calls them with a single parameter.Currently,
collision_detection_penetration_free(lines 3069–3070) invokes:
self.trimesh_collision_detector.vertex_triangle_collision_detection(self.self_contact_margin)self.trimesh_collision_detector.edge_edge_collision_detection(self.self_contact_margin)To activate the rest-shape distance filtering for users who set
rest_shape_contact_exclusion_radius, update these calls to:
self.trimesh_collision_detector.vertex_triangle_collision_detection(self.self_contact_margin, self.rest_shape_contact_exclusion_radius, self.rest_shape)self.trimesh_collision_detector.edge_edge_collision_detection(self.self_contact_margin, self.rest_shape_contact_exclusion_radius, self.rest_shape)newton/_src/geometry/kernels.py (1)
2363-2495: Avoid redefiningvertex_triangle_collision_detection_kerneland silence unused-arg warningThe new kernel body and filtering logic (max/min radii, per-vertex filtering lists, and rest-shape distance filter) look consistent, but there are a couple of issues:
- There is still an older
vertex_triangle_collision_detection_kerneldefined earlier (around Line 2243). Redefining the same kernel name in the same module triggers Ruff’s F811 and makes it unclear which variant is intended to be used by callers (and by Warp’s kernel registry). This should be cleaned up by either removing the old definition (if fully superseded) or renaming one of them to a distinct name.vertex_colliding_triangles_buffer_sizesis not used anymore in this new kernel. If it is only kept for API compatibility, consider explicitly marking it as used (or documenting the deprecation) so static analysis doesn’t keep flagging it.A minimal no-op usage that preserves the signature but fixes ARG001 could look like:
def vertex_triangle_collision_detection_kernel( max_query_radius: float, min_query_radius: float, @@ v_index = wp.tid() v = pos[v_index] vertex_buffer_offset = vertex_colliding_triangles_offsets[v_index] vertex_buffer_size = vertex_colliding_triangles_offsets[v_index + 1] - vertex_buffer_offset + + # Keep for API compatibility; actual buffer sizes are inferred from offsets. + _ = vertex_colliding_triangles_buffer_sizesI’d strongly recommend also deleting the older
vertex_triangle_collision_detection_kernelif there are no remaining call sites that depend on its old signature, to avoid ambiguity and linter errors.
🧹 Nitpick comments (4)
newton/tests/test_collision_cloth.py (2)
801-912: Distance-filter validation kernels match kernel semantics; minor unused-variable nitBoth
validate_vertex_collisions_distance_filterandvalidate_edge_collisions_distance_filtercorrectly mirror the distance-filter logic in the main Warp kernels:
- They verify that actual distances are within
max_query_radius.- They recompute distances in the reference configuration (
ref_pos) and enforce>= min_query_radius.- They also re-check buffer invariants (vertex/edge indices and -1 padding).
Only minor nit:
min_distinvalidate_edge_collisions_distance_filteris assigned but never used (Ruff F841). You can drop that local variable to keep the kernel tidy.
913-1072:test_collision_filteringthoroughly exercises topological and distance filters; clean up unused localsThe new
test_collision_filteringdoes a good job of:
- Verifying that vertex/edge filtering CSR slices are sorted per primitive.
- Checking that external vertex–triangle and edge–edge filter maps are preserved.
- Asserting that remaining filtered primitives satisfy the expected ring‑distance conditions via
leq_n_ring_vertices.- Exercising distance-based filtering by calling the extended collision detector APIs with
max_query_radius,min_query_radius, andvbd.rest_shape, then validating in Warp kernels.A few minor cleanups would make the test sharper:
- The unpacked
collision_detectorfrominit_model(...)is never used; consider renaming it to_or dropping it (RUF059).v_adj_facesandv_adj_faces_offsetsare read fromvbd.adjacencybut never used; you can remove those locals to satisfy Ruff F841.- In the final edge-distance validation, you hard-code
edge_edge_parallel_epsilon=1e-6while the detector itself is configured withedge_edge_parallel_epsilonpassed intoSolverVBD/TriMeshCollisionDetector. Using the same epsilon here (viavbd.trimesh_collision_detector.edge_edge_parallel_epsilon) would keep things perfectly aligned, though the current choice is unlikely to cause failures.newton/_src/solvers/vbd/solver_vbd.py (1)
2673-2721:compute_contact_filtering_listlogic is sound; minor type/doc polish possible
compute_contact_filtering_listcorrectly:
- Builds topological vertex–triangle and edge–edge filter sets when
topological_contact_filter_threshold >= 2.- Merges in optional external vertex and edge filter maps, allocating empty sets when no topological sets exist.
- Converts the final sets into CSR arrays via
_set_to_csr, and uploads them as Warp arrays onself.device.- Stores
Nonefor all four attributes when no filters are configured, which integrates nicely with the optional usage in the kernels.Two minor polish points:
- The type hints on the constructor use
map | None, but hereexternal_*_mapis treated as a standarddict[int, Collection[int]]. If you care about type checking, consider tightening the type annotations accordingly._set_to_csrtakes adtypeargument that’s used for both the offsets and sizes; here you always call it with the default. If you anticipate very large meshes in the future, you might want to explicitly use a 32-bit type (to match Warp’swp.int32) and document that assumption.Functionally, though, the implementation matches the tests and intended behavior.
newton/_src/geometry/kernels.py (1)
2498-2599: Edge-edge kernel: unusededge_colliding_edges_buffer_sizesand stale docstringThe extended
edge_colliding_edges_detection_kernel(min/max radii, per-edge filtering lists, rest-shape filtering, andclosest_point_edge_edgeusage) looks coherent, but there are two maintainability issues:
edge_colliding_edges_buffer_sizesis never referenced, yet it’s part of the signature and flagged by static analysis (ARG001). If it’s only retained for ABI/API compatibility, you can mark it as intentionally unused without changing behavior:def edge_colliding_edges_detection_kernel( @@ e_index = wp.tid() + + # Keep for API compatibility; actual buffer sizes are derived from offsets. + _ = edge_colliding_edges_buffer_sizes
- The docstring above this kernel still refers to
edge_colliding_triangles_*andedge_min_dis_to_triangles, which no longer match the parameter and output names (edge_colliding_edges_*). Updating these names will make the documentation self-consistent and less confusing for future readers.These are non-functional issues but worth addressing to keep the kernels clean and linters quiet.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/geometry/kernels.py(8 hunks)newton/_src/solvers/vbd/solver_vbd.py(8 hunks)newton/_src/solvers/vbd/tri_mesh_collision.py(4 hunks)newton/tests/test_collision_cloth.py(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/vbd/tri_mesh_collision.pynewton/tests/test_collision_cloth.pynewton/_src/solvers/vbd/solver_vbd.pynewton/_src/geometry/kernels.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/tests/test_collision_cloth.pynewton/_src/solvers/vbd/solver_vbd.pynewton/_src/geometry/kernels.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/tests/test_collision_cloth.pynewton/_src/solvers/vbd/solver_vbd.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/kernels.py
🧬 Code graph analysis (3)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
newton/_src/geometry/kernels.py (4)
init_triangle_collision_data_kernel(2225-2239)vertex_triangle_collision_detection_kernel(2243-2344)vertex_triangle_collision_detection_kernel(2363-2494)edge_colliding_edges_detection_kernel(2498-2599)
newton/tests/test_collision_cloth.py (4)
newton/_src/solvers/vbd/solver_vbd.py (2)
SolverVBD(2376-3233)leq_n_ring_vertices(1866-1879)newton/_src/sim/builder.py (4)
color(4737-4778)particle_count(889-893)tri_count(896-900)edge_count(910-914)newton/_src/geometry/kernels.py (1)
triangle_closest_point(111-175)newton/_src/solvers/vbd/tri_mesh_collision.py (3)
refit(320-325)vertex_triangle_collision_detection(345-393)edge_edge_collision_detection(396-422)
newton/_src/solvers/vbd/solver_vbd.py (2)
newton/_src/solvers/vbd/tri_mesh_collision.py (2)
set_collision_filter_list(242-252)set_collision_filter_list(254-258)newton/_src/sim/builder.py (2)
particle_count(889-893)edge_count(910-914)
🪛 GitHub Actions: Pull Request
newton/_src/solvers/vbd/tri_mesh_collision.py
[error] 409-409: AttributeError: 'TriMeshCollisionDetector' object has no attribute 'edge_filtering_list' during edge-edge collision detection.
[error] 377-377: AttributeError: 'TriMeshCollisionDetector' object has no attribute 'vertex_triangle_filtering_list' during vertex-triangle collision detection.
newton/tests/test_collision_cloth.py
[error] 1-1: Test 'test_edge_edge_collision_cpu' failed with ERROR during execution.
[error] 1-1: Test 'test_vertex_triangle_collision_cpu' failed with ERROR during execution.
🪛 GitHub Actions: Pull Request - AWS GPU
newton/_src/solvers/vbd/tri_mesh_collision.py
[error] 409-409: AttributeError: 'TriMeshCollisionDetector' object has no attribute 'edge_filtering_list' during edge-edge collision detection.
[error] 377-377: AttributeError: 'TriMeshCollisionDetector' object has no attribute 'vertex_triangle_filtering_list' during vertex-triangle collision detection.
newton/tests/test_collision_cloth.py
[error] 592-592: Test 'edge_edge_collision_detection' failed due to missing internal filtering state (edge_filtering_list) in TriMeshCollisionDetector during CPU/CUDA runs.
🪛 Ruff (0.14.4)
newton/_src/solvers/vbd/tri_mesh_collision.py
110-110: Unused method argument: vertex_triangle_filtering_list
(ARG002)
111-111: Unused method argument: vertex_triangle_filtering_list_offsets
(ARG002)
116-116: Unused method argument: edge_filtering_list
(ARG002)
117-117: Unused method argument: edge_filtering_list_offsets
(ARG002)
254-254: Redefinition of unused set_collision_filter_list from line 242
(F811)
newton/tests/test_collision_cloth.py
875-875: Local variable min_dist is assigned to but never used
Remove assignment to unused variable min_dist
(F841)
918-918: Unpacked variable collision_detector is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
945-945: Local variable v_adj_faces is assigned to but never used
Remove assignment to unused variable v_adj_faces
(F841)
946-946: Local variable v_adj_faces_offsets is assigned to but never used
Remove assignment to unused variable v_adj_faces_offsets
(F841)
newton/_src/geometry/kernels.py
2363-2363: Redefinition of unused vertex_triangle_collision_detection_kernel from line 2243
(F811)
2370-2370: Unused function argument: vertex_colliding_triangles_buffer_sizes
(ARG001)
2505-2505: Unused function argument: edge_colliding_edges_buffer_sizes
(ARG001)
🔇 Additional comments (5)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
396-422: Edge–edge collision detection wiring to new filters looks correctThe extended
edge_edge_collision_detectionsignature threadsmax_query_radius,min_query_radius, optionaledge_filtering_list/ offsets, and optionalmin_distance_filtering_ref_posthrough toedge_colliding_edges_detection_kernel, matching the kernel’s interface. Defaultingmin_distance_filtering_ref_postoself.vertex_positionswhen not provided keeps the distance-filtering feature opt‑in based onmin_query_radius.No functional issues here from what I can see.
newton/tests/test_collision_cloth.py (1)
385-405:init_modelcolor flag wiring is consistent with SolverVBD requirementsAdding the
colorparameter and callingbuilder.color()when it isTruecorrectly ensures thatmodel.particle_color_groupsis populated in setups that later constructSolverVBD. The existing tests that don’t use VBD still callinit_modelwith default arguments and are unaffected.No issues here.
newton/_src/solvers/vbd/solver_vbd.py (2)
577-580: Body–particle damping integration matches existing patternsThe added damping terms in
evaluate_body_particle_contact:damping_hessian = (soft_contact_kd / dt) * body_contact_hessian body_contact_hessian = body_contact_hessian + damping_hessian body_contact_force = body_contact_force - damping_hessian * dxare consistent with how damping is handled elsewhere in this module (e.g., springs and collision damping), and the algebra looks sound.
No issues here.
2501-2538: Filter list computation and wiring into the collision detector look correctIn the
handle_self_contactbranch, the solver:
- Validates the contact margin/radius relationship.
- Allocates the penetration-free state and conservative bounds.
- Constructs a
TriMeshCollisionDetectorand then calls:self.compute_contact_filtering_list( external_vertex_contact_filtering_map, external_edge_contact_filtering_map ) self.trimesh_collision_detector.set_collision_filter_list( self.vertex_triangle_contact_filtering_list, self.vertex_triangle_contact_filtering_list_offsets, self.edge_edge_contact_filtering_list, self.edge_edge_contact_filtering_list_offsets, )This matches the design in the tests: the CSR filter arrays produced by
compute_contact_filtering_listare exactly whatTriMeshCollisionDetectorexpects, and they will be honored in subsequent calls tovertex_triangle_collision_detection/edge_edge_collision_detection.Once the missing attribute initialization in
TriMeshCollisionDetector.__init__is fixed (separate comment), this wiring looks solid.newton/_src/geometry/kernels.py (1)
2346-2359: Binary-search helper is correct and matches [start, end) semanticsThe implementation looks correct for a lower-bound search over a sorted int32 slice and cleanly handles the
[start, end)contract without off-by-one issues. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/geometry/kernels.py (1)
2243-2345: Remove obsoletevertex_triangle_collision_detection_kerneldefinition to avoid duplicationYou have two
vertex_triangle_collision_detection_kerneldefinitions in this module: the original at lines 2243–2345 and the new version at lines 2363–2495. The old definition uses a singlequery_radiusparameter, while the new one addsmax_query_radiusandmin_query_radius, plus filtering parameters. All call sites (e.g.,tri_mesh_collision.py:366) use the new signature. Drop the old kernel to eliminate the F811 redefinition warning and reduce confusion:-@wp.kernel -def vertex_triangle_collision_detection_kernel( - query_radius: float, - bvh_id: wp.uint64, - pos: wp.array(dtype=wp.vec3), - tri_indices: wp.array(dtype=wp.int32, ndim=2), - vertex_colliding_triangles_offsets: wp.array(dtype=wp.int32), - vertex_colliding_triangles_buffer_sizes: wp.array(dtype=wp.int32), - triangle_colliding_vertices_offsets: wp.array(dtype=wp.int32), - triangle_colliding_vertices_buffer_sizes: wp.array(dtype=wp.int32), - # outputs - vertex_colliding_triangles: wp.array(dtype=wp.int32), - vertex_colliding_triangles_count: wp.array(dtype=wp.int32), - vertex_colliding_triangles_min_dist: wp.array(dtype=float), - triangle_colliding_vertices: wp.array(dtype=wp.int32), - triangle_colliding_vertices_count: wp.array(dtype=wp.int32), - triangle_colliding_vertices_min_dist: wp.array(dtype=float), - resize_flags: wp.array(dtype=wp.int32), -): - ... - vertex_colliding_triangles_min_dist[v_index] = min_dis_to_tris - - -@wp.kernel -def vertex_triangle_collision_detection_kernel( +@wp.kernel +def vertex_triangle_collision_detection_kernel( max_query_radius: float, min_query_radius: float, ...newton/_src/solvers/vbd/tri_mesh_collision.py (1)
102-137: Initialize filtering attributes in__init__to fix AttributeError and ensure proper state managementThe constructor parameters
vertex_triangle_filtering_list,vertex_triangle_filtering_list_offsets,edge_filtering_list, andedge_filtering_list_offsets(lines 107, 109, 114, 116) are never assigned to instance attributes in__init__. However,vertex_triangle_collision_detection()andedge_edge_collision_detection()referenceself.vertex_triangle_filtering_listandself.edge_filtering_list(lines 377, 409), causingAttributeErrorwhen these methods are called beforeset_collision_filter_list()is invoked.Additionally, the file contains duplicate
set_collision_filter_list()method definitions at lines 242 and 254 with identical bodies—the second definition silently overwrites the first.Add the following to
__init__after line 137 (or afterself.collision_detection_block_sizeassignment):# Store filtering lists for collision detection self.vertex_triangle_filtering_list = vertex_triangle_filtering_list self.vertex_triangle_filtering_list_offsets = vertex_triangle_filtering_list_offsets self.edge_filtering_list = edge_filtering_list self.edge_filtering_list_offsets = edge_filtering_list_offsetsRemove the duplicate
set_collision_filter_list()definition at lines 254–258, keeping only the first at lines 242–249.
🧹 Nitpick comments (5)
newton/tests/test_collision_cloth.py (2)
801-849: Distance-filter validation kernels are consistent; consider minor cleanupsThe new
validate_vertex_collisions_distance_filterandvalidate_edge_collisions_distance_filterkernels correctly assert:
- current configuration distance is within
max_query_radius; and- rest-shape distance is at least
min_query_radius; and- buffer indexing invariants (owner indices, padding as -1) hold.
Minor nits you may want to address:
- Line 875:
min_distinvalidate_edge_collisions_distance_filteris dead; removing it will satisfy Ruff (F841).- For consistency with the geometric kernels, you might prefer
dist < max_query_radiusover<=in Line 895, though this is unlikely to matter numerically.Also applies to: 850-912
913-1072: test_collision_filtering provides good coverage; a few small style/lint issuesThe test does a nice job of:
- verifying CSR filter lists are sorted for fast bisection,
- checking external maps are preserved within the filter lists before stripping them off, and
- asserting that remaining entries satisfy the intended n‑ring topological distance semantics for both vertex–triangle and edge–edge filters, plus the rest-shape distance filtering path.
Minor cleanups to consider (to satisfy Ruff and avoid confusion):
- Line 918:
collision_detectorfrominit_model(...)is unused; either drop it or rename to_collision_detector.- Lines 945-946:
v_adj_facesandv_adj_faces_offsetsare unused and can be removed unless you plan to use them in follow‑up assertions.- The test registration at Line 1085 is correct and integrates the new test into the suite.
Functionally this all looks good.
Also applies to: 1085-1085
newton/_src/geometry/kernels.py (2)
2363-2495: Vertex–triangle kernel: filtering and min-distance logic align with intended semanticsThe updated
vertex_triangle_collision_detection_kernelcorrectly augments the original behavior by:
- expanding the AABB queries to
max_query_radius,- applying an optional per-vertex sorted filter slice (via
vertex_triangle_filtering_list+_binary_search_contains_int32), and- optionally discarding contacts whose rest-shape distance (computed via
min_distance_filtering_ref_pos) is belowmin_query_radius.A few points to double‑check:
- The
if vertex_triangle_filtering_list:andif triangle_colliding_vertices:guards rely on Warp’s treatment of array/null arguments inside kernels. Please verify that passingNonefrom Python is supported and that these conditions behave as expected; otherwise, you may want to ensure default non-null arrays and drop theifguards in favor offl_end > fl_startand/or buffer-size checks.vertex_colliding_triangles_buffer_sizesis now unused in the body; either consider removing it from the signature and call sites, or re-introduce it into the buffer-size computation to satisfy Ruff (ARG001).Functionally, the max/min radius and filtering semantics match what the tests and TriMeshCollisionDetector expect.
2497-2599: Edge–edge kernel: extended parameters and filtering behavior look correctThe extended
edge_colliding_edges_detection_kernel:
- uses
max_query_radiusfor the AABB expansion,- supports CSR-style edge filters via
edge_filtering_list+_binary_search_contains_int32, and- applies rest-shape pruning based on
min_distance_filtering_ref_posandmin_query_radiusbefore thedist < max_query_radiuscheck.As with the vertex kernel, a couple of refinements to consider:
- The guard
if edge_filtering_list:assumes Warp is fine with boolean checks on array/None arguments; please confirm this is supported for device code. If not, ensure default arrays are always passed and rely solely onfl_end > fl_startto detect an active filter slice.edge_colliding_edges_buffer_sizesis no longer used inside the kernel; you might remove it from the signature and call sites or re-use it to keep static analysis (ARG001) happy.Otherwise, the logic matches the test expectations for topological and rest-shape distance filtering.
newton/_src/solvers/vbd/solver_vbd.py (1)
1866-1879: Consider dtype consistency.The BFS-based n-ring computation is correct. However, consider using
dtype=np.int32at line 1879 for consistency with other CSR arrays in this module, which usenp.int32.- return np.fromiter(visited, dtype=int) + return np.fromiter(visited, dtype=np.int32)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/geometry/kernels.py(8 hunks)newton/_src/solvers/vbd/solver_vbd.py(8 hunks)newton/_src/solvers/vbd/tri_mesh_collision.py(4 hunks)newton/tests/test_collision_cloth.py(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/tests/test_collision_cloth.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/tests/test_collision_cloth.pynewton/_src/solvers/vbd/tri_mesh_collision.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/kernels.pynewton/_src/solvers/vbd/solver_vbd.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/_src/solvers/vbd/solver_vbd.pynewton/tests/test_collision_cloth.py
🧬 Code graph analysis (3)
newton/_src/solvers/vbd/solver_vbd.py (2)
newton/_src/solvers/vbd/tri_mesh_collision.py (2)
set_collision_filter_list(242-252)set_collision_filter_list(254-258)newton/_src/sim/builder.py (2)
particle_count(889-893)edge_count(910-914)
newton/tests/test_collision_cloth.py (4)
newton/_src/solvers/vbd/solver_vbd.py (2)
SolverVBD(2376-3233)leq_n_ring_vertices(1866-1879)newton/_src/sim/builder.py (4)
color(4737-4778)particle_count(889-893)tri_count(896-900)edge_count(910-914)newton/_src/geometry/kernels.py (1)
triangle_closest_point(111-175)newton/_src/solvers/vbd/tri_mesh_collision.py (3)
refit(320-325)vertex_triangle_collision_detection(345-393)edge_edge_collision_detection(396-422)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
newton/_src/geometry/kernels.py (4)
init_triangle_collision_data_kernel(2225-2239)vertex_triangle_collision_detection_kernel(2243-2344)vertex_triangle_collision_detection_kernel(2363-2494)edge_colliding_edges_detection_kernel(2498-2599)
🪛 GitHub Actions: Pull Request
newton/tests/test_collision_cloth.py
[error] 1-1: Test 'test_edge_edge_collision_cpu' failed. ERROR.
[error] 1-1: Test 'test_vertex_triangle_collision_cpu' failed. ERROR.
newton/_src/solvers/vbd/tri_mesh_collision.py
[error] 409-409: AttributeError: 'TriMeshCollisionDetector' object has no attribute 'edge_filtering_list'.
[error] 377-377: AttributeError: 'TriMeshCollisionDetector' object has no attribute 'vertex_triangle_filtering_list'.
🪛 GitHub Actions: Pull Request - AWS GPU
newton/_src/solvers/vbd/tri_mesh_collision.py
[error] 409-409: AttributeError: TriMeshCollisionDetector has no attribute 'edge_filtering_list'.
[error] 377-377: AttributeError: TriMeshCollisionDetector has no attribute 'vertex_triangle_filtering_list'.
🪛 Ruff (0.14.4)
newton/_src/geometry/kernels.py
2363-2363: Redefinition of unused vertex_triangle_collision_detection_kernel from line 2243
(F811)
2370-2370: Unused function argument: vertex_colliding_triangles_buffer_sizes
(ARG001)
2505-2505: Unused function argument: edge_colliding_edges_buffer_sizes
(ARG001)
newton/tests/test_collision_cloth.py
875-875: Local variable min_dist is assigned to but never used
Remove assignment to unused variable min_dist
(F841)
918-918: Unpacked variable collision_detector is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
945-945: Local variable v_adj_faces is assigned to but never used
Remove assignment to unused variable v_adj_faces
(F841)
946-946: Local variable v_adj_faces_offsets is assigned to but never used
Remove assignment to unused variable v_adj_faces_offsets
(F841)
newton/_src/solvers/vbd/tri_mesh_collision.py
110-110: Unused method argument: vertex_triangle_filtering_list
(ARG002)
111-111: Unused method argument: vertex_triangle_filtering_list_offsets
(ARG002)
116-116: Unused method argument: edge_filtering_list
(ARG002)
117-117: Unused method argument: edge_filtering_list_offsets
(ARG002)
254-254: Redefinition of unused set_collision_filter_list from line 242
(F811)
🔇 Additional comments (9)
newton/tests/test_collision_cloth.py (1)
385-411: init_model extension (color flag and record_triangle_contacting_vertices) looks soundThe added
record_triangle_contacting_verticesandcolorparameters preserve existing call sites (defaults unchanged) while enabling VBD-specific setup via coloring. The TriMeshCollisionDetector construction is still straightforward and consistent with previous usage.newton/_src/solvers/vbd/tri_mesh_collision.py (1)
345-393: Kernel wiring for min/max radii and filtering lists looks correctThe updated
vertex_triangle_collision_detectionandedge_edge_collision_detectionmethods correctly:
- zero/initialize collision buffers,
- pass
max_query_radiusandmin_query_radiusto the new geometry kernels,- forward the BVHs, offsets, buffer sizes, and filtering lists/offsets, and
- use
min_distance_filtering_ref_posfalling back toself.vertex_positionswhenNone.Assuming the geometry kernels’ signatures are as in the diff, the argument ordering and defaults look consistent with the tests and SolverVBD usage.
Also applies to: 396-422
newton/_src/geometry/kernels.py (2)
2346-2359: Binary search helper is correct and appropriate for CSR-style filter lists
_binary_search_contains_int32implements a standard lower‑bound search over the half‑open interval[start, end). The loop and termination condition look correct and are a good fit for probing sorted collision‑filter slices.
2603-2656: triangle_triangle_collision_detection_kernel implementation looks solidThe new
triangle_triangle_collision_detection_kernel:
- queries the BVH using each triangle’s AABB,
- skips neighboring triangles via vertex-adjacency checks,
- calls
wp.intersect_tri_trifor exact intersection, and- records intersecting triangle indices into pre-allocated buffers with proper overflow signaling via
TRI_TRI_COLLISION_BUFFER_OVERFLOW_INDEX.This aligns with how
TriMeshCollisionDetector.triangle_triangle_intersection_detectionsets up offsets and buffers, and should integrate cleanly with the existing resize flag mechanics.newton/_src/solvers/vbd/solver_vbd.py (5)
1830-1848: LGTM - CSR conversion utilities are correctly implemented.The CSR helper functions are well-structured. The
_set_to_csrfunction correctly computes offsets via cumulative sum and populates the flat array.
1850-1863: LGTM - One-ring computation is correct.The function correctly extracts one-ring neighbors by filtering incident edges (order >= 2) and collecting their endpoints.
2439-2451: LGTM - Clear parameter documentation.The documentation for the new filtering parameters is comprehensive and includes helpful warnings about performance implications.
2520-2527: LGTM - Filtering list computation and registration is correct.The collision filtering lists are computed and properly registered with the collision detector.
2673-2721: Note: Filtering lists are computed once at initialization.The collision filtering lists are computed during
__init__and remain static throughout the simulation. This is appropriate for:
- Topological filtering (mesh connectivity doesn't change)
- Rest-shape distance filtering (if rest shape is truly static)
However, ensure that
self.rest_shape(line 2494) remains unchanged during simulation, otherwise distance-based filtering may become inconsistent.
Signed-off-by: Anka Chen <[email protected]>
AnkaChan
force-pushed
the
vbd_collision_filtering
branch
from
8ae7d27 to
e5c3019
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/geometry/kernels.py (1)
2243-2345: Remove the old kernel definition.The original
vertex_triangle_collision_detection_kernel(lines 2243–2345) is redefined at line 2363 with an extended signature. The old version is now dead code and should be removed to avoid confusion and eliminate the Ruff F811 warning.Apply this diff:
-@wp.kernel -def vertex_triangle_collision_detection_kernel( - query_radius: float, - bvh_id: wp.uint64, - pos: wp.array(dtype=wp.vec3), - tri_indices: wp.array(dtype=wp.int32, ndim=2), - vertex_colliding_triangles_offsets: wp.array(dtype=wp.int32), - vertex_colliding_triangles_buffer_sizes: wp.array(dtype=wp.int32), - triangle_colliding_vertices_offsets: wp.array(dtype=wp.int32), - triangle_colliding_vertices_buffer_sizes: wp.array(dtype=wp.int32), - # outputs - vertex_colliding_triangles: wp.array(dtype=wp.int32), - vertex_colliding_triangles_count: wp.array(dtype=wp.int32), - vertex_colliding_triangles_min_dist: wp.array(dtype=float), - triangle_colliding_vertices: wp.array(dtype=wp.int32), - triangle_colliding_vertices_count: wp.array(dtype=wp.int32), - triangle_colliding_vertices_min_dist: wp.array(dtype=float), - resize_flags: wp.array(dtype=wp.int32), -): - """ - This function applies discrete collision detection between vertices and triangles. It uses pre-allocated spaces to - record the collision data. This collision detector works both ways, i.e., it records vertices' colliding triangles to - `vertex_colliding_triangles`, and records each triangles colliding vertices to `triangle_colliding_vertices`. - - This function assumes that all the vertices are on triangles, and can be indexed from the pos argument. - - Note: - - The collision data buffer is pre-allocated and cannot be changed during collision detection, therefore, the space - may not be enough. If the space is not enough to record all the collision information, the function will set a - certain element in resized_flag to be true. The user can reallocate the buffer based on vertex_colliding_triangles_count - and vertex_colliding_triangles_count. - - Attributes: - bvh_id (int): the bvh id you want to collide with - query_radius (float): the contact radius. vertex-triangle pairs whose distance are less than this will get detected - pos (array): positions of all the vertices that make up triangles - vertex_colliding_triangles (array): flattened buffer of vertices' collision triangles - vertex_colliding_triangles_count (array): number of triangles each vertex collides - vertex_colliding_triangles_offsets (array): where each vertex' collision buffer starts - vertex_colliding_triangles_buffer_sizes (array): size of each vertex' collision buffer, will be modified if resizing is needed - vertex_colliding_triangles_min_dist (array): each vertex' min distance to all (non-neighbor) triangles - triangle_colliding_vertices (array): positions of all the triangles' collision vertices, every two elements - records the vertex index and a triangle index it collides to - triangle_colliding_vertices_count (array): number of triangles each vertex collides - triangle_colliding_vertices_offsets (array): where each triangle's collision buffer starts - triangle_colliding_vertices_buffer_sizes (array): size of each triangle's collision buffer, will be modified if resizing is needed - triangle_colliding_vertices_min_dist (array): each triangle's min distance to all (non-self) vertices - resized_flag (array): size == 3, (vertex_buffer_resize_required, triangle_buffer_resize_required, edge_buffer_resize_required) - """ - - v_index = wp.tid() - v = pos[v_index] - vertex_buffer_offset = vertex_colliding_triangles_offsets[v_index] - vertex_buffer_size = vertex_colliding_triangles_offsets[v_index + 1] - vertex_buffer_offset - - lower = wp.vec3(v[0] - query_radius, v[1] - query_radius, v[2] - query_radius) - upper = wp.vec3(v[0] + query_radius, v[1] + query_radius, v[2] + query_radius) - - query = wp.bvh_query_aabb(bvh_id, lower, upper) - - tri_index = wp.int32(0) - vertex_num_collisions = wp.int32(0) - min_dis_to_tris = query_radius - while wp.bvh_query_next(query, tri_index): - t1 = tri_indices[tri_index, 0] - t2 = tri_indices[tri_index, 1] - t3 = tri_indices[tri_index, 2] - if vertex_adjacent_to_triangle(v_index, t1, t2, t3): - continue - - u1 = pos[t1] - u2 = pos[t2] - u3 = pos[t3] - - closest_p, _bary, _feature_type = triangle_closest_point(u1, u2, u3, v) - - dist = wp.length(closest_p - v) - - if dist < query_radius: - # record v-f collision to vertex - min_dis_to_tris = wp.min(min_dis_to_tris, dist) - if vertex_num_collisions < vertex_buffer_size: - vertex_colliding_triangles[2 * (vertex_buffer_offset + vertex_num_collisions)] = v_index - vertex_colliding_triangles[2 * (vertex_buffer_offset + vertex_num_collisions) + 1] = tri_index - else: - resize_flags[VERTEX_COLLISION_BUFFER_OVERFLOW_INDEX] = 1 - - vertex_num_collisions = vertex_num_collisions + 1 - - wp.atomic_min(triangle_colliding_vertices_min_dist, tri_index, dist) - tri_buffer_size = triangle_colliding_vertices_buffer_sizes[tri_index] - tri_num_collisions = wp.atomic_add(triangle_colliding_vertices_count, tri_index, 1) - - if tri_num_collisions < tri_buffer_size: - tri_buffer_offset = triangle_colliding_vertices_offsets[tri_index] - # record v-f collision to triangle - triangle_colliding_vertices[tri_buffer_offset + tri_num_collisions] = v_index - else: - resize_flags[TRI_COLLISION_BUFFER_OVERFLOW_INDEX] = 1 - - vertex_colliding_triangles_count[v_index] = vertex_num_collisions - vertex_colliding_triangles_min_dist[v_index] = min_dis_to_tris -
♻️ Duplicate comments (6)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
248-258: Remove duplicateset_collision_filter_listmethod definition.This method is defined twice (first at line 242, then again at line 248). The second definition shadows the first. Remove the duplicate and keep the multiline version at line 242.
Apply this diff:
def set_collision_filter_list(self, vertex_triangle_filtering_list, vertex_triangle_filtering_list_offsets, edge_filtering_list, edge_filtering_list_offsets ): self.vertex_triangle_filtering_list = vertex_triangle_filtering_list self.vertex_triangle_filtering_list_offsets = vertex_triangle_filtering_list_offsets self.edge_filtering_list = edge_filtering_list self.edge_filtering_list_offsets = edge_filtering_list_offsets - - def set_collision_filter_list(self, vertex_triangle_filtering_list, vertex_triangle_filtering_list_offsets, edge_filtering_list, edge_filtering_list_offsets): - self.vertex_triangle_filtering_list = vertex_triangle_filtering_list - self.vertex_triangle_filtering_list_offsets = vertex_triangle_filtering_list_offsets - self.edge_filtering_list = edge_filtering_list - self.edge_filtering_list_offsets = edge_filtering_list_offsetsnewton/_src/solvers/vbd/solver_vbd.py (5)
1850-1880: N‑ring neighbor logic and docstrings are slightly inconsistent with behavior.Two minor points here:
leq_n_ring_verticesincludes the source vertex.
Forn >= 1,visitedalways containsv, so inbuild_vertex_n_ring_tris_collision_filterandbuild_edge_n_ring_edge_collision_filterthering_n_minus_1arrays for then > 2branch containv/{v1, v2}. You then filterw != v/w != v1 and w != v2inside the loops, but that’s redundant work and inconsistent with then == 2branch which usesone_ring_vertices(which never returns the source vertex).Docstrings no longer match return types.
Both builders’ docstrings still talk about returning CSR arrays (v_two_flat, v_two_offs), but the implementations now return Pythonlist[set[int]], with CSR conversion happening later via_set_to_csrincompute_contact_filtering_list.You can make behavior and docs consistent with a small refactor:
def build_vertex_n_ring_tris_collision_filter(...): - """ - For each vertex v, return ONLY triangles adjacent to v's one ring neighbor vertices. - Excludes triangles incident to v itself (dist 0). - Returns: - v_two_flat, v_two_offs: CSR of strict-2-ring triangle ids per vertex - """ + """ + Build per-vertex collision filter sets based on n-ring topological distance. + + For each vertex v, returns a set of triangle ids adjacent to vertices within + rings [1, n-1] of v, excluding triangles directly incident to v itself. + + Returns a list of sets of triangle ids, one set per vertex. + """ @@ - if n == 2: - ring_n_minus_1 = one_ring_vertices(v, edge_indices, v_adj_edges, v_adj_edges_offsets) - else: - ring_n_minus_1 = leq_n_ring_vertices(v, edge_indices, n-1, v_adj_edges, v_adj_edges_offsets) + ring_n_minus_1 = leq_n_ring_vertices(v, edge_indices, n - 1, v_adj_edges, v_adj_edges_offsets) + ring_n_minus_1 = ring_n_minus_1[ring_n_minus_1 != v] @@ - nei_tri_set = v_nei_tri_sets [v] - for w in ring_n_minus_1: - if w != v: - # preserve only the adjacent edge information, remove the order information - nei_tri_set.update(_csr_row(v_adj_faces, v_adj_faces_offsets, w)[::2]) + nei_tri_set = v_nei_tri_sets[v] + for w in ring_n_minus_1: + # preserve only the adjacent edge information, remove the order information + nei_tri_set.update(_csr_row(v_adj_faces, v_adj_faces_offsets, w)[::2])And similarly for the edge builder:
def build_edge_n_ring_edge_collision_filter(...): - """ - For each vertex v, return ONLY triangles adjacent to v's one ring neighbor vertices. - Excludes triangles incident to v itself (dist 0). - Returns: - v_two_flat, v_two_offs: CSR of strict-2-ring triangle ids per vertex - """ + """ + Build per-edge collision filter sets based on n-ring topological distance. + + For each edge e, returns a set of edge ids that should be excluded from + self-contact because they are adjacent to vertices within rings [1, n-1] + of e's endpoints, excluding edges incident to the endpoints themselves. + + Returns a list of sets of edge ids, one set per edge. + """ @@ - if n == 2: - ring_n_minus_1_v1 = one_ring_vertices(v1, edge_indices, v_adj_edges, v_adj_edges_offsets) - ring_n_minus_1_v2 = one_ring_vertices(v2, edge_indices, v_adj_edges, v_adj_edges_offsets) - else: - ring_n_minus_1_v1 = leq_n_ring_vertices(v1, edge_indices, n-1, v_adj_edges, v_adj_edges_offsets) - ring_n_minus_1_v2 = leq_n_ring_vertices(v2, edge_indices, n-1, v_adj_edges, v_adj_edges_offsets) + ring_n_minus_1_v1 = leq_n_ring_vertices(v1, edge_indices, n - 1, v_adj_edges, v_adj_edges_offsets) + ring_n_minus_1_v1 = ring_n_minus_1_v1[ring_n_minus_1_v1 != v1] + ring_n_minus_1_v2 = leq_n_ring_vertices(v2, edge_indices, n - 1, v_adj_edges, v_adj_edges_offsets) + ring_n_minus_1_v2 = ring_n_minus_1_v2[ring_n_minus_1_v2 != v2]Functionality today is correct, but this makes the intent clearer and removes small redundant checks.
Also applies to: 1881-1921, 1923-1972
1830-1848: Guard CSR/topology helpers against empty adjacency to avoid IndexError on degenerate meshes.
_csr_row()assumesoffshas length>= i+1. For meshes with no edges or no faces,compute_force_element_adjacency()setsv_adj_edges_offsets/v_adj_faces_offsetsto zero-length arrays (lines 2599–2601, 2633–2634, 2667–2669). Whentopological_contact_filter_threshold >= 2,compute_contact_filtering_list()always calls:
build_vertex_n_ring_tris_collision_filter(...), which calls_csr_row(v_adj_edges, v_adj_edges_offsets, v)and_csr_row(v_adj_faces, v_adj_faces_offsets, v),build_edge_n_ring_edge_collision_filter(...), which calls_csr_row(v_adj_edges, v_adj_edges_offsets, w),so on a degenerate mesh you’ll hit an
IndexErroras soon astopological_contact_filter_threshold >= 2.You can fix this at the call site by skipping the builders when the underlying adjacency is empty:
def compute_contact_filtering_list(self, external_vertex_contact_filtering_map, external_edge_contact_filtering_map): - v_tri_filter_sets = None - edge_edge_filter_sets = None - if self.topological_contact_filter_threshold >=2: - v_tri_filter_sets = build_vertex_n_ring_tris_collision_filter( - self.topological_contact_filter_threshold, - self.model.particle_count, - self.model.edge_indices.numpy(), - self.adjacency.v_adj_edges.numpy(), - self.adjacency.v_adj_edges_offsets.numpy(), - self.adjacency.v_adj_faces.numpy(), - self.adjacency.v_adj_faces_offsets.numpy(), - ) - - edge_edge_filter_sets = build_edge_n_ring_edge_collision_filter( - self.topological_contact_filter_threshold, - self.model.edge_indices.numpy(), - self.adjacency.v_adj_edges.numpy(), - self.adjacency.v_adj_edges_offsets.numpy(), - ) + v_tri_filter_sets = None + edge_edge_filter_sets = None + n = self.topological_contact_filter_threshold + + # Only build topological filters when adjacency CSR structures are non-empty. + if n >= 2: + if ( + self.adjacency.v_adj_faces_offsets.shape[0] > 0 + and self.adjacency.v_adj_edges_offsets.shape[0] > 0 + ): + v_tri_filter_sets = build_vertex_n_ring_tris_collision_filter( + n, + self.model.particle_count, + self.model.edge_indices.numpy(), + self.adjacency.v_adj_edges.numpy(), + self.adjacency.v_adj_edges_offsets.numpy(), + self.adjacency.v_adj_faces.numpy(), + self.adjacency.v_adj_faces_offsets.numpy(), + ) + + if ( + self.model.edge_count > 0 + and self.adjacency.v_adj_edges_offsets.shape[0] > 0 + ): + edge_edge_filter_sets = build_edge_n_ring_edge_collision_filter( + n, + self.model.edge_indices.numpy(), + self.adjacency.v_adj_edges.numpy(), + self.adjacency.v_adj_edges_offsets.numpy(), + )This keeps the builders from ever indexing into zero-length CSR structures; on meshes without edges or faces, you’ll simply get no topological filters rather than a hard crash.
Also applies to: 1881-1921, 1923-1972, 2585-2601, 2602-2634, 2667-2669, 2673-2721
2415-2418: Type hints for external filtering maps should usedict/Mapping, notmap.Using
mapinexternal_vertex_contact_filtering_map: map | None = None, external_edge_contact_filtering_map: map | None = None,is misleading:
mapis the built‑in function, not a type representing a mapping. From usage incompute_contact_filtering_list, these are dictionaries keyed by primitive id, with list/set values.Recommend tightening the signature to something like:
@@ - external_vertex_contact_filtering_map: map | None = None, - external_edge_contact_filtering_map: map | None = None, + external_vertex_contact_filtering_map: dict[int, set[int] | list[int]] | None = None, + external_edge_contact_filtering_map: dict[int, set[int] | list[int]] | None = None,(or
Mapping[int, Collection[int]]if you prefertyping-based annotations). This improves IDE/type-checker support and avoids confusion with the built-inmap.
2439-2445:rest_shape_contact_exclusion_radiusis stored and documented but never used in collision detection.You add and document
rest_shape_contact_exclusion_radiusandrest_shape(lines 2439–2445, 2480–2482), andTriMeshCollisionDetectorhas support formin_query_radius/min_distance_filtering_ref_pos, butcollision_detection_penetration_free()still calls:self.trimesh_collision_detector.vertex_triangle_collision_detection(self.self_contact_margin) self.trimesh_collision_detector.edge_edge_collision_detection(self.self_contact_margin)so rest-shape distance filtering is effectively disabled.
To actually enable the feature as described in the PR:
def collision_detection_penetration_free(self, current_state: State, dt: float): self.trimesh_collision_detector.refit(current_state.particle_q) - self.trimesh_collision_detector.vertex_triangle_collision_detection(self.self_contact_margin) - self.trimesh_collision_detector.edge_edge_collision_detection(self.self_contact_margin) + self.trimesh_collision_detector.vertex_triangle_collision_detection( + self.self_contact_margin, + min_query_radius=self.rest_shape_contact_exclusion_radius, + min_distance_filtering_ref_pos=self.rest_shape, + ) + self.trimesh_collision_detector.edge_edge_collision_detection( + self.self_contact_margin, + min_query_radius=self.rest_shape_contact_exclusion_radius, + min_distance_filtering_ref_pos=self.rest_shape, + )With this,
rest_shape_contact_exclusion_radius=0.0preserves existing behavior; positive values enforce the intended rest-shape distance-based pruning.Also applies to: 2480-2482, 2673-2721, 3068-3070
2480-2482: Makerest_shapean immutable snapshot instead of an alias ofmodel.particle_q.Currently:
self.topological_contact_filter_threshold = topological_contact_filter_threshold self.rest_shape_contact_exclusion_radius = rest_shape_contact_exclusion_radius ... self.rest_shape = model.particle_q
self.rest_shapebecomes just another reference tomodel.particle_q, so any subsequent deformation of the model also mutates the “rest” configuration used for distance filtering, defeating the purpose of a rest-shape radius.Given how other buffers are handled in this class, a snapshot copy is more appropriate:
@@ - self.topological_contact_filter_threshold = topological_contact_filter_threshold - self.rest_shape_contact_exclusion_radius = rest_shape_contact_exclusion_radius + self.topological_contact_filter_threshold = topological_contact_filter_threshold + self.rest_shape_contact_exclusion_radius = rest_shape_contact_exclusion_radius @@ - self.rest_shape = model.particle_q + self.rest_shape = wp.zeros_like(model.particle_q, device=self.device) + self.rest_shape.assign(model.particle_q)This makes
rest_shapeindependent of later changes tomodel.particle_qand matches the “rest configuration” semantics described in the docstring.Also applies to: 2494-2494
🧹 Nitpick comments (5)
newton/_src/geometry/kernels.py (1)
2363-2370: Consider removing unused parameter.
vertex_colliding_triangles_buffer_sizes(line 2370) is not referenced in the kernel body. If it's not needed for future compatibility, remove it to simplify the signature.newton/tests/test_collision_cloth.py (3)
875-875: Remove unused variable.
min_distis assigned but never used in the validation loop. Remove the assignment to clean up the code.Apply this diff:
num_cols = edge_colliding_edges_count[e0_index] offset = edge_colliding_edges_offsets[e0_index] - min_dist = edge_colliding_edges_min_dist[e0_index] for col in range(edge_colliding_edges_buffer_sizes[e0_index]):
918-918: Prefix unused return value with underscore.The
collision_detectorreturned frominit_modelis not used in this test. Prefix it with an underscore to indicate it's intentionally unused.Apply this diff:
- model, collision_detector = init_model(vertices, faces, device, False, True) + model, _collision_detector = init_model(vertices, faces, device, False, True)
945-946: Remove unused adjacency variables.
v_adj_facesandv_adj_faces_offsetsare fetched but never used in the test. Remove these assignments.Apply this diff:
v_adj_edges = vbd.adjacency.v_adj_edges.numpy() v_adj_edges_offsets = vbd.adjacency.v_adj_edges_offsets.numpy() - v_adj_faces = vbd.adjacency.v_adj_faces.numpy() - v_adj_faces_offsets = vbd.adjacency.v_adj_faces_offsets.numpy()newton/_src/solvers/vbd/solver_vbd.py (1)
2520-2527: Collision filter CSR wiring looks correct but relies onTriMeshCollisionDetectorhandlingNonegracefully.The new
compute_contact_filtering_listbuilds CSR arrays via_set_to_csrand then calls:self.trimesh_collision_detector.set_collision_filter_list( self.vertex_triangle_contact_filtering_list, self.vertex_triangle_contact_filtering_list_offsets, self.edge_edge_contact_filtering_list, self.edge_edge_contact_filtering_list_offsets, )When no topological or external filters are active, all four attributes remain
None. The setter intri_mesh_collision.pysimply assigns these attributes, so correctness here depends on the downstream collision detection kernels checking forNonebefore using the arrays.Action items:
- Double-check that all uses of
vertex_triangle_filtering_list/edge_filtering_listinTriMeshCollisionDetectorguard againstNone(e.g.,if self.vertex_triangle_filtering_list is not None:before indexing).- If not, either convert the “no filter” case to zero-length arrays instead of
None, or add those guards in the detector.From the Solver side, the CSR build and wiring are otherwise sound.
Also applies to: 2673-2721
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/geometry/kernels.py(8 hunks)newton/_src/solvers/vbd/solver_vbd.py(8 hunks)newton/_src/solvers/vbd/tri_mesh_collision.py(5 hunks)newton/tests/test_collision_cloth.py(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/tests/test_collision_cloth.pynewton/_src/solvers/vbd/solver_vbd.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/tests/test_collision_cloth.pynewton/_src/geometry/kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/_src/solvers/vbd/tri_mesh_collision.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/kernels.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/_src/solvers/vbd/tri_mesh_collision.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/solvers/vbd/solver_vbd.py
📚 Learning: 2025-08-25T20:10:59.536Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_franka.py:0-0
Timestamp: 2025-08-25T20:10:59.536Z
Learning: In Newton's IK examples, when creating solver arrays with `wp.array(source_array, shape=new_shape)`, this creates a view into the same underlying memory rather than a copy. This means updates to the reshaped array are automatically reflected in the original array without needing explicit copy operations like `wp.copy()`.
Applied to files:
newton/_src/solvers/vbd/solver_vbd.py
🧬 Code graph analysis (3)
newton/tests/test_collision_cloth.py (5)
newton/_src/solvers/vbd/solver_vbd.py (2)
SolverVBD(2376-3233)leq_n_ring_vertices(1866-1879)newton/_src/sim/builder.py (4)
color(4737-4778)particle_count(889-893)tri_count(896-900)edge_count(910-914)newton/_src/geometry/kernels.py (1)
triangle_closest_point(111-175)newton/_src/solvers/vbd/tri_mesh_collision.py (3)
refit(320-325)vertex_triangle_collision_detection(345-393)edge_edge_collision_detection(396-422)newton/tests/unittest_utils.py (2)
get_test_devices(98-132)add_function_test(312-331)
newton/_src/solvers/vbd/solver_vbd.py (2)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
set_collision_filter_list(248-258)newton/_src/sim/builder.py (2)
particle_count(889-893)edge_count(910-914)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
newton/_src/geometry/kernels.py (4)
init_triangle_collision_data_kernel(2225-2239)vertex_triangle_collision_detection_kernel(2243-2344)vertex_triangle_collision_detection_kernel(2363-2494)edge_colliding_edges_detection_kernel(2498-2599)
🪛 Ruff (0.14.5)
newton/tests/test_collision_cloth.py
875-875: Local variable min_dist is assigned to but never used
Remove assignment to unused variable min_dist
(F841)
918-918: Unpacked variable collision_detector is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
945-945: Local variable v_adj_faces is assigned to but never used
Remove assignment to unused variable v_adj_faces
(F841)
946-946: Local variable v_adj_faces_offsets is assigned to but never used
Remove assignment to unused variable v_adj_faces_offsets
(F841)
newton/_src/geometry/kernels.py
2363-2363: Redefinition of unused vertex_triangle_collision_detection_kernel from line 2243
(F811)
2370-2370: Unused function argument: vertex_colliding_triangles_buffer_sizes
(ARG001)
2505-2505: Unused function argument: edge_colliding_edges_buffer_sizes
(ARG001)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (9)
newton/_src/geometry/kernels.py (2)
346-360: LGTM!The binary search helper is correctly implemented with proper half-open interval semantics
[start, end)and boundary checks.
2481-2491: LGTM!The optional triangle buffer recording (guarded by
if triangle_colliding_vertices:) correctly allows unidirectional collision detection when the reverse mapping is not needed.newton/_src/solvers/vbd/tri_mesh_collision.py (2)
345-393: LGTM!The updated kernel invocation correctly propagates filtering lists, max/min query radii, and reference positions. The fallback to
self.vertex_positionswhenmin_distance_filtering_ref_posis None (line 379) ensures safe defaults.
396-422: LGTM!Edge-edge collision detection consistently applies the same filtering and distance-based pruning pattern as the vertex-triangle path.
newton/tests/test_collision_cloth.py (4)
33-33: LGTM!Import of
SolverVBDis necessary for the newtest_collision_filteringfunction.
385-411: LGTM!The
colorparameter enables testing VBD-based filtering, which requires particle coloring. The implementation correctly callsbuilder.color()before finalization when requested.
801-848: LGTM!The validation kernel correctly verifies that detected collisions respect both the max distance threshold (on current positions) and the min distance threshold (on reference positions).
913-1072: LGTM!The new test thoroughly exercises collision filtering functionality:
- Topological distance filtering (n-ring neighborhoods)
- External explicit filter maps
- Rest-shape distance-based filtering
- Validates CSR format (sorting and offsets)
- Confirms detected collisions respect both max and min distance thresholds
newton/_src/solvers/vbd/solver_vbd.py (1)
577-580: Normal contact damping addition looks consistent with existing patterns.The new damping terms for body–particle contact reuse the same “H * displacement” structure as springs and self-contact, and the sign choice (
body_contact_force - damping_hessian * dx) is consistent with usingdx = pos - pos_prev. I don’t see correctness or stability issues here.
Signed-off-by: Anka Chen <[email protected]>
AnkaChan
force-pushed
the
vbd_collision_filtering
branch
from
ae1ee0b to
57f8d7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
249-261: Remove duplicateset_collision_filter_listmethod definition.This duplicate method definition was already flagged in previous review comments. Keep only the multiline version (lines 249-252) and remove the duplicate at lines 254-260.
newton/_src/solvers/vbd/solver_vbd.py (1)
2502-2502: Critical:rest_shapemust be a copy, not a reference.Line 2502 assigns
self.rest_shape = model.particle_qas a direct reference. Ifmodel.particle_qis modified after initialization (which is typical during simulation),rest_shapewill reflect those changes instead of preserving the initial rest configuration. This breaks the intended semantics of rest-shape distance filtering described in the docstring and PR objectives.Apply this diff to create an independent copy:
- self.rest_shape = model.particle_q + self.rest_shape = wp.clone(model.particle_q)Or use the established pattern elsewhere in the codebase:
- self.rest_shape = model.particle_q + self.rest_shape = wp.zeros_like(model.particle_q, device=self.device) + self.rest_shape.assign(model.particle_q)
🧹 Nitpick comments (3)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
347-349: Clarify fallback behavior formin_distance_filtering_ref_pos.When
min_distance_filtering_ref_posisNone, the code defaults toself.vertex_positions(lines 383, 416). If a user providesmin_query_radius > 0.0without specifying a reference position, filtering will use current positions instead of rest shape positions, which may not be the intended behavior.Consider either:
- Raising a
ValueErrorifmin_query_radius > 0.0butmin_distance_filtering_ref_pos is None- Adding clear documentation explaining this fallback behavior
Example check to add at the start of both methods:
if min_query_radius > 0.0 and min_distance_filtering_ref_pos is None: raise ValueError( "min_distance_filtering_ref_pos must be provided when min_query_radius > 0.0. " "Typically, you should pass the rest shape positions for distance-based filtering." )Also applies to: 383-383, 399-401, 416-416
newton/_src/solvers/vbd/solver_vbd.py (2)
1888-1905: Update docstring: function returns list of sets, not CSR arrays.The docstring at lines 1897–1901 states the function returns
v_two_flat, v_two_offs: CSR of strict-2-ring triangle ids per vertex, but line 1927 actually returnsv_nei_tri_sets, a list of Python sets. Conversion to CSR format happens later incompute_contact_filtering_listvia_set_to_csr(around line 2723).Revise the docstring:
""" - For each vertex v, return ONLY triangles adjacent to v's one ring neighbor vertices. - Excludes triangles incident to v itself (dist 0). + For each vertex v, return a set of triangles adjacent to vertices within the (n-1)-ring, + excluding triangles incident to v itself. Returns: - v_two_flat, v_two_offs: CSR of strict-2-ring triangle ids per vertex + list[set[int]]: List of sets, one per vertex, containing triangle IDs to filter. """
1930-1944: Update docstring: function returns list of sets, not CSR arrays.The docstring at lines 1936–1940 describes returning CSR arrays, but line 1979 returns
edge_nei_edge_sets, a list of Python sets. CSR conversion occurs later via_set_to_csr(around line 2737).Revise the docstring:
""" - For each vertex v, return ONLY triangles adjacent to v's one ring neighbor vertices. - Excludes triangles incident to v itself (dist 0). + For each edge, return a set of edges adjacent to vertices within the (n-1)-ring of both endpoints, + excluding edges incident to the edge's endpoints. Returns: - v_two_flat, v_two_offs: CSR of strict-2-ring triangle ids per vertex + list[set[int]]: List of sets, one per edge, containing edge IDs to filter. """
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/geometry/kernels.py(7 hunks)newton/_src/solvers/vbd/solver_vbd.py(9 hunks)newton/_src/solvers/vbd/tri_mesh_collision.py(5 hunks)newton/tests/test_collision_cloth.py(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/tests/test_collision_cloth.pynewton/_src/geometry/kernels.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/tests/test_collision_cloth.pynewton/_src/geometry/kernels.pynewton/_src/solvers/vbd/tri_mesh_collision.pynewton/_src/solvers/vbd/solver_vbd.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/tests/test_collision_cloth.pynewton/_src/geometry/kernels.pynewton/_src/solvers/vbd/tri_mesh_collision.pynewton/_src/solvers/vbd/solver_vbd.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/_src/solvers/vbd/solver_vbd.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/solvers/vbd/solver_vbd.py
📚 Learning: 2025-08-25T20:10:59.536Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_franka.py:0-0
Timestamp: 2025-08-25T20:10:59.536Z
Learning: In Newton's IK examples, when creating solver arrays with `wp.array(source_array, shape=new_shape)`, this creates a view into the same underlying memory rather than a copy. This means updates to the reshaped array are automatically reflected in the original array without needing explicit copy operations like `wp.copy()`.
Applied to files:
newton/_src/solvers/vbd/solver_vbd.py
🧬 Code graph analysis (3)
newton/tests/test_collision_cloth.py (3)
newton/_src/solvers/vbd/solver_vbd.py (2)
leq_n_ring_vertices(1870-1885)SolverVBD(2384-3265)newton/_src/solvers/vbd/tri_mesh_collision.py (4)
TriMeshCollisionDetector(102-464)refit(322-327)vertex_triangle_collision_detection(347-397)edge_edge_collision_detection(399-427)newton/_src/geometry/kernels.py (1)
triangle_closest_point(111-175)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
newton/_src/geometry/kernels.py (3)
init_triangle_collision_data_kernel(2225-2239)vertex_triangle_collision_detection_kernel(2258-2391)edge_colliding_edges_detection_kernel(2395-2500)
newton/_src/solvers/vbd/solver_vbd.py (2)
newton/_src/solvers/vbd/tri_mesh_collision.py (3)
set_collision_filter_list(249-260)vertex_triangle_collision_detection(347-397)edge_edge_collision_detection(399-427)newton/_src/sim/builder.py (3)
tri_count(896-900)particle_count(889-893)edge_count(910-914)
🪛 Ruff (0.14.5)
newton/tests/test_collision_cloth.py
863-863: Unused function argument: edge_colliding_edges_min_dist
(ARG001)
newton/_src/geometry/kernels.py
2265-2265: Unused function argument: vertex_colliding_triangles_buffer_sizes
(ARG001)
2402-2402: Unused function argument: edge_colliding_edges_buffer_sizes
(ARG001)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (7)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
110-117: LGTM: Filtering list parameters properly added to constructor.The new filtering list parameters are correctly initialized and stored as instance variables, enabling external and topological collision filtering.
Also applies to: 136-141
newton/_src/geometry/kernels.py (3)
2242-2255: LGTM: Binary search helper correctly implemented.The
_binary_search_contains_int32function correctly implements binary search with exclusive end index convention[start, end). The fast-fail optimization in the calling code (checking first/last elements) helps reduce unnecessary searches.
2335-2345: LGTM: Filtering logic correctly implemented.Both the topological filtering (via binary search on filter lists) and distance-based filtering (using reference positions) are correctly implemented. The filtering properly skips collisions that should be excluded.
Also applies to: 2355-2365
2264-2265: The parameters are actively used throughout the collision detection system and should not be removed.The original analysis was incomplete. Both
vertex_colliding_triangles_buffer_sizesandedge_colliding_edges_buffer_sizesare extensively used:
- Loop bounds: Used in
whileconditions (solver_vbd.py:1690, 1732) andforloops (test_collision_cloth.py:355, 875)- Bounds checking: Conditional checks against
.shape[0](solver_vbd.py:1685, 1728)- Min operations: Capping actual collision counts with pre-allocated sizes (tri_mesh_collision.py:57, 87)
- Resizing management: Tracked for buffer resize operations (tri_mesh_collision.py:171, 219, 181, 226)
These parameters track pre-allocated collision buffer capacities and are essential for the collision detection pipeline to manage dynamic resizing and prevent buffer overruns.
Likely an incorrect or invalid review comment.
newton/tests/test_collision_cloth.py (3)
31-31: LGTM: New imports properly added.The imports of
leq_n_ring_verticesandSolverVBDare necessary for the new collision filtering test and are correctly placed.Also applies to: 33-33
800-848: LGTM: Distance filtering validation kernels correctly implemented.Both validation kernels properly verify:
- Collisions are within
max_query_radiusin the current configuration- Reference distances are
>= min_query_radiusfor proper filtering- Vertex/edge indices match expected values
The unused
edge_colliding_edges_min_distparameter (line 863) is acceptable—validation kernels don't need to use all outputs.Also applies to: 850-914
916-1065: LGTM: Comprehensive collision filtering test coverage.The
test_collision_filteringtest thoroughly validates:
- Filter list sorting for efficient binary search
- Preservation of external filter maps (vertex-triangle and edge-edge)
- Topological distance filtering at multiple ring distances (1-3)
- Distance-based filtering using rest shape reference positions (lines 1025-1027)
The test correctly scales particle positions (line 1023) to create distance between current and rest configurations, then validates that filtering works as expected.
Also applies to: 1078-1078
| def compute_contact_filtering_list( | ||
| self, external_vertex_contact_filtering_map, external_edge_contact_filtering_map | ||
| ): | ||
| if self.model.tri_count: | ||
| v_tri_filter_sets = None | ||
| edge_edge_filter_sets = None | ||
| if self.topological_contact_filter_threshold >= 2: | ||
| v_tri_filter_sets = build_vertex_n_ring_tris_collision_filter( | ||
| self.topological_contact_filter_threshold, | ||
| self.model.particle_count, | ||
| self.model.edge_indices.numpy(), | ||
| self.adjacency.v_adj_edges.numpy(), | ||
| self.adjacency.v_adj_edges_offsets.numpy(), | ||
| self.adjacency.v_adj_faces.numpy(), | ||
| self.adjacency.v_adj_faces_offsets.numpy(), | ||
| ) | ||
|
|
||
| edge_edge_filter_sets = build_edge_n_ring_edge_collision_filter( | ||
| self.topological_contact_filter_threshold, | ||
| self.model.edge_indices.numpy(), | ||
| self.adjacency.v_adj_edges.numpy(), | ||
| self.adjacency.v_adj_edges_offsets.numpy(), | ||
| ) |
There was a problem hiding this comment.
Guard against IndexError when adjacency structures are empty.
When topological_contact_filter_threshold >= 2, lines 2690 and 2700 call build_vertex_n_ring_tris_collision_filter and build_edge_n_ring_edge_collision_filter unconditionally. However, these functions invoke _csr_row(vals, offs, i) which slices offs[i:i+1]. If the mesh has no faces or no edges, compute_force_element_adjacency creates zero-length v_adj_faces_offsets or v_adj_edges_offsets arrays (lines 2609-2610, 2643-2644), causing an IndexError when _csr_row tries to index into them.
Add guards before calling the build functions:
if self.topological_contact_filter_threshold >= 2:
+ if self.adjacency.v_adj_faces_offsets.size > 0:
v_tri_filter_sets = build_vertex_n_ring_tris_collision_filter(
self.topological_contact_filter_threshold,
self.model.particle_count,
self.model.edge_indices.numpy(),
self.adjacency.v_adj_edges.numpy(),
self.adjacency.v_adj_edges_offsets.numpy(),
self.adjacency.v_adj_faces.numpy(),
self.adjacency.v_adj_faces_offsets.numpy(),
)
+ if self.adjacency.v_adj_edges_offsets.size > 0:
edge_edge_filter_sets = build_edge_n_ring_edge_collision_filter(
self.topological_contact_filter_threshold,
self.model.edge_indices.numpy(),
self.adjacency.v_adj_edges.numpy(),
self.adjacency.v_adj_edges_offsets.numpy(),
)Signed-off-by: Anka Chen <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
136-140: Consider validating filtering list inputs.Per the PR objectives, filtering lists must be sorted for bisection search, and offsets should follow the CSR pattern (length = num_entities + 1). Adding validation would catch configuration errors early.
Example validation in
set_collision_filter_listor__init__:if vertex_triangle_filtering_list is not None: assert vertex_triangle_filtering_list_offsets is not None, \ "vertex_triangle_filtering_list_offsets required when vertex_triangle_filtering_list is provided" assert vertex_triangle_filtering_list_offsets.size == self.model.particle_count + 1, \ "vertex_triangle_filtering_list_offsets must have size = particle_count + 1" if edge_filtering_list is not None: assert edge_filtering_list_offsets is not None, \ "edge_filtering_list_offsets required when edge_filtering_list is provided" assert edge_filtering_list_offsets.size == self.model.edge_count + 1, \ "edge_filtering_list_offsets must have size = edge_count + 1"
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/solvers/vbd/tri_mesh_collision.py(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/vbd/tri_mesh_collision.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/vbd/tri_mesh_collision.py
🧬 Code graph analysis (1)
newton/_src/solvers/vbd/tri_mesh_collision.py (1)
newton/_src/geometry/kernels.py (3)
init_triangle_collision_data_kernel(2225-2239)vertex_triangle_collision_detection_kernel(2258-2391)edge_colliding_edges_detection_kernel(2395-2500)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (4)
newton/_src/solvers/vbd/tri_mesh_collision.py (4)
110-111: LGTM! Filtering parameters added appropriately.The new optional filtering list parameters follow a consistent naming convention and have sensible defaults that maintain backward compatibility.
Also applies to: 116-117
249-260: LGTM! Runtime filtering list configuration enabled.The method correctly updates filtering lists at runtime. Same validation consideration as the constructor applies here: consider verifying that lists are provided with their corresponding offsets.
399-427: LGTM! Edge-edge collision detection updated consistently.The method correctly mirrors the vertex-triangle collision detection changes, with the same filtering and min-distance query patterns. The fallback for
min_distance_filtering_ref_pos(line 416) is appropriate.
347-397: Warp correctly handles None for optional array parameters—review concern is resolved.Verification confirms the kernel implementation properly uses Warp's idiomatic pattern for optional arrays. Line 2335 checks
vertex_triangle_filtering_listbefore use, line 2355 checksmin_distance_filtering_ref_pos, and line 2378 checkstriangle_colliding_verticesbefore accessing its associated offsets and buffer sizes (lines 2380, 2384). All useif array:checks rather than explicitNonecomparisons, which is the correct Warp pattern. The calling code properly passesNonewhen these arrays are not needed.
|
|
||
|
|
||
| @wp.func | ||
| def _binary_search_contains_int32(sorted_arr: wp.array(dtype=wp.int32), start: int, end: int, key: int) -> bool: |
There was a problem hiding this comment.
Can you use binary_search from broad_phase_common.py?
| Note: | ||
| The collision data buffer is pre-allocated and cannot be changed during collision detection, therefore, the space | ||
| The collision date buffer is pre-allocated and cannot be changed during collision detection, therefore, the space |
| triangle_colliding_vertices_buffer_sizes (array): size of each triangle's collision buffer, will be modified if resizing is needed | ||
| min_distance_filtering_ref_pos (array): the position that minimal collision distance evaluation uses. | ||
| vertex_colliding_triangles (array): flattened buffer of vertices' collision triangles | ||
| vertex_colliding_triangles_count (array): number of triangles each vertex collides |
There was a problem hiding this comment.
... each vertex collides with
| return nbrs[nbrs != v] | ||
|
|
||
|
|
||
| def leq_n_ring_vertices( |
There was a problem hiding this comment.
Can you add some comments to all the functions you are adding here so we know what they do?
2 participants
Description
Added documentation for new self-contact filtering parameters in SolverVBD, including topological threshold, rest-shape exclusion radius, and external filtering maps. Including:
topological_contact_filter_thresholdthat filters collision using topological distance.rest_shape_contact_exclusion_radiusthat filters collision using Euclidean distance on rest shape.Those features are implemented by adding features to
TrimeshCollisionDetecor.First
TrimeshCollisionDetecortakesvertex_triangle_filtering_listandedge_filtering_listto support explicit filtering. those lists are all sorted for each primitive to allow fast bisection search.Second each collision detection function takes two extra arguments
min_query_radiusandmin_distance_filtering_ref_pos.The minimal query distance is evaluated based on a user-provided position called
min_distance_filtering_ref_pos. Therefore the collision detector don't need to store the rest shape and stay decouple from the simulation.Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Tests