Skip to content

Migrate OVPhysX IMU sensor onto OvPhysxView (view series, part 5)#6228

Open
AntoineRichard wants to merge 13 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_imu_view
Open

Migrate OVPhysX IMU sensor onto OvPhysxView (view series, part 5)#6228
AntoineRichard wants to merge 13 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_imu_view

Conversation

@AntoineRichard

Copy link
Copy Markdown
Collaborator

🔗 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 IMU 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_imu.py — cpu: 12 passed, cuda: 12 passed.

OVPhysX exposes physics attributes as a loose dict of TensorType ->
TensorBinding with no view object, unlike Newton's
selection.ArticulationView and PhysX's typed tensor views. OvPhysxView
wraps the bindings for one prim pattern behind a string-keyed
get_attribute / set_attribute surface, addressing attributes by the
lowercased TensorType enum name (e.g. "articulation_dof_stiffness").
It needs no Model/State/Control source object because the TensorType
already implies where the data lives.

Prototype per docs/superpowers/specs/2026-06-17-ovphysx-view-design.md.
Adds unit tests covering name<->enum resolution, the read-only guard,
discoverability, and get/set dispatch against a fake binding (no native
simulation required).
Reworks the view from a convenience wrapper into a layer that can back the
OVPhysX asset/data classes, per the PR review of isaac-sim#6224:

- read_into(name, dst): zero-copy fill of a caller-owned, possibly
  structured-dtype buffer (e.g. wp.transformf) via a float32 reinterpret
  view -- the mechanism the data containers use today.
- set_attribute: accepts structured-dtype sources via the same reinterpret;
  non-float32-width buffers are rejected rather than silently bit-cast.
- prim_paths + key_aliases: support the fused multi-prim binding form
  (create_tensor_binding(prim_paths=[...])) and storing a binding under a
  different TensorType key, as RigidObjectCollection needs.
- binding_for(): raw TensorBinding accessor for adoption.
- _CPU_ONLY_NAMES is now derived from tensor_types._CPU_ONLY_TYPES (no drift).
- Added joint/tendon/is_fixed_base metadata passthrough; eager construction
  raises if it creates zero bindings; get_attribute allocates a fresh buffer
  (no aliasing); nested error hierarchy; PhysX/binding Protocols.

Device policy: no implicit CPU<->GPU conversion. CPU-resident property types
are read/written on CPU; a buffer on the wrong device raises DeviceMismatch
instead of being staged. Device-less host data (numpy/list) is materialized
on the binding's native device.
The OvPhysxView addition is a significant new public surface for the OVPhysX
backend, so promote the changelog fragment from a minor to a major bump.
Reword the entry to describe the binding-manager surface (read_into, the
no-device-conversion policy) and drop the internal design-note path from the
user-facing changelog.
From the second PR review of isaac-sim#6224:

- Critical: _as_binding_view now requires a float32 scalar dtype before the
  zero-copy reinterpret. A same-byte-width wrong dtype (int32) previously passed
  the count-only guard and was bit-reinterpreted into garbage on the write path;
  sub-4-byte dtypes (float16) produced a misleading "0 elements" error. Both are
  now rejected with a clear message. Regression tests added (verified failing
  without the guard).
- _resolve enforces the str | TensorType union and raises UnknownAttribute on
  anything else, instead of letting a bogus key reach the wheel.
- _binding accesses binding.count directly (a malformed binding surfaces instead
  of being masked as a phantom no-match) and surfaces the underlying
  create_tensor_binding exception in the AttributeUnavailable message.
- Added docstrings to the six metadata properties; dropped the unused
  runtime_checkable decorator.
- Tests: same-byte/sub-4-byte dtype rejection, get_attribute(out=) wrong device,
  both indices+mask forwarded, read/write through a prim_paths+key_aliases view,
  non-str/non-TensorType key, and a read-only-names-are-valid-vocabulary check.
From the API-hardening review of isaac-sim#6224. Validate at the boundary and fail loud
instead of silently corrupting, mis-binding, or no-op'ing:

- Reject non-contiguous buffers in _as_binding_view (a strided/sliced source would
  be reinterpreted as contiguous and read/write the wrong memory).
- Canonicalize the device (wp.get_device) so a "cuda" view accepts a "cuda:0"
  buffer instead of raising a spurious, unsatisfiable DeviceMismatch; falls back
  to the raw string when the device can't be resolved locally.
- Reject TensorType.INVALID via the member path too (string path already did).
- Normalize key_aliases to TensorType members so string keys are honored rather
  than silently dropped, and reject aliases that cross the CPU/GPU residency or
  read-only boundary (the device/read-only guards key on the requested type).
- Reject empty pattern/prim_paths and tensor_types-without-eager at construction.
- Eager construction with an explicit tensor_types list now surfaces a failing
  type instead of swallowing it at debug level (default sweep still skips
  inapplicable types).
- Document binding_for as an unguarded escape hatch, get_attribute's native-device
  return, and the has_attribute name-validity-vs-availability split.

