Skip to content

Migrate OVPhysX contact-sensor pose binding onto OvPhysxView (view series, part 5)#6232

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

Migrate OVPhysX contact-sensor pose binding onto OvPhysxView (view series, part 5)#6232
AntoineRichard wants to merge 13 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_contact_sensor_view

Conversation

@AntoineRichard

Copy link
Copy Markdown
Collaborator

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

Branched off #6224 as a sibling of the other migrations. Targets develop; until #6224 merges the diff includes the Part 1 OvPhysxView commits. Please review #6224 first.

Description

Part 5 of the OVPhysX view-migration series — the contact sensor. This is the partial one: only the RIGID_BODY_POSE pose binding (the track_pose path) is a TensorBinding the view can manage, so only it is migrated. The ContactBinding (net forces via read_net_forces, force matrix via read_force_matrix) is a separate ovphysx wheel API that OvPhysxView does not wrap — it is left exactly as-is.

  • _initialize_impl builds an OvPhysxView for the pose binding (when track_pose) and obtains it via binding_for(TT.RIGID_BODY_POSE); the count-mismatch check and the pose_binding accessor are unchanged.
  • The per-step pose read goes through read_into(TT.RIGID_BODY_POSE, ...) (cached reinterpret).
  • The ContactBinding create/read/destroy paths are untouched.
  • Internal refactor; no public API change.

Type of change

  • Internal refactor (non-breaking)

Checklist

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

Testing

test_contact_sensor.py — cpu: 9 passed, 4 skipped; cuda: 9 passed, 4 skipped (exercises both the ContactBinding force path and the migrated pose-tracking path).

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.
Route the contact sensor's RIGID_BODY_POSE tensor binding (the track_pose
path) through OvPhysxView: _initialize_impl builds an OvPhysxView and gets
the pose binding via binding_for; the per-step pose read goes through
read_into. The ContactBinding (net forces / force matrix) is a separate
wheel API the view does not wrap, so it is left as-is -- this is the
partial migration noted in the series plan. Internal refactor, no public
API change.

Verified on both devices (cpu and cuda:0: 9 passed, 4 skipped each).
@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_contact_sensor_view branch from f1ed035 to 98917ed Compare June 23, 2026 10:02
@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

Part 5 of the OVPhysX view-migration series. It introduces OvPhysxView — a lazy, string-keyed manager over OVPhysX TensorBinding handles — and migrates the ContactSensor's RIGID_BODY_POSE binding onto it; the ContactBinding (net forces / force matrix) is intentionally left on the direct wheel API.

  • OvPhysxView (ovphysx_view.py): New 700-line class with lazy/eager binding creation, per-binding device policy (raises on CPU↔GPU mismatch), float32-reinterpret read-cache warming for structured Warp dtypes (wp.transformf, wp.spatial_vectorf), and a full nested error hierarchy.
  • ContactSensor migration: _initialize_impl now builds an OvPhysxView and retrieves the binding via binding_for(); the per-step read switches from _pose_binding.read(buf) to _root_view.read_into(TT.RIGID_BODY_POSE, buf). The new _root_view field is not cleared in _invalidate_initialize_callback, leaving the view alive with a destroyed cached binding after simulation stop.

Confidence Score: 4/5

Safe to merge with one small fix: _root_view should be nulled in _invalidate_initialize_callback alongside _pose_binding.

The OvPhysxView core and its tests are solid. The one gap is in _invalidate_initialize_callback: _pose_binding.destroy() is called and _pose_binding is set to None, but _root_view — which caches that same binding in _root_view._bindings — is not cleared. Any access to _root_view between simulation stop and the next _initialize_impl would return a destroyed native handle. The fix is a single self._root_view = None line.

contact_sensor.py — specifically the _invalidate_initialize_callback teardown path.

Important Files Changed

