Fix ContactSensor first-contact/first-air transitions missed at large simulation times - #7294
Conversation
…ge sim times The air/contact timers are float32 timestamp differences, so their quantization error grows with the simulation clock magnitude and soon exceeds the default abs_tol of compute_first_contact/compute_first_air. At a touchdown the current contact time equals exactly one update interval, so the t < dt + abs_tol comparison held only by the tolerance margin and float32 rounding silently dropped most transitions. Latch transitions inside the sensor update kernels instead: each buffer refresh records the transition flag and the age of the ended phase (assigned the interval midpoint so the t < dt comparison carries an interval/2 margin on both sides), and the compute kernels test the latched flag and event age. Also refresh outdated buffers before computing so lazy update cadences read fresh latches. Adds kernel-level regression tests driving all three backends (physx, ovphysx, newton) over eager and lazy refresh cadences, fresh and aged clocks.
github-actions
Bot
added
bug
Greptile SummaryThe PR replaces float32 timestamp-boundary inference with latched touchdown and lift-off events across the PhysX, OVPhysX, and Newton contact-sensor backends.
Confidence Score: 5/5The PR appears safe to merge with no concrete blocking or independently actionable non-blocking issue identified. The three backends consistently allocate, update, query, and reset the new transition state, while the regression suite exercises the timing and persistence behavior targeted by the fix. Important Files Changed
Sequence DiagramsequenceDiagram
participant Sim as Simulation step
participant Sensor as Contact sensor update
participant Latch as Transition latch and age
participant Query as compute_first_contact/air
Sim->>Sensor: Advance timestamp and mark stale
Sensor->>Sensor: Refresh forces when outdated
alt Contact state changed
Sensor->>Latch: Set touchdown/lift-off latch
Sensor->>Latch: Initialize age to half elapsed interval
else Same phase
Sensor->>Latch: Advance event age
end
Query->>Sensor: Refresh outdated buffers
Query->>Latch: "Test latch and age < dt + tolerance"
Latch-->>Query: First-transition mask
Reviews (1): Last reviewed commit: "Fix contact sensor missing first-contact..." | Re-trigger Greptile |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The transition-latch implementation is consistent across PhysX, OVPhysX, and Newton, but it changes first-query behavior after reset, leaves several public docstrings describing the removed timer-based logic, and uses a patch-bump changelog fragment for a test-only isaaclab package change.
- Design and architecture: The private latch-and-age state is wired symmetrically through allocation, reset, update, and query paths in all three backends. However, because transition detection now requires an observed phase change, the first sampled contact or air phase after reset is no longer reported as it was by the previous timer-based implementation; this compatibility decision should be addressed or explicitly documented.
- API: Method signatures, defaults, and ProxyArray return behavior remain unchanged. The observable post-reset semantics do change, and the PhysX compute_first_air docstring plus both Newton method docstrings still describe comparisons against current phase timers rather than latched transitions, so the public contract is currently inconsistent with the implementation.
- Implementation: The midpoint age bookkeeping, phase-specific latch clearing, track_air_time-gated allocation, and reset handling are implemented consistently across backends. The source/isaaclab package otherwise contains only a new regression test, so its changelog should use a .skip fragment rather than causing a patch bump and publishing backend implementation details.
Minor fixes needed. Posted 3 actionable findings inline.
Automated review; human maintainers own approval decisions.
| # against dt in compute_first_transition_kernel. | ||
| fct = first_contact_time[env, sensor] | ||
| fat = first_air_time[env, sensor] | ||
| if is_first_contact: |
There was a problem hiding this comment.
🟡 Warning · Api — First update after reset reports no transition
After reset() both timers are zero, so on the first refresh is_first_contact/is_first_detached are false and no latch is set. Previously current_contact_time/current_air_time became one interval (>0, <dt) and the query returned 1.0, so bodies starting an episode in contact (or in air) were reported once. Same in the Newton and OVPhysX kernels. Latch the initial phase entry after reset, or document the change.
| @@ -0,0 +1,10 @@ | |||
| Fixed | |||
There was a problem hiding this comment.
🔵 Suggestion · Implementation — isaaclab fragment should use .skip suffix
The only source/isaaclab change here is a new test file, yet a patch-tier .rst fragment is added, producing a version bump and a user-facing entry in isaaclab for fixes that live in other packages; it also cites internal paths. Repository rules prescribe <slug>.skip for test-only changes and discourage internal implementation details in entries.
| compute_first_transition_kernel, | ||
| dim=(self._num_envs, self._num_sensors), | ||
| inputs=[float(dt + abs_tol), self._data._current_air_time], | ||
| inputs=[ |
There was a problem hiding this comment.
🔵 Suggestion · Api — Stale docstrings describe removed timer comparison
compute_first_contact was reworded for the latch semantics, but compute_first_air here (and both Newton methods) still state the result comes "by comparing the current air time with the given time period". The documented contract no longer matches the implementation, especially for the post-reset case. Update the remaining docstrings the same way.
…p fragment - The first refresh after reset (both phase timers zero) now latches the initial contact/air phase so bodies starting an episode in contact or in air are reported once, matching the pre-latch timer behaviour. - Update the remaining compute_first_contact/compute_first_air docstrings (PhysX air, Newton both) to describe latched transitions. - Replace the isaaclab changelog fragment with an empty .skip marker; the package only gains the regression test. - Add a regression test for the post-reset first-phase report.
|
Thanks for the review — all three findings addressed in ce56f0d:
Full suite: 21 passed (was 18). |
1 participant
Description
Fixes #7283
ContactSensor.compute_first_contact()/compute_first_air()silently miss most touchdown / lift-off transitions once the simulation clock grows.Root cause. The air/contact timers are computed as differences of float32 timestamps, so their quantization error grows with the clock magnitude (
~4.6e-7 satt ≈ 2–16 s, roughly 400x the defaultabs_tol = 1e-8). At a touchdown the current contact time equals exactly one update interval, so thet < dt + abs_tolcomparison holds only by the tolerance margin — and float32 rounding tips it over, dropping the event. Withhistory_length = 0, ~73% of transitions were lost in the bad clock ranges.Fix. Latch transitions inside the sensor update kernels instead of inferring them from a timer comparison at query time:
0.5 * elapsed) so thet < dtcomparison carries aninterval / 2margin on both sides instead of resting exactly on the boundary;compute_first_transition_kernelnow tests the latched flag and event age (latch > 0 and transition_time < dt + abs_tol);compute_first_contact()/compute_first_air()refresh outdated buffers first, so lazy update cadences read fresh latches instead of stale timers;track_air_timeis set.Applied uniformly across all three backends (
isaaclab_physx,isaaclab_ovphysx,isaaclab_newton).Tests
New kernel-level regression test
source/isaaclab/test/sensors/test_contact_sensor_first_transition.pydrives the sensor update / compute kernels of all three backends directly (no simulation needed), over:Verified TDD-style: 12 failed / 6 passed before the fix, 18 passed after; stashing the kernel changes makes the suite fail again (12 failed), restoring it makes it pass — the tests bite.
Checklist
ruff format/ruff checkclean)isaaclab,isaaclab_physx,isaaclab_newtonNotes
isaaclab_ovphysxhas nochangelog.d(not managed by towncrier), so no fragment was added there.