From 1ae1e00829b1bc612f27af4e5ac91b245e2e5cf1 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Wed, 8 Jul 2026 11:37:11 +0000 Subject: [PATCH 1/3] Fix OVPhysX articulation indexing and manager runtime compatibility Actuator joint indices now follow the common actuator indexing contract; initialization alongside Kit reuses Kit's registered PhysX schema provider instead of double-registering; the manager accepts both the declared public runtime API and the current runtime API. Validated by full dexterous training runs on the OVPhysX backend as part of the Task Clean-up campaign. --- .../changelog.d/task-cleanup-dex-part02.rst | 6 + .../assets/articulation/articulation.py | 27 ++-- .../physics/ovphysx_manager.py | 124 +++++++++++++----- .../test/assets/test_articulation.py | 4 + .../test_ovphysx_scene_data_backend.py | 120 ++++++++++++++++- 5 files changed, 234 insertions(+), 47 deletions(-) create mode 100644 source/isaaclab_ovphysx/changelog.d/task-cleanup-dex-part02.rst diff --git a/source/isaaclab_ovphysx/changelog.d/task-cleanup-dex-part02.rst b/source/isaaclab_ovphysx/changelog.d/task-cleanup-dex-part02.rst new file mode 100644 index 000000000000..d756671cf3c2 --- /dev/null +++ b/source/isaaclab_ovphysx/changelog.d/task-cleanup-dex-part02.rst @@ -0,0 +1,6 @@ +Fixed +^^^^^ + +* Fixed OVPhysX actuator joint indices to follow the common actuator indexing contract. +* Fixed OVPhysX initialization alongside Kit by reusing Kit's registered PhysX schema provider. +* Fixed the OVPhysX manager to support both the declared public runtime API and the current runtime API. diff --git a/source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation.py b/source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation.py index 1b5b75a47f8f..e4c6917fcaf7 100644 --- a/source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation.py +++ b/source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation.py @@ -3924,6 +3924,10 @@ def _process_actuators_cfg(self) -> None: if not joint_ids: logger.warning("Actuator '%s': no joints matched '%s'", name, act_cfg.joint_names_expr) continue + if len(joint_names) == self.num_joints: + actuator_joint_ids = slice(None) + else: + actuator_joint_ids = torch.tensor(joint_ids, device=self.device, dtype=torch.int32) act_cfg_copy = act_cfg.copy() # seed the actuator with the simulation's already-correct DOF defaults # (USD-authored ``physxJoint:maxJointVelocity`` etc. parsed at scene-load). @@ -3933,17 +3937,17 @@ def _process_actuators_cfg(self) -> None: act = act_cfg_copy.class_type( act_cfg_copy, joint_names=joint_names, - joint_ids=joint_ids, + joint_ids=actuator_joint_ids, num_envs=self._num_instances, device=self._device, - stiffness=self._data.joint_stiffness.torch[:, joint_ids], - damping=self._data.joint_damping.torch[:, joint_ids], - armature=self._data.joint_armature.torch[:, joint_ids], - friction=self._data.joint_friction_coeff.torch[:, joint_ids], - dynamic_friction=self._data.joint_dynamic_friction_coeff.torch[:, joint_ids], - viscous_friction=self._data.joint_viscous_friction_coeff.torch[:, joint_ids], - effort_limit=self._data.joint_effort_limits.torch[:, joint_ids].clone(), - velocity_limit=self._data.joint_vel_limits.torch[:, joint_ids], + stiffness=self._data.joint_stiffness.torch[:, actuator_joint_ids], + damping=self._data.joint_damping.torch[:, actuator_joint_ids], + armature=self._data.joint_armature.torch[:, actuator_joint_ids], + friction=self._data.joint_friction_coeff.torch[:, actuator_joint_ids], + dynamic_friction=self._data.joint_dynamic_friction_coeff.torch[:, actuator_joint_ids], + viscous_friction=self._data.joint_viscous_friction_coeff.torch[:, actuator_joint_ids], + effort_limit=self._data.joint_effort_limits.torch[:, actuator_joint_ids].clone(), + velocity_limit=self._data.joint_vel_limits.torch[:, actuator_joint_ids], ) self.actuators[name] = act self._joint_ids_per_actuator[name] = joint_ids @@ -3978,10 +3982,7 @@ def _apply_actuator_model(self) -> None: from isaaclab.utils.types import ArticulationActions for name, act in self.actuators.items(): - jids = act.joint_indices - if jids is None: - continue - jids_t = jids if isinstance(jids, list) else list(jids) + jids_t = self._joint_ids_per_actuator[name] all_joints = len(jids_t) == self._num_joints # Warp -> torch (zero-copy on same device via DLPack). diff --git a/source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py b/source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py index 626d693c10bd..f3c9e5afeb11 100644 --- a/source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py +++ b/source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py @@ -13,6 +13,7 @@ from __future__ import annotations import atexit +import inspect import logging import os import re @@ -260,8 +261,10 @@ def _ensure_physx_schemas_registered(cls) -> None: runs it must be registered manually before the wheel can match ``PhysxContactReportAPI`` and friends on the stage. The wheel bundles the plugin under ``ovphysx/plugins/usd/PhysxSchema``. This - method is idempotent — :meth:`pxr.Plug.Registry.RegisterPlugins` - is a no-op once the plugin is registered. + method is idempotent and leaves an existing Kit ``physxSchema`` + provider authoritative. Registering the wheel's provider after Kit's + provider raises duplicate-type errors even though both plugins share + the same name. """ if cls._physx_schemas_registered: return @@ -273,11 +276,15 @@ def _ensure_physx_schemas_registered(cls) -> None: from pxr import Plug # noqa: PLC0415 except Exception: return + registry = Plug.Registry() + if any(plugin.name == "physxSchema" for plugin in registry.GetAllPlugins()): + cls._physx_schemas_registered = True + return plugin_root = os.path.join(os.path.dirname(ovphysx.__file__), "plugins", "usd") for sub in ("PhysxSchema/resources", "PhysxSchemaAddition/resources"): path = os.path.join(plugin_root, sub) if os.path.isdir(path): - Plug.Registry().RegisterPlugins(path) + registry.RegisterPlugins(path) cls._physx_schemas_registered = True @classmethod @@ -338,10 +345,25 @@ def step(cls) -> None: if cls._physx is None: return dt = cls.get_physics_dt() - cls._physx.step_sync(dt=dt) + cls._step_physx(cls._physx, dt=dt, sim_time=PhysicsManager._sim_time) cls._physx.update_articulations_kinematic() PhysicsManager._sim_time += dt + @staticmethod + def _step_physx(physx: Any, dt: float, sim_time: float) -> None: + """Step either the declared legacy runtime or the trusted current runtime.""" + if hasattr(physx, "reset_stage"): + physx.step_sync(dt=dt) + else: + physx.step_sync(dt=dt, sim_time=sim_time) + + @staticmethod + def _reset_physx_stage(physx: Any) -> None: + """Clear the loaded stage through the runtime's available reset API.""" + reset = physx.reset_stage if hasattr(physx, "reset_stage") else physx.reset + operation = reset() + physx.wait_op(operation) + @classmethod def close(cls) -> None: """Release ovphysx resources and clean up.""" @@ -366,7 +388,7 @@ def close(cls) -> None: def _release_physx(cls) -> None: """Soft-reset the ovphysx runtime stage; keep the C++ instance alive. - Calls ``physx.reset_stage()`` to clear the loaded scene, but does **not** + Clears the loaded scene through the runtime's reset API, but does **not** drop the Python reference. The cached :class:`ovphysx.PhysX` is reused by the next :class:`~isaaclab.sim.SimulationContext` via the reuse path in :meth:`_warmup_and_load`. Safe to call multiple times. @@ -383,8 +405,7 @@ def _release_physx(cls) -> None: namespace-isolated Carbonite (different soname / hidden visibility). """ if cls._physx is not None: - op = cls._physx.reset_stage() - cls._physx.wait_op(op) + cls._reset_physx_stage(cls._physx) @classmethod def get_physx_instance(cls) -> Any: @@ -571,13 +592,12 @@ def _warmup_and_load(cls) -> None: cls._locked_device = ovphysx_device else: # Reuse path: the cached PhysX may still hold the prior stage (the - # wheel allows only one loaded USD at a time). ``physx.reset_stage()`` - # is idempotent on an already-cleared stage and required when this is + # wheel allows only one loaded USD at a time). Clearing the stage is + # idempotent on an already-cleared stage and required when this is # a second :meth:`_warmup_and_load` within the same SimulationContext # (e.g. when a caller manually clears ``_warmup_done`` to force a # re-warmup). - op = cls._physx.reset_stage() - cls._physx.wait_op(op) + cls._reset_physx_stage(cls._physx) usd_handle, op_idx = cls._physx.add_usd(stage_file) cls._physx.wait_op(op_idx) @@ -649,28 +669,7 @@ def _construct_physx(cls, ovphysx_device: str, gpu_index: int) -> None: _sys.modules.update(_hidden_pxr) ovphysx = import_ovphysx() - ovphysx.PhysX.set_cpu_mode(ovphysx_device == "cpu") - - carbonite_overrides = { - "/physics/physxDispatcher": True, - "/physics/updateToUsd": False, - "/physics/updateVelocitiesToUsd": False, - "/physics/updateParticlesToUsd": False, - } - if ovphysx_device == "gpu": - carbonite_overrides.update( - { - "/physics/suppressReadback": True, - "/physics/suppressFabricUpdate": True, - } - ) - physx_kwargs = { - "config": ovphysx.PhysXConfig(num_threads=8, carbonite_overrides=carbonite_overrides), - } - if ovphysx_device == "gpu": - physx_kwargs["active_cuda_gpus"] = str(gpu_index) - - cls._physx = ovphysx.PhysX(**physx_kwargs) + cls._physx = cls._create_physx_instance(ovphysx, ovphysx_device, gpu_index) # FIXME(malesiani): re-evaluate this when carbonite ships an isolated copy. # At process exit, two Carbonite instances are in memory: @@ -705,6 +704,65 @@ def _atexit_release_and_exit(): atexit.register(_atexit_release_and_exit) cls._atexit_registered = True + @staticmethod + def _create_physx_instance(ovphysx: Any, ovphysx_device: str, gpu_index: int) -> Any: + """Create a PhysX instance for the declared or current OVPhysX runtime API. + + Args: + ovphysx: Imported OVPhysX runtime module. + ovphysx_device: Physics device, either ``"cpu"`` or ``"gpu"``. + gpu_index: CUDA device ordinal selected for GPU physics. + + Returns: + The configured ``ovphysx.PhysX`` instance. + """ + + carbonite_overrides = { + "/physics/physxDispatcher": True, + "/physics/updateToUsd": False, + "/physics/updateVelocitiesToUsd": False, + "/physics/updateParticlesToUsd": False, + } + if ovphysx_device == "gpu": + carbonite_overrides.update( + { + "/physics/suppressReadback": True, + "/physics/suppressFabricUpdate": True, + } + ) + if hasattr(ovphysx.PhysX, "set_cpu_mode"): + ovphysx.PhysX.set_cpu_mode(ovphysx_device == "cpu") + physx_kwargs = { + "config": ovphysx.PhysXConfig(num_threads=8, carbonite_overrides=carbonite_overrides), + } + if ovphysx_device == "gpu": + physx_kwargs["active_cuda_gpus"] = str(gpu_index) + return ovphysx.PhysX(**physx_kwargs) + + physx_kwargs = {"device": ovphysx_device} + physx_parameters = inspect.signature(ovphysx.PhysX).parameters + if "active_cuda_gpus" in physx_parameters and ovphysx_device == "gpu": + physx_kwargs["active_cuda_gpus"] = str(gpu_index) + physx_kwargs["config"] = ovphysx.PhysXConfig( + carbonite_overrides={ + "/physics/suppressReadback": True, + "/physics/suppressFabricUpdate": True, + } + ) + elif "gpu_index" in physx_parameters: + physx_kwargs["gpu_index"] = gpu_index + + physx = ovphysx.PhysX(**physx_kwargs) + if hasattr(physx, "set_setting"): + physx.set_setting("/persistent/physics/numThreads", "8") + physx.set_setting("/physics/physxDispatcher", "true") + physx.set_setting("/physics/updateToUsd", "false") + physx.set_setting("/physics/updateVelocitiesToUsd", "false") + physx.set_setting("/physics/updateParticlesToUsd", "false") + else: + physx.set_config_int32(ovphysx.ConfigInt32.NUM_THREADS, 8) + return physx + @staticmethod def _configure_physx_scene_prim(scene_prim, cfg, device: str) -> None: """Apply PhysxSceneAPI schema and device-specific scene attributes to the diff --git a/source/isaaclab_ovphysx/test/assets/test_articulation.py b/source/isaaclab_ovphysx/test/assets/test_articulation.py index 5adf38ad88de..9ac4bcef8a1b 100644 --- a/source/isaaclab_ovphysx/test/assets/test_articulation.py +++ b/source/isaaclab_ovphysx/test/assets/test_articulation.py @@ -376,6 +376,7 @@ def test_initialization_floating_base_non_root(sim, num_articulations, device, a for actuator_name, actuator in articulation.actuators.items(): is_implicit_model_cfg = isinstance(articulation_cfg.actuators[actuator_name], ImplicitActuatorCfg) assert actuator.is_implicit_model == is_implicit_model_cfg + assert actuator.joint_indices == slice(None) # Simulate physics for _ in range(10): @@ -505,6 +506,9 @@ def test_initialization_fixed_base(sim, num_articulations, device): for actuator_name, actuator in articulation.actuators.items(): is_implicit_model_cfg = isinstance(articulation_cfg.actuators[actuator_name], ImplicitActuatorCfg) assert actuator.is_implicit_model == is_implicit_model_cfg + assert isinstance(actuator.joint_indices, torch.Tensor) + assert actuator.joint_indices.dtype == torch.int32 + assert actuator.joint_indices.device == torch.device(device) # Simulate physics for _ in range(10): diff --git a/source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py b/source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py index d57efdddfdd1..dab45f37a803 100644 --- a/source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py +++ b/source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py @@ -7,7 +7,8 @@ from __future__ import annotations -from types import SimpleNamespace +import sys +from types import ModuleType, SimpleNamespace import pytest @@ -16,6 +17,123 @@ pytest.importorskip("ovphysx.types", reason="ovphysx wheel not installed") +def test_manager_supports_declared_legacy_runtime_api(): + """The declared public OVPhysX wheel keeps its constructor, step, and reset API.""" + from isaaclab_ovphysx.physics import OvPhysxManager + + class LegacyPhysX: + def __init__(self, *, device, active_cuda_gpus=None, config=None): + self.constructor = {"device": device, "active_cuda_gpus": active_cuda_gpus, "config": config} + self.calls = [] + + def set_config_int32(self, key, value): + self.calls.append(("set_config_int32", key, value)) + + def step_sync(self, *, dt, sim_time): + self.calls.append(("step_sync", dt, sim_time)) + + def reset(self): + self.calls.append(("reset",)) + return 17 + + def wait_op(self, operation): + self.calls.append(("wait_op", operation)) + + runtime = SimpleNamespace( + PhysX=LegacyPhysX, + PhysXConfig=lambda **kwargs: SimpleNamespace(**kwargs), + ConfigInt32=SimpleNamespace(NUM_THREADS="num_threads"), + ) + + physx = OvPhysxManager._create_physx_instance(runtime, "gpu", 2) + OvPhysxManager._step_physx(physx, dt=0.01, sim_time=1.5) + OvPhysxManager._reset_physx_stage(physx) + + assert physx.constructor["device"] == "gpu" + assert physx.constructor["active_cuda_gpus"] == "2" + assert physx.calls == [ + ("set_config_int32", "num_threads", 8), + ("step_sync", 0.01, 1.5), + ("reset",), + ("wait_op", 17), + ] + + +def test_manager_supports_current_runtime_api(): + """The trusted current OVPhysX wheel keeps its class-mode, step, and reset API.""" + from isaaclab_ovphysx.physics import OvPhysxManager + + class CurrentPhysX: + cpu_mode = None + + @classmethod + def set_cpu_mode(cls, enabled): + cls.cpu_mode = enabled + + def __init__(self, *, active_cuda_gpus=None, config=None): + self.constructor = {"active_cuda_gpus": active_cuda_gpus, "config": config} + self.calls = [] + + def step_sync(self, *, dt): + self.calls.append(("step_sync", dt)) + + def reset_stage(self): + self.calls.append(("reset_stage",)) + return 23 + + def wait_op(self, operation): + self.calls.append(("wait_op", operation)) + + runtime = SimpleNamespace( + PhysX=CurrentPhysX, + PhysXConfig=lambda **kwargs: SimpleNamespace(**kwargs), + ) + + physx = OvPhysxManager._create_physx_instance(runtime, "cpu", 0) + OvPhysxManager._step_physx(physx, dt=0.02, sim_time=3.0) + OvPhysxManager._reset_physx_stage(physx) + + assert CurrentPhysX.cpu_mode is True + assert physx.constructor["active_cuda_gpus"] is None + assert physx.constructor["config"].num_threads == 8 + assert physx.calls == [("step_sync", 0.02), ("reset_stage",), ("wait_op", 23)] + + +def test_manager_keeps_existing_kit_physx_schema_provider(monkeypatch, tmp_path): + """Kit's registered ``physxSchema`` provider prevents duplicate wheel registration.""" + from isaaclab_ovphysx.physics import OvPhysxManager + + class FakeRegistry: + def __init__(self): + self.get_all_calls = 0 + self.registered_paths = [] + + def GetAllPlugins(self): + self.get_all_calls += 1 + return [SimpleNamespace(name="physxSchema")] + + def RegisterPlugins(self, path): + self.registered_paths.append(path) + + registry = FakeRegistry() + fake_pxr = ModuleType("pxr") + fake_pxr.Plug = SimpleNamespace(Registry=lambda: registry) + fake_ovphysx = ModuleType("ovphysx") + fake_ovphysx.__file__ = str(tmp_path / "ovphysx" / "__init__.py") + monkeypatch.setitem(sys.modules, "pxr", fake_pxr) + monkeypatch.setitem(sys.modules, "ovphysx", fake_ovphysx) + + previous = OvPhysxManager._physx_schemas_registered + OvPhysxManager._physx_schemas_registered = False + try: + OvPhysxManager._ensure_physx_schemas_registered() + finally: + OvPhysxManager._physx_schemas_registered = previous + + assert registry.get_all_calls == 1 + assert registry.registered_paths == [] + + def _make_stub_binding(prim_paths: list[str]) -> SimpleNamespace: """Stub an ovphysx ``TensorBinding`` exposing ``shape``, ``count``, ``prim_paths``, and ``read(dst)``.""" n = len(prim_paths) From 1f7a4331057586ce6a49a4e9827c1972a2640e1b Mon Sep 17 00:00:00 2001 From: jichuanh Date: Thu, 9 Jul 2026 01:04:44 +0000 Subject: [PATCH 2/3] Guard runtime signature introspection in the PhysX factory C-extension constructors may not expose a Python-visible signature; fall back to an empty parameter set instead of raising. Also documents the legacy runtime's settings limitation. --- .../isaaclab_ovphysx/physics/ovphysx_manager.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py b/source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py index f3c9e5afeb11..128fdde63c37 100644 --- a/source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py +++ b/source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py @@ -740,7 +740,11 @@ def _create_physx_instance(ovphysx: Any, ovphysx_device: str, gpu_index: int) -> return ovphysx.PhysX(**physx_kwargs) physx_kwargs = {"device": ovphysx_device} - physx_parameters = inspect.signature(ovphysx.PhysX).parameters + try: + physx_parameters = inspect.signature(ovphysx.PhysX).parameters + except (TypeError, ValueError): + # C-extension constructors may not expose a Python-visible signature + physx_parameters = {} if "active_cuda_gpus" in physx_parameters and ovphysx_device == "gpu": physx_kwargs["active_cuda_gpus"] = str(gpu_index) physx_kwargs["config"] = ovphysx.PhysXConfig( @@ -760,6 +764,8 @@ def _create_physx_instance(ovphysx: Any, ovphysx_device: str, gpu_index: int) -> physx.set_setting("/physics/updateVelocitiesToUsd", "false") physx.set_setting("/physics/updateParticlesToUsd", "false") else: + # the declared legacy runtime exposes no generic settings API, so + # only the thread count can be applied post-construction physx.set_config_int32(ovphysx.ConfigInt32.NUM_THREADS, 8) return physx From 9f4cb14a32486cae074b634142eee8caf2b62477 Mon Sep 17 00:00:00 2001 From: jichuanh Date: Wed, 15 Jul 2026 01:47:01 -0700 Subject: [PATCH 3/3] Broaden scene-data-backend test module docstring The module also carries OvPhysxManager tests added in this PR; the docstring now names both coverage areas. --- .../test/physics/test_ovphysx_scene_data_backend.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py b/source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py index dab45f37a803..aa8ebc3e9cc6 100644 --- a/source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py +++ b/source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py @@ -3,7 +3,7 @@ # # SPDX-License-Identifier: BSD-3-Clause -"""Unit tests for OvPhysxSceneDataBackend (new SceneDataBackend interface, post-#5128).""" +"""Unit tests for OvPhysxSceneDataBackend (new SceneDataBackend interface, post-#5128) and OvPhysxManager.""" from __future__ import annotations