Filename Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/contact_sensor/contact_sensor.py Migrates the RIGID_BODY_POSE binding onto OvPhysxView; _root_view is not cleared in _invalidate_initialize_callback, leaving a zombie view holding a destroyed binding after simulation stop.
source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/ovphysx_view.py New string-keyed TensorBinding manager; well-structured with lazy/eager modes, read-cache warming, device policy enforcement, and thorough error hierarchy.
source/isaaclab_ovphysx/test/sim/test_ovphysx_view.py Comprehensive unit tests for OvPhysxView API mechanics against mock bindings; covers device policy, dtype safety, alias resolution, read-cache warming, and adversarial construction.
source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/init.pyi Adds OvPhysxView to the public type-stub exports; straightforward and correct.
docs/source/api/lab_ovphysx/isaaclab_ovphysx.sim.views.rst New RST page wiring the sim.views module into the API docs via automodule.
docs/source/api/index.rst Adds sim.views entry to the isaaclab_ovphysx extension index.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant CS as ContactSensor
    participant OPV as OvPhysxView
    participant PB as TensorBinding(RIGID_BODY_POSE)
    participant CB as ContactBinding

    CS->>OPV: OvPhysxView(physx, pattern, device)
    CS->>OPV: binding_for(TT.RIGID_BODY_POSE)
    OPV->>PB: physx.create_tensor_binding(RIGID_BODY_POSE)
    OPV-->>CS: _pose_binding (cached in _root_view._bindings)

    loop "Per step (track_pose=True)"
        CS->>OPV: read_into(TT.RIGID_BODY_POSE, _poses_flat_buf)
        OPV->>PB: binding.read(_read_view(dst))
    end

    Note over CS,CB: ContactBinding path unchanged
    CS->>CB: read_net_forces / read_force_matrix

    CS->>PB: _pose_binding.destroy()
    CS->>CS: "_pose_binding = None"
    Note over OPV: _root_view._bindings still holds destroyed PB
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 CS as ContactSensor
    participant OPV as OvPhysxView
    participant PB as TensorBinding(RIGID_BODY_POSE)
    participant CB as ContactBinding

    CS->>OPV: OvPhysxView(physx, pattern, device)
    CS->>OPV: binding_for(TT.RIGID_BODY_POSE)
    OPV->>PB: physx.create_tensor_binding(RIGID_BODY_POSE)
    OPV-->>CS: _pose_binding (cached in _root_view._bindings)

    loop "Per step (track_pose=True)"
        CS->>OPV: read_into(TT.RIGID_BODY_POSE, _poses_flat_buf)
        OPV->>PB: binding.read(_read_view(dst))
    end

    Note over CS,CB: ContactBinding path unchanged
    CS->>CB: read_net_forces / read_force_matrix

    CS->>PB: _pose_binding.destroy()
    CS->>CS: "_pose_binding = None"
    Note over OPV: _root_view._bindings still holds destroyed PB
Loading

Comments Outside Diff (1)

  1. source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/contact_sensor/contact_sensor.py, line 510-523 (link)

    P1 _root_view not cleared on simulation stop

    _pose_binding.destroy() is called and _pose_binding is set to None, but _root_view is left alive with the destroyed binding still cached in _root_view._bindings[TT.RIGID_BODY_POSE]. _root_view._read_views also retains the float32 reinterpret of _poses_flat_buf. If anything calls _root_view.binding_for(TT.RIGID_BODY_POSE) between simulation stop and the next _initialize_impl, it gets a destroyed native handle back. The pattern is inconsistent: every other backend handle (_contact_binding, _pose_binding, _physx_instance) is explicitly nulled out, but the view that wraps _pose_binding is not. Add self._root_view = None after self._pose_binding = None to complete the teardown.

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

_invalidate_initialize_callback destroyed _pose_binding but left _root_view
caching that same destroyed binding. Clear _root_view too so no destroyed
native handle is reachable after a simulation stop (addresses the review
note); _initialize_impl rebuilds a fresh OvPhysxView on the next play.
@AntoineRichard

Copy link
Copy Markdown
Collaborator Author

Fixed — _invalidate_initialize_callback now clears self._root_view alongside _pose_binding (commit b8453bd), so the destroyed pose binding the view caches is no longer reachable after a sim stop; _initialize_impl rebuilds a fresh view on the next play. The separate read-cache concern doesn't apply here: _poses_flat_buf is flat float32, so it's passed straight through to the binding and never cached.

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