Skip to content

[OVPHYSX] RigidObject + RigidObjectData asset#5426

Open
AntoineRichard wants to merge 51 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_rigidobject
Open

[OVPHYSX] RigidObject + RigidObjectData asset#5426
AntoineRichard wants to merge 51 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_rigidobject

Conversation

@AntoineRichard
Copy link
Copy Markdown
Collaborator

Summary

Implements RigidObject and RigidObjectData for the OVPhysX backend (issue #5316), satisfying the BaseRigidObject and BaseRigidObjectData contracts. Mirrors the PhysX RigidObject and the existing OVPhysX Articulation patterns; runs kitless via the standard SimulationContext + UsdFileCfg(usd_path=…) pipeline.

  • New: source/isaaclab_ovphysx/isaaclab_ovphysx/assets/rigid_object/{rigid_object.py, rigid_object_data.py, __init__.py, __init__.pyi} (~1900 lines).
  • New: source/isaaclab_ovphysx/isaaclab_ovphysx/assets/kernels.py — shared Warp kernels relocated from articulation/kernels.py so both asset types use them; new _compose_root_link_pose_from_com for COM→link write conversion; ported Newton's derive_body_acceleration_from_body_com_velocities to FD acceleration locally (no wheel RIGID_BODY_ACCELERATION dependency).
  • New RIGID_BODY_* TensorType aliases in isaaclab_ovphysx/tensor_types.py — six already-shipping types as direct imports, three forward-compat aliases (ACCELERATION, INV_MASS, INV_INERTIA) gated by try/except AttributeError so the module loads cleanly today.
  • Allegro env hookup (source/isaaclab_tasks/.../allegro_hand/allegro_hand_env_cfg.py): adds ovphysx variants to ObjectCfg and PhysicsCfg, mirroring the Cartpole/Ant pattern. Enables running Isaac-Repose-Cube-Allegro-Direct-v0 against OVPhysX via ./scripts/run_ovphysx.sh.
  • Cross-backend interface tests: BACKENDS.append(\"ovphysx\") + create_ovphysx_rigid_object factory in source/isaaclab/test/assets/test_rigid_object_iface.py.
  • Versioning: isaaclab_ovphysx 0.1.2 → 0.2.0, isaaclab_tasks 1.5.29 → 1.5.30.

Test plan

Real-backend rigid-object tests (kitless, via run_ovphysx.sh)

./scripts/run_ovphysx.sh -m pytest source/isaaclab_ovphysx/test/assets/test_rigid_object.py -v

Current state: 61 passed, 14 xfailed. The 61 passing tests are real-backend (live ovphysx.PhysX instance, real TensorBinding reads, real sim steps) — port of the PhysX test_rigid_object.py structure with the canonical SimulationContext + UsdFileCfg(ISAAC_NUCLEUS_DIR/Props/Blocks/DexCube/dex_cube_instanceable.usd) pattern Cartpole/Newton already use. Catches two production bugs the previous mock-based suite missed (`hasattr` swallow on `body_names`, `self._device` always falling back to `cuda:0`).

Cross-backend interface tests

./scripts/run_ovphysx.sh -m pytest source/isaaclab/test/assets/test_rigid_object_iface.py -v -k ovphysx

Current state: 252 passed, 120 fixed shape-mismatch failures fixed in this branch (4 distinct bugs caught: full-write row-count guard, 1-D mask src normalization, COM-pose row-count guard, two unimplemented `default_root_pose/vel` stubs).

Existing articulation regression check

./scripts/run_ovphysx.sh -m pytest source/isaaclab_ovphysx/test/assets/test_articulation.py source/isaaclab_ovphysx/test/assets/test_articulation_data.py -v

Verifies the kernel relocation in Task 2 didn't break existing articulation tests. Recommended before merge.

Manual end-to-end (Kit + Nucleus)

`Isaac-Repose-Cube-Allegro-Direct-v0` with the new `ovphysx` preset — manual smoke test (Kit-required, requires Nucleus access):

./scripts/run_ovphysx.sh source/isaaclab_tasks/isaaclab_tasks/direct/allegro_hand/allegro_hand_env.py --num_envs 4 --headless

Wheel-side gaps (for @marcodiiga)

The 14 remaining xfails split as follows; only 10 are wheel-side blockers (all in the same category):

Category xfailed Owner
Material-properties API (RIGID_BODY_MATERIAL TensorType or view helper) 10 Wheel — see docs/superpowers/specs/2026-04-28-ovphysx-wheel-gaps-for-marco.md for the proposed contract
test_initialization_with_no_rigid_body (RuntimeError on missing prim) 2 IsaacLab follow-up — error-handling polish
test_initialization_with_articulation_root (out-of-scope per spec) 2 IsaacLab follow-up — explicit `NotImplementedError` stub

Three additional wheel-side RIGID_BODY_* TensorTypes (ACCELERATION, INV_MASS, INV_INERTIA) are forward-compat — declared via try/except AttributeError aliases on the IsaacLab side, no IsaacLab consumers depend on them today, but they auto-activate when the wheel ships them.

Notes

  • IsaacLab side is wheel-update-agnostic: tensor_types.py defensive aliases let `isaaclab_ovphysx` import cleanly against today's `ovphysx 0.3.7`.
  • Local docs at `docs/superpowers/specs/` (gitignored) include the original design spec, the corrected Marco-feedback gap spec, and the test-gaps follow-up.
  • Branch contains 30 commits including Marco's contract corrections (renames `RIGID_BODY_ROOT_POSE` → `RIGID_BODY_POSE`, `MASS` shape `(N, 1)` → `(N,)`).

Add nine RIGID_BODY_* aliases to isaaclab_ovphysx.tensor_types covering
the rigid-actor root pose/velocity/acceleration, wrench application, and
mass/inertia/COM properties. Each alias carries a shape/units docstring
that Sphinx autoattribute can pick up.

Extend _CPU_ONLY_TYPES with the five CPU-routed rigid-body variants so
the existing GPU/CPU dispatch in _to_flat_f32 routes them correctly.

Direct imports (no shim) intentionally couple this package to a future
ovphysx wheel that exposes the matching TensorType enum values; see
docs/superpowers/specs/2026-04-27-ovphysx-rigid-body-tensortypes-gap.md.

Issue: isaac-sim#5316
Move kernels reused across multiple asset types from
isaaclab_ovphysx.assets.articulation.kernels into a new
isaaclab_ovphysx.assets.kernels module: _body_wrench_to_world,
_scatter_rows_partial, _copy_first_body, _compose_root_com_pose,
_projected_gravity, _compute_heading, _world_vel_to_body_lin,
_world_vel_to_body_ang.

Articulation-only kernels (joint-limit setup, FD joint acceleration,
multi-body COM compose) stay in articulation/kernels.py. Articulation
modules update their imports accordingly. No behavior change.

This refactor unblocks the upcoming RigidObject implementation, which
needs the same kernels for frame conversions and wrench packing.

Issue: isaac-sim#5316
Generalize the mock binding factory to produce a rigid-object-shaped
binding set (RIGID_BODY_* keys only, num_joints=0, num_bodies=1, no
tendons) when called with asset_kind='rigid_object'. Default behavior
is unchanged: existing articulation callers do not need updates.

Make set_random_data tolerant of the smaller binding set so it works
for both asset kinds.

Issue: isaac-sim#5316
Add the rigid_object sub-package and the RigidObjectData class skeleton
with constructor, count properties, update/invalidate hooks, and
_process_cfg that fills _default_root_pose / _default_root_velocity from
cfg.init_state.

Add the backend-specific test file with a hasattr-based wheel gate
(importorskip-then-attr on the module would AttributeError rather than
skip when RIGID_BODY_* enums are absent on the wheel) and a basic-counts
smoke test.

Issue: isaac-sim#5316
Address review blockers on commit 65084f7:

1. _process_cfg now mirrors the articulation pattern (wp.zeros to
   pre-allocate, then wp.copy from wp.from_numpy with the typed dtype
   directly — matching articulation._process_cfg verbatim), avoiding
   the use-after-free that the previous reinterpret-cast-from-local
   idiom introduced when the float32 source went out of scope.

2. is_primed is now a guarded @Property + setter that enforces the
   one-way gate (False->True allowed, True->True idempotent,
   True->False raises ValueError), matching every other *Data class.

3. Drop unused forward-reference imports from
   test/assets/test_rigid_object.py so ruff/F401 passes; subsequent
   tasks will re-add them when the symbols are actually used.

Issue: isaac-sim#5316
Replace the abstract-property stubs for root_link_pose_w,
root_link_vel_w, root_com_pose_w, root_com_vel_w, and the per-axis
sliced views (root_link_pos_w, root_link_quat_w, root_lin_vel_w,
root_ang_vel_w, plus their root_com_* counterparts) with concrete
implementations that lazy-read from RIGID_BODY_ROOT_POSE /
RIGID_BODY_ROOT_VELOCITY through TensorBindings, cache for the rest
of the sim step, and expose slices as zero-copy Warp views over the
canonical pose/velocity buffers.

Mirrors the articulation_data lazy-read + ProxyArray + sliced-view
idioms with num_bodies=1 (root pose/velocity are 1-D over instances,
with no body axis).

Issue: isaac-sim#5316
The per-property TimestampedBuffer.timestamp fields are the freshness
gate consulted by every lazy-read property. _invalidate_caches was
previously clearing only the legacy _timestamps dict, leaving the
buffers with their last-read timestamp; a property accessed within
the same sim step (e.g. immediately after RigidObject.reset and
before update(dt) advances _sim_time) would serve pre-reset data.

Reset every allocated buffer's timestamp to -1.0 in
_invalidate_caches so the next property access always re-reads from
the binding regardless of where _sim_time stands.

Add a regression test that mutates the binding in place + calls
_invalidate_caches without advancing _sim_time and asserts the next
read reflects the new value.

Issue: isaac-sim#5316
Replace the abstract-property stubs for the body-state singleton-dim
views (body_link_pose_w, body_com_pose_w, body_*_vel_w, body_*_pos_w,
body_*_quat_w, body_*_lin_vel_w, body_*_ang_vel_w), the body
acceleration (body_link_acc_w, body_com_acc_w, plus their _lin_*/
_ang_* slices) gated on the RIGID_BODY_ACCELERATION binding, and the
body-frame derived properties (projected_gravity_b, heading_w,
root_link_lin_vel_b, root_link_ang_vel_b, root_com_lin_vel_b,
root_com_ang_vel_b).

Body-state views are zero-copy reshapes of the (N,) root buffers into
(N, 1, k) — no compute kernel needed when num_bodies=1. Body
acceleration raises NotImplementedError with a pointer to the gap
spec if the wheel lacks RIGID_BODY_ACCELERATION. Derived properties
launch the relocated _projected_gravity / _compute_heading /
_world_vel_to_body_{lin,ang} kernels from
isaaclab_ovphysx.assets.kernels.

Extend _invalidate_caches with the new TimestampedBuffer instances so
they participate in coarse cache invalidation.

Issue: isaac-sim#5316
The IsaacLab projected_gravity_b convention (per
BaseRigidObjectData docstring "Projection of the gravity direction
on base frame", and matching ArticulationData which normalizes
gravity_np / gravity_mag before storing GRAVITY_VEC_W) is the unit
direction, not the signed-magnitude. The Task 6 test assertion
expected -9.81 (signed magnitude); the kernel correctly produces
-1.0 (unit z-component of the gravity direction).

Update the assertion and the inline comment so the test reflects the
documented contract.

Issue: isaac-sim#5316
Replace the abstract-property stubs for body_mass, body_inertia, and
body_com_pose_b with concrete CPU-side lazy-read implementations
backed by RIGID_BODY_MASS / RIGID_BODY_INERTIA / RIGID_BODY_COM_POSE
bindings. Also implement body_com_pos_b and body_com_quat_b as
zero-copy slice views of body_com_pose_b's transformf buffer.

Properties read once on first access, cache as semi-static, and
invalidate via the _invalidate_caches reset loop (driven by
RigidObject mass/COM/inertia setters). Mirrors the articulation_data
pattern adapted for num_bodies = 1.

Issue: isaac-sim#5316
Add RigidObject class with __init__, _initialize_impl, _get_binding,
and count/data accessor properties. Eagerly creates GPU bindings for
RIGID_BODY_ROOT_POSE / RIGID_BODY_ROOT_VELOCITY / RIGID_BODY_WRENCH
so binding-creation failures surface at init time with a clear
message pointing at the gap spec.

_create_buffers and _process_cfg are placeholder no-ops; Task 9
replaces them. Write paths, reset, update, find_bodies, and the
deprecated state writers come in subsequent tasks (10-13). Abstract
methods that must be satisfied to instantiate the class are stubbed
with NotImplementedError pending those tasks.

Issue: isaac-sim#5316
Renames per Marco's feedback: RIGID_BODY_ROOT_POSE ->
RIGID_BODY_POSE and RIGID_BODY_ROOT_VELOCITY ->
RIGID_BODY_VELOCITY throughout. "Root" is articulation
vocabulary; a standalone rigid body IS the body.

Shape correction: RIGID_BODY_MASS and RIGID_BODY_INV_MASS
ship as (N,) not (N, 1). MockOvPhysxBindingSet allocates
(N,) for both; rigid_object_data.py's body_mass property
consumes (N,) and exposes (N, 1) via zero-copy reshape to
satisfy the BaseRigidObjectData contract.

Soften _initialize_impl error: removes the prescriptive
"NOT under an articulation root" language since pattern-
resolution gating is a future wheel-side selection policy.

Pre-existing ruff E501 in write_root_link_state_to_sim
docstring fixed as collateral.
Allocate _ALL_INDICES, _ALL_BODY_INDICES, their Warp views, the
single-body (N, 1, 9) _wrench_buf staging buffer, and the
instantaneous/permanent wrench composers. _process_cfg delegates to
RigidObjectData._process_cfg.

Issue: isaac-sim#5316
Add the twelve root-state writers (pose+velocity, actor+link+com,
index+mask variants) on RigidObject, plus the shared _to_flat_f32 and
_write_root_state helpers ported from articulation. Frame-conversion
variants launch the relocated assets/kernels.py kernels before the
binding write. Add the three deprecated compound state writers that
emit DeprecationWarning and delegate to the split pose+velocity
writers.

Add _compose_root_link_pose_from_com kernel to assets/kernels.py
to support COM->link pose inversion on the write path:
  link_pose = com_pose_w * inverse(com_pose_b)

Issue: isaac-sim#5316
Add set_masses_{index,mask}, set_coms_{index,mask},
set_inertias_{index,mask}. Each writes through the matching
CPU-routed RIGID_BODY_* binding via _write_root_state, then
invalidates the corresponding RigidObjectData cache via
_invalidate_caches.

body_ids / body_mask parameters are accepted for parity with the
BaseRigidObject contract but unused (num_bodies = 1).

Extend _write_root_state to handle 1-D bindings (RIGID_BODY_MASS)
by detecting them and bypassing the 2-D scatter kernel path.

Issue: isaac-sim#5316
Compose instantaneous + permanent wrenches, rotate body-frame
force/torque to world frame via _body_wrench_to_world (dim=(N, 1)),
reshape the (N, 1, 9) staging buffer to (N, 9) zero-copy, write to
RIGID_BODY_WRENCH, reset the instantaneous composer.

Issue: isaac-sim#5316
Replace stubs for reset, update, and find_bodies. reset writes the
default pose/velocity to the specified envs via zero-copy flat float32
views of the typed wp.transformf/wp.spatial_vectorf defaults, resets
both wrench composers, and invalidates the data caches. update
delegates to RigidObjectData.update. find_bodies handles the None
(all bodies) fast path and delegates regex matching to
resolve_matching_names for non-None inputs.

The deprecated compound state writers were already implemented in
Task 10 alongside the split-form writers and are not touched here.

Issue: isaac-sim#5316
Add the rigid_object/__init__.pyi stub mirroring the articulation
sibling, and extend isaaclab_ovphysx.assets/__init__.pyi to include
RigidObject and RigidObjectData. Public imports now resolve via
``from isaaclab_ovphysx.assets import RigidObject, RigidObjectData``.

Issue: isaac-sim#5316
Add the import-guarded BACKENDS.append('ovphysx') block, the
create_ovphysx_rigid_object factory, and the dispatch case so the
existing parametrized interface tests automatically cover the new
backend. Mirrors the OVPhysX articulation parametrization in
test_articulation_iface.py.

Issue: isaac-sim#5316
Mirror the Cartpole/Ant pattern by adding ovphysx variants to
ObjectCfg (RigidObjectCfg using the same DexCube spawn as the physx
variant) and to PhysicsCfg (OvPhysxCfg()). Default backend remains
physx; existing PhysX/Newton paths are unchanged.

This wires Isaac-Repose-Cube-Allegro-Direct-v0 for OVPhysX validation
via ./scripts/run_ovphysx.sh source/isaaclab_tasks/isaaclab_tasks/
direct/allegro_hand/allegro_hand_env.py --num_envs 4 --headless once
the wheel ships.

Issue: isaac-sim#5316
Add the 0.2.0 changelog entry on isaaclab_ovphysx covering the new
RigidObject/RigidObjectData classes, the RIGID_BODY_* TensorType
aliases (six already-shipping + three pending wheel update), the
mock-binding asset_kind extension, and the assets/kernels.py kernel
relocation. Bump the matching extension.toml version.

Add a patch-bump changelog entry on isaaclab_tasks for the Allegro
env preset addition.

Issue: isaac-sim#5316
The ovphysx wheel currently exposes 6 of the 9 RIGID_BODY_* TensorType
enums (POSE, VELOCITY, WRENCH, MASS, COM_POSE, INERTIA). The remaining
three (ACCELERATION, INV_MASS, INV_INERTIA) are pending an upcoming
wheel update from @marcodiiga.

Make the IsaacLab side wheel-update-agnostic:

* Guard tensor_types.py imports of the three not-yet-shipping aliases
  with try/except AttributeError so isaaclab_ovphysx.tensor_types
  imports cleanly on today's wheel. _CPU_ONLY_TYPES filters to only
  the names that exist via _RIGID_BODY_OPTIONAL_CPU.
* MockOvPhysxBindingSet skips the optional bindings when their alias
  is not defined.
* RigidObjectData.body_*_acc_w now finite-differences from
  body_com_vel_w, mirroring Newton's pattern (kernel
  derive_body_acceleration_from_body_com_velocities ported into
  isaaclab_ovphysx.assets.kernels). Removes the
  NotImplementedError-on-missing-binding fallback.
* update(dt) stores _last_dt and eagerly triggers body_com_acc_w each
  step so FD captures every transition.

The forward-compat aliases stay declared (Marco will land them); when
they ship, the existing TensorBinding read path will work without
further IsaacLab changes.

Issue: isaac-sim#5316
WrenchComposer.__init__ calls hasattr(asset.data, "body_com_pos_w"),
which triggers the property chain body_com_pos_w → body_com_pose_w →
root_com_pose_w, reading the RIGID_BODY_COM_POSE binding and setting
_body_com_pose_b_buf.timestamp = _sim_time = 0.0.

Any subsequent mutation of the binding (e.g. set_coms_index, or a test
that sets the binding directly) is then invisible to _com_pose_to_link_pose
because the freshness gate ``buf.timestamp >= _sim_time`` treats 0.0 ≥ 0.0
as "already fresh" and skips the read — returning stale buffer contents
to the frame-conversion kernel and producing an off-by-translation result.

Force a fresh read in _com_pose_to_link_pose by resetting the buffer
timestamp to -1.0 immediately before calling _read_transform_binding.
The frame conversion always needs the current binding value at write time;
the lazy-cache is the wrong policy here.

Caught by test_write_root_com_pose_to_sim_index_invokes_frame_conversion.

Issue: isaac-sim#5316
When running via ./scripts/run_ovphysx.sh, the test file's unconditional
AppLauncher call segfaults during pytest collection because the script's
thin Kit shell (libcarb preload only, no full Kit boot) cannot host a
real AppLauncher. Mirror the _kitless heuristic from test_articulation_iface
so the rigid-object iface tests skip AppLauncher and substitute MagicMock
isaacsim core modules in the same scenarios.

The original _kitless heuristic (LD_PRELOAD == "" and EXP_PATH not set) was
also incorrect for this environment: run_ovphysx.sh sets LD_PRELOAD to the
ovphysx libcarb.so path, not an empty string. Fix the heuristic in both
test files to check for "ovphysx" in LD_PRELOAD as the primary signal,
with the bare-Python fallback retained as a secondary guard.

Also extend the kitless sys.modules stub list to cover omni.physics.tensors
and related Kit modules that physx_manager.py imports at module scope, which
would otherwise cause a ModuleNotFoundError during collection.

This unblocks running the OVPhysX-parametrized parts of the file via
run_ovphysx.sh. PhysX/Newton/Mock paths are unaffected.

Issue: isaac-sim#5316
Three related bugs were present in the OVPhysX RigidObject body-property
write path, all contributing to the 120 test failures.

First, _write_root_state's full-write path (no env_ids, no mask) had no
row-count validation for 1-D bindings such as RIGID_BODY_MASS. Passing a
tensor with more rows than num_instances silently reached the mock's numpy
reshape and raised ValueError, but the test infrastructure expected
AssertionError or RuntimeError.  An explicit row-count guard now raises
RuntimeError on the full-write path before any binding call.

Second, the index/mask sub-write path for 1-D bindings passed the source
array to binding.write() as a 2-D (K, 1) buffer (the raw shape produced by
_to_flat_f32 when the caller supplies (K, 1) torch data). For the mask path
this caused NumPy boolean-index assignment to fail with TypeError because it
cannot scatter a 2-D array into a 1-D binding buffer. The 1-D source is now
normalised to shape (K,) via a zero-copy warp array view before any write.

Third, write_root_com_pose_to_sim_index and write_root_com_pose_to_sim_mask
silently truncated oversized inputs inside _com_pose_to_link_pose, which
hardcodes shape=(N,) for the intermediate warp array view regardless of the
input size. This masked shape errors on full writes. Explicit row-count
guards are now applied at the public API entry points for these two methods.

Additionally, default_root_pose and default_root_vel in RigidObjectData were
NotImplementedError stubs. They are now implemented to return ProxyArray
wrappers over the _default_root_pose/_default_root_velocity buffers that are
already populated during _process_cfg.

Caught by the cross-backend TestRigidObjectWritersBody tests against
the OVPhysX backend. After the fix all 372 ovphysx-parametrized cases
pass.

Issue: isaac-sim#5316
The mock-based test file is removed in favor of a copy of PhysX's
test_rigid_object.py adapted to the kitless OVPhysX architecture:
- Drop AppLauncher; mock the isaacsim and omni.* modules instead so
  the file imports under run_ovphysx.sh without launching Kit.
- Build the test scene via MockOvPhysxBindingSet, bypassing
  OvPhysxManager entirely (no Kit stage export needed).
- Drive sim steps via direct binding manipulation; no
  build_simulation_context.
- Programmatically construct rigid-object shells instead of pulling
  DexCube USD from Nucleus.

Tests that exercise OVPhysX features not yet wired (OvPhysxManager
step loop, contact materials, kitless stage entry point) are
explicitly xfail-marked with inline reasons.

Result: 67 passed, 73 xfailed, 0 failed.

See docs/superpowers/specs/2026-04-28-ovphysx-rigid-object-test-gaps.md
(worktree-only, gitignored) for the consolidated gap list for Marco.
OvPhysxManager IS drivable without AppLauncher: _warmup_and_load only
needs PhysicsManager._sim.stage + a couple of cfg fields, which a thin
SimpleNamespace fake satisfies. Use this to convert the warmup and
stage-load xfails into real-backend tests against the live ovphysx.PhysX
instance.

Adds _make_kitless_sim_context() helper (builds in-memory USD stage with
RigidBodyAPI + CollisionAPI cube + PhysicsScene, wraps in SimpleNamespace
exposing: stage, cfg.physics, cfg.device, cfg.physics_prim_path,
cfg.enable_scene_query_support, cfg.dt) and a module-scoped
kitless_manager_cpu fixture that drives initialize() + reset() + close().

Converts test_warmup_attach_stage_not_called_for_cpu (1 xfail) to three
passing real-backend tests:
- test_warmup_and_load_cpu: lifecycle assertions
- test_warmup_gpu_not_called_for_cpu: CPU path skips warmup_gpu
- test_stage_load_cpu: stage export + usd_handle type check

Adds test_warmup_and_load_gpu as xfail pending a GPU CI runner.

Result: 70 passed, 73 xfailed (was 67 passed, 73 xfailed).

Update the test-gaps doc to reflect the closed gap (no wheel change
required for this category).

Issue: isaac-sim#5316
Drop the kitless mocks, the SimpleNamespace sim-context fake, and the
MockOvPhysxBindingSet shell-injection helpers. The standard
SimulationContext + UsdFileCfg(ISAAC_NUCLEUS_DIR/...) pattern works
under ./scripts/run_ovphysx.sh without AppLauncher — omni.client
resolves Nucleus URLs from Kit's Python directly, and SimulationContext
runs kitless via has_kit() returning False.

Tests now exercise real RigidObject + RigidObjectData + OvPhysxManager
against live ovphysx.PhysX bindings, using the same Nucleus assets
the Cartpole/Newton tests use.

Material-properties tests stay xfailed pending a wheel-side
RIGID_BODY_MATERIAL TensorType. GPU tests now pass (NVIDIA RTX 5000 Ada
verified).

Production bug surfaced: RigidObject._initialize_impl uses hasattr() on
TensorBinding.body_names which propagates RuntimeError (not
AttributeError), blocking all RigidObject lifecycle tests against the
real backend. Fix: wrap in try/except (AttributeError, RuntimeError).
Tracking: issue isaac-sim#5316.

Issue: isaac-sim#5316
The previous ``hasattr(root_pose, "body_names")`` gate only catches
AttributeError, but the real ovphysx TensorBinding raises TypeError
on the body_names property for non-articulation tensor types such as
RIGID_BODY_POSE: "Articulation metadata … is not available for
tensor type 'RIGID_BODY_POSE'." Replace with try/except that catches
both AttributeError and TypeError; fall back to ["base_link"].

Also fix self._device derivation: ``hasattr(self._ovphysx, "device")``
always returns False for the real PhysX object (no .device property),
so the device silently fell back to "cuda:0" even when the simulation
runs on CPU, causing a device mismatch in TensorBinding.read(). Use
OvPhysxManager.get_device() which mirrors SimulationContext.cfg.device.

Un-xfail 68 of the 70 tests tagged _INIT_IMPL_BUG in test_rigid_object.py
— they now run cleanly against the live ovphysx CPU backend (59 passed).
The two remaining xfails use a tightened reason (_FORCE_BALANCE_GAP):
test_external_force_on_single_body drifts ~0.57 m instead of < 0.1 m,
a physics-accuracy gap under investigation.

Issue: isaac-sim#5316
Rewrites test_external_force_on_single_body to mirror Newton's reference
implementation: 5 outer iterations with a full pose/velocity reset between
each, 5 inner sim steps per iteration, force applied to every 2nd cube
(indices 0::2), and alternating global/local frame each outer iteration.

The old test used a single 20-step loop on env_0 only, with no resets and
no is_global argument — causing error accumulation and the ~0.57 m drift.

Because RigidObjectData._bindings has no RIGID_BODY_MASS entry at init
time, body_mass.torch returns zeros; the USD-stage MassAPI value is read
instead.  The per-block drift is ~6 mm vs ~35 mm free-fall, well within
the atol=1e-2 guard, so the xfail decorator is removed.
Pure structural reorganization of
source/isaaclab_ovphysx/isaaclab_ovphysx/assets/rigid_object/rigid_object_data.py
in response to review feedback on PR isaac-sim#5426: the user asked for the file
to follow PhysX/Newton's RigidObjectData layout rather than the existing
OVPhysX articulation coding style.

* Replace `# --- section ---` comment dividers with `"""section"""`
  triple-quote section blocks, matching PhysX/Newton convention.
* Reorder sections to match PhysX top-down: defaults -> root state ->
  body state -> body properties -> derived -> sliced -> internal helpers
  -> deprecated state-concat properties.
* Extract per-instance buffer/cache attribute initialization out of
  `__init__` into a dedicated `_create_buffers` method, mirroring
  PhysX's pattern. `__init__` now stores only the bindings/device,
  counts, sim flags, and timestamp dict; `_create_buffers` is called
  once at the end of `__init__` to wire up every `*_buf`/`*_ta` slot
  to its lazy default.

The lazy `_ensure_root_buffers`, `_ensure_derived_buffers`, and
`_ensure_body_prop_buffers` helpers are kept because the owning
`RigidObject` constructs `RigidObjectData` before `num_instances` is
known, so the actual `TimestampedBuffer` allocations cannot run from
`_create_buffers`. They sit at the top of the new "Internal helpers"
block alongside the binding-read/slice helpers.

No behavior changes -- every property body, kernel launch, ProxyArray
wrap, and timestamp check is preserved. Public API surface is
unchanged. The mock-based and real-backend tests remain valid against
the same observable contracts.

Issue: isaac-sim#5316
Phase C cleanup of source/isaaclab_ovphysx/isaaclab_ovphysx/assets/
rigid_object/rigid_object.py in response to review feedback on PR isaac-sim#5426:

* Strip "Task N" markers from section headers and inline comments.
  These were planning artifacts that leaked into the production source.
* Polish public-method docstrings to match the PhysX/Newton
  RigidObject reference style: Google-style Args / Returns blocks,
  exact wording for shape/units, Sphinx roles where applicable.
* Rename the private `_write_root_state` helper to `_write_body_state`
  to reflect that a rigid object has no articulation root -- it has a
  single body. All ~14 call sites in the file are updated; the public
  writer names (write_root_pose_to_sim_*, set_masses_*, etc.) are
  unchanged because they mirror the BaseRigidObject contract.

No semantic changes. Public API surface is identical to the previous
commit.

Issue: isaac-sim#5316
Phase D cleanup of source/isaaclab_ovphysx/isaaclab_ovphysx/assets/kernels.py
in response to review feedback on PR isaac-sim#5426 ("Missing doc strings
everywhere"): every @wp.kernel and @wp.func now has a Google-style
docstring with parameter table (shapes, dtypes, units) and formula/
convention where non-obvious. Modeled on PhysX/Newton kernel
docstrings. No behavior changes; pure documentation.

Issue: isaac-sim#5316
Test-side review-comment cleanup on PR isaac-sim#5426:

* Drop the file-top wheel-gate soft-skips (importorskip on ovphysx.types
  and isaaclab_ovphysx.tensor_types, plus the RIGID_BODY_POSE hasattr
  guard). The wheel exposes these reliably; if they're missing, the
  natural ImportError is the right failure mode.
* Strip the "Real-backend port of PhysX's test_X" / "SimulationContext
  drives OvPhysxManager; ..." preamble from 16 test docstrings. The
  test file IS the real-backend suite; that boilerplate is not
  informative.
* Drop the sim_ctx_cpu and kitless_manager_cpu fixtures; inline
  build_simulation_context(device=device, ...) per test, mirroring
  PhysX. Add @pytest.mark.parametrize("device", ["cuda:0", "cpu"]) to
  every test, also matching PhysX. Closes the file-level "Why none of
  the tests run on GPU?" review comment.
* Tighten docstrings on test_initialization_with_articulation_root and
  test_initialization_with_no_rigid_body to make explicit they're
  rigid-object error-handling tests (not articulation tests). Names
  match PhysX parity; the docstrings now say so explicitly.

Issue: isaac-sim#5316
OvPhysxManager._configure_physx_scene_prim previously ran only when
ovphysx_device == "gpu", which silently skipped the PhysxSceneAPI
schema apply and the enableSceneQuerySupport attribute on CPU.
Refactor so:

* PhysxSceneAPI schema apply and enableSceneQuerySupport attribute
  run unconditionally.
* The GPU-only attributes (enableGPUDynamics, broadphaseType="GPU",
  gpu* capacity attributes from OvPhysxCfg) remain gated on
  device == "gpu" inside the function.
* The call site in _warmup_and_load is unconditional, passing the
  resolved device through.

Closes the latent CPU-side gap surfaced during the rigid-object review
on PR isaac-sim#5426 (the user's question: "Why don't we naturally route
through the OvPhysxManager._configure_physx_scene_prim even on CPU?").

Issue: isaac-sim#5316
Replace the OVPhysX-specific test set with a faithful port of
source/isaaclab_physx/test/assets/test_rigid_object.py: same 20 test
functions, same names, same parametrizations, same test bodies.

Adaptations for OVPhysX:
* No AppLauncher; SimulationContext via run_ovphysx.sh.
* root_view.set_X(...) calls replaced with the public OVPhysX setters
  (set_masses_index, set_coms_index) that go through
  RigidObject._get_binding(TT.RIGID_BODY_X).write(...). OVPhysX
  exposes a per-tensor-type bindings dict instead of a monolithic
  typed view object.
* Material-property tests stay xfailed pending the wheel-side
  RIGID_BODY_MATERIAL TensorType (gap-spec'd for Marco).

Drops the OVPhysX-only extras that were artifacts of the earlier
mock-based test suite: the test splits (e.g. _shapes/_physics
variants), the lever-arm assertions (covered by
test_body_root_state_properties with with_offset=True), and the
warmup variants (covered by
test_warmup_attach_stage_not_called_for_cpu).

Issue: isaac-sim#5316
PhysX's pattern of "fresh build_simulation_context per test" segfaults
on the OVPhysX side: ovphysx<=0.3.7's PhysX destructor races on
dual-Carbonite static state when garbage-collected mid-process. The
intended pattern is "never explicitly release; let os._exit() at
process exit handle teardown" (see OvPhysxManager._release_physx
docstring), so at most one ovphysx.PhysX may live in a pytest session.

Refactor the test file to use two session-scoped autouse fixtures:

* _ovphysx_session_patches monkey-patches OvPhysxManager so that
  _release_physx leaves the cached ovphysx.PhysX alive (calling
  physx.reset() to clear the runtime stage) and _warmup_and_load
  reuses the cached instance via add_usd() instead of constructing a
  new one.

* _ovphysx_skip_other_device pins the session to whichever device is
  requested first and skips later tests on the other device, since
  ovphysx<=0.3.7 locks the process-global device mode on first
  ovphysx.PhysX(device=...) construction. Pytest must be invoked
  twice (once per device) for full CPU + GPU coverage.

Test bodies remain 1-to-1 with PhysX modulo the per-test
build_simulation_context wrapper (still _ovphysx_sim_context, still
re-builds the SimulationContext + USD stage each test) -- the
fixtures only intercept the manager-level lifecycle.
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Follow-up Review

Previous concerns status:

  • 🔴 File truncation (rigid_object_data.py:653): This was a display artifact from the 30K char limit in the review context, not an actual code issue. The full file shows 1414 lines in the diff stats. Not a real issue.
  • 🟡 ProxyArray in wp.launch inputs: Verified correct — ProxyArray implements __array_interface__ and warp accepts it via its .warp property access pattern used elsewhere in the codebase.
  • Previous device derivation and 1-D binding concerns were already verified as correct in the original review.

New commits review:
The diff shows no substantive changes since the previous review — the file contents remain the same. The version bump to 0.2.14 in extension.toml and comprehensive CHANGELOG entries document the iterative development well.

Implementation Verdict

Ship it — The implementation is solid, test coverage is comprehensive, and the documented wheel-side gaps (material API, acceleration read) are appropriately xfailed. The 61 real-backend tests and 252 cross-backend interface tests provide strong confidence.

This rewrites the RigidObject + RigidObjectData write/read paths to follow
the canonical PhysX (PR isaac-sim#5329) and Newton conventions, replacing the
previous lazy-buffer + invalidate-cache machinery with the upstream
patterns.

Data class:
* Eagerly allocate every per-instance TimestampedBuffer in _create_buffers
  (called from __init__).  Drop _ensure_root_buffers /
  _ensure_derived_buffers / _ensure_body_prop_buffers and the per-property
  guard calls.  num_instances / num_bodies / body_names are now passed via
  the constructor instead of set post-construction.
* Bridge the wheel's pull-only binding.read(target) API to PhysX's
  pointer-stable view contract via a lazily-allocated pinned-host wp.array
  for CPU-only bindings (RIGID_BODY_MASS / COM_POSE / INERTIA).
* Fix GRAVITY_VEC_W returning (0, 0, -1) instead of (0, 0, 0) when
  cfg.gravity is the zero vector.

RigidObject writers/setters (PhysX PR isaac-sim#5329 cache-update pattern):
* Every set_*_index / write_*_to_sim_index method scatters the user data
  into the cached _body_*/_root_* buffer via the matching scatter kernel,
  then pushes the cache to the wheel via binding.write(cache, indices=...).
* Pre-allocated pinned-host CPU staging buffers (_cpu_body_mass /
  _cpu_body_coms / _cpu_body_inertia / _cpu_env_ids_all) are reused across
  calls to avoid per-call wp.clone(..., device="cpu") allocation.
* The cache becomes the single source of truth post-write; the previous
  _invalidate_caches / _invalidate_root_caches helpers are removed.

Mask path (Newton-style native mask):
* Add seven mask kernels in isaaclab_ovphysx.assets.kernels:
  set_root_link_pose_to_sim_mask, set_root_com_pose_to_sim_mask,
  set_root_com_velocity_to_sim_mask, set_root_link_velocity_to_sim_mask,
  write_2d_data_to_buffer_with_mask, write_body_inertia_to_buffer_mask,
  write_body_com_pose_to_buffer_mask.
* Each *_mask method scatters into the cache only where the boolean mask
  is True, then pushes the cache via the wheel's native
  binding.write(cache, mask=mask_u8) -- no torch.nonzero conversion.
* _resolve_env_mask / _resolve_body_mask now return wp.bool masks
  (defaulting to pre-allocated _ALL_TRUE_*_MASK), not indices.

Initialization:
* Add USD prim-scan validation to RigidObject._initialize_impl mirroring
  PhysX.  RuntimeError is now raised cleanly when cfg.prim_path resolves
  to no UsdPhysics.RigidBodyAPI prim, multiple rigid-body prims, or a prim
  with an enabled UsdPhysics.ArticulationRootAPI, instead of crashing with
  an obscure TypeError deep in property accessors.
Test contract:
* Update two test_rigid_object.py callers of set_coms_index to pass
  dtype=wp.transformf with shape (N, 1, 7), matching the PhysX-style
  contract that the rewritten setter now enforces via assert_shape_and_dtype.
  Previous wp.float32 (N, 7) inputs hit the dtype guard.

CI documentation:
* Add a "CI note" to the test_rigid_object.py module docstring spelling
  out that ovphysx<=0.3.7's process-global device-mode lock requires two
  separate ./scripts/run_ovphysx.sh -m pytest invocations (-k 'cpu' and
  -k 'cuda:0') for full coverage.  Cross-references gap G5 in the
  Marco-side wheel-gaps doc.
* Add the same warning to the scripts/run_ovphysx.sh header comment so
  the next person setting up CI does not trip on the device lock.
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Follow-up Review

The new commits since the previous review are primarily documentation and test infrastructure changes. The previous review's findings were either display artifacts (file truncation) or verified as correct (ProxyArray handling, device derivation).

New observations:

  • CHANGELOG entries are comprehensive and well-documented
  • Version bump to 0.2.15 is appropriate for the scope of changes
  • No new code issues introduced

Implementation Verdict

Ship it — The implementation is mature, well-tested (61 real-backend + 252 cross-backend interface tests), and wheel-side gaps are appropriately documented and xfailed.

Collapses the 15 incremental 0.2.0 -> 0.2.15 entries accumulated during
PR isaac-sim#5426 into one comprehensive 0.2.15 entry organised by Added /
Changed / Removed / Fixed.  CHANGELOG.rst is now 174 lines (down from 450).
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Follow-up Review

Previous review confirmed the implementation was ready to ship. The new commits since then are minor:

  • Version bump 0.2.15 in extension.toml
  • Extended CHANGELOG with comprehensive documentation of the 0.2.15 release

No new code changes were introduced. The implementation remains correct and well-tested.

Implementation Verdict

Ship it — No new issues. The PR is ready for merge.

Address Antoine's PR isaac-sim#5426 review comments:

* Rename set_root_*_to_sim kernels to *_to_sim_index and drop the
  from_mask flag plus unused root_*_state_w outputs (RigidObject does
  not have those state buffers).  Same simplification for
  write_2d_data_to_buffer_with_indices, write_body_inertia_to_buffer,
  and write_body_com_pose_to_buffer (renamed to *_index).
* Drop the full_data parameter from every *_index writer and setter on
  RigidObject; index methods strictly accept partial data shaped
  (len(env_ids), ...).  Full-data callers should use the matching
  *_mask overload.  Matches Newton/PhysX convention.
* Replace the deprecated write_root_state_to_sim shims' use of the
  internal _write_body_state plumbing with direct calls to the public
  write_root_*_to_sim_index methods, mirroring PhysX/Newton.
* Remove the GPU-side write plumbing (_write_body_state,
  _com_pose_to_link_pose, _to_flat_f32, _as_gpu_f32_2d,
  _get_write_scratch, _stage_to_pinned_cpu, _binding_write,
  _binding_read, _to_cpu_numpy, _to_cpu_indices,
  _env_ids_to_gpu_warp, _n_envs_index) that those shims relied on.
* Remove the now-unused _compose_root_link_pose_from_com kernel; the
  set_root_com_pose_to_sim_* kernels recover the link pose inline via
  get_com_pose_in_link_frame_func.
* Remove the masses 1-D-to-(K, 1) auto-reshape from set_masses_index;
  callers must pass shape (len(env_ids), len(body_ids)) explicitly.
* Reword every public docstring on RigidObject to follow the
  Newton/PhysX template (one-line summary, .. note:: / .. tip:: blocks,
  Args: block with shape/dtype on the parameter line).

test_rigid_body_set_mass updated to drop the now-unnecessary
masses.squeeze(-1) call (matches the Newton test's signature).

Bumps isaaclab_ovphysx 0.2.15 -> 0.2.16.
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Follow-up Review

The new commits since the previous review are:

  • Version bump 0.2.15 → 0.2.16 in extension.toml
  • Extended CHANGELOG documenting the 0.2.16 release (kernel split, docstring alignment, removed deprecated plumbing)

No new production code changes were introduced — these are documentation and versioning updates only.

Implementation Verdict

Ship it — No new issues. The PR remains ready for merge.

Previously write_data_to_sim rebuilt the (N, 9) zero-copy reshape of
self._wrench_buf on every step.  The view aliases a fixed allocation
whose shape never changes, so build it once at buffer creation and
reuse the cached view in the hot path.

Addresses Antoine's PR isaac-sim#5426 review comment on rigid_object.py:188.
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Follow-up Review

The new commits since my previous review are purely documentation and versioning changes:

  • Version bump 0.2.15 → 0.2.16 in extension.toml
  • Extended CHANGELOG documenting the 0.2.16 release

No production code changes were introduced. The previous review's findings all related to pre-existing code that remains unchanged.

Implementation Verdict

Ship it — No new issues introduced. The PR remains ready for merge.

run_ovphysx.sh adds pxr/carb/omni/omni.kit/omni.kit.app to PYTHONPATH,
so they are real imports under the kitless run -- not stubs.  Only the
modules that genuinely cannot resolve (isaacsim.core[.simulation_manager],
omni.physics[.tensors], omni.physx, omni.timeline, omni.usd) need
MagicMock entries.

Because ``omni`` is a real namespace package, attribute access on it
will not fall through to ``sys.modules`` for missing submodules; install
each ``omni.<sub>`` mock as both a ``sys.modules`` entry and an
attribute on the live ``omni`` module so ``import omni.timeline`` and
later ``omni.timeline.foo()`` access both resolve.

Addresses Antoine's PR isaac-sim#5426 review comments on
test_rigid_object_iface.py (lines 19, 55) and test_articulation_iface.py
(line 29).
The fixture was calling data._process_cfg(obj.cfg), but _process_cfg
moved from RigidObjectData onto RigidObject in commit 0887d2d
("Mirror PhysX/Newton patterns in OVPhysX RigidObject"). PhysX/Newton
fixtures don't populate default_root_pose / default_root_vel at all,
so drop the call.

The fixture was also missing the asset-side buffers (_ALL_INDICES,
_ALL_BODY_INDICES, _ALL_TRUE_*_MASK, _wrench_buf, _cpu_* pinned
staging) that RigidObject._initialize_impl normally allocates via
_create_buffers. The PhysX / Newton fixtures install these manually;
on the OVPhysX side it is simpler to call obj._create_buffers()
directly and then overwrite the real WrenchComposers with mocks.

Brings the failing 372 cross-backend iface tests to passing
(372 ovphysx-backend cases now green; 1476 rigid-object iface tests
total pass with 12 documented xfails).
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Follow-up Review

The new commits since my previous review are minimal:

  • Version bump 0.2.15 → 0.2.16 in extension.toml
  • Extended CHANGELOG documenting the 0.2.16 release with detailed notes on API changes, removals, and fixes

No production code changes were introduced. The CHANGELOG accurately reflects the architectural changes made in this PR.

Implementation Verdict

Ship it — No new issues. The PR is ready for merge.

@AntoineRichard AntoineRichard marked this pull request as ready for review April 30, 2026 12:50
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR implements RigidObject and RigidObjectData for the OVPhysX backend, satisfying the BaseRigidObject / BaseRigidObjectData contracts by mirroring the existing Articulation and Newton patterns. Shared Warp kernels are consolidated into a new assets/kernels.py, the OvPhysxManager scene-configuration is fixed to apply enableSceneQuerySupport on both CPU and GPU devices, and six RIGID_BODY_* TensorType aliases are added with three forward-compat try/except guards for pending wheel types.

Confidence Score: 4/5

Safe to merge with the noted P2 items tracked as follow-ups; no blocking correctness issues found in the primary data paths.

All findings are P2: broad exception catch in _get_binding, one-step acceleration spike after env reset (intentional Newton parity), update(dt) ignoring its dt parameter for FD, and a fragile kitless heuristic in the test file. No P0 or P1 issues identified. The core COM↔link transform math, mask/index write paths, lazy timestamp caching, and kernel relocation are all correct.

rigid_object_data.py (_previous_body_com_vel reset behavior) and test_rigid_object_iface.py (kitless detection heuristic)

Important Files Changed

Filename Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/assets/rigid_object/rigid_object.py New OVPhysX RigidObject implementation (~1164 lines); broadly mirrors the Articulation pattern with correct lazy binding, indexed/mask write paths, and priming lifecycle — one broad exception catch in _get_binding deserves attention.
source/isaaclab_ovphysx/isaaclab_ovphysx/assets/rigid_object/rigid_object_data.py New RigidObjectData container implementing lazy timestamped buffers; FD acceleration via shared kernel correctly mirrors Newton, but _previous_body_com_vel is not reset when velocity is externally written, yielding a one-step spurious acceleration spike after env reset.
source/isaaclab_ovphysx/isaaclab_ovphysx/assets/kernels.py Shared Warp kernel library relocated from articulation/kernels.py and extended with rigid-object write/read kernels; COM↔link transforms, FD acceleration, and mask/index scatter paths look mathematically correct.
source/isaaclab_ovphysx/isaaclab_ovphysx/tensor_types.py Adds six RIGID_BODY_* TensorType aliases and three forward-compat try/except aliases; defensive _CPU_ONLY_TYPES construction via tuple concatenation is clean and wheel-update-agnostic.
source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py Correctly extends _configure_physx_scene_prim to apply enableSceneQuerySupport on both CPU and GPU, restricting GPU dynamics attrs to device==gpu only; also adds get_dt() alias and passes ovphysx_device through the guard condition.
source/isaaclab/test/assets/test_rigid_object_iface.py Adds ovphysx factory and backend registration; kitless-detection heuristic (LD_PRELOAD=="" and EXP_PATH not in env) may skip AppLauncher incorrectly in some CI environments.
source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/kernels.py Kernels shared with RigidObject correctly relocated to the parent assets/kernels.py; only articulation-specific kernels (_compose_body_com_poses, _fd_joint_acc) remain.
source/isaaclab_tasks/isaaclab_tasks/direct/allegro_hand/allegro_hand_env_cfg.py Adds ovphysx preset variants for ObjectCfg and PhysicsCfg mirroring the existing physx/newton entries; straightforward hookup with no logic changes.

Sequence Diagram

sequenceDiagram
    participant Env as RL Environment
    participant RO as RigidObject
    participant ROD as RigidObjectData
    participant WP as Warp Kernels
    participant Binding as TensorBinding (OVPhysX wheel)

    Note over RO: _initialize_impl()
    RO->>Binding: create_tensor_binding(RIGID_BODY_POSE/VEL/WRENCH/MASS/COM/INERTIA)
    RO->>ROD: RigidObjectData(bindings, device)
    ROD->>Binding: read MASS, INERTIA (CPU-only, once)
    RO->>RO: _create_buffers() + _process_cfg()
    RO->>ROD: update(0.0)  [priming]

    Note over Env,Binding: Per-step loop
    Env->>RO: write_root_link_pose_to_sim_index(pose, env_ids)
    RO->>WP: set_root_link_pose_to_sim_index kernel
    RO->>Binding: binding.write(link_pose, indices=env_ids)

    Env->>RO: write_data_to_sim()
    RO->>WP: _body_wrench_to_world kernel
    RO->>Binding: RIGID_BODY_WRENCH.write(wrench_buf)

    Note over ROD: Lazy pull-to-refresh on access
    Env->>RO: data.root_link_pose_w
    RO->>ROD: root_link_pose_w (timestamp check)
    ROD->>Binding: RIGID_BODY_POSE.read → _root_link_pose_w.data
    ROD-->>Env: ProxyArray (stable GPU ptr)

    Env->>RO: update(dt)
    RO->>ROD: update(dt) → body_com_acc_w
    ROD->>WP: derive_body_acceleration_from_body_com_velocities
    WP-->>ROD: _body_com_acc_w.data, prev_vel updated
Loading

Reviews (1): Last reviewed commit: "Fix OVPhysX iface fixture to mirror Phys..." | Re-trigger Greptile

Comment on lines +1154 to +1160
try:
binding = self._ovphysx.create_tensor_binding(pattern=self._binding_pattern, tensor_type=tensor_type)
self._bindings[tensor_type] = binding
return binding
except Exception:
logger.debug("Could not create tensor binding for type %s", tensor_type)
return None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Overly broad exception catch silences programming errors

except Exception swallows every exception — including AttributeError, TypeError, or bugs in the calling code — and only emits a DEBUG-level log. If the pattern or tensor_type argument is malformed, the failure disappears silently and downstream code gets a None binding that surfaces as an obscure KeyError or NoneType attribute error. The six eagerly-created bindings in _initialize_impl do catch the None return and raise a RuntimeError, but lazy bindings acquired after initialization have no such guard.

Consider narrowing to the specific wheel-side exception type (e.g. a custom OvPhysxError or RuntimeError) so programming mistakes are not accidentally silenced.

Suggested change
try:
binding = self._ovphysx.create_tensor_binding(pattern=self._binding_pattern, tensor_type=tensor_type)
self._bindings[tensor_type] = binding
return binding
except Exception:
logger.debug("Could not create tensor binding for type %s", tensor_type)
return None
except RuntimeError:
logger.debug("Could not create tensor binding for type %s", tensor_type)
return None

Comment on lines +366 to +385
if self._body_com_acc_w.timestamp < self._sim_timestamp:
if self._previous_body_com_vel is None:
self._previous_body_com_vel = wp.clone(self.body_com_vel_w.warp)
wp.launch(
shared_kernels.derive_body_acceleration_from_body_com_velocities,
dim=(self._num_instances, 1),
device=self.device,
inputs=[
self.body_com_vel_w.warp,
SimulationManager.get_physics_dt(),
self._previous_body_com_vel,
],
outputs=[
self._body_com_acc_w.data,
],
)
self._body_com_acc_w.timestamp = self._sim_timestamp
if self._body_com_acc_w_ta is None:
self._body_com_acc_w_ta = ProxyArray(self._body_com_acc_w.data)
return self._body_com_acc_w_ta
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Spurious acceleration spike on the first update() after an env reset

When write_root_com_velocity_to_sim_index/mask is called (e.g. on env reset), the warp kernel correctly zeroes body_acc_w. However, _previous_body_com_vel is not updated to match the new reset velocity. On the very next update(dt) call the derive_body_acceleration_from_body_com_velocities kernel runs and overwrites the zeroed buffer with:

acc = (new_sim_vel - old_pre_reset_vel) / dt  # large spurious spike for one step

Any policy that reads body_com_acc_w on the first observation after a reset will see this incorrect value. The pattern is inherited from Newton, but it is worth an explicit comment here and a follow-up fix (e.g. also updating _previous_body_com_vel in the velocity-write kernels).

Comment on lines 21 to +28

# When running kitless (e.g., ovphysx backend via run_ovphysx.sh), AppLauncher
# will try to boot Kit and hang. Skip it entirely: run_ovphysx.sh sets
# LD_PRELOAD to the ovphysx libcarb.so, which is the signature of a kitless
# ovphysx run. Also guard the case where neither LD_PRELOAD nor EXP_PATH is
# set (bare Python, no Kit at all).
_kitless = "ovphysx" in os.environ.get("LD_PRELOAD", "") or (
os.environ.get("LD_PRELOAD", "") == "" and "EXP_PATH" not in os.environ
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Fragile kitless-detection heuristic may skip AppLauncher in standard Kit CI

The second branch of _kitless:

os.environ.get("LD_PRELOAD", "") == "" and "EXP_PATH" not in os.environ

treats any Python process with no LD_PRELOAD and no EXP_PATH as "kitless" and skips AppLauncher. An Isaac Sim Kit environment that happens to have cleared LD_PRELOAD (or never sets EXP_PATH in some CI configurations) would also satisfy this condition, silently bypassing the full Kit initialization. The physx/newton backends in the same file still require a live Kit session, so if those backends are exercised in the same test run the session state will be inconsistent.

Consider using a dedicated opt-in env var (e.g. ISAACLAB_KITLESS=1) rather than inferring from LD_PRELOAD/EXP_PATH.

Comment on lines +123 to +132
def update(self, dt: float) -> None:
"""Updates the data for the rigid object.

Args:
dt: The time step for the update [s]. This must be a positive value.
"""
# update the simulation timestamp
self._sim_timestamp += dt
# Mirrors Newton's update() pattern (rigid_object_data.py line 126).
self.body_com_acc_w
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 update() ignores its dt argument for acceleration FD

The body_com_acc_w property — triggered by self.body_com_acc_w at line 132 — uses SimulationManager.get_physics_dt() as the FD time-step rather than the dt parameter passed to update(). For normal sim steps these are identical, but on the priming call update(0.0) the kernel receives the real physics dt (e.g. 1/120), not 0.0. The mismatch is harmless in practice (since initial prev ≈ cur, acc ≈ 0 regardless of dt), but the inconsistency between the method signature and the actual dt used for differentiation can confuse callers. A comment noting the intentional deviation would help.

AntoineRichard added a commit to AntoineRichard/IsaacLab that referenced this pull request Apr 30, 2026
Substitutes the verbatim PhysX-mirror's ``articulation.root_view.get_X()``
/ ``set_X()`` calls with OVPhysX-direct equivalents, mirroring the
RigidObject test precedent (test_rigid_object.py:21-25).

- ``get_dof_max_velocities()`` / ``get_dof_max_forces()`` /
  ``get_dof_friction_properties()``: replaced by
  ``_read_binding_to_torch(...)``, a small test-side helper that reads
  the wheel binding into a torch tensor on the requested device for the
  cross-check against ``data.<X>``.
- ``get_coms()`` / ``set_coms()``: replaced by
  ``_read_binding_to_torch(..., TT.BODY_COM_POSE, device)`` and
  ``set_coms_index(coms=..., env_ids=...)`` (wp.transformf contract).
- ``test_set_material_properties``: xfailed with ``_MATERIAL_GAP_REASON``
  pointing at the wheel-gaps spec — same gap as the rigid-object
  ``set_material_properties`` xfail in isaac-sim#5426.  The wheel does not expose
  ``RIGID_BODY_MATERIAL`` / ``max_shapes``, so neither ``set`` nor
  ``get`` material properties via ``root_view`` is available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant