Migrate OVPhysX Articulation onto OvPhysxView (view series, part 2)

[AntoineRichard]

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

This PR depends on #6224 and is stacked on top of it. It targets
develop, but because #6224 has not merged yet, the diff below includes the
Part 1 OvPhysxView commits as well as the Part 2 migration. Please
review #6224 first. Once #6224 merges, this branch will be rebased onto
develop and the diff will shrink to the articulation migration only.

Description

Part 2 of the OVPhysX view-migration series. It migrates the OVPhysX
Articulation onto the new OvPhysxView binding manager introduced in
#6224 (Part 1).

The goal of the series is to dogfood OvPhysxView on the real asset classes
to surface weaknesses before the rigid-object/collection/sensor migrations, and
to consolidate the scattered create_tensor_binding / binding.read /
binding.write calls behind one managed surface.

What changed

• Articulation._initialize_impl now builds a single
  OvPhysxView(physx, pattern, device) and populates it via try_binding_for
  (best-effort: tensor types that do not apply to these prims are skipped, the
  asset still raises if no binding could be created). All binding creation,
  caching, and the CPU/GPU device policy now live in the view.
• ArticulationData is constructed from the view; its counts come from the
  view metadata, and both _get_binding helpers (asset + data container)
  delegate to OvPhysxView.try_binding_for.
• Every write now goes through root_view.set_attribute(...) instead of
  calling binding.write on a binding obtained from the view — the ~70 property
  / state / target / wrench / tendon writes, including the per-step actuation
  path. This brings the view's read-only, device, dtype, and contiguity guards
  to the asset write path, which previously bypassed them.
• Every read now goes through root_view.read_into(...). ArticulationData
  drops its bespoke _get_read_view reinterpret cache; the view now caches the
  float32 reinterpret per destination buffer (keeping the wheel's read cache
  warm) and derives the structured layout from the binding shape, so the
  transform / spatial-vector / scalar read helpers collapse to one
  implementation. CPU-only reads keep their pinned-host staging.
• Articulation.root_view now returns the OvPhysxView instead of a raw
  dict[TensorType, TensorBinding] (breaking — see migration note below).
• The articulation tests route through the new API
  (root_view.get_attribute(...), root_view.try_binding_for(...)).

Breaking change / migration

Articulation.root_view returns an OvPhysxView, not a dict:

• root_view[tensor_type] → root_view.try_binding_for(tensor_type)
• tensor_type in root_view (availability) → root_view.try_binding_for(tensor_type) is not None
• full-tensor read → root_view.get_attribute(tensor_type)

Findings surfaced (the point of the exercise)

• CPU-only property writes work through set_attribute. mass/COM/inertia/DOF
  limits/stiffness/… are CPU-resident even on a GPU sim, and set_attribute
  refuses to stage CPU↔GPU. This is fine because the asset already pre-stages
  those to pinned CPU buffers, so the source is on the binding's native (CPU)
  device and the no-stage policy is satisfied with no change. (An initial
  assumption that the device policy would block these turned out to be wrong —
  verified by the green cuda:0 run.)
• read_into allocates a fresh float32 reinterpret view per call, which would
  defeat the wheel binding's internal read cache that the data container caches
  by (tensor_type, ptr); hot-path reads were kept on the cached-view path.
• Per-step writes re-resolve the binding through set_attribute (a dict lookup
  • cheap guards) rather than the previously cached binding handle; the cost is
    negligible because the pre-built float32 write-view passes through the
    reinterpret unchanged. A "bound writer" handle on the view would remove even
    that lookup — a possible Part 1 follow-up.
• The view rejects 0-count bindings (stricter than the old loop, which could
  store a phantom binding) — a correctness improvement.

Type of change

• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Bug fix (non-breaking change which fixes an issue)
• This change requires a documentation update

Checklist

• I have run the pre-commit checks with ./isaaclab.sh --format
• I have added tests that prove my fix is effective or that my feature works (articulation tests route through the new API)
• I have updated the changelog (source/isaaclab_ovphysx/changelog.d/)
• I have updated the documentation accordingly
• I have added my changes to the right changelog.d fragment(s)

Testing

