Add OvPhysxView: a string-keyed binding manager for OVPhysX tensor bindings (view series, part 1)

[AntoineRichard]

📦 Part 1 of a 5-part series adding an OvPhysX binding view and migrating the assets onto it:

1. This PR — add OvPhysxView (the API) + tests.
2. Migrate the OvPhysX Articulation to root_view.
3. Migrate the RigidObject.
4. Migrate the RigidObjectCollection (uses the fused prim_paths+key_aliases path).
5. Migrate the sensors (pose tracking; contact forces use a separate ContactBinding API and may not fully adopt the view).

Parts 2–5 each depend only on this PR and can be reviewed independently.

---

Description

Adds OvPhysxView, a string-keyed binding-management layer over the OVPhysX tensor bindings, in isaaclab_ovphysx.

Why. OVPhysX is the odd backend out: it exposes physics attributes as a loose dict[TensorType, TensorBinding] with no view object, whereas Newton has selection.ArticulationView and PhysX has typed tensor views. Today callers must hold the right TensorType enum member, manage a destination buffer, and create bindings against a USD glob — and the create_tensor_binding calls are scattered across the asset classes behind a private _get_binding. OvPhysxView gives OVPhysX a single, discoverable surface as pleasant as Newton's selection API, and one owner of binding creation/caching that the asset/data classes can delegate to. It is the narrow, independently-useful slice carved out of the (shelved) cross-backend get_property/set_property effort; the full rationale and decision log live in docs/superpowers/specs/2026-06-17-ovphysx-view-design.md.

What it provides

• String-keyed access by lowercased TensorType name (auto-derived from the wheel enum — no hand-maintained table); a TensorType member is also accepted.
• get_attribute(name, out=) — fresh-allocates a typed array (wp.transformf for poses, wp.spatial_vectorf for velocities, flat float32 otherwise; via a maintained _ATTR_DTYPE map), no aliasing; read_into(name, dst) — zero-copy fill of a caller-owned, possibly structured-dtype buffer (wp.transformf, …) via a float32 reinterpret view that is cached per destination buffer so the wheel's object-identity read cache stays warm even when callers hand a structured buffer each step; set_attribute(name, values, indices=/mask=) (structured sources reinterpreted); and a raw binding_for(name) accessor for adoption.
• The fused multi-prim form (prim_paths=[...] + key_aliases) so RigidObjectCollection can use it.
• Discoverability (attribute_names, available_attributes, has_attribute), metadata passthrough, and a nested error hierarchy (UnknownAttribute / ReadOnlyAttribute / AttributeUnavailable / ShapeMismatch / DeviceMismatch).

Device policy: no implicit CPU↔GPU conversion — CPU-resident property types are read/written on CPU, and a buffer on the wrong device raises DeviceMismatch rather than being staged.

Scope / status: this PR adds the view plus 48 mock-based unit tests (no live sim). The asset adoption that replaces _get_binding/_binding_read/_binding_write lives in the follow-up PRs (Articulation #6225, RigidObject #6226), which dogfood the view against a live sim on CPU and GPU and have fed fixes back here (e.g. try_binding_for, the read-view cache, typed get_attribute).

Fixes # — N/A (internal design tracking; surviving slice of the shelved cross-backend view effort).

Type of change

• New feature (non-breaking change which adds functionality)

Screenshots

N/A — no user-facing visual change.

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my feature works (test/sim/test_ovphysx_view.py, 48 tests)
• 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)
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[greptile-apps]

Greptile Summary

This PR introduces OvPhysxView, a string-keyed binding-management layer over OVPhysX tensor bindings that gives OVPhysX a Newton-style ergonomic surface: attributes are addressed by lowercased TensorType name, bindings are lazily created and cached, and an explicit no-implicit-CPU↔GPU device policy is enforced.

• Adds OvPhysxView with lazy/eager binding creation, get_attribute / read_into / set_attribute / binding_for / try_binding_for APIs, a nested error hierarchy, and metadata passthrough from sample bindings.
• Includes a float32 read-view cache (_read_views) designed to keep the wheel's object-identity read cache warm for persistent destination buffers, key-alias support for the RigidObjectCollection fused-binding shape, and 48 mock-based unit tests covering construction, dtype safety, device policy, and the cache mechanism.

