Skip to content

Commit f1f9576

Browse files
AntoineRichardisaaclab-bot[bot]
authored andcommitted
Fix OVPhysX device routing for CPU-resident tensor types (#7563)
# Description `_CPU_ONLY_TYPES` in `isaaclab_ov` is the single constant deciding whether an `OvPhysxView` tensor buffer is allocated on, and validated against, host or device memory. It omitted eight tensor types that are CPU-resident even on a GPU simulation: | tensor type | alias | shape | |---|---|---| | `ARTICULATION_CONTACT_OFFSET` | `CONTACT_OFFSET` | `[N, S]` | | `ARTICULATION_REST_OFFSET` | `REST_OFFSET` | `[N, S]` | | `RIGID_BODY_CONTACT_OFFSET` | `RIGID_BODY_CONTACT_OFFSET` | `[N, S]` | | `RIGID_BODY_REST_OFFSET` | `RIGID_BODY_REST_OFFSET` | `[N, S]` | | `ARTICULATION_BODY_DISABLE_GRAVITY` | `BODY_DISABLE_GRAVITY` | `[N, L]` | | `RIGID_BODY_DISABLE_GRAVITY` | `RIGID_BODY_DISABLE_GRAVITY` | `[N]` | | `ARTICULATION_DOF_DRIVE_TYPE` | `DOF_DRIVE_TYPE` | `[N, D]` | | `ARTICULATION_DOF_DRIVE_MODEL` | `DOF_DRIVE_MODEL` | `[N, D, 3]` | Because `_native_device` consults this set, it returned the simulation device for host-resident data. Two consequences: 1. A hidden per-call host-to-device staging copy on every access, on a path the view believed was device-native. Silent, and not attributable to Isaac Lab in a profile. Values are still correct — ovphysx stages the mismatch transparently, so this is not a data-integrity bug. 2. `_check_device` rejected a **correctly** placed buffer: allocating one of these on the host, where the data actually lives, raised `OvPhysxView.DeviceMismatch`, whose message advises moving the buffer yourself — precisely the advice that fails here. No shipped Isaac Lab workflow reads these eight through the tensor path today, so this is latent rather than a live outage. It is not an obscure corner: Isaac Lab already authors these same properties through USD schema writes at scene construction, and the tensor path exists to change such values per-env at runtime. Each of the eight is also exposed as a public alias carrying its shape, dtype and units, taken from the wheel's own type table. This module is the alias layer over the ovphysx enum, so referencing them raw would have left the candidate tuple a mix of aliases and raw enum members with no stated reason why these eight differed. Naming follows the existing convention: the `ARTICULATION_` prefix is dropped for articulation types, as with `DOF_STIFFNESS` and `BODY_MASS`, and retained for rigid-body ones, as with `RIGID_BODY_MASS`. A second, orthogonal gap surfaced during the audit: ovphysx documents `ARTICULATION_DOF_DRIVE_TYPE` as read-only, but it was missing from `_READ_ONLY_NAMES`, so `set_attribute` accepted writes and forwarded them. An audit of the full classification against the wheel's type table found this to be the only such gap — of the 21 types ovphysx documents read-only, 20 were already classified, with no misclassified members. ## Type of change - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) - **Breaking change** — writes to `articulation_dof_drive_type` now raise `OvPhysxView.ReadOnlyAttribute` instead of being silently forwarded. Migration: remove the write; drive type is authored through the USD drive schema, not the tensor path. ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` ## Screenshots Not applicable. ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there ## How it was tested ovphysx exposes no residency query (`ovphysx_tensor_spec_t` carries dtype/ndim/shape and no device field), so residency was **measured**, not assumed: CUDA memcpys were counted via CUPTI around each binding read into a host buffer and into a device buffer, under the DirectGPU configuration `isaaclab_ov` itself sets (`"/physics/suppressReadback": True`). A host read showing `DtoH` means GPU-resident; no memcpy on the host read plus `HtoD` on the device read means CPU-resident. Measured across every GPU sample scene in the ovphysx 0.5.11 wheel: | scene | before | after | |---|---|---| | `two_articulations_gpu` | `agree=40 MISMATCH=8` | `agree=48 MISMATCH=0` | | `links_chain_sample_gpu` | `agree=40 MISMATCH=8` | `agree=48 MISMATCH=0` | | `mixed_base_articulations_gpu` | `agree=36 MISMATCH=8` | `agree=44 MISMATCH=0` | | `boxes_falling_on_groundplane_gpu` | `agree=9 MISMATCH=3` | `agree=12 MISMATCH=0` | | `volume_deformable_multi` | `MISMATCH=0` | `MISMATCH=0` | | `surface_deformable_material` | `MISMATCH=0` | `MISMATCH=0` | Every previously classified member measured CPU-resident as declared in all scenes, so the set was incomplete rather than wrong — there were no false positives to correct. The guarding test was a tautology: it compared the view's derived set against the canonical set it is derived from, so it could not fail for any contents. It now asserts against an independent inventory of measured residency, mirroring the existing `_EXPECTED_READ_ONLY_NAMES` pattern. Both new assertions were confirmed to fail before the fix and pass after it; the read-only case fails with `DID NOT RAISE ReadOnlyAttribute` without the fix. - `pytest source/isaaclab_ov/test/sim/test_ovphysx_view.py` — 81 passed - `test_articulation_helpers`, `test_rigid_object_helpers`, `test_deformable_object_helpers`, `test_ovphysx_scene_data_backend`, `test_randomize_rigid_body_material_mdp` — all pass - `uv run isaaclab -f` — passes - `tools/changelog/cli.py check develop` — passes `test_views_xform_prim_ovphysx.py` reports 23 failed / 28 passed both with and without this change; every failure is the process-global device lock covered by the `device_split` marker, which requires per-device re-invocation. Baselined on a clean tree to confirm it is unrelated. Environment: ovphysx 0.5.11, Linux x86_64, Python 3.12, NVIDIA RTX 5000 Ada Generation Laptop GPU. (cherry picked from commit ec58631)
1 parent 1c75487 commit f1f9576

4 files changed

Lines changed: 132 additions & 2 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
Added
2+
^^^^^
3+
4+
* Added :attr:`~isaaclab_ov.tensor_types.DOF_DRIVE_TYPE`,
5+
:attr:`~isaaclab_ov.tensor_types.DOF_DRIVE_MODEL`,
6+
:attr:`~isaaclab_ov.tensor_types.BODY_DISABLE_GRAVITY`,
7+
:attr:`~isaaclab_ov.tensor_types.CONTACT_OFFSET`,
8+
:attr:`~isaaclab_ov.tensor_types.REST_OFFSET`,
9+
:attr:`~isaaclab_ov.tensor_types.RIGID_BODY_DISABLE_GRAVITY`,
10+
:attr:`~isaaclab_ov.tensor_types.RIGID_BODY_CONTACT_OFFSET`, and
11+
:attr:`~isaaclab_ov.tensor_types.RIGID_BODY_REST_OFFSET` tensor type aliases,
12+
documenting the shape, dtype and units of each.
13+
14+
Fixed
15+
^^^^^
16+
17+
* Fixed :class:`~isaaclab_ov.sim.views.OvPhysxView` routing eight CPU-resident
18+
tensor types to the simulation device. The per-collision-shape contact and rest
19+
offsets, the articulation and rigid-body gravity-disable flags, and the DOF drive
20+
type and drive model are CPU-resident even on a GPU simulation, but were absent
21+
from the internal CPU-only classification. Reads and writes of these types
22+
incurred a hidden per-call host-to-device staging copy, and a correctly placed
23+
host buffer was rejected with ``OvPhysxView.DeviceMismatch``. Residency was
24+
measured on a GPU simulation by counting CUDA memcpys around a binding read.
25+
* **Breaking:** Fixed ``articulation_dof_drive_type`` not being classified as
26+
read-only. The underlying tensor type is read-only, but
27+
:meth:`~isaaclab_ov.sim.views.OvPhysxView.set_attribute` previously accepted
28+
writes to it and silently forwarded them. Such calls now raise
29+
``OvPhysxView.ReadOnlyAttribute``. Remove any write to this attribute; drive
30+
type is authored through the USD drive schema, not the tensor path.

source/isaaclab_ov/isaaclab_ov/sim/views/ovphysx_view.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@
7979
"articulation_body_inv_mass",
8080
"articulation_body_inv_inertia",
8181
"articulation_dof_projected_joint_force",
82+
"articulation_dof_drive_type",
8283
"articulation_jacobian",
8384
"articulation_mass_center_world",
8485
"articulation_mass_center_local",

source/isaaclab_ov/isaaclab_ov/tensor_types.py

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,19 @@
149149
Shape is ``[N, D, 3]``, dtype ``float32``.
150150
"""
151151

152+
DOF_DRIVE_TYPE = _TT.ARTICULATION_DOF_DRIVE_TYPE
153+
"""DOF drive type (0 = none, 1 = force, 2 = acceleration).
154+
155+
Shape is ``[N, D]``, dtype ``uint8``. Read-only.
156+
"""
157+
158+
DOF_DRIVE_MODEL = _TT.ARTICULATION_DOF_DRIVE_MODEL
159+
"""DOF performance-envelope drive model (speed-effort gradient, maximum actuator
160+
velocity, velocity-dependent resistance).
161+
162+
Shape is ``[N, D, 3]``, dtype ``float32``.
163+
"""
164+
152165
"""
153166
External wrench (GPU, write-only)
154167
"""
@@ -193,6 +206,24 @@
193206
Shape is ``[N, L, 9]``, dtype ``float32``.
194207
"""
195208

209+
BODY_DISABLE_GRAVITY = _TT.ARTICULATION_BODY_DISABLE_GRAVITY
210+
"""Per-link gravity disable flag (nonzero disables gravity for that link).
211+
212+
Shape is ``[N, L]``, dtype ``uint8``.
213+
"""
214+
215+
CONTACT_OFFSET = _TT.ARTICULATION_CONTACT_OFFSET
216+
"""Contact offset of each collision shape.
217+
218+
Shape is ``[N, S]``, dtype ``float32`` [m].
219+
"""
220+
221+
REST_OFFSET = _TT.ARTICULATION_REST_OFFSET
222+
"""Rest offset of each collision shape.
223+
224+
Shape is ``[N, S]``, dtype ``float32`` [m].
225+
"""
226+
196227
"""
197228
Rigid-body TensorTypes
198229
@@ -225,6 +256,16 @@
225256
``(N, 9)``, row-major flatten of the 3×3 inertia matrix
226257
``(Ixx, Ixy, Ixz, Iyx, Iyy, Iyz, Izx, Izy, Izz)`` [kg·m²]."""
227258

259+
RIGID_BODY_DISABLE_GRAVITY = _TT.RIGID_BODY_DISABLE_GRAVITY
260+
"""Gravity disable flag — read/write, CPU. Shape ``(N,)``, dtype ``uint8``;
261+
nonzero disables gravity for that actor."""
262+
263+
RIGID_BODY_CONTACT_OFFSET = _TT.RIGID_BODY_CONTACT_OFFSET
264+
"""Contact offset of each collision shape — read/write, CPU. Shape ``(N, S)`` [m]."""
265+
266+
RIGID_BODY_REST_OFFSET = _TT.RIGID_BODY_REST_OFFSET
267+
"""Rest offset of each collision shape — read/write, CPU. Shape ``(N, S)`` [m]."""
268+
228269
# These three aliases are pending an upcoming ovphysx wheel update.
229270
# When the wheel ships them, the corresponding ``hasattr`` checks below
230271
# in IsaacLab consumers will start returning True and the bindings will
@@ -433,15 +474,23 @@
433474
DOF_MAX_FORCE,
434475
DOF_ARMATURE,
435476
DOF_FRICTION_PROPERTIES,
477+
DOF_DRIVE_TYPE,
478+
DOF_DRIVE_MODEL,
436479
BODY_MASS,
437480
BODY_COM_POSE,
438481
BODY_INERTIA,
439482
BODY_INV_MASS,
440483
BODY_INV_INERTIA,
484+
BODY_DISABLE_GRAVITY,
485+
CONTACT_OFFSET,
486+
REST_OFFSET,
441487
# Rigid-body CPU-only entries (always available)
442488
RIGID_BODY_MASS,
443489
RIGID_BODY_COM_POSE,
444490
RIGID_BODY_INERTIA,
491+
RIGID_BODY_DISABLE_GRAVITY,
492+
RIGID_BODY_CONTACT_OFFSET,
493+
RIGID_BODY_REST_OFFSET,
445494
DEFORMABLE_MATERIAL_DYNAMIC_FRICTION,
446495
DEFORMABLE_MATERIAL_YOUNGS_MODULUS,
447496
DEFORMABLE_MATERIAL_POISSONS_RATIO,

source/isaaclab_ov/test/sim/test_ovphysx_view.py

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
"articulation_body_inv_mass",
5555
"articulation_body_inv_inertia",
5656
"articulation_dof_projected_joint_force",
57+
"articulation_dof_drive_type",
5758
"articulation_link_incoming_joint_force",
5859
"articulation_mass_center_world",
5960
"articulation_mass_center_local",
@@ -70,6 +71,48 @@
7071
}
7172
)
7273

