Migrate OVPhysX PVA sensor onto OvPhysxView (view series, part 5)

[AntoineRichard]

🔗 Stacked on top of #6224 (Part 1 — OvPhysxView)

Branched off #6224 as a sibling of the asset migrations (#6225/#6226/#6227). Targets develop; until #6224 merges, the diff below includes the Part 1 OvPhysxView commits. Please review #6224 first.

Description

Part 5 of the OVPhysX view-migration series — route the PVA sensor's tensor-binding management through OvPhysxView. Internal refactor; no public API change (sensors do not expose a root_view).

• _initialize_impl builds one OvPhysxView instead of separate create_tensor_binding calls; binding handles come from binding_for.
• Pose/state reads route through read_into (cached float32 reinterpret, typed off the binding shape), replacing the manual wp.array(ptr=...) reinterpret views.
• The CPU-only RIGID_BODY_COM_POSE keeps its pinned-host staging + wp.copy to the sim device.
• Numeric behavior unchanged.

Type of change

• This change requires no public API change (internal refactor)

Checklist

• I have run the pre-commit checks with ./isaaclab.sh --format
• My changes generate no new warnings
• I have added a changelog fragment (.skip — internal refactor, no user-facing change / no bump)

Testing

test_pva.py — cpu: 13 passed, cuda: 13 passed.

[greptile-apps]

Greptile Summary

This PR is part 5 of the OVPhysX view-migration series: it introduces OvPhysxView, a string-keyed lazy binding manager over OVPhysX TensorBinding handles, and rewires the PVA sensor's _initialize_impl and _update_buffers_impl to use it in place of three direct create_tensor_binding calls.

• OvPhysxView (ovphysx_view.py) provides get_attribute / read_into / set_attribute with device-policy enforcement, a float32-reinterpret cache for structured-dtype buffers, and a binding_for escape hatch; no public API change to the sensor layer.
• Pva._initialize_buffers_impl removes the manual wp.array(ptr=...) reinterpret views; CPU staging for RIGID_BODY_COM_POSE is preserved using a pinned wp.transformf buffer and wp.copy, keeping numeric behavior identical.
• A 587-line unit test suite exercises construction, get/set/read-into paths, device policy, dtype safety, and key-alias remapping against mock bindings.

Confidence Score: 3/5

The sensor migration is correct and safe to merge; the new OvPhysxView class has a subtle read-cache design issue that can silently corrupt data in a caller-misuse scenario.

The _read_view cache maps id(dst) to a float32 reinterpret but does not re-validate the cached view's shape against the current binding on a cache hit. If a structured-dtype buffer is passed to read_into for two different attributes with different flat shapes, the stale reinterpret from the first call is silently handed to the second binding's read, writing the wrong number of floats with no error. The sensor avoids this because each buffer is owned by one attribute, but the defect lives in the shared OvPhysxView class used by the asset-migration PRs in this series.

source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/ovphysx_view.py — specifically the _read_view method and the _read_views cache design.

Important Files Changed