Confidence Score: 3/5

The new API is well-designed and test coverage is thorough, but the no-out get_attribute branch has a memory-management defect that should be fixed before the follow-up PRs adopt this layer in a live sim.

The no-out branch of get_attribute routes a freshly-allocated buf through _read_view on every call — cached entries are never reused and warp copy=False views may keep the base array alive, causing indefinite growth of _read_views in any simulation loop that calls get_attribute for convenience. The fix is a one-line change but should land before the series continues.

source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/ovphysx_view.py — specifically the no-out branch of get_attribute (line 328) and the _as_binding_view dtype-check error type (line 588).

Important Files Changed

Filename	Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/ovphysx_view.py	Core OvPhysxView implementation — well-structured with solid error hierarchy and device-policy enforcement, but get_attribute (no-out) feeds a freshly-allocated buffer through the read-view cache on every call causing unbounded _read_views growth; dtype errors in _as_binding_view are surfaced as ShapeMismatch; torch detection is fragile.
source/isaaclab_ovphysx/test/sim/test_ovphysx_view.py	48 mock-based unit tests covering construction, get/set/read-into paths, device policy, key aliases, read-view caching, dtype safety, and adversarial resolution — comprehensive and well-annotated.
source/isaaclab_ovphysx/isaaclab_ovphysx/sim/views/init.pyi	Adds OvPhysxView to all and import list — trivial, correct.
docs/source/api/lab_ovphysx/isaaclab_ovphysx.sim.views.rst	New Sphinx automodule stub — correct and minimal.
source/isaaclab_ovphysx/changelog.d/ovphysx-view.major.rst	Changelog fragment for the new OvPhysxView — accurate and well-written.

_{Reviews (1): Last reviewed commit: "docs(ovphysx): add isaaclab_ovphysx.sim...." | Re-trigger Greptile}

[greptile-apps]

_read_views grows without bound on every no-out structured-dtype get_attribute call

buf is freshly allocated on every call, so id(buf) is unique and the cached float32 reinterpret stored at _read_views[id(buf)] can never be hit on a subsequent call. Because warp's copy=False arrays commonly keep a reference to their base buffer, the float32 view in the dict may also keep buf alive, preventing GC. In a simulation loop that calls view.get_attribute("rigid_body_pose") each step, _read_views accumulates one float32-view (and potentially one structured array) per step, growing indefinitely for the life of the view.

The cache only pays off for a persistent dst that is reused across calls — which is exactly the read_into / get_attribute(out=) path. In the no-out allocation path, _as_binding_view should be called directly to bypass the cache.

[greptile-apps]

ShapeMismatch is raised here but the actual fault is a dtype mismatch — the array's scalar type is not float32, not that the shape is wrong. A caller who supplies wp.int32 data will see ShapeMismatch in their traceback and assume they got the dimensions wrong when the real issue is the element type. Consider a dedicated error subclass or at minimum reusing OvPhysxViewError with a dtype-focused message.

Suggested change

	if scalar is not wp.float32:
	raise OvPhysxView.ShapeMismatch(
	f"{role} must have float32 scalar elements (got dtype "
	f"{getattr(arr.dtype, '__name__', arr.dtype)}); the view reinterprets bits, "
	"not values, so a non-float32 dtype would silently corrupt the buffer."
	)
	if scalar is not wp.float32:
	raise OvPhysxView.OvPhysxViewError(
	f"{role} must have float32 scalar elements (got dtype "
	f"{getattr(arr.dtype, '__name__', arr.dtype)}); the view reinterprets bits, "
	"not values, so a non-float32 dtype would silently corrupt the buffer."
	)

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!

[greptile-apps]

Detecting PyTorch tensors by string-matching the module name is fragile: it can be fooled by any user class in a package named torch, and silently skips torch.Tensor subclasses whose __module__ is not torch.*. The standard pattern is an isinstance guard after a lazy import.