74+
# CPU-resident on a GPU sim. Each name was measured, not assumed: with DirectGPU
75+
# (``suppressReadback=True``) the residency of every tensor type was determined by counting
76+
# CUDA memcpys around a binding read into a host buffer and into a device buffer.
77+
_EXPECTED_CPU_ONLY_NAMES = frozenset(
78+
{
79+
"articulation_body_com_pose",
80+
"articulation_body_disable_gravity",
81+
"articulation_body_inertia",
82+
"articulation_body_inv_inertia",
83+
"articulation_body_inv_mass",
84+
"articulation_body_mass",
85+
"articulation_contact_offset",
86+
"articulation_dof_armature",
87+
"articulation_dof_damping",
88+
"articulation_dof_drive_model",
89+
"articulation_dof_drive_type",
90+
"articulation_dof_friction_properties",
91+
"articulation_dof_limit",
92+
"articulation_dof_max_force",
93+
"articulation_dof_max_velocity",
94+
"articulation_dof_stiffness",
95+
"articulation_rest_offset",
96+
"articulation_shape_friction_and_restitution",
97+
"deformable_material_bending_damping",
98+
"deformable_material_bending_stiffness",
99+
"deformable_material_dynamic_friction",
100+
"deformable_material_elasticity_damping",
101+
"deformable_material_poissons_ratio",
102+
"deformable_material_thickness",
103+
"deformable_material_youngs_modulus",
104+
"rigid_body_com_pose",
105+
"rigid_body_contact_offset",
106+
"rigid_body_disable_gravity",
107+
"rigid_body_inertia",
108+
"rigid_body_inv_inertia",
109+
"rigid_body_inv_mass",
110+
"rigid_body_mass",
111+
"rigid_body_rest_offset",
112+
"rigid_body_shape_friction_and_restitution",
113+
}
114+
)
115+
73116
# Per-type shapes used by the fakes (only the types touched by the tests).
74117
_SHAPES = {
75118
TensorType.RIGID_BODY_POSE: lambda n: (n, 7),
@@ -201,18 +244,25 @@ def test_read_only_names_are_valid_vocabulary():
201244
def test_read_only_and_cpu_only_classification():
202245
assert is_read_only("articulation_jacobian")
203246
assert is_read_only("rigid_body_acceleration")
247+
assert is_read_only("articulation_dof_drive_type")
204248
assert not is_read_only("articulation_dof_stiffness")
205249
assert is_cpu_only("articulation_dof_stiffness")
206250
assert is_cpu_only("rigid_body_mass")
251+
assert is_cpu_only("rigid_body_disable_gravity")
252+
assert is_cpu_only("articulation_dof_drive_type")
207253
assert not is_cpu_only("rigid_body_pose")
208254

209255

210256
def test_cpu_only_names_match_canonical_set():
211-
# The view derives its CPU-only set from tensor_types so the two cannot drift.
257+
# Comparing the view's derived set against the canonical one cannot fail, since the
258+
# former is built from the latter. The expected inventory is what gives this test
259+
# teeth: it is measured residency, so it also rejects a set that is merely incomplete.
212260
from isaaclab_ov.sim.views import ovphysx_view as mod
213261
from isaaclab_ov.tensor_types import _CPU_ONLY_TYPES
214262

215-
assert frozenset(tt.name.lower() for tt in _CPU_ONLY_TYPES) == mod._CPU_ONLY_NAMES
263+
assert frozenset(tt.name.lower() for tt in _CPU_ONLY_TYPES) == _EXPECTED_CPU_ONLY_NAMES
264+
assert mod._CPU_ONLY_NAMES == _EXPECTED_CPU_ONLY_NAMES
265+
assert set(attribute_vocabulary()) >= mod._CPU_ONLY_NAMES
216266

217267

218268
# -----------------------------------------------------------------------------

0 commit comments

Comments
 (0)