Opt-in support for contact matching - simplifies solver warm starting#2446
Opt-in support for contact matching - simplifies solver warm starting#2446nvtw wants to merge 63 commits intonewton-physics:mainfrom
Conversation
…vs mesh collisions
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Nice feature — well-scoped opt-in with strong invariant tests. The scratch-buffer reuse (point0/normal between sort_full and next-frame match) is genuinely clever and well-documented, and the deterministic binary-search-plus-verify shape is the right choice (O(n log n), graph-capture-safe). Graph-capture discipline throughout (preallocated buffers, dim=self._capacity launches, writing MATCH_NOT_FOUND to inactive slots so the gather-permuted result is well-defined on trailing capacity).
A few questions inline — mostly API-shape / doc-clarity, nothing blocking. One reminder: mergeStateStatus: DIRTY (40 ahead / 22 behind upstream/main), so you'll want to rebase before merge.
Independent of the inline threads, also worth engaging with:
- camevor's API-shape question: should
rigid_contact_match_indexbe an extended contact attribute (EXTENDED_ATTRIBUTES+requested_attributes={"match_index"}like"force"), and shouldnew/broken_contact_indiceslive onContactswhencontact_report=Truesocontact_matcherstays internal? The current split (match_index on Contacts, index lists on ContactMatcher) is asymmetric, and surfacingpipeline.contact_matcher.new_contact_indicesleaks a lot of internal state (_prev_sorted_keys,_prev_was_matched,_prev_countare implementation details). Worth a discussion before locking the public shape. - jumyungc's
save_sorted_statevsbuild_reportordering question (line 266): the current order is correct —save_sorted_stateoverwrites_prev_countwith the new count, and_collect_broken_contacts_kernelneeds the old_prev_countto know how many_prev_was_matchedslots to scan. There's a good comment at collide.py:1020-1021 explaining this. Could you reply with that rationale and lift a shorter form into theContactMatcherclass docstring (under "typical per-frame call sequence"), so the ordering constraint is visible from the class API? - Key-uniqueness invariant: the matcher relies on
sort_key = (shape_a, shape_b, sub_key)being unique per active contact.make_contact_sort_keysilently masks overflow (mesh-triangle contacts drop to 19 effective bits → ~524K triangles after the<<3multi-contact expansion). In scenes exceeding those bit budgets two new contacts could map to the same key, the binary search lands on the same old range, both pick the samebest_idx→ duplicatematch_indexvalues plus anatomic_maxonprev_was_matchedconflating them. The invariant is inherited from the sorter, fine to treat as out of scope — but worth a one-liner in the module docstring so the next reader knows whymatch_indexis assumed unique. - Broken-on-both-sides test missing:
test_broken_pos_threshold_all_contactsverifies the new side gets-2but not that the same old contact appears inbroken_contact_indices. Combined assertion would close the most interesting semantic gap. has_report: intis the codebase's int-as-bool convention; no change needed — noted for future cleanup when Warp gains native struct-field bools.
| contact_report: bool = False, | ||
| device: Devicelike = None, | ||
| ): | ||
| with wp.ScopedDevice(device): |
There was a problem hiding this comment.
Q on CPU support: all tests use get_cuda_test_devices(), and the sorter depends on radix_sort_pairs (CUDA-only in practice), but the matcher's own kernels don't depend on anything CUDA-specific. Is CPU intentionally unsupported, or just inherited from the sorter? If intentional, a runtime check here (when device is CPU) would be friendlier than a later kernel crash — something like:
if not wp.get_device(device).is_cuda:
raise RuntimeError("ContactMatcher requires a CUDA device (radix_sort_pairs is CUDA-only).")(Or keep silent and document it as a known limitation — either works; the current state is the least kind.)
| # Only buffer we must own: sorted keys survive across frames | ||
| # (_sort_keys_copy is overwritten by _prepare_sort each frame). | ||
| self._prev_sorted_keys = wp.zeros(capacity, dtype=wp.int64) | ||
| self._prev_count = wp.zeros(1, dtype=wp.int32) |
There was a problem hiding this comment.
Cross-episode reset semantics question: _prev_count persists across collide() calls (the whole point). But in RL-style workflows where a user wants to "start fresh" after state.reset() or teleporting all bodies, there's no way to zero _prev_count without rebuilding the pipeline. Contacts.clear() resets match_index=-1 but the matcher still has old previous-frame data, so the next frame produces spurious matches against bodies in their new poses.
Would you consider either (a) a ContactMatcher.reset() that zeros _prev_count (+ optionally _prev_was_matched), or (b) having Contacts.clear() coordinate with the matcher when contact_matching=True? If out of scope, a .. note:: in the class docstring flagging the persistence behavior would help users notice.
|
|
||
| # Only buffer we must own: sorted keys survive across frames | ||
| # (_sort_keys_copy is overwritten by _prepare_sort each frame). | ||
| self._prev_sorted_keys = wp.zeros(capacity, dtype=wp.int64) |
There was a problem hiding this comment.
Nit: wp.zeros(capacity, dtype=wp.int64) — harmless but prev-keys are always overwritten by the first save_sorted_state before being read, so the initial zero values are never consumed. Initializing with SORT_KEY_SENTINEL from contact_sort.py would make a debug inspection of the buffer before the first save_sorted_state less confusing (zeros look like valid keys for shape_a=0, shape_b=0). Very minor.
| data = _MatchData() | ||
| data.prev_keys = self._prev_sorted_keys | ||
| # Reuse sorter scratch buffers for prev-frame world-space data. | ||
| data.prev_pos_world = self._sorter._full_point0_buf |
There was a problem hiding this comment.
This (plus lines 382, 440, 441) reaches into ContactSorter's private _full_point0_buf / _full_normal_buf. The scratch-reuse optimization itself is great and well-motivated in the module docstring — the leaky abstraction is the only concern. Would a narrow pair of ContactSorter properties (e.g. scratch_pos_world, scratch_normal) — public names, maybe with a short "for use by ContactMatcher; do not write outside the documented window" comment — make the coupling explicit and refactor-safe? Feel free to push back if the underscore convention already communicates enough.
| MATCH_NOT_FOUND = wp.constant(wp.int32(-1)) | ||
| """Sentinel: no matching key found in last frame's contacts.""" | ||
|
|
||
| MATCH_BROKEN = wp.constant(wp.int32(-2)) |
There was a problem hiding this comment.
Nit: MATCH_NOT_FOUND / MATCH_BROKEN are module-level wp.constants but aren't re-exported through newton.geometry or the top-level. Users who'd rather write match_idx == MATCH_NOT_FOUND than match_idx == -1 can't import them. Documenting the numeric values on the public rigid_contact_match_index docstring is already done — re-export is optional, but would make user code self-documenting.
| model, | ||
| broad_phase="nxn", | ||
| contact_matching=True, | ||
| contact_matching_pos_threshold=0.001, # 1 mm — very tight |
There was a problem hiding this comment.
Using the tight 0.001 threshold here (vs the default 0.02) means the test silently becomes a pass if the default ever gets re-tuned. Could this be rewritten to exercise the default? The scene uses an infinite ground plane, so shifting spheres by e.g. 0.2 m (10× the default 0.02) still keeps them in contact while exceeding the position threshold — same invariant, and if someone later changes the default the test follows along:
pipeline = newton.CollisionPipeline(
model,
broad_phase="nxn",
contact_matching=True,
# rely on default contact_matching_pos_threshold (0.02 m)
)
...
# Shift all dynamic bodies by 0.2 m (10x default threshold).
for i in range(len(q)):
q[i][0] += 0.2Same story for the # 10x the threshold comment below at line 178.
Description
DON'T MERGE YET. Something seems to be broken.
Add frame-to-frame contact matching to
CollisionPipeline. Each frame, contacts are matched against the previous frame's sorted contacts using binary search on deterministic sort keys, then verified against world-space position distance and surface normal dot-product thresholds. The result is a per-contactmatch_indexonContacts(>= 0matched,-1new,-2broken). Optional contact reports list newly formed and broken contact indices.Enabled via
CollisionPipeline(contact_matching=True), which impliesdeterministic=Truebecause they use the same underlying sort and the same contact fingerprints.See #2248
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
14 tests covering: first-frame sentinel values, stable-scene identity matching across multiple frames, new contact detection, position and normal threshold breaking, contact report index correctness, deterministic flag implication, and disabled-matching allocation.
New feature / API change
Summary by CodeRabbit
New Features
Documentation
Tests