Fix stale gravity in OvPhysxManager.get_gravity and in IMU/PVA sensors - #7538
Closed
AntoineRichard wants to merge 2 commits into
Closed
Fix stale gravity in OvPhysxManager.get_gravity and in IMU/PVA sensors#7538AntoineRichard wants to merge 2 commits into
AntoineRichard wants to merge 2 commits into
Conversation
set_gravity() authored the new gravity into OvStage and applied it to the running scene, but get_gravity() read SimulationCfg.gravity, which the setter never touched. Every call after a live update returned the construction-time value. Track the applied gravity on the manager instead of writing back to cfg.gravity: randomization terms resample from cfg.gravity as the nominal base, so mutating it would compound successive 'add'/'scale' randomizations.
Imu and Pva read scene gravity once in _initialize_impl and baked it into a static buffer, so gravity randomized at runtime never reached the accelerometer bias or the projected gravity direction. Both backends were affected. Refresh the buffer on every update, skipping the refill when the scene gravity is unchanged. The buffer is filled in place rather than reallocated: the PhysX sensors consume it through a recorded wp.Launch that holds the array pointer, and PVA additionally exposes it as the public GRAVITY_VEC_W proxy. Newton's Pva already passes model.gravity into its kernel each update, so it needed no change.
github-actions
Bot
added
bug
8 tasks
Collaborator
Author
|
Superseded by #7416. The gravity-staleness fixes here touch the same IMU/PVA kernels as the solver-acceleration work in #7416, so the two could not be reviewed or merged independently. All three commits from this branch are now merged into #7416, along with the PVA solver-acceleration change and a scene-wide-vector refactor of the gravity parameters. Closing in favor of #7416. |
kellyguo11
added a commit
that referenced
this pull request
Sep 5, 2026
…and OvPhysX IMU/PVA sensors (#7416) # Description Three related defects in the IMU and PVA sensors, across PhysX and OvPhysX. They were originally split across this PR and #7538 (now closed as superseded); they are combined here because they touch the same kernels and could not be reviewed or merged independently. ## 1. Accelerations were finite-differenced (#1294) Both sensors computed linear acceleration by finite-differencing body velocity between updates. This made the reading depend on the sensor update period, produced a zero or stale reading on the first update, and generated large spurious spikes whenever velocities were written directly (resets, teleports). **PhysX**: the sensors now read `RigidBodyView.get_accelerations()` and transport it to the sensor frame as `a_sensor = a_com + α×r + ω×(ω×r)`. The IMU adds the gravity bias; PVA does not, since PVA reports kinematic acceleration. **OvPhysX**: the sensors read accelerations through the `RIGID_BODY_ACCELERATION` tensor binding with the same transport math. An earlier revision of this PR guarded that behind `hasattr(TT, "RIGID_BODY_ACCELERATION")` and fell back to finite differencing, because the binding had not shipped yet. It has since shipped and the project pins `ovphysx==0.5.11` exactly, so the fallback was unreachable on every supported install and has been removed along with its duplicate kernels and previous-velocity state. This also aligns PhysX and OvPhysX with Newton, whose PVA already computed acceleration from `body_qdd` with exactly this transport. ### `get_accelerations()` accuracy Verified on Isaac Sim 6.0.1 with a constant applied force (expected `a = F/m`): | mass | solver acc | expected | rel. err | |---|---|---|---| | 1 kg | 0.00199992 | 0.002 | 0.004% | | 1 g | 2.00004 | 2.0 | 0.002% | | 1 µg | 2000.0 | 2000.0 | <0.001% | The historical small-mass inaccuracy that motivated the finite-difference implementation is no longer present. ## 2. `OvPhysxManager.get_gravity()` returned a stale value `set_gravity()` authored the new gravity into OvStage and applied it to the running simulation, but `get_gravity()` read `SimulationCfg.gravity`, which the setter never touched. Every call after a live update returned the construction-time value, silently and permanently: ``` get_gravity before set : (0.0, 0.0, -9.81) set_gravity called with: (0.0, 0.0, -3.72) get_gravity after set : (0.0, 0.0, -9.81) ``` The manager now tracks the applied gravity rather than writing back to `cfg.gravity`. Writing back would have been wrong: both `_call_physx` and `_call_ovphysx` in `randomize_physics_scene_gravity` resample from `env.sim.cfg.gravity` as the pristine base, so mutating it would make successive `"add"`/`"scale"` randomizations compound. ## 3. Sensors snapshotted gravity at initialization IMU and PVA read gravity once in `_initialize_impl` and baked it into a static buffer, so gravity randomized at runtime never reached the accelerometer bias or the projected gravity direction. This affected **both** backends and was independent of defect 2 — fixing `get_gravity()` alone would not have changed sensor behavior. Both sensors now re-read scene gravity on every update, skipping the work when it is unchanged. This matters more after change 1, not less: the IMU reading is now literally `solver_a + (−g)`, so a stale `g` enters as a clean additive bias. Randomizing to Mars gravity made a resting body read +9.81 instead of +3.72 — a systematic 6.09 m/s² offset on every sample. Gravity is scene-wide on both backends, so the bias and the gravity direction are passed to the kernels **by value** rather than as per-body buffers. The PhysX recorded launches re-bind the parameter with `set_param_by_name` when it changes, mirroring how `env_mask` is already handled. If per-env gravity ever reaches these backends these go back to arrays, as Newton already does. ## Breaking change The PhysX PVA sensor's `GRAVITY_VEC_W` proxy array becomes the internal `_gravity_vec_w` scene-wide vector, matching what the OvPhysX PVA sensor already called it. It was absent from the data class, the type stubs and the docs. Consumers wanting this quantity should read `pva_sensor.data.projected_gravity_b`. **The identically-named attribute on the asset data classes is untouched.** ## Known remaining gaps (not addressed here) - `GRAVITY_VEC_W` is still snapshotted at construction in the **asset** data classes (`rigid_object_data.py`, `rigid_object_collection_data.py`, `articulation_data.py`) for both backends. Newton fixed its three by binding `ProxyArray(model.gravity[:world_count])` zero-copy, which is not possible here: OvPhysX exposes no scene-gravity tensor binding, and PhysX exposes gravity only as a host-side `Float3`. A fix must use the same pull-based refresh used here, and first needs a decision on whether to converge PhysX/OV onto Newton's m/s² convention (they currently store a normalized direction), which would touch consumers in `isaaclab_experimental` and `isaaclab_tasks_experimental`. - Newton's `Imu` applies no gravity bias at all, unlike the PhysX and OvPhysX IMUs. This looks like a cross-backend behavioral gap rather than a staleness bug. Fixes #1294 ## Type of change - Bug fix (non-breaking change which fixes an issue) - Breaking change (existing functionality will not work without user modification) - Documentation update ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` ## Screenshots Not applicable — no visual change. ## Checklist Docker and GPU tests run on demand. Push the commits you want tested, then comment `run-ci` on the pull request. - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package (do **not** edit `CHANGELOG.rst` or bump `extension.toml` — CI handles that) - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there ## Testing All suites run locally, one file at a time: | Suite | Result | |---|---| | `isaaclab_physx` IMU | 11 passed | | `isaaclab_physx` PVA | 11 passed | | `isaaclab_physx` recorded-launch | 13 passed | | `isaaclab_ov` IMU (default / `-k cuda`) | 12 passed, 14 skipped / 12 passed | | `isaaclab_ov` PVA (default / `-k cuda`) | 13 passed, 15 skipped / 13 passed | | `isaaclab_ov` physics | 53 passed | Regression coverage, each verified to fail without its fix: - `test_set_gravity_writes_and_releases_ovstage_control_resources` — asserts `get_gravity()` round-trips after a successful `set_gravity()`, still reports the previously applied value when the OvStage write fails, and leaves `cfg.gravity` untouched as the randomization base. Without the fix: `assert (0.0, 0.0, -9.81) == approx((0.0, 0.0, -3.72))`, reproducing the reported symptom. - `test_sensor_tracks_runtime_gravity_changes` and `test_imu_replayed_launch_applies_new_gravity_bias` — a mid-run gravity change must reach the *replayed* recorded launch, with an explicit guard that the test is on the recorded path rather than the eager fallback. - `test_velocity_writes_do_not_produce_spurious_acceleration` (IMU and PVA) — velocity teleports must not produce an acceleration spike. Without the fix the PVA case fails with a `99.9998` difference, exactly the `0.1 / dt = 100 m/s²` finite-difference artifact. The OvPhysX IMU tests were updated where they encoded finite-difference artifacts as expected values — most notably `test_gravity_at_rest`, whose old scenario never had the ball at rest (it fell continuously while zero velocity was rewritten each step). It now applies a real support force so the body is genuinely at rest and the accelerometer reads the bias alone, as a real IMU on a table does. --------- Co-authored-by: Kelly Guo <kellyg@nvidia.com>
isaaclab-bot Bot
pushed a commit
that referenced
this pull request
Sep 5, 2026
…and OvPhysX IMU/PVA sensors (#7416) # Description Three related defects in the IMU and PVA sensors, across PhysX and OvPhysX. They were originally split across this PR and #7538 (now closed as superseded); they are combined here because they touch the same kernels and could not be reviewed or merged independently. ## 1. Accelerations were finite-differenced (#1294) Both sensors computed linear acceleration by finite-differencing body velocity between updates. This made the reading depend on the sensor update period, produced a zero or stale reading on the first update, and generated large spurious spikes whenever velocities were written directly (resets, teleports). **PhysX**: the sensors now read `RigidBodyView.get_accelerations()` and transport it to the sensor frame as `a_sensor = a_com + α×r + ω×(ω×r)`. The IMU adds the gravity bias; PVA does not, since PVA reports kinematic acceleration. **OvPhysX**: the sensors read accelerations through the `RIGID_BODY_ACCELERATION` tensor binding with the same transport math. An earlier revision of this PR guarded that behind `hasattr(TT, "RIGID_BODY_ACCELERATION")` and fell back to finite differencing, because the binding had not shipped yet. It has since shipped and the project pins `ovphysx==0.5.11` exactly, so the fallback was unreachable on every supported install and has been removed along with its duplicate kernels and previous-velocity state. This also aligns PhysX and OvPhysX with Newton, whose PVA already computed acceleration from `body_qdd` with exactly this transport. ### `get_accelerations()` accuracy Verified on Isaac Sim 6.0.1 with a constant applied force (expected `a = F/m`): | mass | solver acc | expected | rel. err | |---|---|---|---| | 1 kg | 0.00199992 | 0.002 | 0.004% | | 1 g | 2.00004 | 2.0 | 0.002% | | 1 µg | 2000.0 | 2000.0 | <0.001% | The historical small-mass inaccuracy that motivated the finite-difference implementation is no longer present. ## 2. `OvPhysxManager.get_gravity()` returned a stale value `set_gravity()` authored the new gravity into OvStage and applied it to the running simulation, but `get_gravity()` read `SimulationCfg.gravity`, which the setter never touched. Every call after a live update returned the construction-time value, silently and permanently: ``` get_gravity before set : (0.0, 0.0, -9.81) set_gravity called with: (0.0, 0.0, -3.72) get_gravity after set : (0.0, 0.0, -9.81) ``` The manager now tracks the applied gravity rather than writing back to `cfg.gravity`. Writing back would have been wrong: both `_call_physx` and `_call_ovphysx` in `randomize_physics_scene_gravity` resample from `env.sim.cfg.gravity` as the pristine base, so mutating it would make successive `"add"`/`"scale"` randomizations compound. ## 3. Sensors snapshotted gravity at initialization IMU and PVA read gravity once in `_initialize_impl` and baked it into a static buffer, so gravity randomized at runtime never reached the accelerometer bias or the projected gravity direction. This affected **both** backends and was independent of defect 2 — fixing `get_gravity()` alone would not have changed sensor behavior. Both sensors now re-read scene gravity on every update, skipping the work when it is unchanged. This matters more after change 1, not less: the IMU reading is now literally `solver_a + (−g)`, so a stale `g` enters as a clean additive bias. Randomizing to Mars gravity made a resting body read +9.81 instead of +3.72 — a systematic 6.09 m/s² offset on every sample. Gravity is scene-wide on both backends, so the bias and the gravity direction are passed to the kernels **by value** rather than as per-body buffers. The PhysX recorded launches re-bind the parameter with `set_param_by_name` when it changes, mirroring how `env_mask` is already handled. If per-env gravity ever reaches these backends these go back to arrays, as Newton already does. ## Breaking change The PhysX PVA sensor's `GRAVITY_VEC_W` proxy array becomes the internal `_gravity_vec_w` scene-wide vector, matching what the OvPhysX PVA sensor already called it. It was absent from the data class, the type stubs and the docs. Consumers wanting this quantity should read `pva_sensor.data.projected_gravity_b`. **The identically-named attribute on the asset data classes is untouched.** ## Known remaining gaps (not addressed here) - `GRAVITY_VEC_W` is still snapshotted at construction in the **asset** data classes (`rigid_object_data.py`, `rigid_object_collection_data.py`, `articulation_data.py`) for both backends. Newton fixed its three by binding `ProxyArray(model.gravity[:world_count])` zero-copy, which is not possible here: OvPhysX exposes no scene-gravity tensor binding, and PhysX exposes gravity only as a host-side `Float3`. A fix must use the same pull-based refresh used here, and first needs a decision on whether to converge PhysX/OV onto Newton's m/s² convention (they currently store a normalized direction), which would touch consumers in `isaaclab_experimental` and `isaaclab_tasks_experimental`. - Newton's `Imu` applies no gravity bias at all, unlike the PhysX and OvPhysX IMUs. This looks like a cross-backend behavioral gap rather than a staleness bug. Fixes #1294 ## Type of change - Bug fix (non-breaking change which fixes an issue) - Breaking change (existing functionality will not work without user modification) - Documentation update ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` ## Screenshots Not applicable — no visual change. ## Checklist Docker and GPU tests run on demand. Push the commits you want tested, then comment `run-ci` on the pull request. - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package (do **not** edit `CHANGELOG.rst` or bump `extension.toml` — CI handles that) - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there ## Testing All suites run locally, one file at a time: | Suite | Result | |---|---| | `isaaclab_physx` IMU | 11 passed | | `isaaclab_physx` PVA | 11 passed | | `isaaclab_physx` recorded-launch | 13 passed | | `isaaclab_ov` IMU (default / `-k cuda`) | 12 passed, 14 skipped / 12 passed | | `isaaclab_ov` PVA (default / `-k cuda`) | 13 passed, 15 skipped / 13 passed | | `isaaclab_ov` physics | 53 passed | Regression coverage, each verified to fail without its fix: - `test_set_gravity_writes_and_releases_ovstage_control_resources` — asserts `get_gravity()` round-trips after a successful `set_gravity()`, still reports the previously applied value when the OvStage write fails, and leaves `cfg.gravity` untouched as the randomization base. Without the fix: `assert (0.0, 0.0, -9.81) == approx((0.0, 0.0, -3.72))`, reproducing the reported symptom. - `test_sensor_tracks_runtime_gravity_changes` and `test_imu_replayed_launch_applies_new_gravity_bias` — a mid-run gravity change must reach the *replayed* recorded launch, with an explicit guard that the test is on the recorded path rather than the eager fallback. - `test_velocity_writes_do_not_produce_spurious_acceleration` (IMU and PVA) — velocity teleports must not produce an acceleration spike. Without the fix the PVA case fails with a `99.9998` difference, exactly the `0.1 / dt = 100 m/s²` finite-difference artifact. The OvPhysX IMU tests were updated where they encoded finite-difference artifacts as expected values — most notably `test_gravity_at_rest`, whose old scenario never had the ball at rest (it fell continuously while zero velocity was rewritten each step). It now applies a real support force so the body is genuinely at rest and the accelerometer reads the bias alone, as a real IMU on a table does. --------- Co-authored-by: Kelly Guo <kellyg@nvidia.com> (cherry picked from commit 2d88d27)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two related gravity defects, both surfaced by a QA report against the OVPhysX backend.
1.
OvPhysxManager.get_gravity()returned a stale value.set_gravity()authors the new gravity into OvStage and applies the ordinal to the running simulation, butget_gravity()readSimulationCfg.gravity, which the setter never touched. Every call after a live update returned the construction-time value, silently and permanently:The fix tracks the applied gravity on the manager rather than writing back to
cfg.gravity. Writing back would have been wrong: both_call_physxand_call_ovphysxinrandomize_physics_scene_gravityresample fromenv.sim.cfg.gravityas the pristine base, so mutating it would make successive"add"/"scale"randomizations compound and skew the distribution.2. IMU and PVA snapshotted gravity at initialization. Both sensors read gravity once in
_initialize_impland baked it into a static Warp buffer, so gravity randomized at runtime never reached the accelerometer bias or the projected gravity direction. This affected both PhysX and OVPhysX — it was not specific to defect 1, and fixingget_gravity()alone would not have changed sensor behavior.Both sensors now refresh the buffer at the top of
_update_buffers_impl, skipping the refill when the scene gravity is unchanged (a tuple compare, so steady-state cost is oneget_gravity()call per sensor per update and no GPU work). The buffer is filled in place rather than reallocated: the PhysX sensors consume it through a recordedwp.Launchthat captures the array pointer, and PVA additionally exposes it as the publicGRAVITY_VEC_Wproxy. PVA's normalization mirrorsmath_utils.normalize's eps-clamp, so zero gravity yields a zero direction rather than NaNs.Newton's
Pvaalready passes the livemodel.gravityarray into its kernel each update and needed no change.Known remaining gaps (not addressed here)
GRAVITY_VEC_Wis still snapshotted at construction in the asset data classes (rigid_object_data.py,rigid_object_collection_data.py,articulation_data.py) for bothisaaclab_physxandisaaclab_ov. Newton already fixed its three by bindingProxyArray(model.gravity[:world_count])zero-copy. Extending that here needs an API decision first: Newton'sGRAVITY_VEC_Wcarries the actual m/s² vector with consumers normalizing on read, whereas PhysX/OV carry a pre-normalized unit direction. Converging on Newton's convention would change a public attribute's units and touch consumers inisaaclab_experimentalandisaaclab_tasks_experimental.Imuapplies no gravity bias at all, unlike the PhysX and OVPhysX IMUs which bias the accelerometer by-g. This looks like a cross-backend behavioral gap rather than a staleness bug.Type of change
Screenshots
None.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists thereTesting
Regression coverage, each verified to fail without the corresponding fix and pass with it:
test_set_gravity_writes_and_releases_ovstage_control_resources— extended to assertget_gravity()round-trips after a successfulset_gravity(), still reports the previously applied value when the OvStage write fails, and thatcfg.gravityis left untouched as the randomization base. Without the fix it fails withassert (0.0, 0.0, -9.81) == approx((0.0, 0.0, -3.72)), reproducing the reported symptom exactly.test_sensor_tracks_runtime_gravity_changes(new) — a mid-run gravity change must reach a replayed recorded launch, with the buffer pointer unchanged.test_sensor_skips_gravity_refill_when_unchanged(new) — an unchanged gravity must not clobber the buffer every update.Suites run locally: PhysX IMU 11 passed, PhysX PVA 11 passed, OV IMU 12 passed / 14 skipped, OV PVA 13 passed / 15 skipped,
isaaclab_ov/test/physics/53 passed (includes the real-backend gravity test).pre-commit run --all-filesclean.