Repository navigation
[OVPHYSX] Add PVA sensor - #5702
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: [OVPHYSX] Add PVA sensor
Thank you for this well-structured implementation! The PR adds a PVA (Pose Velocity Acceleration) sensor for the OVPhysX backend with excellent documentation and comprehensive tests. Below are observations and suggestions for consideration.
🟡 Observations
1. Potential Division by Zero in Kernel (Low Risk)
File: source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/pva/kernels.py, line 58-59
lin_acc_w = (lin_vel_w - prev_lin_vel_w[idx]) * inv_dt
ang_acc_w = (ang_vel_w - prev_ang_vel_w[idx]) * inv_dtIf dt is extremely small (approaching zero), inv_dt becomes very large, potentially amplifying numerical noise. While the sensor note mentions keeping timestep ≥200 Hz, consider defensive clamping:
inv_dt_safe = wp.min(inv_dt, wp.float32(10000.0)) # Cap at 10kHzSeverity: Low — unlikely in practice but worth noting.
2. Dimension Variable Inconsistency (Cosmetic)
File: source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/pva/pva.py
The code validates self._num_bodies == self._num_envs (line 162), but then uses self._num_envs for kernel launch dimensions (line 91, 218) while buffers use self._num_bodies (line 232). This is functionally correct but slightly inconsistent with other sensors that pick one or the other. Consider using one consistently after the assertion.
Severity: Cosmetic — no functional impact.
3. Torch-Warp Memory Aliasing in Buffer Initialization
File: source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/pva/pva_data.py, lines 120-125
quat_torch = wp.to_torch(self._quat_w)
quat_torch[:, 3] = 1.0
# ...
pg_torch = wp.to_torch(self._projected_gravity_b)
pg_torch[:, 2] = -1.0This in-place modification of warp arrays via torch aliasing is a valid pattern used elsewhere in Isaac Lab, but relies on the arrays sharing memory. The pattern is correct — just noting it uses the same approach as the IMU sensor for consistency.
Severity: None — pattern is correct.
4. Test Process-Global Device Lock Documentation (Helpful Context)
File: source/isaaclab_ovphysx/test/sensors/test_pva.py
The docstring thoroughly explains the process-global device lock and test categorization. This is excellent documentation that will help maintainers understand why tests might need to be run in separate passes.
✅ Positive Observations
-
Excellent kernel alignment — The Warp kernel is byte-for-byte aligned with the PhysX implementation, ensuring consistent behavior across backends.
-
Comprehensive test coverage — 15+ test cases covering constant velocity/acceleration, pose packing, projected gravity, freefall, reset behavior, and indirect attachment.
-
Good error handling — Proper RuntimeError raises for uninitialized manager and mismatched body counts.
-
Thoughtful
.gitignorefix — The whitelist for__init__.py/__init__.pyiprevents future contributor friction withgit add -f. -
Clear coordinate acceleration semantics — Documentation clearly distinguishes PVA's coordinate acceleration (zero at rest,
-gin freefall) from IMU's proper acceleration.
📋 Summary
This is a well-implemented feature that follows Isaac Lab patterns and includes solid test coverage. The observations above are minor and none are blocking. The PR is ready for merge once CI passes.
Verdict: ✅ Approve (no blocking issues found)
Update (31c8a05): ✅ P1 Fixed — The str.replace issue in rigid-body ancestor resolution has been replaced with self._resolve_rigid_body_ancestor_expr() (a shared base-class method), eliminating the substring-stripping bug.
New commits also add:
- Timestamp guard in
pva_update_kernelto prevent stale pre-reset velocity/acceleration from leaking through lazy sensor updates (#4970 fix) - CPU/GPU-aware COM buffer handling (avoids unnecessary copy on CPU device)
- Regression test
test_no_stale_data_after_scene_reset - Kitless Fabric notice guard fix (graceful no-op when
usdrtunavailable)
No new issues found in the incremental changes. Previous P2 comments (hardcoded gravity in test, uninitialized _dt) remain as-is — minor and non-blocking.
Update (7004413):
✅ P1 fully resolved — The str.replace suffix-stripping bug is now properly fixed in sensor_base.py with an endswith guard + slice operation, plus a regression test (test_rigid_body_ancestor_expr_trims_only_terminal_suffix).
Additional changes in this push (no new issues):
build_simulation_contextnow correctly honors explicitdevicekwarg whensim_cfgis also provided (fixes multi-GPU test mismatches)- OVPhysX assets no longer enqueue redundant USD replication work during scene cloning
- Newton visualizer gains
show_particlesandparticle_colorconfiguration with proper caching and tests
Previous P2 comments (hardcoded gravity in test, uninitialized _dt) remain as-is — minor and non-blocking. No new issues introduced.
Update (dd2cf82):
Housekeeping commit — removes @pytest.mark.isaacsim_ci decorators from all OVPhysX test files (test_articulation.py, test_rigid_object.py, test_rigid_object_collection.py, test_contact_sensor.py). No functional changes.
No new issues introduced. Previous P2 comments remain as-is (non-blocking).
Update (48365b5):
Release consolidation and multi-package bugfix commit. Bumps isaaclab → 6.6.2, isaaclab_newton → 0.15.5, isaaclab_ovphysx → 3.0.4, isaaclab_tasks → 5.0.1, isaaclab_visualizers → 0.1.5. Key changes:
- Docker volume mount fix — new
volume_mounts.pyutility parsesdocker-compose.yamland pre-creates/chowns named-volume mount points in the image so the non-rootisaaclabuser can write them at runtime. Properset -o pipefailguard ensures build failures on parse errors. - Clone template position authoring — new
split_clone_template()helper ensures grid positions/quaternions are only authored on env-instance root destinations, not nested replicated assets (e.g. cameras). Includes regression test. - Newton Fabric body binding refactor —
_rename_builder_labelsnow returns Fabric body binding records;_initialize_fabric_body_primsis extracted as a static method for testability. Avoids invalid Fabric attribute writes without cloning USD specs per environment. - Livestream experience validation — rejects
isaacsim.exp.full-dependent experiences when livestreaming (known hang); auto-injects Kit visualizer when livestreaming requires a viewport. - Teleop device fix — moves keyboard/gamepad/SpaceMouse devices from shared
ReachEnvCfgto rel-IK config only (6D delta matches rel-IK, not abs-IK's 7D pose). - Visualizer fixes — Newton
set_camera_viewnow updates cfg and active viewer; tiled camera tutorial uses current imports and task-appropriate camera sources. - CI — disk cleanup step in license-check workflow; rendering_test_utils refactored with
_make_sensor_data_type_paramshelper. - Docs — Rerun
record_to_rrdand timeline scrubbing documentation added.
No new issues found. Previous P2 comments (hardcoded gravity in test, uninitialized _dt) remain as-is (non-blocking).
Greptile SummaryThis PR adds
Confidence Score: 3/5The sensor logic and kernels are solid, but a path-manipulation bug in _initialize_impl can silently bind the wrong rigid-body ancestor for certain prim-path layouts, producing incorrect sensor output with no runtime error. The str.replace call removes every occurrence of the suffix from the prim-path expression. For paths where the same component name appears at multiple depth levels, the first match is stripped instead of the trailing one, leaving _rigid_parent_expr pointing at the wrong ancestor. All downstream outputs are then silently wrong, and the current test suite does not cover this case. source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/pva/pva.py — the _initialize_impl path-stripping logic and missing _dt initialisation warrant a second look before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Pva.__init__] --> B[PvaData created]
B --> C[sim_ctx.reset triggers _initialize_impl]
C --> D{prim is rigid body?}
D -- yes --> E[_rigid_parent_expr = cfg.prim_path]
D -- no --> F[find rigid-body ancestor]
F --> G[MakeRelativePath to relative_path]
G --> H[_rigid_parent_expr = prim_path.removesuffix]
H --> I[resolve_prim_pose fixed offset]
E --> J[create tensor bindings pose/vel/com]
I --> J
J --> K[get_gravity to gravity_vec_w]
K --> L[_initialize_buffers_impl]
L --> M{fixed offset?}
M -- yes --> N[compose offset with ancestor transform]
M -- no --> O[Sensor ready]
N --> O
O --> P[update called]
P --> Q[_update_buffers_impl]
Q --> R[binding.read for pose/vel/com]
R --> S[pva_update_kernel]
S --> T[PvaData buffers filled]
Reviews (1): Last reviewed commit: "Add ovphysx changelog fragment for PVA s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| relative_path = prim.GetPath().MakeRelativePath(ancestor_prim.GetPath()).pathString | ||
| self._rigid_parent_expr = self.cfg.prim_path.replace("/" + relative_path, "") |
There was a problem hiding this comment.
str.replace strips every occurrence of "/" + relative_path in the prim-path expression, not just the trailing one. For a path like "/World/link/env_*/robot/link" where relative_path = "link", both /link segments are removed, yielding "/World/env_*/robot" instead of the correct "/World/link/env_*/robot". The resulting _rigid_parent_expr binds to the wrong rigid-body ancestor pattern, and the sensor silently reads incorrect state. Using removesuffix removes only the trailing suffix.
| relative_path = prim.GetPath().MakeRelativePath(ancestor_prim.GetPath()).pathString | |
| self._rigid_parent_expr = self.cfg.prim_path.replace("/" + relative_path, "") | |
| relative_path = prim.GetPath().MakeRelativePath(ancestor_prim.GetPath()).pathString | |
| self._rigid_parent_expr = self.cfg.prim_path.removesuffix("/" + relative_path) |
| expected_vel = torch.tensor([[1.0, 0.0, -dt * 9.81]], dtype=torch.float32, device=device).repeat( | ||
| NUM_ENVS, 1 | ||
| ) |
There was a problem hiding this comment.
9.81 rather than reading the actual gravity magnitude from the simulation. If SimulationCfg or the OvPhysxCfg defaults ever change the gravity value, this assertion silently diverges from what the sensor actually reports.
| expected_vel = torch.tensor([[1.0, 0.0, -dt * 9.81]], dtype=torch.float32, device=device).repeat( | |
| NUM_ENVS, 1 | |
| ) | |
| gravity_magnitude = float(torch.linalg.norm(torch.tensor(sim_ctx.cfg.gravity, dtype=torch.float32))) | |
| expected_vel = torch.tensor([[1.0, 0.0, -dt * gravity_magnitude]], dtype=torch.float32, device=device).repeat( | |
| NUM_ENVS, 1 | |
| ) |
| def _update_buffers_impl(self, env_mask: wp.array | None = None): | ||
| """Fills the buffers of the sensor data.""" | ||
| env_mask = self._resolve_indices_and_mask(None, env_mask) |
There was a problem hiding this comment.
_dt is first written in update(), but _update_buffers_impl is reachable via the data property (through _update_outdated_buffers) before update is ever called. Accessing data in that window references self._dt which does not exist and raises AttributeError. Initialising self._dt: float = 0.0 in __init__ would make the failure easier to diagnose.
marcodiiga
left a comment
marcodiiga
left a comment
There was a problem hiding this comment.
Blocking finding from reviewing this against current develop:
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/pva/kernels.py:35-58 reads pose/velocity and finite-differences velocities whenever env_mask[idx] is true, and _update_buffers_impl() launches that kernel at source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/pva/pva.py:210-231 without passing the sensor timestamp. The PhysX PVA backend has the reset guard at source/isaaclab_physx/isaaclab_physx/sensors/pva/kernels.py:41-44 and passes _timestamp at source/isaaclab_physx/isaaclab_physx/sensors/pva/pva.py:198-209, with the public regression at source/isaaclab_physx/test/sensors/test_pva.py:776-812.
This leaves the OVPhysX PVA vulnerable to the same reset/lazy-access bug: after scene.reset(env_ids) zeros the Isaac Lab buffers and timestamps, the native pose/velocity binding can still contain previous-step values until another physics step runs. A public pva.data read in that window can lazy-refetch stale state and repopulate the reset env with non-zero velocity/acceleration. The current OVPhysX reset test at source/isaaclab_ovphysx/test/sensors/test_pva.py:597-625 explicitly reads raw buffers because pva.data would lazy refill, so it does not cover this public path.
Please mirror the timestamp guard in the OVPhysX PVA kernel/launch and add the same public reset regression to the OVPhysX test (scene.reset(env_ids) followed by pva.data before another sim step).
Additional merge/cleanup items:
- This branch currently conflicts with current
developin.gitignore.developno longer has the broad**/__*ignore block, so the PR's!**/__init__.py/!**/__init__.pyihunk can be dropped rather than resolved forward. - The standalone/test docs still reference
./scripts/run_ovphysx.sh(source/isaaclab_ovphysx/test/sensors/test_pva.py:10-11,source/isaaclab_ovphysx/test/sensors/check_pva.py:8-9, andsource/isaaclab_ovphysx/test/sensors/test_pva.py:790-794), but that script is no longer present on currentdevelop. Please update those instructions to the current runner. - The already-noted path-strip issue at
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/pva/pva.py:156-157is still worth fixing here: removing"/" + relative_pathwith a rawstr.replace()can delete more than the trailing sensor suffix if the same suffix appears earlier in the expression. Use a suffix/path-aware removal instead.
|
Adding one additional item to the existing requested-changes review: Can the body/ancestor resolution in The raw suffix-removal bug is already covered by the existing review comments; this request is the broader fix: use the shared helper instead of local stage-scan resolution so cloned-scene binding expressions stay aligned with the reference backend. |
Mirrors PhysX's SimulationView.get_gravity() so backend-agnostic sensor code can read scene gravity through one classmethod regardless of backend. Reads from the active SimulationCfg; raises RuntimeError if the manager has not been initialized. Identical to the addition in the OVPhysX IMU PR (isaac-sim#5421) so the two PRs merge cleanly in either order.
Implements the OVPhysX backend for isaaclab.sensors.pva.{Pva,PvaData},
satisfying the BasePva / BasePvaData contracts. Reports world-frame
pose, body-frame linear and angular velocities, body-frame coordinate
linear and angular accelerations (not the IMU's proper acceleration),
and projected gravity as a unit direction vector.
- kernels.py is byte-for-byte aligned with isaaclab_physx PVA: same
signature, same operation order, same intermediate variable names.
- pva.py follows the OVPhysX IMU convention verbatim where applicable
(binding setup, buffer naming, error wording, __str__), diverging
only for PVA-specific outputs and gravity-direction-vs-bias semantics.
- pva_data.py uses the ProxyArray pattern from the IMU/Newton data
classes; pose_w is rebuilt on read via the shared
concat_pos_and_quat_to_pose_1d_kernel, matching PhysX's lazy access.
- create_buffers seeds quat_w to identity and projected_gravity_b to
(0, 0, -1) so reads between init and the first reset don't return
zero quaternions / zero gravity directions.
- _debug_vis_callback filters bodies with effectively zero
acceleration before create_rotation_matrix_from_view, matching PhysX
(otherwise markers at rest produce NaN orientations).
- sensors/__init__.pyi re-exports Pva and PvaData so the backend
factory dispatch resolves both symbols.
Issue: isaac-sim#5319
test_pva.py mirrors source/isaaclab_physx/test/sensors/test_pva.py but runs kitless under ./scripts/run_ovphysx.sh — no AppLauncher needed, SimulationContext is instantiated directly, Nucleus assets download via omni.client. Same procedural / Nucleus split and process-global device lock as the IMU test. PVA-specific coverage (vs. the IMU sibling): test_constant_velocity also asserts steady-state lin_vel_b, test_constant_acceleration asserts the coordinate-acceleration semantics (no gravity bias), test_projected_gravity_at_rest checks (0, 0, -1) in body frame, test_freefall_lin_acc checks |lin_acc_b| ≈ g, test_indirect_attachment_usd asserts all six output channels match between direct and indirect attachment, test_pose_w_packing verifies the lazy concat kernel packs [pos | quat] correctly. The two URDF pendulum tests are skipped pending a USD-converted pendulum asset (the PhysX test uses the Kit URDF importer, which is not loaded under the kitless launcher); a pytest.fail body ensures they don't pass vacuously if the skip is removed without porting the assertions. check_pva.py is a standalone validator: freefalling sphere, prints all six channels per step, sanity-checks |lin_acc_b| ≈ 9.81 and |projected_gravity_b| ≈ 1.
When IFabricUsd bindings are available in a kitless runtime, disabled_fabric_change_notifies still tried to import usdrt and failed before it could fall back. Treat only a missing top-level usdrt module like unavailable Fabric support, while preserving other import failures.
Use the sensor timestamp guard in the PVA update kernel so reset environments do not lazily refetch stale OVPhysX native velocities before the next simulation step. Also reuse the shared rigid-body ancestor resolution and stage COM reads on CPU only when needed.
436fd56 to
31c8a05
Compare
|
The code-review items I was checking look addressed on this head. The remaining visible blocker I see is the |
marcodiiga
left a comment
marcodiiga
left a comment
There was a problem hiding this comment.
Re-reviewed the current head. The prior PVA code-review items I checked are addressed: timestamp guard, reset-before-next-step public data regression, shared rigid-body ancestor resolver, runner docs, and the .gitignore cleanup. No new code-review issues from my pass. The remaining visible blocker is the isaaclab_ov CI failure noted in the PR comments.
Replace the fragile ANYmal-backed OVPhysX PVA checks with procedural rigid-body scenes so the full kitless test file no longer depends on omni.client loader state. Also align the PVA kernel signatures and docstrings with the IMU cleanup pattern across OVPhysX and PhysX.
Resolve the OVPhysX sensor export conflict by keeping both IMU and PVA exports.
# Description Implements `Pva` and `PvaData` for the OVPhysX backend, satisfying the `BasePva` / `BasePvaData` contracts. The sensor reports world-frame pose, body-frame linear and angular velocities, body-frame coordinate linear and angular accelerations (zero at rest, `-g` in freefall - not the IMU's proper acceleration), and projected gravity as a unit direction vector. - The kernel is byte-for-byte aligned with `isaaclab_physx`'s PVA kernel. - The sensor class mirrors the OVPhysX IMU's structure verbatim wherever applicable (binding setup, buffer naming, error wording, `__str__`), diverging only for PVA-specific outputs and gravity-direction vs gravity-bias semantics. - `PvaData` uses the same `ProxyArray` pattern as the IMU / Newton data classes; `pose_w` is rebuilt on read via the shared `concat_pos_and_quat_to_pose_1d_kernel` (matches PhysX's lazy access). - `OvPhysxManager.get_gravity()` is added so backend-agnostic sensor code can read scene gravity through one classmethod; identical to the same addition in isaac-sim#5421, so the two PRs merge cleanly in either order. - The branch is merged on top of latest `develop` after isaac-sim#6072 and isaac-sim#6085. The kitless `usdrt` guard, OVPhysX USD-clone removal, and OVPhysX `isaacsim_ci` cleanup are now in the base branch, so this PR carries only the PVA sensor changes. Validated end-to-end against the real OVPhysX backend on both CPU and CUDA. The procedural and Nucleus-backed cases are run in separate pytest processes because mixing them in one process can hit the known process-global `omni.client` symbol issue. Validation run: ```bash ./isaaclab.sh -p -m pytest --collect-only -q -m device_split source/isaaclab_ovphysx/test/sensors/test_pva.py # 28 tests collected ./isaaclab.sh -p -m pytest -q source/isaaclab_ovphysx/test/sensors/test_pva.py -k 'cpu and not offset and not env_ids' # 11 passed, 17 deselected ./isaaclab.sh -p -m pytest -q source/isaaclab_ovphysx/test/sensors/test_pva.py -k 'cpu and (offset or env_ids)' # 2 passed, 26 deselected ./isaaclab.sh -p -m pytest -q source/isaaclab_ovphysx/test/sensors/test_pva.py -k 'cuda and not offset and not env_ids' # 11 passed, 17 deselected ./isaaclab.sh -p -m pytest -q source/isaaclab_ovphysx/test/sensors/test_pva.py -k 'cuda and (offset or env_ids)' # 2 passed, 26 deselected ./isaaclab.sh -p -m pytest -q source/isaaclab_ovphysx/test/sensors/test_pva.py -k 'single_dof_pendulum or indirect_attachment' # 1 passed, 3 skipped, 24 deselected PATH=/tmp/git-lfs-v3.7.1:$PATH ./isaaclab.sh -f # Passed rg -n "isaacsim_ci" source/isaaclab_ovphysx/test # no matches ``` The skipped pendulum cases are pending a USD-converted pendulum asset; the PhysX equivalents use the Kit URDF importer, which is not loaded under the kitless launcher. Fixes isaac-sim#5319 ## Type of change - New feature (non-breaking change which adds functionality) ## Checklist - [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
Description
Implements
PvaandPvaDatafor the OVPhysX backend, satisfying theBasePva/BasePvaDatacontracts. The sensor reports world-framepose, body-frame linear and angular velocities, body-frame coordinate
linear and angular accelerations (zero at rest,
-gin freefall - notthe IMU's proper acceleration), and projected gravity as a unit
direction vector.
isaaclab_physx's PVA kernel.applicable (binding setup, buffer naming, error wording,
__str__),diverging only for PVA-specific outputs and gravity-direction vs
gravity-bias semantics.
PvaDatauses the sameProxyArraypattern as the IMU / Newton dataclasses;
pose_wis rebuilt on read via the sharedconcat_pos_and_quat_to_pose_1d_kernel(matches PhysX's lazy access).OvPhysxManager.get_gravity()is added so backend-agnostic sensor codecan read scene gravity through one classmethod; identical to the same
addition in [OVPHYSX] Add IMU sensor #5421, so the two PRs merge cleanly in either order.
developafter Fix kitless OVPhysX scene cloning #6072 and [CI] Drop OVPhysX tests from the Isaac Sim short CI #6085.The kitless
usdrtguard, OVPhysX USD-clone removal, and OVPhysXisaacsim_cicleanup are now in the base branch, so this PR carriesonly the PVA sensor changes.
Validated end-to-end against the real OVPhysX backend on both CPU and
CUDA. The procedural and Nucleus-backed cases are run in separate pytest
processes because mixing them in one process can hit the known
process-global
omni.clientsymbol issue.Validation run:
The skipped pendulum cases are pending a USD-converted pendulum asset;
the PhysX equivalents use the Kit URDF importer, which is not loaded
under the kitless launcher.
Fixes #5319
Type of change
Checklist
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