Fix compute_first_contact/air missing transitions as the sensor clock ages - #7574
Fix compute_first_contact/air missing transitions as the sensor clock ages#7574AntoineRichard wants to merge 3 commits into
Conversation
compute_first_contact and compute_first_air compared the contact/air timer against dt + 1e-8. On a transition step the timer is exactly one sensor update interval, so the check degenerated to dt < dt + 1e-8. The sensor clock is a float32 accumulator whose rounding error reaches ~1e-6 after a few seconds of simulated time, roughly 100x the tolerance, so most touchdowns and lift-offs were silently dropped. Resolve abs_tol at call time instead: it now defaults to None, meaning half the sensor update interval. Valid timers are integer multiples of that interval, so half an interval is the midpoint between "one update ago" and "two updates ago", which stays robust for hours of simulated time and works for both the substep and the lazy refresh cadence. Callers that relied on the old default can pass abs_tol=1e-8 explicitly. Fixes #7283
Cover the tolerance contract at two levels. Kernel-level (no simulation, CPU, one file per backend): drive each backend's contact update kernel from an aged clock over an alternating contact schedule and assert that a threshold of dt + half an update interval flags every touchdown and lift-off and nothing else. These exercise the threshold math the new default produces, for both the substep and the lazy refresh cadence. Public-API: drop or teleport a body and poll compute_first_contact and compute_first_air with the default tolerance after ageing the sensor clock to 2.5, 10 and 30 s, comparing against transitions observed from the reported contact forces. These fail on the old 1e-8 default. The PhysX and OvPhysx public-API cases cover only the substep cadence: with zero history those backends serve stale contact forces on GPU when buffers refresh solely on data access, independently of this change.
github-actions
Bot
added
bug
Greptile SummaryThe PR replaces the fixed contact-transition comparison tolerance with half the configured sensor update interval across Newton, PhysX, and OVPhysX, and adds aged-clock regression coverage.
Confidence Score: 4/5The long-running transition-detection failure should be addressed before merging because simulations around 18 hours can again silently lose contact and air events. The new half-interval tolerance fixes short aged-clock runs but remains below the quantized float32 timestamp increment at sufficiently large, realistic simulation times, causing all three backends to fail the same strict transition comparison. Files Needing Attention: source/isaaclab/isaaclab/sensors/contact_sensor/base_contact_sensor.py and the aged-clock kernel tests Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Float32 sensor timestamp] --> B[Backend timer update]
B --> C[Current contact or air timer]
D[dt plus resolved tolerance] --> E[Transition comparison]
C --> E
E --> F[First contact or first air result]
Reviews (1): Last reviewed commit: "Add first-transition tolerance regressio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if abs_tol is not None: | ||
| return abs_tol | ||
| # An update period of 0.0 means the sensor is updated on every physics step. | ||
| return 0.5 * max(self.cfg.update_period, self._sim_physics_dt) |
There was a problem hiding this comment.
When a simulation with a 0.005-second physics step reaches 65536 seconds, the next float32 timestamp increment is 0.0078125 seconds while the new default threshold is only 0.0075 seconds. The strict timer comparison therefore silently drops touchdowns and lift-offs again across all three backends.
Knowledge Base Used: Simulation, rendering, and sensors
![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 PR centralizes the default first-transition tolerance in BaseContactSensor and applies it consistently across Newton, PhysX, and OVPhysX while preserving explicit tolerance behavior. No candidate findings were supplied or supported for acceptance.
- Design and architecture: The shared helper is an appropriate ownership point for backend-independent transition semantics. Each backend resolves the tolerance before launching its unchanged Warp transition kernel, avoiding duplicated policy logic in kernels.
- API: The abstract base and all concrete overrides consistently change
abs_tolfromfloat = 1e-8tofloat | None = None. Explicit float values retain their prior behavior, documentation includes[s]units, and changelog fragments describe the changed default and theabs_tol=1e-8compatibility path. - Implementation: The threshold path was traced as
dt + 0.5 * max(cfg.update_period, physics_dt)through all three backends and both refresh cadences represented by the tests. Public API regression tests exercise the new default against aged clocks. The duplicated kernel-level tests validate the numerical threshold rather than the helper itself, but the backend API tests cover helper integration, so this remains a non-blocking test-maintenance tradeoff.
No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.
Automated review; human maintainers own approval decisions.
|
Thanks for the quick turnaround and for the archaeology on the 2.x torch sensor — good to know this predates the Newton backend entirely. The half-interval default is the right minimal fix, and the reasoning for rejecting option (2) is airtight. One pushback on the reason for rejecting option (1), though, since #7294 implements exactly that path: the policy-step-visibility concern is solvable, and that PR solves it. The latches are cleared by the reset kernels and — the part that matters for substep refresh — Not arguing to swap fixes — the tolerance change is smaller, and the calibrated-thresholds argument for keeping the |
The kernels are unchanged by the fix, so these tests passed with or without it and only duplicated the threshold arithmetic already covered by the public-API aged-clock tests in each backend's contact sensor suite.
2 participants
Description
ContactSensor.compute_first_contact(dt)/compute_first_air(dt)silently missed most touchdowns and lift-offs once the simulation had run for a few seconds. On a transition step the contact (resp. air) timer is exactly one sensor update interval, so the check degenerated todt < dt + 1e-8. The sensor clock is a float32 accumulator whose rounding error reaches ~1e-6 after a few seconds of simulated time, roughly 100x the default tolerance, so the events vanished depending on the magnitude of the sim clock. The reporter's CPU reproducer shows 352/500 touchdowns and lift-offs missed at the default tolerance; it reproduces exactly ondevelop. All three backends (Newton, PhysX, OVPhysX) share the same logic, and the 2.x torch sensor had the same defect, so this is long-standing rather than a Newton regression.Fix.
abs_tolnow defaults toNoneand is resolved at call time by a single helper inBaseContactSensorto half the sensor update interval, i.e.0.5 * max(cfg.update_period, physics_dt). Valid timer values are integer multiples of that interval, so half an interval is the midpoint between "one update ago" and "two updates ago". This stays robust to float32 clock drift for hours of simulated time and works for bothhistory_length == 0(lazy, once-per-policy-step refresh) andhistory_length > 0(every physics substep). The Warp kernels are unchanged. Passing an explicitabs_tolstill works; callers who want the previous behaviour can passabs_tol=1e-8.Tests. Public-API regression tests in the Newton, PhysX and OVPhysX contact sensor suites: settle a body in contact, age the sensor clock to 2.5 / 10 / 30 s, lift and land the body, and poll
compute_first_contact/compute_first_airwith default arguments. These fail on the unfixed code (e.g.compute_first_air missed the lift-off at clock 2.833s: reported [], expected [0]) and pass with the fix. The Newton variant covers bothhistory_lengthmodes.Out of scope, noted for follow-up.
last_air_time/last_contact_timeoverestimate each phase by one update interval (also raised in the issue). That bias is present since 2.x and baked into tunedfeet_air_timethresholds, so changing it would rescale rewards for trained velocity policies and deserves its own PR.compute_first_*read the private timer buffers directly and therefore bypass the lazy refresh that thedataproperty triggers whenhistory_length == 0.cuda:0withhistory_length == 0, the PhysX and OVPhysX sensors serve stale net forces when buffers refresh only on data access, independently of this change. That is why the PhysX/OVPhysX public-API tests cover only the substep cadence.Fixes #7283
Type of change
Release backport
developChecklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there