Filename	Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/ovphysx_view.py	New OvPhysxView class — string-keyed binding manager with lazy creation, float32 reinterpret cache, device-policy enforcement; cache keyed only by id(dst) without per-binding shape re-validation on hit and no lifetime management.
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/pva/pva.py	PVA sensor migrated from three separate create_tensor_binding calls to one OvPhysxView; CPU/GPU staging for RIGID_BODY_COM_POSE preserved correctly with structured-dtype pinned buffer and wp.copy.
source/isaaclab_ovphysx/test/sim/test_ovphysx_view.py	587-line unit test suite covering construction, get/set/read_into paths, device policy, dtype safety, and key-alias remapping — no live binding tests as documented.
source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/init.pyi	Stub updated to export OvPhysxView; lazy_export() in init.py reads this stub at runtime so no separate init.py change needed.
docs/source/api/lab_ovphysx/isaaclab_ovphysx.sim.views.rst	New Sphinx automodule page for isaaclab_ovphysx.sim.views; correctly hooked into index.rst.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant PVA as Pva._update_buffers_impl
    participant OPV as OvPhysxView
    participant BC as _binding cache
    participant RC as _read_view cache
    participant PX as OVPhysX TensorBinding

    PVA->>OPV: read_into(RIGID_BODY_POSE, self._transforms)
    OPV->>BC: _binding(RIGID_BODY_POSE)
    BC-->>OPV: binding
    OPV->>RC: _read_view(transforms, binding)
    RC-->>OPV: float32 reinterpret view
    OPV->>PX: binding.read(float32_view)
    PX-->>OPV: fills transforms buffer GPU

    PVA->>OPV: read_into(RIGID_BODY_VELOCITY, self._velocities)
    OPV->>BC: _binding(RIGID_BODY_VELOCITY)
    BC-->>OPV: binding
    OPV->>RC: _read_view(velocities, binding)
    RC-->>OPV: float32 reinterpret view
    OPV->>PX: binding.read(float32_view)
    PX-->>OPV: fills velocities buffer GPU

    PVA->>OPV: read_into(RIGID_BODY_COM_POSE, self._coms_read_view)
    note over OPV: CPU-only type native device is cpu
    OPV->>BC: _binding(RIGID_BODY_COM_POSE)
    BC-->>OPV: binding
    OPV->>RC: _read_view(coms_read_view, binding)
    RC-->>OPV: float32 reinterpret pinned CPU
    OPV->>PX: binding.read(float32_view)
    PX-->>OPV: fills coms_read_view CPU

    PVA->>PVA: wp.copy(coms_buffer_GPU, coms_read_view_CPU)
    note over PVA: CPU to GPU copy only on GPU sim

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant PVA as Pva._update_buffers_impl
    participant OPV as OvPhysxView
    participant BC as _binding cache
    participant RC as _read_view cache
    participant PX as OVPhysX TensorBinding

    PVA->>OPV: read_into(RIGID_BODY_POSE, self._transforms)
    OPV->>BC: _binding(RIGID_BODY_POSE)
    BC-->>OPV: binding
    OPV->>RC: _read_view(transforms, binding)
    RC-->>OPV: float32 reinterpret view
    OPV->>PX: binding.read(float32_view)
    PX-->>OPV: fills transforms buffer GPU

    PVA->>OPV: read_into(RIGID_BODY_VELOCITY, self._velocities)
    OPV->>BC: _binding(RIGID_BODY_VELOCITY)
    BC-->>OPV: binding
    OPV->>RC: _read_view(velocities, binding)
    RC-->>OPV: float32 reinterpret view
    OPV->>PX: binding.read(float32_view)
    PX-->>OPV: fills velocities buffer GPU

    PVA->>OPV: read_into(RIGID_BODY_COM_POSE, self._coms_read_view)
    note over OPV: CPU-only type native device is cpu
    OPV->>BC: _binding(RIGID_BODY_COM_POSE)
    BC-->>OPV: binding
    OPV->>RC: _read_view(coms_read_view, binding)
    RC-->>OPV: float32 reinterpret pinned CPU
    OPV->>PX: binding.read(float32_view)
    PX-->>OPV: fills coms_read_view CPU

    PVA->>PVA: wp.copy(coms_buffer_GPU, coms_read_view_CPU)
    note over PVA: CPU to GPU copy only on GPU sim

Loading

Comments Outside Diff (2)

1. source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/ovphysx_view.py, line 822-838 (link)

   Cache keyed by id(dst) alone — no shape re-validation on cache hit for structured dtypes

   The _read_view cache maps id(dst) → the float32 reinterpret produced against a specific binding. On a cache hit the cached view is returned directly, without re-checking its flat shape against the current binding.shape. If the same structured-dtype buffer is ever passed to read_into for two different attributes whose flat shapes differ (e.g. a (N,) wp.transformf buffer used first for rigid_body_pose shape (N, 7), then for rigid_body_velocity shape (N, 6)), the stale (N, 7) reinterpret is handed to the velocity binding's read, silently writing seven floats per body into a six-float-wide region and corrupting the destination buffer with no error raised. Flat float32 buffers are safe because they are never cached, so only structured-dtype destinations are affected. Keying the cache by (id(dst), binding.shape) (or adding a shape assert on hit) would close the gap.

2. source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/ovphysx_view.py, line 474-478 (link)

   _read_views cache has no eviction and does not hold a back-reference to dst

   The cache maps id(dst) → reinterpret view and is never evicted. Each reinterpret holds a raw ptr into dst's backing allocation but carries no Python reference to dst. If dst is GC'd its warp allocation may be freed; any subsequent access through the cached view would touch freed memory. The stale-pointer guard (cached.ptr == dst.ptr) only fires when a new dst arrives at that id(), not when the cached view itself is used in a later step loop. For the sensor's persistent-buffer pattern this does not manifest, but making _read_views a WeakKeyDictionary keyed by dst directly would give both correct lifetime tracking and automatic eviction.

_{Reviews (1): Last reviewed commit: "Merge branch 'develop' into antoiner/fea..." | Re-trigger Greptile}

[AntoineRichard]

We dug into the read-cache "silent corruption" concern closely, including the wheel's native code, and it's a latent gap rather than a silent-corruption bug:

• The wheel's read/write validate the tensor shape (dtype + ndim + every dim) before any copy (validateTensorShape in ovphysxTensorBinding.cpp) and return a non-SUCCESS status that the Python wrapper raises as RuntimeError (api.py:472). A cross-attribute reuse therefore raises, it does not silently corrupt.
• It is unreachable by shipped code: each asset/sensor owns one persistent buffer per attribute (never reused across two TensorTypes), and a binding's flat shape is constant for a session — it can only change on a full sim reset, which rebuilds the view from scratch.

So we deliberately did not add a per-hit shape guard (it would add hot-path cost to defend an unreachable, already-raising case); a dtype-aware cache key is tracked as a wheel-metadata follow-up (the wheel exposes no dtype today). We did make the reset boundary explicitly tidy — every consumer now clears _root_view in _invalidate_initialize_callback.