Adds regression tests for each (contiguity and INVALID verified failing without
the guard).
Surfaced by dogfooding the view in the articulation migration: the assets branch
on a "binding or None" pattern for optional/absent bindings (tendon types on a
tendon-less articulation, not-yet-created bindings), which the raising binding_for
can't express. try_binding_for returns None when the attribute is valid but not
available for the view's prims, while still raising UnknownAttribute for an invalid
name (a programming error, not an availability question).
…xView

read_into now reuses the float32 reinterpret of a destination buffer
across calls (keyed by buffer id, with a pointer-staleness guard), so the
wheel's object-identity read cache stays warm even when callers hand a
structured buffer each step -- they no longer need to maintain their own
reinterpret cache. get_attribute returns a typed array for attributes with
a known structured layout (transformf for poses, spatial_vectorf for
velocities, via a hand-maintained _ATTR_DTYPE map) and flat float32
otherwise.

This lets the asset data containers drop their bespoke _get_read_view
caching and read structured buffers straight through the view.
OvPhysxView (and OvPhysxFrameView) live in isaaclab_ovphysx.sim.views,
which had no API-docs page. Add the automodule stub and wire it into the
isaaclab_ovphysx autosummary so the new binding-manager view shows up in
the rendered API reference alongside assets / cloner / physics.
Three fixes from the Part 1 review:

- get_attribute (no out): route the freshly allocated buffer through
  _as_binding_view directly instead of the id()-keyed read cache. A fresh
  buffer can never hit the cache and would leak one entry (keeping the
  buffer alive) per call in a step loop; the cache only pays off for a
  reused out/dst buffer. Add a regression test asserting the no-out path
  leaves _read_views empty.

- Raise a dedicated DtypeMismatch instead of ShapeMismatch when a buffer's
  scalar element type is not float32, so a dtype error no longer reads as a
  dimensions error. Update the affected tests and the Raises docstrings.

- Make the view Warp-native: drop the fragile __module__ string-match that
  auto-converted Torch tensors on writes. Callers bridge a Torch tensor
  with wp.from_torch(t), keeping the device policy explicit and avoiding an
  optional Torch dependency.
Documentation/comment clarifications from the isaac-sim#6224 review (no behavior change):

- Narrow the documented contract to float32-only: attribute_names/has_attribute
  and the module docstring now state that a listed name is name-validity, not a
  dtype-support promise; non-float dtype handling awaits wheel dtype metadata.
- Mark _READ_ONLY_NAMES explicitly temporary; name the three access modes
  (read/write, read-only, write-only) and the wheel access_mode enum that should
  replace the table.
- Document key_aliases as an internal collection adapter, not general public API,
  pending descriptor metadata.
- Make the view test scope explicit: mock API mechanics here; live
  CPU-only-on-GPU / read-only+write-only / structured read_into coverage lives in
  the asset-integration tests.
…xView

Replace the three standalone create_tensor_binding calls in the OVPhysX
IMU sensor with a single OvPhysxView over the rigid-body pattern, and route
the per-step reads through read_into. The pose and velocity buffers are
filled directly as structured-dtype destinations (read_into builds and
caches the float32 reinterpret itself), dropping the manual wp.array
reinterpret views. RIGID_BODY_COM_POSE is CPU-only, so the pinned-host
staging is preserved: read_into fills the pinned CPU buffer and the
existing wp.copy moves it to the GPU kernel buffer (skipped on a CPU sim
where the two alias).

No public API or numeric behavior change.
@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_imu_view branch from 0d68984 to 7682051 Compare June 23, 2026 10:01
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jun 23, 2026
@AntoineRichard AntoineRichard marked this pull request as ready for review June 25, 2026 11:25
@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR routes the OVPhysX IMU sensor's tensor-binding management through the OvPhysxView abstraction introduced in Part 1 (#6224). It is a pure internal refactor — public API and numeric behavior are unchanged.

  • Three direct physx_instance.create_tensor_binding(...) calls are replaced by OvPhysxView(physx_instance, ...) construction plus binding_for per type; the raw binding objects remain available as _pose_binding / _vel_binding / _com_binding for shape/count metadata used in _initialize_buffers_impl.
  • Manual float32 reinterpret views (_transforms_view, _velocities_view) are removed; read_into now builds and caches those views inside OvPhysxView._read_views, preserving the wheel's object-identity read cache warm-path.
  • The CPU-only RIGID_BODY_COM_POSE staging pattern (pinned CPU read buffer → wp.copy to GPU) is kept intact; read_into enforces the device check on the caller-supplied CPU buffer, which is correct.

Confidence Score: 5/5

Safe to merge — the change is a drop-in substitution with no observable behavioral difference.

Every read path was traced end-to-end: read_into for the two GPU-resident state bindings (pose, velocity) replicates the old manual wp.array(ptr=..., dtype=float32) reinterpret-then-read, now cached inside OvPhysxView._read_views. The CPU-only COM-pose staging (pinned CPU buffer → wp.copy to the GPU kernel buffer) is structurally identical to the previous code. Raw binding objects are still stored on self._*_binding so _initialize_buffers_impl's shape/count queries are unaffected. No data transformation, no API surface, and no kernel changes.

No files require special attention.

Important Files Changed