Run kitless; cpu and cuda must be separate invocations (process-global device lock):

101 passed, 4 xfailed   # -k cpu
101 passed, 4 xfailed   # -k cuda

[greptile-apps]

Greptile Summary

This PR migrates the OVPhysX Articulation and ArticulationData classes from a raw dict[TensorType, TensorBinding] to the OvPhysxView binding manager introduced in Part 1 (#6224). Every read and write — roughly 70 property/state/target/tendon paths — now flows through root_view.read_into / root_view.set_attribute, bringing the view's device, dtype, and contiguity guards to all write paths.

• Articulation: _bindings dict replaced by _root_view: OvPhysxView; bindings created eagerly via try_binding_for during _initialize_impl; all write calls migrated from binding.write(...) to root_view.set_attribute(...); root_view property now returns OvPhysxView (breaking change, documented in changelog and migration note).
• ArticulationData: Accepts OvPhysxView instead of raw bindings dict; drops the bespoke _get_read_view reinterpret cache and three specialized read helpers, replacing them with a single unified _read_binding_into_buf; CPU-only staging path unchanged.
• Mock and tests: MockOvPhysxView added to mirror the consumed OvPhysxView surface for kitless unit tests; all initialization and binding-shape invariant checks in test_articulation.py updated to use the new API.

Confidence Score: 4/5

The production code paths are mechanically correct and well-guarded; the real-backend test suite passes cleanly. The only open items are in the new MockOvPhysxView test double, where a structured-dtype mismatch in read_into and a StopIteration edge case on empty bindings are latent gaps that do not affect current tests but will surface when mock-based tests exercise transform or velocity reads.

The core migration is thorough and consistent — every write and read path has been updated, the per-step guards are semantically equivalent to the old binding-existence checks, and the CPU staging path is preserved intact. The three retained binding instance attributes are unused after init but harmless. The mock surface gaps are real but confined to test infrastructure and do not affect simulation correctness.

mock_ovphysx_bindings.py — MockOvPhysxView.read_into and _resolve have latent correctness gaps that will surface when mock-based tests exercise transform/velocity reads.

Important Files Changed

Filename	Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation.py	Core migration: replaces _bindings dict with OvPhysxView; all ~70 property/state writes now route through set_attribute and all reads through read_into/_binding_read. Correctness logic is sound and well-guarded; minor cleanup gap with retained _effort_binding / _pos_target_binding / _vel_target_binding attributes.
source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation_data.py	Migrated to accept OvPhysxView instead of raw bindings dict; removed _get_read_view reinterpret cache and three specialized read helpers, replacing them with a single _read_binding_into_buf aliased to all three names. CPU staging path for structured dtypes now derived from staging.shape rather than binding.shape directly — equivalent and correct.
source/isaaclab_ovphysx/isaaclab_ovphysx/test/mock_interfaces/views/mock_ovphysx_bindings.py	New MockOvPhysxView mirrors the OvPhysxView surface for unit tests; two latent issues: _resolve throws StopIteration on empty bindings, and read_into passes raw structured-dtype arrays to MockTensorBinding.read which will fail in future mock-based tests that read transform/velocity bindings.
source/isaaclab_ovphysx/test/assets/test_articulation.py	Correctly migrated to use root_view.try_binding_for / root_view.get_attribute API instead of dict indexing; removed unused numpy import; all binding-shape invariant checks updated throughout.
source/isaaclab_ovphysx/test/assets/test_articulation_helpers.py	Single-line change: injects bindings.view (MockOvPhysxView) instead of bindings.bindings (raw dict) into _root_view; correctly adapts the helper shell for the migrated Articulation API.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Sim as Simulation Step
    participant Art as Articulation
    participant View as OvPhysxView
    participant Bind as TensorBinding
    participant Data as ArticulationData

    Note over Art,View: _initialize_impl
    Art->>View: OvPhysxView(physx, pattern, device)
    loop eager_types
        Art->>View: try_binding_for(tt)
        View->>Bind: create_tensor_binding(pattern, tt)
        View-->>Art: binding (cached)
    end
    Art->>Data: ArticulationData(root_view, device)

    Note over Art,Bind: write_data_to_sim (per-step)
    Sim->>Art: write_data_to_sim()
    Art->>View: set_attribute(DOF_ACTUATION_FORCE, effort_view)
    View->>Bind: binding.write(values, indices, mask)
    Art->>View: set_attribute(DOF_POSITION_TARGET, pos_view)
    View->>Bind: binding.write(values, indices, mask)

    Note over Data,Bind: lazy state read (property access)
    Sim->>Data: joint_pos (property)
    Data->>Data: _read_binding_into_buf(DOF_POSITION, buf)
    Data->>View: read_into(DOF_POSITION, buf.data)
    View->>Bind: binding.read(float32_view)
    Data-->>Sim: cached tensor

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 Sim as Simulation Step
    participant Art as Articulation
    participant View as OvPhysxView
    participant Bind as TensorBinding
    participant Data as ArticulationData

    Note over Art,View: _initialize_impl
    Art->>View: OvPhysxView(physx, pattern, device)
    loop eager_types
        Art->>View: try_binding_for(tt)
        View->>Bind: create_tensor_binding(pattern, tt)
        View-->>Art: binding (cached)
    end
    Art->>Data: ArticulationData(root_view, device)

    Note over Art,Bind: write_data_to_sim (per-step)
    Sim->>Art: write_data_to_sim()
    Art->>View: set_attribute(DOF_ACTUATION_FORCE, effort_view)
    View->>Bind: binding.write(values, indices, mask)
    Art->>View: set_attribute(DOF_POSITION_TARGET, pos_view)
    View->>Bind: binding.write(values, indices, mask)

    Note over Data,Bind: lazy state read (property access)
    Sim->>Data: joint_pos (property)
    Data->>Data: _read_binding_into_buf(DOF_POSITION, buf)
    Data->>View: read_into(DOF_POSITION, buf.data)
    View->>Bind: binding.read(float32_view)
    Data-->>Sim: cached tensor

Loading

Comments Outside Diff (3)

1. source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation.py, line 3671-3697 (link)

   Binding references retained post-migration but no longer used for writes

   self._effort_binding, self._pos_target_binding, and self._vel_target_binding are each stored as instance attributes during _initialize_impl but are now used only to derive the write-view shape. All three writes go through root_view.set_attribute using the per-step guards on the corresponding *_write_view. The stored binding references keep live objects alive on self without serving a runtime purpose — using local variables during init and not assigning them to self would complete the cleanup the PR sets out to do.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

2. source/isaaclab_ovphysx/isaaclab_ovphysx/test/mock_interfaces/views/mock_ovphysx_bindings.py, line 1333-1342 (link)

   _resolve throws StopIteration on empty bindings dict

   next(iter(self._bindings)) raises StopIteration (not KeyError or a readable error) when _bindings is empty and any string name is passed to a method that calls _resolve. In practice this cannot occur because MockOvPhysxView is always constructed via MockOvPhysxBindingSet.view with a non-empty dict, but if a caller constructs a bare MockOvPhysxView({}) for an edge-case test, the failure will be cryptic. Guarding with if not self._bindings: raise KeyError(...) before the next(iter(...)) call would give a clearer diagnostic.

3. source/isaaclab_ovphysx/isaaclab_ovphysx/test/mock_interfaces/views/mock_ovphysx_bindings.py, line 1361-1363 (link)

   read_into passes structured dtypes directly to MockTensorBinding.read, causing a shape/dtype mismatch for transform and spatial-vector bindings

   MockTensorBinding.read does wp.copy(tensor, tmp) where tmp is always a flat float32 array of shape binding.shape (e.g. (N, 7)) and tensor is whatever dst the caller passes. The production _binding_read path now sends the raw wp.transformf buffer (shape (N, 1)) directly to read_into; the old _read_transform_binding always passed a pre-built float32 reinterpret view of matching shape. Warp rejects a wp.copy between arrays whose element counts differ even when the total byte size matches, so any future mock-based test that reads a transform or spatial-vector binding (e.g. ROOT_POSE, LINK_POSE, ROOT_VELOCITY) through _binding_read will fail. The real view handles the reinterpret internally, so the real-backend tests pass, but the gap is latent in the mock surface.

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