Suggested change

	if type(values).__module__.split(".")[0] == "torch":
	return wp.from_torch(values)
	try:
	import torch as _torch # noqa: PLC0415 (lazy import: torch is optional)
	if isinstance(values, _torch.Tensor):
	return wp.from_torch(values)
	except ImportError:
	pass

[marcodiiga]

A few API-level comments. Overall this looks useful as an IsaacLab-side consolidation layer; my main concern is keeping the public contract narrow until dtype/access metadata comes from the wheel instead of local tables.

[marcodiiga]

API concern: this advertises every TensorType as an OvPhysxView attribute, but the implementation is still largely float32-oriented. The tensor surface includes non-float bindings such as rigid_body_disable_simulation (uint8/bool), so attribute_names / has_attribute can imply support for names that get_attribute will allocate/read with the wrong dtype. I think this should either filter to the currently supported subset, or wait on wheel-exposed dtype metadata before exposing the full TensorType vocabulary as supported attributes.

[AntoineRichard]

Good point. Note that in 0.4.13 all 55 TensorType members are float32 — rigid_body_disable_simulation isn't in the wheel yet — so nothing is mis-read today; this is forward-looking. Rather than guess a subset with another local table, I narrowed the documented contract instead: the module docstring + attribute_names/has_attribute now state they report name-validity, not dtype support, and that the view is float32-only pending wheel dtype metadata. Filed TensorBinding.dtype as the upstream ask (design §7 ask 2); once it lands we can filter attribute_names to a dtype-aware subset and drop the guesswork. Confirmed offline you're planning to expose this.

[marcodiiga]

Can we keep this local read-only list clearly temporary? The wheel has at least three access modes in practice: read/write, read-only, and write-only control tensors. A local _READ_ONLY_NAMES table will drift as TensorType grows, and a future wheel API should probably expose an access_mode enum rather than a boolean is_writable flag.

[AntoineRichard]

Agreed. Marked _READ_ONLY_NAMES explicitly temporary and reframed the upstream ask around an access_mode enum (read/write, read-only, write-only) rather than a boolean is_writable, so write-only control tensors stay distinguishable. It replaces the hand-maintained table (design §7 ask 3).

[marcodiiga]

key_aliases makes sense for the collection/fused-binding adapter path, but it is a subtle semantic escape hatch: the requested key and the created binding type can differ. That is fine as an internal IsaacLab adapter, but I would avoid presenting it as the long-term public API without descriptor metadata that records the requested key, source tensor type, shape, native device, and access mode. Otherwise callers can reason from the visible key and get different runtime semantics.

[AntoineRichard]

Agreed — documented key_aliases as an internal IsaacLab adapter for the fused-collection binding path, explicitly not general public API (requested key vs created type intentionally differ), and noted that a public form should carry descriptor metadata (requested key, source tensor type, shape, native device, access mode), deferred to wheel metadata.

[marcodiiga]

This PR itself appears to add mock/fake-binding coverage only. That is good for the API mechanics, but before this becomes the hot-path root_view implementation I would like the follow-up/live coverage to explicitly exercise CPU-only properties on a GPU sim, write-only/read-only failures, and at least one structured read_into path against a real ovphysx binding. If that is intentionally covered by later asset-integration PRs, maybe make that expectation explicit here so the current test scope is not overstated.

[AntoineRichard]

Made the scope explicit in the test module docstring: these cover the view's API mechanics against mock bindings; live coverage — CPU-only properties on a GPU sim, read-only/write-only failures, and structured read_into against real bindings — is in the asset-integration tests (test_articulation.py / test_rigid_object.py / test_rigid_object_collection.py) that adopt this as root_view.

[AntoineRichard]

For the record (post-merge): the no-out get_attribute growth flagged here was fixed before merge — that path bypasses _read_view and calls _as_binding_view directly, with test_get_attribute_no_out_does_not_grow_read_cache asserting _read_views stays empty. The cross-attribute / eviction concerns later raised on the stacked PRs were verified as latent and unreachable by shipped code (replies on #6229 / #6231 / #6232): the wheel validates tensor shape natively before any copy and raises on mismatch, and every consumer uses one persistent buffer per attribute, so the cache stays correct and bounded.