Filename Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/imu/imu.py Core refactor: replaces three direct create_tensor_binding calls with OvPhysxView, drops manually managed float32 reinterpret views, and delegates cached reinterpret-and-read to read_into. Logic is functionally equivalent; COM-pose staging pattern preserved correctly.
source/isaaclab_ovphysx/changelog.d/antoiner-feat-ovphysx_imu_view.skip Empty skip file — correctly signals no user-facing changelog entry for this internal refactor.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Imu
    participant OvPhysxView
    participant PhysXBinding

    Note over Imu: _initialize_impl
    Imu->>OvPhysxView: OvPhysxView(physx, pattern, device)
    Imu->>OvPhysxView: binding_for(RIGID_BODY_POSE)
    OvPhysxView->>PhysXBinding: create_tensor_binding(RIGID_BODY_POSE)
    PhysXBinding-->>OvPhysxView: pose_binding (cached)
    OvPhysxView-->>Imu: pose_binding
    Imu->>OvPhysxView: binding_for(RIGID_BODY_VELOCITY)
    OvPhysxView->>PhysXBinding: create_tensor_binding(RIGID_BODY_VELOCITY)
    PhysXBinding-->>OvPhysxView: vel_binding (cached)
    OvPhysxView-->>Imu: vel_binding
    Imu->>OvPhysxView: binding_for(RIGID_BODY_COM_POSE)
    OvPhysxView->>PhysXBinding: create_tensor_binding(RIGID_BODY_COM_POSE)
    PhysXBinding-->>OvPhysxView: com_binding (cached, CPU-only)
    OvPhysxView-->>Imu: com_binding

    Note over Imu: _update_buffers_impl (each step)
    Imu->>OvPhysxView: read_into(RIGID_BODY_POSE, _transforms)
    OvPhysxView->>OvPhysxView: "_read_view(_transforms) -> float32 reinterpret (cached)"
    OvPhysxView->>PhysXBinding: pose_binding.read(reinterpret_view)
    Imu->>OvPhysxView: read_into(RIGID_BODY_VELOCITY, _velocities)
    OvPhysxView->>OvPhysxView: "_read_view(_velocities) -> float32 reinterpret (cached)"
    OvPhysxView->>PhysXBinding: vel_binding.read(reinterpret_view)
    Imu->>OvPhysxView: read_into(RIGID_BODY_COM_POSE, _coms_read_view[CPU])
    OvPhysxView->>PhysXBinding: com_binding.read(_coms_read_view)
    alt GPU simulation
        Imu->>Imu: "wp.copy(_coms_gpu_view <- _coms_read_view)"
    end
    Imu->>Imu: wp.launch(imu_update_kernel)
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 Imu
    participant OvPhysxView
    participant PhysXBinding

    Note over Imu: _initialize_impl
    Imu->>OvPhysxView: OvPhysxView(physx, pattern, device)
    Imu->>OvPhysxView: binding_for(RIGID_BODY_POSE)
    OvPhysxView->>PhysXBinding: create_tensor_binding(RIGID_BODY_POSE)
    PhysXBinding-->>OvPhysxView: pose_binding (cached)
    OvPhysxView-->>Imu: pose_binding
    Imu->>OvPhysxView: binding_for(RIGID_BODY_VELOCITY)
    OvPhysxView->>PhysXBinding: create_tensor_binding(RIGID_BODY_VELOCITY)
    PhysXBinding-->>OvPhysxView: vel_binding (cached)
    OvPhysxView-->>Imu: vel_binding
    Imu->>OvPhysxView: binding_for(RIGID_BODY_COM_POSE)
    OvPhysxView->>PhysXBinding: create_tensor_binding(RIGID_BODY_COM_POSE)
    PhysXBinding-->>OvPhysxView: com_binding (cached, CPU-only)
    OvPhysxView-->>Imu: com_binding

    Note over Imu: _update_buffers_impl (each step)
    Imu->>OvPhysxView: read_into(RIGID_BODY_POSE, _transforms)
    OvPhysxView->>OvPhysxView: "_read_view(_transforms) -> float32 reinterpret (cached)"
    OvPhysxView->>PhysXBinding: pose_binding.read(reinterpret_view)
    Imu->>OvPhysxView: read_into(RIGID_BODY_VELOCITY, _velocities)
    OvPhysxView->>OvPhysxView: "_read_view(_velocities) -> float32 reinterpret (cached)"
    OvPhysxView->>PhysXBinding: vel_binding.read(reinterpret_view)
    Imu->>OvPhysxView: read_into(RIGID_BODY_COM_POSE, _coms_read_view[CPU])
    OvPhysxView->>PhysXBinding: com_binding.read(_coms_read_view)
    alt GPU simulation
        Imu->>Imu: "wp.copy(_coms_gpu_view <- _coms_read_view)"
    end
    Imu->>Imu: wp.launch(imu_update_kernel)
Loading

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

Add _invalidate_initialize_callback to clear _root_view and the borrowed
pose/velocity/com bindings on simulation stop, so no stale native handle is
held across the reset; _initialize_impl rebuilds a fresh OvPhysxView on play.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant