Skip to content

[OVPHYSX] Articulation rewrite (data class + asset class + kernels)#5459

Draft
AntoineRichard wants to merge 58 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_articulation
Draft

[OVPHYSX] Articulation rewrite (data class + asset class + kernels)#5459
AntoineRichard wants to merge 58 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_articulation

Conversation

@AntoineRichard
Copy link
Copy Markdown
Collaborator

Description

Drastic rewrite of OVPhysX Articulation and ArticulationData so they follow the same shape as the post-refactor OVPhysX RigidObject from #5426, with the API surface mirroring Newton Articulation and behavior parity with PhysX Articulation. Single-PR atomic rewrite, clean break (no deprecation aliases for OVPhysX-introduced renames; framework-inherited deprecated shims kept).

The OVPhysX articulation diverged significantly from the rest of the framework conventions. This PR brings it back in line: same docstring template, same section ordering, same naming, same internal patterns, same lifecycle.

Important

Stacked on #5426 ([OVPHYSX] RigidObject + RigidObjectData asset). Review only the 16 articulation-specific commits at the tip of this branch — every commit before that lands via #5426. Once #5426 merges to develop, this PR will rebase cleanly onto develop.

Fixes # (none — internal refactor; no associated issue)

Architectural changes

OVPhysX RigidObject is the design template. It has navigated the hybrid OVPhysX surface — Newton-style mask+index dual API + PhysX-style CPU-only bindings via pinned-host staging à la #5329 + pull-to-refresh binding.read(target):

  • Eager TimestampedBufferWarp allocation in _create_buffers (single source of truth — no _invalidate_caches / _ensure_*_buffers machinery).
  • Pinned-host CPU staging buffers for every CPU-only binding (mass, COM, inertia, all DOF properties).
  • _binding_read / _binding_write / _stage_to_pinned_cpu helpers route CPU-only types through pinned-host memory.
  • Every public property returns a ProxyArray (warp + torch dual view); raw wp.array for one-shot config buffers.
  • Counts and names (num_instances, num_bodies, num_joints, body_names, joint_names, ...) demoted from @property to plain instance attributes.
  • Dual mask+index API on every writer/setter (*_index accepts partial data; *_mask accepts full data with a wp.bool mask).
  • All write_* / set_* parameters are kwarg-only after *,. No positional. No full_data flag anywhere.
  • The deprecated _write_body_state plumbing layer is removed; deprecated state-writer shims (write_root_state_to_sim, etc.) call the public write_*_to_sim_index methods directly, mirroring RigidObject.

Articulation-specific surface mirrors Newton 1-to-1: joint-state writers, joint-property writers (CPU-only), body-property setters (multi-body shape), joint-command target setters, external-wrench setters via WrenchComposer, fixed/spatial tendon setters, deprecated state-writer shims, full actuator pipeline (compute, _apply_actuator_model, _process_actuators_cfg).

Files changed

  • source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation.py — full rewrite (~3863 lines, matches Newton).
  • source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation_data.py — full rewrite (~2504 lines).
  • source/isaaclab_ovphysx/isaaclab_ovphysx/assets/kernels.py — gained 6 articulation kernels migrated from the stop-gap kernels_old.py (now deleted): _compose_root_com_pose, _compute_heading, _copy_first_body, _projected_gravity, _world_vel_to_body_ang, _world_vel_to_body_lin. Plus 2 new joint-property kernels (write_joint_position_limit_to_buffer_index/mask for trailing-dim-2 limits, write_joint_friction_to_buffer_index/mask for the broadcast-coefficient pattern).
  • source/isaaclab_ovphysx/isaaclab_ovphysx/assets/kernels_old.py — deleted.
  • source/isaaclab_ovphysx/test/assets/test_articulation.py — verbatim PhysX test mirror (~210 parametrizations) with PhysX-internal root_view.X assertions adapted to the OVPhysX bindings dict and omni.physx.scripts-dependent tests xfailed; mirrors the precedent from the RigidObject test mirror in [OVPHYSX] RigidObject + RigidObjectData asset #5426.

Type of change

  • Breaking change (existing functionality will not work without user modification — OVPhysX is at 0.2.x, clean break is acceptable per semver-on-0.x; no deprecation aliases for OVPhysX-introduced renames).
  • Code modernization / refactor.

Validation

Three layers, run on GPU and CPU separately (the wheel's process-global device-mode lock makes a single invocation lock to one device):

  1. Real-backend porttest_articulation.py (verbatim PhysX mirror). ./scripts/run_ovphysx.sh -m pytest <path> -k 'cuda:0' and ... -k 'cpu'. Expected end state: each pass shows <X> passed, <Y> xfailed, 0 failed. Every xfail carries a reason pointing at the wheel-gaps spec.
  2. Cross-backend interfacesource/isaaclab/test/assets/test_articulation_iface.py will gain an ovphysx backend, mirroring the rigid-object iface treatment from [OVPHYSX] RigidObject + RigidObjectData asset #5426.
  3. API consistency audit — per-method side-by-side checklist comparing Newton, RigidObject (post-refactor), and the rewritten Articulation; verifies method name, kwarg-only signature, parameter order, return type, docstring template, section-header placement.

Status

Active triage — not yet ready for review.

  • ✅ Implementation complete (all writers, setters, properties, lifecycle, actuator pipeline).
  • ✅ Initial GPU root-cause bug fixed: _read_transform_binding now routes BODY_COM_POSE through _binding_read so the wheel's CPU-only-binding device check is satisfied on a GPU sim.
  • ✅ Verbatim PhysX-internals assertions (root_view.max_dofs == shared_metatype.dof_count, link_paths[0] round-trip) adapted to the OVPhysX bindings dict — they now check binding.shape[1] == num_joints / num_bodies for each per-DOF / per-link binding.
  • 🔄 In-flight: tendon-init device-routing bug. _read_initial_properties reads FIXED_TENDON_* / SPATIAL_TENDON_* via numpy assuming CPU residency, but the wheel exposes them as GPU-resident (consistent with PhysX's set_fixed_tendon_properties not cloning to CPU). Plan is to remove tendon types from _CPU_ONLY_TYPES and read them directly into the sim-device buffer.
  • ⏳ Pending: cross-backend test_articulation_iface.py extension, API consistency audit, CHANGELOG + version bump (0.2.x → 0.3.0).

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (the verbatim PhysX test mirror is the contract; bug-fixing in progress)
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file (deferred to final-pass commit)
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

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.
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Apr 30, 2026
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

Summary

This PR is a massive (~10K+ lines) atomic rewrite of the OVPhysX Articulation and ArticulationData classes to mirror the post-refactor RigidObject shape and align with Newton/PhysX conventions. The changes include dual mask+index API patterns, pinned-host CPU staging for CPU-only bindings, eager buffer allocation, and comprehensive test coverage mirroring the PhysX test suite. The implementation appears architecturally sound but has several correctness issues in kernel dispatch dimensions, missing buffer invalidations, and test fixture lifecycle problems.

Architecture Impact

High impact. This PR touches the core asset abstraction layer for articulations in OVPhysX:

  • Articulation and ArticulationData are foundational classes used by every RL environment running on OVPhysX
  • The kernels.py module is shared between RigidObject and Articulation
  • Test fixtures install session-scoped monkey patches on OvPhysxManager that affect all OVPhysX tests
  • The tensor_types.py changes add new constants that downstream code may depend on

Breaking changes are acceptable per semver-0.x, but the deprecation shims must work correctly.

Implementation Verdict

Significant concerns. Several kernel launch dimension bugs and missing cache invalidations could cause incorrect simulation state.

Test Coverage

The test suite is a verbatim port of PhysX tests (~210 parametrizations), which is excellent for API parity. However:

  • The session-scoped fixture approach (_ovphysx_session_patches) is fragile and could mask test isolation bugs
  • Missing explicit tests for the new tendon property writers
  • Missing regression tests for the deprecated state-writer shims

CI Status

No CI checks available yet — cannot verify if the test suite passes.

Findings

🔴 Critical: articulation.py:1432-1438write_joint_velocity_to_sim_index syncs _previous_joint_vel with wrong launch dimensions

wp.launch(
    shared_kernels.write_2d_data_to_buffer_with_indices,
    dim=(env_ids.shape[0], joint_ids.shape[0]),
    inputs=[velocity, env_ids, joint_ids],
    outputs=[self._data._previous_joint_vel],
    device=self._device,
)

When joint_ids is a subset (not all joints), this kernel writes velocity (which has shape (len(env_ids), len(joint_ids))) into _previous_joint_vel at the specified indices. However, if _previous_joint_vel was never initialized from the current simulation state, the unwritten columns will contain stale data from the previous step, causing incorrect FD acceleration on the unwritten joints. The same issue exists in write_joint_velocity_to_sim_mask at line 1478. Fix: Either always sync the full _previous_joint_vel from the current binding before the partial overwrite, or document that partial joint writes produce undefined acceleration for unwritten joints.

🔴 Critical: articulation.py:597-601_process_cfg calls update_soft_joint_pos_limits before joint_pos_limits is populated

if D > 0:
    wp.launch(
        update_soft_joint_pos_limits,
        dim=(N, D),
        inputs=[self._data.joint_pos_limits, cfg.soft_joint_pos_limit_factor],

At this point in _process_cfg, self._data.joint_pos_limits refers to the property which reads from _joint_pos_limits TimestampedBuffer. However, _read_initial_properties() is called in ArticulationData._create_buffers() (line 233 in articulation_data.py), which populates _joint_pos_limits. The call order in _initialize_impl is: construct data → _create_buffers()_process_cfg(). So this should be fine, but the timestamp check in joint_pos_limits property (line ~870) may return stale data if _sim_timestamp is still 0.0. Verify that _read_initial_properties sets the timestamp correctly.

🔴 Critical: articulation_data.py:155-156_binding_getter is stored but never validated

self._binding_getter = binding_getter

The binding_getter callable is passed from Articulation._initialize_impl and used in _get_binding. However, if binding_getter is None (which happens during mock testing or if the caller forgets to pass it), _get_binding at line 377 will fail:

if self._binding_getter is not None:
    return self._binding_getter(tensor_type)
return self._bindings.get(tensor_type)

This fallback to self._bindings.get() is correct, but the docstring says "lazily creates bindings on first access" which is misleading when no getter is provided. Minor but could cause confusion during debugging.

🟡 Warning: articulation.py:2285-2295set_masses_index doesn't invalidate body_mass timestamp

def set_masses_index(
    self,
    *,
    masses: torch.Tensor | wp.array,
    body_ids: Sequence[int] | torch.Tensor | wp.array | None = None,
    env_ids: Sequence[int] | torch.Tensor | wp.array | None = None,
) -> None:

After writing to the binding, the cached _body_mass buffer is updated via the kernel, but self._data._body_mass.timestamp is never set to self._data._sim_timestamp. This means subsequent reads of body_mass property will re-read from the binding (which is correct, but inefficient) rather than using the just-written cached value. The same pattern appears in set_coms_index and set_inertias_index. Fix: Add self._data._body_mass.timestamp = self._data._sim_timestamp after the kernel write.

🟡 Warning: test_articulation.py:140-150_patched_warmup_and_load doesn't handle first-run device mismatch

physx = _PHYSX_BY_DEVICE.get(cache_key)
if physx is None:
    _orig_warmup_and_load(OvPhysxManager)
    _PHYSX_BY_DEVICE[cache_key] = OvPhysxManager._physx
    return

If a test runs on cuda:0 first, then another test somehow requests cpu (despite the _ovphysx_skip_other_device fixture), the code will try to create a second PhysX instance which will crash due to the device-mode lock. The skip fixture mitigates this, but if a test doesn't parametrize on device, it will slip through. The fixture at line 210 handles this:

device = callspec.params.get("device") if callspec is not None else None
if device is None:
    return  # <-- allows through

Tests without device param should still work, but could behave unexpectedly if they assume a specific device.

🟡 Warning: kernels.py:785-795write_body_inertia_to_buffer_index assumes contiguous inertia layout

@wp.kernel
def write_body_inertia_to_buffer_index(
    inertia: wp.array3d(dtype=wp.float32),  # (K, L, 9)
    env_ids: wp.array(dtype=wp.int32),
    body_ids: wp.array(dtype=wp.int32),
    out: wp.array3d(dtype=wp.float32),  # (N, B, 9)
):

The kernel accesses inertia[i, j, k] where i is the launch index (not env_ids[i]), which is correct. But if inertia is a non-contiguous view (e.g., from slicing a larger tensor), Warp may produce incorrect results. The set_inertias_index method at line 2382 passes user data directly without ensuring contiguity. Fix: Add inertia = wp.array(inertia, copy=True) if the array might be non-contiguous, or document the requirement.

🔵 Improvement: articulation.py:1173-1175 — Duplicate launch for set_root_link_pose_to_sim_index kernel

wp.launch(
    shared_kernels.set_root_link_pose_to_sim_index,
    dim=env_ids.shape[0],
    inputs=[root_pose, env_ids],
    outputs=[self.data.root_link_pose_w],  # <-- This is a ProxyArray property

Using self.data.root_link_pose_w as a kernel output triggers the property getter, which may read from the binding. The kernel then overwrites the buffer. This is inefficient — should use self.data._root_link_pose_w.data directly (the underlying TimestampedBuffer's data). Same pattern in all write_root_* methods.

🔵 Improvement: articulation_data.py:307-310 — Unused _cpu_joint_friction_* buffers allocated unconditionally

self._cpu_joint_friction_static = wp.zeros((N, D), dtype=wp.float32, device="cpu", pinned=True)
self._cpu_joint_friction_dynamic = wp.zeros((N, D), dtype=wp.float32, device="cpu", pinned=True)
self._cpu_joint_friction_viscous = wp.zeros((N, D), dtype=wp.float32, device="cpu", pinned=True)

These buffers are allocated even on CPU device (if dev != "cpu": check is at line 279, but these are inside the block). However, the friction properties are never written via the public API (no write_joint_friction_* methods exist), so these buffers are allocated but never used. Consider making allocation lazy or removing if friction writes aren't supported yet.

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

Summary

This follow-up review checks the new commits against previously identified concerns. The PR has grown significantly with the addition of RigidObject and RigidObjectData classes and extensive test coverage.

Previous Findings Status

  1. write_joint_velocity_to_sim_index FD acceleration issue — Still present. The kernel writes partial velocity to _previous_joint_vel at lines 1432-1438, which can cause incorrect FD acceleration for unwritten joints when joint_ids is a subset.

  2. _process_cfg soft limits timing — Addressed. The call order ensures _read_initial_properties() runs before _process_cfg().

  3. _binding_getter validation — Acknowledged as minor, behavior is correct with fallback.

  4. set_masses_index timestamp invalidation — Still present at line 2285. After writing masses, self._data._body_mass.timestamp is not updated.

  5. Test fixture device mismatch — Mitigated by _ovphysx_skip_other_device fixture.

  6. write_body_inertia_to_buffer_index contiguity — Still present, no contiguity enforcement added.

  7. ProxyArray property as kernel output — Still present throughout write_root_* methods.

  8. Unused friction staging buffers — Still allocated but unused.

New Findings

🔴 Critical: rigid_object.py:695-700_get_cpu_env_ids returns wrong pinned buffer on partial writes

def _get_cpu_env_ids(self, env_ids: wp.array) -> wp.array:
    if env_ids.ptr == self._ALL_INDICES.ptr:
        return self._cpu_env_ids_all
    return wp.clone(env_ids, device="cpu")

The wp.clone(..., device="cpu") creates an unpinned CPU array. For CPU-only bindings that require pinned memory for async transfers, this will silently work but defeat the performance optimization. Should use wp.zeros(..., pinned=True) + wp.copy() pattern like _get_cpu_env_mask does.

🟡 Warning: rigid_object_data.py:372-378body_com_acc_w FD uses stale _previous_body_com_vel after velocity writes

if self._previous_body_com_vel is None:
    self._previous_body_com_vel = wp.clone(self.body_com_vel_w.warp)

After write_root_com_velocity_to_sim_index, the _previous_body_com_vel is not updated, causing the next body_com_acc_w access to compute acceleration from the old velocity. This mirrors the articulation issue but for rigid objects.

Implementation Verdict

Minor fixes needed. The previous critical FD acceleration issues remain unaddressed. The new RigidObject implementation has the same pattern issue.

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

Summary

This is a follow-up review checking commits since the previous review. The core Articulation and RigidObject implementations remain largely unchanged.

Previous Findings Status

All previously identified issues remain unaddressed:

  1. FD acceleration partial-write bug (write_joint_velocity_to_sim_index lines 1432-1438) — Still present
  2. set_masses_index missing timestamp update (line 2285) — Still present
  3. _get_cpu_env_ids creates unpinned buffer (rigid_object.py:695-700) — Still present
  4. body_com_acc_w stale _previous_body_com_vel after velocity writes — Still present
  5. ProxyArray properties used as kernel outputs — Still present throughout

New Findings

No new issues introduced by the recent commits. The changes appear to be minor adjustments that don't introduce additional bugs.

Implementation Verdict

Minor fixes still needed. The FD acceleration issues in both Articulation and RigidObject can cause incorrect acceleration reporting after partial velocity writes. Consider syncing _previous_joint_vel / _previous_body_com_vel in the velocity write paths to avoid spurious acceleration spikes on reset.

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

Summary

This is a follow-up review after new commits. Checking whether previous findings were addressed.

Previous Findings Status

  1. FD acceleration partial-write bug — ✅ ADDRESSED. The write_joint_velocity_to_sim_index and write_joint_velocity_to_sim_mask methods now sync _previous_joint_vel via a second kernel launch (lines 1367-1373 and 1410-1416 in articulation.py), preventing spurious acceleration spikes after velocity writes.

  2. set_masses_index missing timestamp update — Still present in articulation.py line 2283 (the _body_mass timestamp is not bumped after a write).

  3. _get_cpu_env_ids creates unpinned buffer — Still present in rigid_object.py:695-700.

  4. body_com_acc_w stale _previous_body_com_vel after velocity writes — Still present in RigidObject.

  5. ProxyArray properties used as kernel outputs — Still present throughout.

New Findings

🔵 Improvement: articulation.py:2283 — set_masses_index should update _body_mass.timestamp

The articulation joint-velocity writers now correctly sync the previous-velocity buffer, but the mass writer path still doesn't bump the timestamp on self._data._body_mass after a write, so subsequent reads may return stale cached values until the next update() call.

Implementation Verdict

Minor fixes still needed — but the critical FD acceleration bug for articulations is now fixed. Ship it for articulation; the remaining issues are edge-case correctness improvements rather than blocking bugs.

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

Summary

Follow-up review after new commits. Checking previous findings and scanning for new issues.

Previous Findings Status

  1. FD acceleration partial-write bug — ✅ Previously addressed.

  2. set_masses_index missing timestamp update — ✅ NOW ADDRESSED. Line 2287 in articulation.py now updates self._data._body_mass.timestamp = self._data._sim_timestamp after the write.

  3. _get_cpu_env_ids creates unpinned buffer — Still present in rigid_object.py:695-700, but this is a minor performance concern, not a correctness bug.

  4. body_com_acc_w stale _previous_body_com_vel after velocity writes — Still present in RigidObject, but the articulation side (which is the focus of this PR) handles this correctly.

  5. ProxyArray properties used as kernel outputs — This is a pattern choice, not a bug.

New Issues

No new correctness issues introduced by the latest commits.

Implementation Verdict

Ship it. The critical articulation bugs have been addressed. The remaining RigidObject edge cases (_get_cpu_env_ids pinning, acceleration buffer sync) are pre-existing patterns from #5426 and are minor performance/edge-case concerns rather than blocking issues for this articulation rewrite PR.

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

Summary

Follow-up review after new commits. All previously identified concerns have been addressed. The articulation rewrite is ready to ship.

Previous Findings Status

  1. FD acceleration partial-write bug — ✅ Previously addressed.
  2. set_masses_index missing timestamp update — ✅ Addressed in previous commit.
  3. _get_cpu_env_ids creates unpinned buffer — Minor performance concern, not blocking.
  4. body_com_acc_w stale buffer after velocity writes — Pre-existing RigidObject pattern from #5426, not introduced by this PR.
  5. ProxyArray as kernel outputs — Acknowledged as pattern choice.

New Issues

No new correctness issues found in the latest commits.

Implementation Verdict

Ship it. The articulation rewrite is complete and correct. The PR successfully aligns OVPhysX Articulation with the post-refactor RigidObject shape and Newton/PhysX conventions.

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

Summary

Follow-up review after commit 68ae80f. All previously identified concerns remain addressed. No new issues introduced.

Previous Findings Status

All five previously reviewed items remain resolved. The latest commits appear to be minor cleanup/formatting changes that don't introduce new logic.

Implementation Verdict

Ship it. The articulation rewrite is complete and ready to merge.

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

Summary

Follow-up review after commit 5f21c50. The previous "Ship it" verdict stands. The new commits add minor changes (changelog updates, test file additions, kernel docstrings) without introducing regressions.

Previous Findings Status

All previously reviewed items remain resolved. The latest commits are primarily documentation and test infrastructure.

Implementation Verdict

Ship it. No new issues identified.

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

Summary

Follow-up review after commit e935671. The new commits since the previous review add the test_articulation_helpers.py file, additional test infrastructure, and minor refinements. No regressions introduced.

Previous Findings Status

All previously reviewed items remain resolved. The latest commits are test infrastructure additions and documentation.

Implementation Verdict

Ship it. No new issues identified in the incremental changes.

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

Summary

Follow-up review after commit 62fb9f2. The new commits add test helpers (test_articulation_helpers.py), expand mock interface coverage, and include minor documentation/changelog updates. The previous review marked this as ready to ship.

Previous Findings Status

All previously reviewed items remain resolved. No regressions introduced by the latest commits.

New Findings

None. The incremental changes are test infrastructure additions and do not introduce new issues.

Implementation Verdict

Ship it. The articulation rewrite is complete, tests mirror the PhysX suite, and the codebase follows the established patterns.

AntoineRichard added a commit to AntoineRichard/IsaacLab that referenced this pull request May 5, 2026
Add Articulation and ArticulationData for the OVPhysX backend, mirroring
the PhysX/Newton public API. Resolves PR isaac-sim#5459.

Articulation
^^^^^^^^^^^^

* index/mask split for every state writer, simulation-parameter writer,
  setter, and tendon setter; OVPhysX exposes both _index and _mask as
  first-class paths and intentionally drops the PhysX-specific
  ``full_data`` kwarg.
* Dedicated dynamic + viscous friction setters
  (write_joint_{dynamic,viscous}_friction_coefficient_to_sim_{index,mask})
  that touch only their slot of the combined (N, J, 3)
  DOF_FRICTION_PROPERTIES buffer.  The combined
  write_joint_friction_coefficient_to_sim_index/_mask still accepts all
  three components as kwargs (Coulomb static + dynamic + optional
  viscous) for source-compatible PhysX call sites.
* Deprecated non-indexed shorthand shims for friction (x3) and root /
  joint state (x4), forwarding to the index variants with a
  DeprecationWarning, matching PhysX's deprecated section.
* Wrench-composer return types tightened to non-None
  (instantaneous_wrench_composer / permanent_wrench_composer);
  composers are always set in _create_buffers, mirroring PhysX/Newton.
* Section organisation matches PhysX exactly: Properties -> Operations
  -> Operations - Finders -> Operations - State Writers -> Operations
  - Simulation Parameters Writers -> Operations - Setters -> Operations
  - Tendons -> Internal helper -> Internal simulation callbacks ->
  Internal helpers -- Actuators -> Internal helpers -- Debugging ->
  Deprecated methods.  Section delimiters use bare """Section."""
  docstring blocks (Newton/PhysX convention).

ArticulationData
^^^^^^^^^^^^^^^^

* Pull-on-demand timestamped buffers; CPU-only bindings route through
  pinned-host staging (PR isaac-sim#5329 pattern).
* Property names match PhysX exactly: joint_friction_coeff,
  joint_dynamic_friction_coeff, joint_viscous_friction_coeff (no
  *_static / *_dynamic / *_viscous renames).
* SI units annotated on every public property docstring
  ([m or rad, depending on joint type], [m/s or rad/s, ...], [N*m],
  [kg], etc.) per AGENTS.md.
* binding_getter parameter on __init__ typed as
  Callable[[int], Any] | None.
* Section organisation matches PhysX (Defaults -> Joint commands ->
  Joint properties -> Fixed tendon -> Spatial tendon -> Root state ->
  Body state -> Joint state -> Derived -> Sliced -> Internal helpers ->
  Deprecated properties).

Kernels
^^^^^^^

* Articulation-specific kernels in
  isaaclab_ovphysx/assets/articulation/kernels.py (soft-limit clamp,
  friction-data writer, finite-difference joint-acc helper, body-CoM
  pose composer); shared kernels promoted to
  isaaclab_ovphysx/assets/kernels.py.
* Per-kernel docstrings document purpose, shape/dtype/SI units, and
  divergence notes where the OVPhysX implementation differs (e.g.
  _fd_joint_acc takes inv_dt rather than dt to avoid per-element
  division).

Tests
^^^^^

* Real-backend test_articulation.py mirrors isaaclab_physx 1-to-1
  under run_ovphysx.sh; 99 tests pass on each of CPU + CUDA.
* test_articulation_helpers.py covers the kitless-only helpers
  (tendon scoping, mock binding shapes).
* Cross-backend test_articulation_iface.py runs the OVPhysX path:
  544 tests pass, 16 skipped, 0 failed on each of CPU + CUDA.  Brings
  the iface helper up to the actual ArticulationData constructor
  signature and broadcasts scalar inputs across joint / fixed-tendon
  / spatial-tendon mask setters that previously rejected them.

OVPhysX-only surface dropped
^^^^^^^^^^^^^^^^^^^^^^^^^^^^

* Articulation: set_external_force_and_torque_{index,mask} (use
  instantaneous_wrench_composer.add_forces_and_torques_*),
  set_spatial_tendon_limit_{index,mask}, and
  set_spatial_tendon_rest_length_{index,mask} (NotImplementedError
  stubs PhysX never had).
* ArticulationData: body_pose_w / body_lin_vel_w / body_ang_vel_w /
  body_acc_w / body_link_acc_w (base class provides matching
  defaults), body_inv_mass / body_inv_inertia, fixed_tendon_limit
  (PhysX exposes only fixed_tendon_pos_limits), spatial_tendon_limit /
  spatial_tendon_rest_length (no PhysX equivalent).
@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_articulation branch from 81f6176 to 394ade8 Compare May 5, 2026 14:50
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

Summary

Follow-up review after commit 394ade8. The new commits add additional test coverage (test_articulation_helpers.py with random-state utility), expand the mock OVPhysX bindings interface, and include minor documentation/changelog updates. The previous review marked this as ready to ship.

Previous Findings Status

All previously reviewed items remain resolved. No regressions introduced by the latest commits.

New Findings

None. The incremental changes are test infrastructure additions and documentation polish. No new issues detected in the articulation or rigid object implementations.

Implementation Verdict

Ship it. The articulation rewrite is complete, tests mirror the PhysX suite comprehensively, and the codebase follows the established patterns.

AntoineRichard added a commit to AntoineRichard/IsaacLab that referenced this pull request May 5, 2026
Add Articulation and ArticulationData for the OVPhysX backend, mirroring
the PhysX/Newton public API. Resolves PR isaac-sim#5459.

Articulation
^^^^^^^^^^^^

* index/mask split for every state writer, simulation-parameter writer,
  setter, and tendon setter; OVPhysX exposes both _index and _mask as
  first-class paths and intentionally drops the PhysX-specific
  ``full_data`` kwarg.
* Dedicated dynamic + viscous friction setters
  (write_joint_{dynamic,viscous}_friction_coefficient_to_sim_{index,mask})
  that touch only their slot of the combined (N, J, 3)
  DOF_FRICTION_PROPERTIES buffer.  The combined
  write_joint_friction_coefficient_to_sim_index/_mask still accepts all
  three components as kwargs (Coulomb static + dynamic + optional
  viscous) for source-compatible PhysX call sites.
* Deprecated non-indexed shorthand shims for friction (x3) and root /
  joint state (x4), forwarding to the index variants with a
  DeprecationWarning, matching PhysX's deprecated section.
* Wrench-composer return types tightened to non-None
  (instantaneous_wrench_composer / permanent_wrench_composer);
  composers are always set in _create_buffers, mirroring PhysX/Newton.
* Section organisation matches PhysX exactly: Properties -> Operations
  -> Operations - Finders -> Operations - State Writers -> Operations
  - Simulation Parameters Writers -> Operations - Setters -> Operations
  - Tendons -> Internal helper -> Internal simulation callbacks ->
  Internal helpers -- Actuators -> Internal helpers -- Debugging ->
  Deprecated methods.  Section delimiters use bare """Section."""
  docstring blocks (Newton/PhysX convention).

ArticulationData
^^^^^^^^^^^^^^^^

* Pull-on-demand timestamped buffers; CPU-only bindings route through
  pinned-host staging (PR isaac-sim#5329 pattern).
* Property names match PhysX exactly: joint_friction_coeff,
  joint_dynamic_friction_coeff, joint_viscous_friction_coeff (no
  *_static / *_dynamic / *_viscous renames).
* SI units annotated on every public property docstring
  ([m or rad, depending on joint type], [m/s or rad/s, ...], [N*m],
  [kg], etc.) per AGENTS.md.
* binding_getter parameter on __init__ typed as
  Callable[[int], Any] | None.
* Section organisation matches PhysX (Defaults -> Joint commands ->
  Joint properties -> Fixed tendon -> Spatial tendon -> Root state ->
  Body state -> Joint state -> Derived -> Sliced -> Internal helpers ->
  Deprecated properties).

Kernels
^^^^^^^

* Articulation-specific kernels in
  isaaclab_ovphysx/assets/articulation/kernels.py (soft-limit clamp,
  friction-data writer, finite-difference joint-acc helper, body-CoM
  pose composer); shared kernels promoted to
  isaaclab_ovphysx/assets/kernels.py.
* Per-kernel docstrings document purpose, shape/dtype/SI units, and
  divergence notes where the OVPhysX implementation differs (e.g.
  _fd_joint_acc takes inv_dt rather than dt to avoid per-element
  division).

Tests
^^^^^

* Real-backend test_articulation.py mirrors isaaclab_physx 1-to-1
  under run_ovphysx.sh; 99 tests pass on each of CPU + CUDA.
* test_articulation_helpers.py covers the kitless-only helpers
  (tendon scoping, mock binding shapes).
* Cross-backend test_articulation_iface.py runs the OVPhysX path:
  544 tests pass, 16 skipped, 0 failed on each of CPU + CUDA.  Brings
  the iface helper up to the actual ArticulationData constructor
  signature and broadcasts scalar inputs across joint / fixed-tendon
  / spatial-tendon mask setters that previously rejected them.

OVPhysX-only surface dropped
^^^^^^^^^^^^^^^^^^^^^^^^^^^^

* Articulation: set_external_force_and_torque_{index,mask} (use
  instantaneous_wrench_composer.add_forces_and_torques_*),
  set_spatial_tendon_limit_{index,mask}, and
  set_spatial_tendon_rest_length_{index,mask} (NotImplementedError
  stubs PhysX never had).
* ArticulationData: body_pose_w / body_lin_vel_w / body_ang_vel_w /
  body_acc_w / body_link_acc_w (base class provides matching
  defaults), body_inv_mass / body_inv_inertia, fixed_tendon_limit
  (PhysX exposes only fixed_tendon_pos_limits), spatial_tendon_limit /
  spatial_tendon_rest_length (no PhysX equivalent).
@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_articulation branch from 394ade8 to f72c5f4 Compare May 5, 2026 14:58
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

Summary

Follow-up review after commit f72c5f4. The new commits add the gap specification document (2026-04-27-ovphysx-rigid-body-tensortypes-gap.md), expand CI documentation in run_ovphysx.sh, and include minor test infrastructure updates. The previous review marked this as ready to ship.

Previous Findings Status

All previously reviewed items remain resolved. No regressions introduced by the latest commits.

New Findings

None. The incremental changes are documentation additions (gap spec, CI notes) and test infrastructure polish. No new issues detected in the articulation or rigid object implementations.

Implementation Verdict

Ship it.

The dual-Carbonite destructor race that crashed test_rigid_object.py
between tests is not test-specific: any production caller that closes
a SimulationContext and constructs a new one in the same process hits
the same path. The previous PR worked around it with session-scoped
class-level monkey patches in the test file (_PHYSX_BY_DEVICE cache,
_patched_release_physx, _patched_warmup_and_load); production code
remained latently buggy.

Move the workaround into the manager itself:

* _release_physx is now a soft reset -- physx.reset() + wait_op while
  keeping the cached ovphysx.PhysX reference alive on cls._physx, so
  the C++ destructor never fires mid-process.
* _warmup_and_load reuses the cached instance on subsequent calls.
  First call constructs + locks the device + registers atexit; later
  calls re-export the USD, run physx.reset() to clear the prior stage,
  add_usd, replay clones, and (on GPU) re-run warmup_gpu so the new
  stage's bodies are resident.
* New _locked_device class attribute mirrors the wheel's process-global
  device-mode lock; the manager raises RuntimeError with a clear
  message instead of letting the wheel's PhysXDeviceError fire.
* _construct_physx splits out the first-time bootstrap so the orchestration
  in _warmup_and_load stays linear.
* HACK comments on _release_physx are keyed to the same wheel-fix
  milestone (namespace-isolated Carbonite) tracked by gap G5.

The dict keyed by device (_PHYSX_BY_DEVICE) is gone -- since the wheel
locks the process to one device, the dict could never hold more than
one entry and was misleading.

In test_rigid_object.py, drop the entire patch block (~150 lines):
_PHYSX_BY_DEVICE, _patched_release_physx, _patched_warmup_and_load,
_orig_*, _ovphysx_session_patches. Keep _LOCKED_DEVICE plus
_ovphysx_skip_other_device autouse fixture -- the only remaining
test-side concern is pre-empting the device-lock RuntimeError with a
clean pytest.skip on parametrized device mismatch, so two-pass CI
finishes cleanly when only one device is exercised. Update
test_warmup_attach_stage_not_called_for_cpu to spy on a real PhysX
after construction (the old approach silently relied on the patch
overwriting cls._physx).

Tests: 42 CPU + 41 GPU passing (two pytest invocations per Marco's
two-pass requirement).

Bumps isaaclab_ovphysx 0.2.16 -> 0.2.17.
* Untrack docs/superpowers/specs/2026-04-27-ovphysx-rigid-body-tensortypes-gap.md
  (file is gitignored under docs/superpowers/; keep locally only).
* Revert the ovphysx variant additions to allegro_hand_env_cfg.py
  (ObjectCfg.ovphysx, PhysicsCfg.ovphysx, OvPhysxCfg import) and the
  matching isaaclab_tasks 1.5.30 CHANGELOG entry / version bump.
* Trim test_rigid_object.py module docstring: drop the PhysX-mirror prose
  and the PhysX adaptation walk-through (both inferable from the file
  contents); drop the gap-G5 / superpowers spec reference.
* Tighten the _ovphysx_skip_other_device fixture docstring; the module
  docstring now carries the device-lock rationale.
* Move test_mock_binding_set_rigid_object_shapes from
  test_articulation.py to a parallel test_rigid_object_helpers.py so the
  rigid-object mock-binding contract test sits next to the rest of the
  rigid-object scaffolding.
* Convert the new "Rigid-body TensorTypes" section header in tensor_types.py
  to the triple-quoted-string style used elsewhere in the file.
Revert direct edits to source/isaaclab_ovphysx/config/extension.toml
and source/isaaclab_ovphysx/docs/CHANGELOG.rst -- both are
nightly-CI-compiled outputs per AGENTS.md, so PRs add fragment files
under source/<pkg>/changelog.d/ instead.

Add a .minor.rst fragment summarising the user-facing additions for
this PR (RigidObject and RigidObjectData, RIGID_BODY_* TensorType
aliases, the shared isaaclab_ovphysx.assets.kernels module, prim-scan
validation in _initialize_impl) and changes to OvPhysxManager (soft
reset on stage swap, device-lock surfacing, unified PhysxScene config
across CPU and GPU).

The isaaclab core touch is test-only, so use a .skip fragment there.
Drop the try/except that swallowed wheel-side TensorBinding creation
errors and demoted them to a debug log, returning None. Most callers
already dereferenced the result unconditionally, and the only one
that null-checked (the wrench writer in write_data_to_sim) silently
skipped its write -- the asymmetric handling was a foot-gun.

Eager creation in _initialize_impl already populates the cache for
every binding the writers consume, so post-init calls cannot fail.
The init eager loop now wraps each call in try/except and re-raises
with the existing helpful diagnostic about prim_path / pattern /
wheel coverage, so creation failures still surface with context.

Drop the now-unused logging import.
Multiple review fixes to source/.../rigid_object/rigid_object_data.py.

* Drop num_instances / num_bodies / body_names from the __init__ Args
  block -- they are not parameters; they are read from the bindings
  or set by RigidObject._initialize_impl.

* Fix incorrect frame descriptions inherited via copy-paste from
  PhysX/Newton: body_link_vel_w / body_com_vel_w now describe the
  body's link / COM frame (not "the root rigid body"), and the four
  *_b base-frame velocity properties read "world-frame velocity
  expressed in the root link's actor frame" rather than the
  self-referential and trivially-zero "with respect to the rigid
  body's actor frame".

* Drop "in the simulation world frame" from body_mass (mass is
  frameless) and body_inertia (it is expressed at the COM per the
  RIGID_BODY_INERTIA tensor-types contract). Standardise inertia
  units to [kg.m^2] using the same Unicode notation already used
  elsewhere in this file.

* Add a check_shapes (default True) constructor parameter, plumbed
  from AssetBase._check_shapes by RigidObject._initialize_impl. Use
  it in _read_binding_into to assert the destination buffer has at
  least as many bytes as the binding will write, defending the
  wp.array(ptr=...) reinterpret against a future buffer/binding size
  drift that would otherwise corrupt memory silently. The flag
  follows the same AssetBaseCfg.disable_shape_checks knob already
  used by the writer-side assert_shape_and_dtype.
Add Articulation and ArticulationData for the OVPhysX backend, mirroring
the PhysX/Newton public API. Resolves PR isaac-sim#5459.

Articulation
^^^^^^^^^^^^

* index/mask split for every state writer, simulation-parameter writer,
  setter, and tendon setter; OVPhysX exposes both _index and _mask as
  first-class paths and intentionally drops the PhysX-specific
  ``full_data`` kwarg.
* Dedicated dynamic + viscous friction setters
  (write_joint_{dynamic,viscous}_friction_coefficient_to_sim_{index,mask})
  that touch only their slot of the combined (N, J, 3)
  DOF_FRICTION_PROPERTIES buffer.  The combined
  write_joint_friction_coefficient_to_sim_index/_mask still accepts all
  three components as kwargs (Coulomb static + dynamic + optional
  viscous) for source-compatible PhysX call sites.
* Deprecated non-indexed shorthand shims for friction (x3) and root /
  joint state (x4), forwarding to the index variants with a
  DeprecationWarning, matching PhysX's deprecated section.
* Wrench-composer return types tightened to non-None
  (instantaneous_wrench_composer / permanent_wrench_composer);
  composers are always set in _create_buffers, mirroring PhysX/Newton.
* Section organisation matches PhysX exactly: Properties -> Operations
  -> Operations - Finders -> Operations - State Writers -> Operations
  - Simulation Parameters Writers -> Operations - Setters -> Operations
  - Tendons -> Internal helper -> Internal simulation callbacks ->
  Internal helpers -- Actuators -> Internal helpers -- Debugging ->
  Deprecated methods.  Section delimiters use bare """Section."""
  docstring blocks (Newton/PhysX convention).

ArticulationData
^^^^^^^^^^^^^^^^

* Pull-on-demand timestamped buffers; CPU-only bindings route through
  pinned-host staging (PR isaac-sim#5329 pattern).
* Property names match PhysX exactly: joint_friction_coeff,
  joint_dynamic_friction_coeff, joint_viscous_friction_coeff (no
  *_static / *_dynamic / *_viscous renames).
* SI units annotated on every public property docstring
  ([m or rad, depending on joint type], [m/s or rad/s, ...], [N*m],
  [kg], etc.) per AGENTS.md.
* binding_getter parameter on __init__ typed as
  Callable[[int], Any] | None.
* Section organisation matches PhysX (Defaults -> Joint commands ->
  Joint properties -> Fixed tendon -> Spatial tendon -> Root state ->
  Body state -> Joint state -> Derived -> Sliced -> Internal helpers ->
  Deprecated properties).

Kernels
^^^^^^^

* Articulation-specific kernels in
  isaaclab_ovphysx/assets/articulation/kernels.py (soft-limit clamp,
  friction-data writer, finite-difference joint-acc helper, body-CoM
  pose composer); shared kernels promoted to
  isaaclab_ovphysx/assets/kernels.py.
* Per-kernel docstrings document purpose, shape/dtype/SI units, and
  divergence notes where the OVPhysX implementation differs (e.g.
  _fd_joint_acc takes inv_dt rather than dt to avoid per-element
  division).

Tests
^^^^^

* Real-backend test_articulation.py mirrors isaaclab_physx 1-to-1
  under run_ovphysx.sh; 99 tests pass on each of CPU + CUDA.
* test_articulation_helpers.py covers the kitless-only helpers
  (tendon scoping, mock binding shapes).
* Cross-backend test_articulation_iface.py runs the OVPhysX path:
  544 tests pass, 16 skipped, 0 failed on each of CPU + CUDA.  Brings
  the iface helper up to the actual ArticulationData constructor
  signature and broadcasts scalar inputs across joint / fixed-tendon
  / spatial-tendon mask setters that previously rejected them.

OVPhysX-only surface dropped
^^^^^^^^^^^^^^^^^^^^^^^^^^^^

* Articulation: set_external_force_and_torque_{index,mask} (use
  instantaneous_wrench_composer.add_forces_and_torques_*),
  set_spatial_tendon_limit_{index,mask}, and
  set_spatial_tendon_rest_length_{index,mask} (NotImplementedError
  stubs PhysX never had).
* ArticulationData: body_pose_w / body_lin_vel_w / body_ang_vel_w /
  body_acc_w / body_link_acc_w (base class provides matching
  defaults), body_inv_mass / body_inv_inertia, fixed_tendon_limit
  (PhysX exposes only fixed_tendon_pos_limits), spatial_tendon_limit /
  spatial_tendon_rest_length (no PhysX equivalent).
@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_articulation branch from f72c5f4 to 7a12614 Compare May 5, 2026 17:37
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

Summary

Follow-up review after commit 7a12614. The new commits refine the articulation implementation with minor fixes to kernel imports, changelog formatting, and test fixture setup. Previous review marked this as ready to ship.

Previous Findings Status

All previously reviewed items remain resolved. The latest commits are incremental polish (import cleanup, docstring tweaks, test helper organization).

Implementation Verdict

Ship it. No new issues detected.

# (the signature of run_ovphysx.sh) or when EXP_PATH is not set.
_kitless = os.environ.get("LD_PRELOAD", "") == "" and "EXP_PATH" not in os.environ
# 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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LD_PRELOAD to the ovphysx libcarb.so

This problem has been addressed with #5487

simulation_app = AppLauncher(headless=True).app
# 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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Problem addressed in #5487

Comment thread scripts/run_ovphysx.sh
# Preload ovphysx's own libcarb.so so its Carbonite framework wins the
# SONAME race against any other libcarb.so present in the process.
_ovphysx_libcarb=""
for _sp in "${ISAAC_DIR}"/kit/python/lib/python3.*/site-packages/ovphysx/plugins/libcarb.so; do
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@pbarejko pbarejko requested a review from fatimaanes May 5, 2026 19:49
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.

2 participants