Enable OVPhysX shape-material test via OvPhysxView#6239
Draft
AntoineRichard wants to merge 14 commits into
Draft
Enable OVPhysX shape-material test via OvPhysxView#6239AntoineRichard wants to merge 14 commits into
AntoineRichard wants to merge 14 commits into
Conversation
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.
github-actions
Bot
added
documentation
This was referenced Jun 23, 2026
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.
Migrate the OVPhysX Articulation onto the OvPhysxView binding manager (view migration series, part 2; rebased onto develop). _initialize_impl builds one OvPhysxView and populates it via try_binding_for; the data container is built from the view; both _get_binding helpers delegate to it. All reads route through read_into (cached reinterpret, structured buffers handled off the binding shape -- the transform/spatial/scalar read helpers collapse to one) and all writes through set_attribute (including CPU-only properties, pre-staged to pinned CPU). root_view now returns the OvPhysxView (breaking). Tests route through the new API. Verified on both devices (cpu and cuda:0: 99 passed, 4 xfailed each).
…View The migration changed Articulation._process_tendons to read tendon counts off self._root_view (the OvPhysxView) instead of self._bindings, but the helpers unit test still mock-injected _bindings -- so the test broke (caught by the isaaclab_ov CI job, which runs the full suite). Wrap the mock binding set in a real OvPhysxView over a fake PhysX and inject _root_view instead. test_articulation_helpers.py: 2 passed.
The articulation-helpers tendon test wrapped the mock bindings in a real OvPhysxView over a fake PhysX to satisfy the migrated Articulation, which defeats the purpose of the mock binding layer. Add a MockOvPhysxView that mirrors the consumed OvPhysxView surface (binding_for/try_binding_for, get_attribute/read_into/set_attribute, the discoverability helpers, and the metadata passthrough) over the mock bindings, exposed via MockOvPhysxBindingSet.view. Inject that as the articulation's _root_view so the test stays within the mock framework.
The ovphysx wheel now exposes per-shape material as the ARTICULATION_SHAPE_FRICTION_AND_RESTITUTION / RIGID_BODY_SHAPE_FRICTION_AND_RESTITUTION tensor types, closing the gap that kept test_set_material_properties xfailed. - Add tensor_types aliases SHAPE_FRICTION_AND_RESTITUTION and RIGID_BODY_SHAPE_FRICTION_AND_RESTITUTION (guarded for older wheels). - Rewrite test_set_material_properties to read/write the material through OvPhysxView.get_attribute / set_attribute on the new tensor type instead of the PhysX-only root_view.get/set_material_properties / max_shapes API; derive the per-articulation shape count from the binding shape [N, S, 3] and skip on wheels that lack the type. Drop the now-unused gap reason. The binding is device-resident; verified the set -> step -> read round-trip on both cpu and cuda:0.
AntoineRichard
force-pushed
the
antoiner/feat/ovphysx_shape_materials
branch
from
442c8fe to
13c58a1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The ovphysx wheel now exposes per-collision-shape material as the
ARTICULATION_SHAPE_FRICTION_AND_RESTITUTION/RIGID_BODY_SHAPE_FRICTION_AND_RESTITUTIONtensor types. This closes the gap that kepttest_set_material_propertiesxfailed: the test had been written against the PhysX-onlyroot_view.get_material_properties()/set_material_properties()/max_shapesview API, whichOvPhysxViewdoes not provide.tensor_typesaliasesSHAPE_FRICTION_AND_RESTITUTIONandRIGID_BODY_SHAPE_FRICTION_AND_RESTITUTION, guarded withtry/except AttributeErrorso the module stays import-safe against older wheels.test_set_material_propertiesto read/write the material throughOvPhysxView.get_attribute/set_attributeon the new tensor type. The per-articulation shape count is derived from the binding shape[N, S, 3]([static_friction, dynamic_friction, restitution]); the test skips on wheels that lack the type. The now-unused_MATERIAL_GAP_REASONis removed.Verification
The material binding is device-resident (it follows the simulation device). The
set → step → readround-trip passes on bothcpuandcuda:0fornum_articulations1 and 2.