Make hydroelastic contacts respect margin and gap - #3719
Make hydroelastic contacts respect margin and gap#3719mzamoramora-nvidia wants to merge 27 commits into
Conversation
Classify hydroelastic faces from both margin-adjusted SDF values and carry pair separation through reduced and unreduced contact export. Keep speculative contacts force-free while retaining local activation stiffness, and size generated SDFs for margin plus gap.
Add a non-gallery sphere-on-box example with visibly large margins and gaps. Its final checks require both speculative detection and penetrating activation before the sphere settles near the margin boundary.
Describe active, speculative, and absent hydroelastic contacts, SDF padding requirements, and the margin-zero/gap-zero compatibility setting. Correct the series material slope and retire the old speculative-force wording.
Prune candidate voxel blocks with the combined margin-adjusted SDF bound and the sum of both gaps. This preserves valid contacts when the shapes contribute unequal shares of the speculative envelope.
Keep reduced speculative contacts at their raw sampled positions and normals. This prevents force and wrench matching from moving a speculative representative into the active margin band.
Keep speculative reduction in disjoint slots so it cannot perturb penetrating force or wrench matching. Verify MuJoCo Warp activation, validate attached SDF padding metadata, exempt particle-only shapes, and add visual band guides.
Keep speculative reduction in one disjoint voxel-key family so a raw patch cannot be exported again through a normal-bin entry. Exercise cached MuJoCo Warp contacts across the activation boundary and verify their solver reference values. Keep attached-SDF remediation limited to the supported rebuild path.
Store one area value per hydroelastic face and reuse the cached pressure through reduction and export. This removes redundant buffer state while preserving speculative activation and custom pressure behavior. Deprecate the unused margin contact area setting and cover zero-gap and deprecation behavior with focused tests.
Define pair separation and the default gap cost in plain language. Update the implementation plan and migration guidance to match the deprecated margin contact area setting.
Keep margin_contact_area effective during its deprecation window instead of turning it into a no-op immediately. Cover both reduced and unreduced speculative contacts and correct the custom-pressure explanation.
Capture the final ASV timing, contact-count growth, and buffer cost. Note that setting gap to zero removes most of the speculative-band overhead.
Keep the public configuration docstring consistent with Newton's SI unit conventions.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHydroelastic contacts now account for per-shape margins and gaps, cache pressure through reduction and export, support deterministic ordering and accumulation, validate SDF padding, deprecate ChangesHydroelastic margin-gap contacts
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (3)
docs/concepts/collisions.rst (1)
1584-1587: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark
margin_contact_areaas deprecated at this first mention. Lines 1023 and 1659 both flag it; readers hitting this paragraph first may take it as a supported knob.📝 Suggested wording
-Speculative contacts carry an activation stiffness based on -``margin_contact_area``. This allows a compatible solver to activate a cached -contact after closing motion without treating the speculative contact as a -current force. +Speculative contacts carry an activation stiffness based on the deprecated +``margin_contact_area`` compatibility setting. This allows a compatible solver +to activate a cached contact after closing motion without treating the +speculative contact as a current force.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/concepts/collisions.rst` around lines 1584 - 1587, Update the first mention of margin_contact_area in the speculative contacts paragraph to mark it as deprecated, matching the existing deprecation notation used at the other documented mentions while preserving the surrounding explanation.newton/_src/sim/builder.py (1)
10414-10424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated hydroelastic
sdf_padding >= margin + gapcheck.
_validate_shapes()(10414-10424) and the SDF-generation loop infinalize()(11476-11485) both computemargin + gapfor hydroelastic colliding shapes and raise an equivalentValueErrorwith the same message pattern. Since thefinalize()copy is unconditional,skip_validation_shapes=Truedoesn't actually skip this specific invariant — it still gets validated (and can still raise) from the second site, which is a bit at odds with the flag's stated purpose of skipping "validation of shapes having valid contact margins."Consider extracting a single
_check_hydroelastic_sdf_padding(shape_index, margin, gap, sdf_padding)helper (or computingrequired_sdf_paddingonce and reusing it) to avoid two independently-maintained copies of the same error text and threshold logic.Also applies to: 11476-11485
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sim/builder.py` around lines 10414 - 10424, The hydroelastic sdf_padding invariant is duplicated in _validate_shapes() and finalize(), causing validation to run even when skip_validation_shapes=True. Extract a shared _check_hydroelastic_sdf_padding helper, or otherwise centralize the required margin + gap calculation and error, then invoke it only through the intended validation path so finalize() does not independently revalidate skipped shapes.newton/_src/geometry/contact_reduction_hydroelastic.py (1)
205-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate voxel-registration logic vs. the existing penetrating-contact path (lines 302-328).
The new speculative branch (compute voxel index → clamp → split into group/slot →
hashtable_find_or_insert→ setentry_k_eff→reduction_update_slot) is structurally identical to "Part 2: Voxel-based reduction" below it, differing only in thebin_idbase offset (SPECULATIVE_BIN_OFFSETvsNUM_NORMAL_BINS). Extracting a shared@wp.funchelper parametrized by the bin offset (and the value to store) would remove ~20 lines of copy-pasted logic and keep the two paths from silently drifting apart in the future.♻️ Sketch of a shared helper
`@wp.func` def _register_voxel_slot( shape_a: int, shape_b: int, bin_offset: int, voxel_idx: int, score: float, contact_id: int, shape_material_k_hydro: wp.array(dtype=wp.float32), reducer_data: GlobalContactReducerData, ) -> int: voxels_per_group = wp.static(NUM_SPATIAL_DIRECTIONS + 1) voxel_group = voxel_idx // voxels_per_group voxel_local_slot = voxel_idx % voxels_per_group entry_idx = hashtable_find_or_insert( make_contact_key(shape_a, shape_b, bin_offset + voxel_group), reducer_data.ht_keys, reducer_data.ht_active_slots, ) if entry_idx >= 0: reducer_data.entry_k_eff[entry_idx] = _effective_stiffness( shape_material_k_hydro[shape_a], shape_material_k_hydro[shape_b] ) reduction_update_slot( entry_idx, voxel_local_slot, _make_contact_value_fast(score, 0, contact_id), reducer_data.ht_values, reducer_data.ht_capacity, ) else: wp.atomic_add(reducer_data.ht_insert_failures, 0, 1) return entry_idxBoth the
depth >= 0.0branch and "Part 2" could then call this withbin_offset=wp.static(SPECULATIVE_BIN_OFFSET)/wp.static(NUM_NORMAL_BINS)respectively.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/contact_reduction_hydroelastic.py` around lines 205 - 246, Extract the duplicated voxel-registration flow into a shared `@wp.func`, such as _register_voxel_slot, parameterized by bin offset, voxel index, score, and contact ID while reusing the existing stiffness, hash-table, slot-update, and insertion-failure logic. Replace both the depth >= 0.0 speculative branch and the Part 2 voxel-reduction path with calls using their respective bin offsets, preserving their existing values and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/geometry/contact_reduction_hydroelastic.py`:
- Around line 535-536: Update the margin_contact_area documentation in both the
kernel-factory parameter doc and HydroelasticReductionConfig.margin_contact_area
attribute doc to state the SI unit [m²], matching
HydroelasticSDF.Config.margin_contact_area. Keep the existing deprecation
description unchanged.
In `@newton/examples/contacts/example_hydroelastic_margin_gap.py`:
- Around line 113-116: Update capture() to initialize self.graph to None before
entering wp.ScopedCapture, and guard the capture context so it is used only on
CUDA devices while still running self.simulate() on non-CUDA devices. Preserve
step()’s existing self.graph check and assign the captured graph only when
capture succeeds.
---
Nitpick comments:
In `@docs/concepts/collisions.rst`:
- Around line 1584-1587: Update the first mention of margin_contact_area in the
speculative contacts paragraph to mark it as deprecated, matching the existing
deprecation notation used at the other documented mentions while preserving the
surrounding explanation.
In `@newton/_src/geometry/contact_reduction_hydroelastic.py`:
- Around line 205-246: Extract the duplicated voxel-registration flow into a
shared `@wp.func`, such as _register_voxel_slot, parameterized by bin offset,
voxel index, score, and contact ID while reusing the existing stiffness,
hash-table, slot-update, and insertion-failure logic. Replace both the depth >=
0.0 speculative branch and the Part 2 voxel-reduction path with calls using
their respective bin offsets, preserving their existing values and behavior.
In `@newton/_src/sim/builder.py`:
- Around line 10414-10424: The hydroelastic sdf_padding invariant is duplicated
in _validate_shapes() and finalize(), causing validation to run even when
skip_validation_shapes=True. Extract a shared _check_hydroelastic_sdf_padding
helper, or otherwise centralize the required margin + gap calculation and error,
then invoke it only through the intended validation path so finalize() does not
independently revalidate skipped shapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0872dfa-fa57-46d4-be21-2694db58dc4a
📒 Files selected for processing (12)
CHANGELOG.mddocs/concepts/collisions.rstissue_2947_hydroelastic_margin_gap_plan.mdnewton/_src/geometry/contact_reduction_global.pynewton/_src/geometry/contact_reduction_hydroelastic.pynewton/_src/geometry/narrow_phase.pynewton/_src/geometry/sdf_hydroelastic.pynewton/_src/geometry/sdf_utils.pynewton/_src/sim/builder.pynewton/examples/contacts/example_hydroelastic_margin_gap.pynewton/tests/test_examples.pynewton/tests/test_hydroelastic.py
| def capture(self): | ||
| with wp.ScopedCapture() as capture: | ||
| self.simulate() | ||
| self.graph = capture.graph |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -A6 'def capture\(self\)' newton/examples/contactsRepository: newton-physics/newton
Length of output: 5524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline newton/examples/contacts/example_hydroelastic_margin_gap.py --view compact || true
echo "== relevant lines =="
sed -n '1,180p' newton/examples/contacts/example_hydroelastic_margin_gap.py | nl -ba
echo "== constructor/delegation around Simulation class =="
rg -n "class .*Simulation|def __init__|super().__init__|self\.use_graph|GraphBuilder|enable_graph|def step|self.graph|capture\(" newton/examples/contacts/example_hydroelastic_margin_gap.py | head -80
echo "== nearby captures in examples with CUDA guards / pre-initialization =="
sed -n '130,180p' newton/examples/contacts/example_domino_spiral.py | nl -ba
sed -n '140,175p' newton/examples/contacts/example_newton_cradle.py | nl -ba
sed -n '150,170p' newton/examples/contacts/example_balance_bird.py | nl -ba
sed -n '245,265p' newton/examples/contacts/example_nut_bolt_sdf.py | nl -ba
sed -n '945,965p' newton/examples/contacts/example_brick_stacking.py | nl -baRepository: newton-physics/newton
Length of output: 390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Read-only semantic probe for the Python control-flow issue as written:
# this class does not define self.graph in __init__, then defines it conditionally
# inside capture(); after a failure on the first with block, step() checks if self.graph.
class OffendingSimulation:
def __init__(self):
self.sim_substeps = 1
def capture(self):
# In Warp, this would raise on CPU before anything in the with block runs.
raise RuntimeError("no CUDA")
def step(self):
if self.graph:
pass
sim = OffendingSimulation()
try:
sim.capture()
except RuntimeError as exc:
print(f"capture raised: {type(exc).__name__}: {exc}")
try:
sim.step()
except AttributeError as exc:
print(f"step raised: {type(exc).__name__}: {exc}")
# Compare with the guard pattern from other hydroelastic examples:
class GuardedSimulation:
def __init__(self):
self.sim_substeps = 1
def capture(self):
self.graph = None
if not True: # simulating is_cuda=False
return
with None as graph: # would be ScopedCapture
pass
self.graph = graph.graph
def step(self):
if self.graph:
pass
sim2 = GuardedSimulation()
try:
sim2.capture()
sim2.step()
print("guarded example did not raise in capture/step")
except Exception as exc:
print(f"guarded example raised: {type(exc).__name__}: {exc}")
PYRepository: newton-physics/newton
Length of output: 330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant lines =="
python3 - <<'PY'
from pathlib import Path
p = Path("newton/examples/contacts/example_hydroelastic_margin_gap.py")
for i, line in enumerate(p.read_text().splitlines(), start=1):
if 1 <= i <= 180:
print(f"{i:4}: {line}")
PY
echo "== Simulation constructor/delegation and graph usage =="
python3 - <<'PY'
from pathlib import Path
p = Path("newton/examples/contacts/example_hydroelastic_margin_gap.py")
text = p.read_text()
for needle in ["class Example", "def __init__", "Simulation.__init__", "enable_graph", "self.graph", "def step"]:
for i, line in enumerate(text.splitlines(), start=1):
if needle in line:
start=max(1,i-2); end=min(len(text.splitlines()),i+3)
lines=text.splitlines()
print(f"\n--- {needle} at {i} ---")
for j in range(start,end+1):
print(f"{j:4}: {lines[j-1]}")
PY
echo "== compare guarded initializations =="
python3 - <<'PY'
from pathlib import Path
for path in [
"newton/examples/contacts/example_domino_spiral.py",
"newton/examples/contacts/example_newton_cradle.py",
"newton/examples/contacts/example_balance_bird.py",
"newton/examples/contacts/example_nut_bolt_sdf.py",
"newton/examples/contacts/example_hydroelastic_margin_gap.py",
]:
print(f"\n--- {path} ---")
lines=Path(path).read_text().splitlines()
for i, line in enumerate(lines, start=1):
if "self.graph" in line or "self.use_graph" in line or f"with wp.ScopedCapture" in line or f"wp.get_device().is_cuda" in line:
start=max(1,i-1); end=min(len(lines),i+3)
for j in range(start,end+1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: newton-physics/newton
Length of output: 15161
Guard graph capture for non-CUDA devices. capture() calls wp.ScopedCapture unconditionally, but self.graph is only assigned after the capture body; on CPU this raises before step() runs, and step() later checks self.graph. Drop the CUDA guard and initialize self.graph = None before calling wp.ScopedCapture, as several capture-enabled examples already do.
[low effort_and_high_reward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@newton/examples/contacts/example_hydroelastic_margin_gap.py` around lines 113
- 116, Update capture() to initialize self.graph to None before entering
wp.ScopedCapture, and guard the capture context so it is used only on CUDA
devices while still running self.simulate() on non-CUDA devices. Preserve
step()’s existing self.graph check and assign the captured graph only when
capture succeeds.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Give the Sphinx reference explicit link text because its target is a labeled paragraph rather than a titled section. This prevents the documentation build from treating the missing caption as an error.
Keep the issue planning notes local instead of including them in the pull request.
Mark the nearby setting reference as deprecated and document its SI unit consistently.
Keep the hydroelastic padding check in shape validation so the public skip flags work as documented. Preserve the padding calculation used for SDF generation.
Use one registration path for penetrating and speculative voxel contacts while retaining their disjoint key ranges. Verify speculative contacts cannot move into the penetrating margin region.
Record that model finalization now honors the public validation-skip flags for hydroelastic SDF padding.
# Conflicts: # newton/_src/geometry/contact_reduction_hydroelastic.py # newton/_src/geometry/sdf_hydroelastic.py # newton/tests/test_hydroelastic.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
newton/_src/geometry/sdf_hydroelastic.py (1)
1738-1964: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPre-prune export path never sets
contact_fingerprints, corruptingsort_sub_keyin the default configuration.
export_hydroelastic_contact_to_buffer(the direct, non-pre-prune path) correctly stores a fingerprint (tid * MAX_MC_FACES_PER_VOXEL + fi), but the local top‑K compaction path (best_pen0_*/best_pen1_*/best_nonpen_*) never captures or writes a fingerprint anywhere in the selection loop (1847-1899) or the final write-out block (1901-1964) — onlyposition_depth,normal,shape_pairs,contact_area, andcontact_pressureare written toreducer_dataatout_idx.
GlobalContactReducer.contact_fingerprintsis never cleared byclear_active()(only hashtable/aggregate/counter state is), so anyout_idxwritten via this path retains a stale value from whatever contact previously occupied that buffer slot (or 0). Sincepre_prune = reduce_contacts and pre_prune_contacts, and both default toTrue(deterministic mode is the only thing that disables pre-prune), this is the default active code path forHydroelasticSDF. Downstream,contact_fingerprints[contact_id]feedscontact_data.sort_sub_keyinexport_hydroelastic_reduced_contacts_kerneland the fingerprint tiebreaker inmake_contact_value, so pre-pruned contacts get bogus/duplicated sort keys — this can corrupt persistent contact-ID assignment used for solver warm-starting. Note the existing determinism tests (test_deterministic_hydroelastic_contacts*) run withdeterministic=True, which forcespre_prune_contacts=False, so they don't exercise this path.🐛 Proposed fix (representative, apply the pattern to all three candidate slots)
best_pen0_valid = int(0) best_pen0_score = float(-MAXVAL) best_pen0_depth = float(0.0) best_pen0_area = float(0.0) best_pen0_pressure = float(0.0) + best_pen0_fingerprint = int(0) best_pen0_normal = wp.vec3(0.0, 0.0, 1.0) @@ best_pen1_valid = int(0) best_pen1_score = float(-MAXVAL) best_pen1_depth = float(0.0) best_pen1_area = float(0.0) best_pen1_pressure = float(0.0) + best_pen1_fingerprint = int(0) best_pen1_normal = wp.vec3(0.0, 0.0, 1.0) @@ best_nonpen_valid = int(0) best_nonpen_depth = float(MAXVAL) best_nonpen_area = float(0.0) best_nonpen_pressure = float(0.0) + best_nonpen_fingerprint = int(0) best_nonpen_normal = wp.vec3(0.0, 0.0, 1.0) @@ for fi in range(num_faces): + face_fingerprint = tid * MAX_MC_FACES_PER_VOXEL + fi force_area, geometric_area, normal, face_center, adjusted_sdf_shape_b, pair_separation, face_verts = ( mc_calc_face_texture(...) ) @@ if pair_separation < 0.0: score = force_area * face_pressure if best_pen0_valid == 0 or score > best_pen0_score: best_pen1_valid = best_pen0_valid + best_pen1_fingerprint = best_pen0_fingerprint ... best_pen0_valid = int(1) + best_pen0_fingerprint = face_fingerprint ... elif wp.static(PRE_PRUNE_MAX_PENETRATING > 1): if best_pen1_valid == 0 or score > best_pen1_score: best_pen1_valid = int(1) + best_pen1_fingerprint = face_fingerprint else: if pair_separation < best_nonpen_depth: best_nonpen_valid = int(1) + best_nonpen_fingerprint = face_fingerprint @@ if best_pen0_valid == 1 and out_idx < reducer_data.capacity: ... reducer_data.contact_pressure[out_idx] = best_pen0_pressure + reducer_data.contact_fingerprints[out_idx] = best_pen0_fingerprint ...(repeat the
contact_fingerprints[out_idx] = ...write for thebest_pen1andbest_nonpenoutput blocks)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/sdf_hydroelastic.py` around lines 1738 - 1964, Capture each selected face’s fingerprint, using tid * MAX_MC_FACES_PER_VOXEL + fi, alongside the candidate fields in the best_pen0_*, best_pen1_*, and best_nonpen_* selection paths. Write the corresponding fingerprint to reducer_data.contact_fingerprints at each pre-prune output index in all three final write-out blocks, matching the existing direct export path.newton/tests/test_hydroelastic.py (2)
2010-2033: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not use
abs()to detectint64underflow.Line 2032 passes for
np.int64minimum becausenp.abs(np.int64(-2**63))overflows back to a negative value, which is still less thanint64.max. Explicitly reject both extrema.Proposed assertion fix
- test.assertTrue(np.all(np.abs(fixed_np[1:]) < np.iinfo(np.int64).max)) + int64_info = np.iinfo(np.int64) + test.assertFalse(np.any(fixed_np[1:] == int64_info.min)) + test.assertFalse(np.any(fixed_np[1:] == int64_info.max))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/tests/test_hydroelastic.py` around lines 2010 - 2033, Update the assertions in test_fixed_point_extreme_exponents to avoid np.abs when validating int64 bounds. Explicitly assert that each converted value is neither np.iinfo(np.int64).min nor np.iinfo(np.int64).max, rejecting both extrema without overflow.
528-563: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompare every exported contact field.
Lines 547-563 only compare
point0andnormal, so reordered IDs, shape pairs, offsets, margins, or material fields can regress while this “bit-identical contacts” test passes. Snapshot the same exported fields covered by the reduced-path test.Proposed test expansion
- snapshots.append( - ( - count, - contacts.rigid_contact_point0.numpy()[:count].copy(), - contacts.rigid_contact_normal.numpy()[:count].copy(), - ) - ) + snapshots.append( + ( + count, + tuple(getattr(contacts, name).numpy()[:count].copy() for name in contact_fields), + ) + ) ... - for count, point0, normal in snapshots[1:]: + for count, fields in snapshots[1:]: test.assertEqual(count, snapshots[0][0]) - np.testing.assert_array_equal(point0, snapshots[0][1], err_msg="rigid_contact_point0") - np.testing.assert_array_equal(normal, snapshots[0][2], err_msg="rigid_contact_normal") + for name, expected, actual in zip(contact_fields, snapshots[0][1], fields, strict=True): + np.testing.assert_array_equal(actual, expected, err_msg=name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/tests/test_hydroelastic.py` around lines 528 - 563, Expand test_deterministic_hydroelastic_contacts_unreduced to snapshot and compare every exported contact field covered by the reduced-path determinism test, not only rigid_contact_point0 and rigid_contact_normal. Include contact IDs, shape pairs, offsets, margins, and material-related fields, preserving the existing count and bit-identical comparisons across repeated collisions.newton/_src/geometry/contact_reduction_hydroelastic.py (1)
383-403: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeterministic normal-bin pre-registration wastes hashtable capacity on speculative contacts.
Unlike
reduce_hydroelastic_contacts_kernel, which only registers penetrating (depth < 0.0) contacts into normal bins (speculative contacts are diverted into the disjointSPECULATIVE_BIN_OFFSETvoxel keyspace and never reach normal-bin registration),_register_hydroelastic_normal_bins_kernelregisters every buffered contact — including speculative ones — into a normal bin. Downstream consumers (accumulate_reduced_depth_kernel, the export kernel's normal-bin path) all gate ondepth < 0.0before readingcontact_nbin_entry, so the normal-bin entry created for a speculative contact is never actually used for anything.This wastes shared hashtable capacity specifically in deterministic mode, and since this PR's whole purpose is enabling margin/gap-driven speculative contacts (which can be numerous relative to penetrating ones), this risks premature hashtable saturation and
ht_insert_failuresfor legitimate penetrating contacts in gap-heavy scenes — something the stacked-cubes-based determinism tests wouldn't surface.🐛 Proposed fix
tid = wp.tid() num_contacts = wp.min(reducer_data.contact_count[0], reducer_data.capacity) for contact_id in range(tid, num_contacts, total_num_threads): + pd = reducer_data.position_depth[contact_id] + if pd[3] >= 0.0: + continue pair = reducer_data.shape_pairs[contact_id] normal = decode_oct(reducer_data.normal[contact_id]) key = make_contact_key(pair[0], pair[1], get_slot(normal))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/contact_reduction_hydroelastic.py` around lines 383 - 403, Update _register_hydroelastic_normal_bins_kernel to register normal-bin keys only for contacts with depth < 0.0, matching reduce_hydroelastic_contacts_kernel. Leave speculative contacts with contact_nbin_entry unset or otherwise unregistered so they do not consume normal-bin hashtable capacity, while preserving existing stiffness initialization and insertion-failure handling for penetrating contacts.
🧹 Nitpick comments (1)
newton/_src/geometry/contact_reduction_hydroelastic.py (1)
675-675: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
int()cast on an already-integer literal.🧹 Suggested fix
- entry_idx = int(-1) + entry_idx = -1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/contact_reduction_hydroelastic.py` at line 675, Remove the redundant int() conversion from the entry_idx initialization in the contact-reduction logic, assigning the integer sentinel directly while preserving its existing value and behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@newton/_src/geometry/contact_reduction_hydroelastic.py`:
- Around line 383-403: Update _register_hydroelastic_normal_bins_kernel to
register normal-bin keys only for contacts with depth < 0.0, matching
reduce_hydroelastic_contacts_kernel. Leave speculative contacts with
contact_nbin_entry unset or otherwise unregistered so they do not consume
normal-bin hashtable capacity, while preserving existing stiffness
initialization and insertion-failure handling for penetrating contacts.
In `@newton/_src/geometry/sdf_hydroelastic.py`:
- Around line 1738-1964: Capture each selected face’s fingerprint, using tid *
MAX_MC_FACES_PER_VOXEL + fi, alongside the candidate fields in the best_pen0_*,
best_pen1_*, and best_nonpen_* selection paths. Write the corresponding
fingerprint to reducer_data.contact_fingerprints at each pre-prune output index
in all three final write-out blocks, matching the existing direct export path.
In `@newton/tests/test_hydroelastic.py`:
- Around line 2010-2033: Update the assertions in
test_fixed_point_extreme_exponents to avoid np.abs when validating int64 bounds.
Explicitly assert that each converted value is neither np.iinfo(np.int64).min
nor np.iinfo(np.int64).max, rejecting both extrema without overflow.
- Around line 528-563: Expand test_deterministic_hydroelastic_contacts_unreduced
to snapshot and compare every exported contact field covered by the reduced-path
determinism test, not only rigid_contact_point0 and rigid_contact_normal.
Include contact IDs, shape pairs, offsets, margins, and material-related fields,
preserving the existing count and bit-identical comparisons across repeated
collisions.
---
Nitpick comments:
In `@newton/_src/geometry/contact_reduction_hydroelastic.py`:
- Line 675: Remove the redundant int() conversion from the entry_idx
initialization in the contact-reduction logic, assigning the integer sentinel
directly while preserving its existing value and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0be00e27-330e-4907-a306-94e5443285ac
📒 Files selected for processing (8)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/contact_reduction_global.pynewton/_src/geometry/contact_reduction_hydroelastic.pynewton/_src/geometry/narrow_phase.pynewton/_src/geometry/sdf_hydroelastic.pynewton/tests/test_examples.pynewton/tests/test_hydroelastic.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/concepts/collisions.rst
- CHANGELOG.md
Carry each selected face fingerprint through local pre-pruning. This gives reduced contacts stable sort keys instead of stale buffer values.
Skip penetrating normal-bin registration for speculative contacts. They remain deterministic in their separate speculative voxel key range.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/_src/geometry/sdf_hydroelastic.py (1)
761-812: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing SI unit for
marginin the newshape_datadocstring entry.
shape_data: Per-shape scale and collision margin.omits the unit for the physicalmarginquantity, unlike sibling docstrings for the same quantity elsewhere (e.g.margin_contact_areadocuments[m^2]).📝 Suggested doc fix
- shape_data: Per-shape scale and collision margin. + shape_data: Per-shape scale and collision margin [m].As per coding guidelines,
**/*.{py,pyi}: "Include SI units for physical quantities in public API docstrings, including appropriate units for joint-dependent values, spatial vectors, and compound arrays; omit units for non-physical fields."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/sdf_hydroelastic.py` around lines 761 - 812, Update the launch method’s shape_data docstring entry to specify the SI unit [m] for the physical collision margin, while retaining the scale description and leaving non-physical fields unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/_src/geometry/sdf_hydroelastic.py`:
- Around line 761-812: Update the launch method’s shape_data docstring entry to
specify the SI unit [m] for the physical collision margin, while retaining the
scale description and leaving non-physical fields unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6026368c-a0c1-4f78-8b5a-55f34867ae5f
📒 Files selected for processing (3)
newton/_src/geometry/contact_reduction_hydroelastic.pynewton/_src/geometry/sdf_hydroelastic.pynewton/tests/test_hydroelastic.py
|
Looks good in general. An AI review found the following:
|
2 participants
Description
Closes #2947.
Hydroelastic contacts now respect each shape's collision margin and gap.
For two shapes, define the margin-relative pair separation as:
The contact bands are:
d < 0: penetrating hydroelastic contact;0 <= d <= gap_a + gap_b: speculative contact candidate;d > gap_a + gap_b: no contact.Speculative contacts are exported with nonnegative separation and no current
pressure force. They retain activation stiffness so a compatible solver can
activate them if motion carries the cached contact into penetration. Reduction
keeps speculative and penetrating contacts in separate key ranges, preventing
force or wrench matching from moving a speculative representative into the
penetrating region.
The implementation also:
margin + gap;for contact classification and exported separation;
for reduction and export;
contacts;
HydroelasticSDF.Config.margin_contact_areawithout removing itscompatibility behavior in the same release;
and buffer cost;
separation is visible.
The
hydroelastic_margin_gapexample is included only for testing and visualverification. It will be removed before this PR is ready to merge.
This PR does not implement a gradient-aware hydroelastic pressure surface. The
margin/gap classification is kept separate so a later change can add that mode
without redefining the contact bands. This is related to, but does not implement,
the work discussed in #3503.
Compatibility
Hydroelastic contacts are experimental, but existing scenes may still depend on
the earlier geometric-surface behavior. Setting only
margin=0.0is not enoughwhen a nonzero gap is inherited or configured. Use both:
This is the closest equivalent to the earlier geometric contact surface. It is
not claimed to be bit-for-bit identical.
gap=Nonecontinues to inheritModelBuilder.rigid_gap, whose default isnonzero. Users who do not want speculative hydroelastic contacts should set
gap=0.0explicitly.Determinism
This PR overlaps with the deterministic hydroelastic work in #3661. Merging
either PR may require coordination with the other.
Performance and memory
A nonzero gap generates additional contact candidates and therefore uses more
collision time and buffer memory. In the 20-frame hydroelastic nut-and-bolt ASV
benchmark on an NVIDIA RTX PRO 6000 Blackwell:
main:62.4 +/- 0.4 ms;gap=0.005:80.7 +/- 0.4 ms;1.29.Measured mean contact counts were:
maingap=0.0gap=0.005The remaining cached-pressure field costs one float per raw-face slot, about
6.8 MiB for the measured nut-and-bolt buffer. A separate geometric-area buffer
was removed during review.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Focused hydroelastic tests:
Result after merging the latest
main: 39 passed, 2 skipped.Verification-only margin/gap example:
Hydroelastic nut-and-bolt:
Panda hydroelastic scenes:
Repository checks:
Result after merging the latest
main: 5,025 passed, 158 skipped.Manual visual check:
Verify that the guide lines show the margin boundary and outer gap boundary,
that the sphere passes through the speculative band without being supported
there, and that it settles at the margin boundary.
Bug fix
Before this PR, hydroelastic contact extraction used raw SDF values and could
produce contacts outside the intended margin/gap bands.
Steps to reproduce:
the margin-plus-gap sum.
follow the configured margin and gap.
The
hydroelastic_margin_gapexample provides a visual reproduction and checksall three bands during a falling-sphere simulation.
New feature / API change
No new public symbol is added. Existing shape configuration now applies to
hydroelastic contact generation:
Summary by CodeRabbit
margin_contact_areais deprecated but remains effective during the transition period.