Skip to content

[OVPHYSX] ContactSensor#5422

Merged
AntoineRichard merged 27 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_contactsensor
May 19, 2026
Merged

[OVPHYSX] ContactSensor#5422
AntoineRichard merged 27 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/feat/ovphysx_contactsensor

Conversation

@AntoineRichard

@AntoineRichard AntoineRichard commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Description

Implements ContactSensor, ContactSensorCfg, and ContactSensorData for the isaaclab_ovphysx backend, mirroring the existing PhysX implementation.

The contact sensor reports normal contact forces in the world frame using the ovphysx 0.3.7 ContactBinding API (PhysX.create_contact_binding / read_net_forces / read_force_matrix). Optional pose tracking is wired through a RIGID_BODY_POSE TensorBinding.

Validation environment: Isaac-Velocity-Flat-Anymal-C-Direct-v0 (Anymal-C foot-contact tracking for locomotion).

v1 scope (this PR):

  • Net contact forces + history
  • Force matrix (filtered partner forces) + history
  • Pose tracking (track_pose)
  • Air/contact time tracking + compute_first_contact / compute_first_air
  • Reset semantics + native handle teardown on simulation stop

Deferred (raise NotImplementedError if cfg flag enabled):

  • track_contact_points
  • track_friction_forces

These features are blocked on tensor-friendly per-sensor read APIs in ovphysx (ContactBinding.read_contact_points / read_friction_forces). A maintainer-facing spec listing the missing APIs is attached at docs/superpowers/specs/2026-04-27-ovphysx-contact-api-gaps.md. The data properties still return None per the BaseContactSensorData contract; the implementation only raises if the cfg flag is set.

Fixes #5325
Parent issue: #5315

Stacked on:

Dependencies:

Type of change

  • New feature (non-breaking change which adds functionality)

Screenshots

N/A — backend feature, no UI surface.

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 (changelog fragment at source/isaaclab_ovphysx/changelog.d/antoiner-feat-ovphysx_contactsensor.minor.rst, docstrings, gap spec for the maintainer of ovphysx)
  • My changes generate no new warnings
  • I have added tests that prove my feature works (9 tests adapted from the PhysX backend; 3 skipped for the deferred features so they're trivially un-skip-able once ovphysx ships the missing APIs)
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file (changelog fragment, minor tier — version bump compiled by the nightly CI workflow per the upstream convention)
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

Notes for the reviewer

  • The Warp kernels (sensors/contact_sensor/kernels.py and the shared sensors/kernels.py) are physics-engine-agnostic and ported verbatim from the PhysX backend — only module docstrings differ.
  • ContactSensorCfg and ContactSensorData are also near-verbatim mirrors; the only edits are the backend-specific class_type pointer and the shared-kernels import path.
  • The ovphysx-specific glue lives in _initialize_impl (USD prim discovery + regex→fnmatch glob conversion + create_contact_binding + optional RIGID_BODY_POSE binding), _create_buffers (pre-allocated DLPack read buffers), _update_buffers_impl (per-step reads), and _invalidate_initialize_callback (native handle release).
  • Tests not yet executed. They are currently authored in the Isaac-Sim/AppLauncher style (mirroring isaaclab_physx/test/sensors/test_contact_sensor.py); they need to be re-adapted to the kitless ./scripts/run_ovphysx.sh -m pytest flow and the _ovphysx_sim_context helper used by the articulation/rigid-object suites in [OVPHYSX] Articulation rewrite (data class + asset class + kernels) #5459. Will land as a follow-up commit on this branch once that adaptation is reviewed.

@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Apr 28, 2026

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

Copy link
Copy Markdown

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 implements ContactSensor, ContactSensorCfg, and ContactSensorData for the ovphysx backend, providing contact force reporting via the ovphysx ContactBinding API. The implementation mirrors the PhysX backend structure, supporting net forces, force matrix filtering, history buffers, air/contact time tracking, and pose tracking. Two features (track_contact_points, track_friction_forces) are explicitly deferred with NotImplementedError. The implementation appears structurally sound but has several bugs in the Warp kernels and test setup that need addressing.

Architecture Impact

  • Cross-module: The sensor integrates with isaaclab.sensors.contact_sensor.BaseContactSensor and isaaclab.scene.InteractiveScene. The ContactSensorCfg uses lazy class resolution via "{DIR}.contact_sensor:ContactSensor".
  • Data flow: Contact data flows from ovphysx ContactBinding.read_net_forces() → flat Warp buffers → structured ContactSensorData buffers via Warp kernels.
  • Dependency: Blocked on #5316 for test execution (tests use RigidObject which requires ovphysx RigidObject support).

Implementation Verdict

Significant concerns — Several Warp kernel bugs will cause incorrect behavior or crashes at runtime. The tests are well-structured but cannot run until #5316 lands.

Test Coverage

The test file is comprehensive, ported from the PhysX backend with appropriate modifications. However:

  • Tests are not executable due to dependency on #5316
  • 3 tests properly skipped for deferred features
  • No unit tests for the Warp kernels in isolation
  • @flaky decorator on contact time tests suggests known instability

CI Status

  • Check for Broken Links: failure — Likely unrelated to this PR (documentation link issue)
  • Most CI jobs are skipped — expected given the test dependency on #5316
  • pre-commit: success confirms formatting is correct

Findings

🔴 Critical: contact_sensor.py:309-310 — reset() passes None to optional buffers causing kernel crash

The reset_contact_sensor_kernel is launched with self._data._friction_forces_w and self._data._contact_pos_w as outputs, but these are None when track_friction_forces=False and track_contact_points=False. Warp kernels cannot safely write to None arrays. The kernel guards (if friction_forces_w:) only prevent the write, but the array is still passed as an argument.

outputs=[
    ...
    self._data._friction_forces_w,  # None when track_friction_forces=False
    self._data._contact_pos_w,      # None when track_contact_points=False
],

Fix: Create empty placeholder buffers or restructure the kernel to not require these arguments when disabled.

🔴 Critical: kernels.py:229-230 — force_matrix_w_history accessed without null guard

In update_net_forces_kernel, when net_forces_matrix_flat is not None but force_matrix_w_history could be None (if history tracking is disabled differently than matrix tracking), the kernel will crash:

if net_forces_matrix_flat:
    for f in range(num_filter_shapes):
        force_matrix_w[env, sensor, f] = net_forces_matrix_flat[src_idx, f]
        for i in range(history_length - 1, 0, -1):
            force_matrix_w_history[env, i, sensor, f] = ...  # No null guard!

Looking at _create_buffers(), both are allocated together, so this may be safe in practice, but the asymmetric guarding is fragile.

🔴 Critical: contact_sensor.py:271-278 — Timestamp arrays passed incorrectly to kernel

The kernel expects wp.array(dtype=wp.float32) for timestamp and timestamp_last_update, but self._timestamp and self._timestamp_last_update from the base class are likely per-environment scalars or structured differently. Need to verify these match the expected (num_envs,) shape.

inputs=[
    ...
    self._timestamp,           # What shape/type is this?
    self._timestamp_last_update,
]

🟡 Warning: contact_sensor.py:207-208 — Pose binding pattern may match unintended prims

The pose binding uses a wildcard pattern f"{base_glob}/*" which could match more prims than intended if the scene has siblings under the base path:

single_pose_pattern = f"{base_glob}/*"
self._pose_binding = physx_instance.create_tensor_binding(
    pattern=single_pose_pattern,
    tensor_type=TT.RIGID_BODY_POSE,
)

The count check at line 215-221 catches mismatches, but a more precise pattern construction (using the actual body names) would be safer.

🟡 Warning: contact_sensor_data.py:25-35 — pose_w property launches kernel on every access

Every call to pose_w launches a Warp kernel to concatenate position and quaternion:

@property
def pose_w(self) -> ProxyArray | None:
    if self._pose_w is None:
        return None
    wp.launch(
        concat_pos_and_quat_to_pose_kernel,  # Launched every access!
        ...
    )

This is inefficient if accessed multiple times per step. Consider caching or only recomputing when data is dirty.

🟡 Warning: check_contact_sensor.py:51 — Test imports ovphysx before checking availability

The test file calls pytest.importorskip("ovphysx.types") but then has unconditional imports from isaaclab_ovphysx.sensors at line 53. If ovphysx is not installed, the skip happens but the subsequent import will still execute during collection:

pytest.importorskip("ovphysx.types", reason="ovphysx wheel not installed")
from isaaclab_ovphysx.sensors import ContactSensorCfg  # noqa: E402

This should work due to lazy export, but the order is fragile.

🔵 Improvement: kernels.py:280-292 — Duplicate kernel definition

concat_pos_and_quat_to_pose_kernel is defined both in contact_sensor/kernels.py and sensors/kernels.py. The one in contact_sensor_data.py imports from sensors/kernels.py, while contact_sensor.py also defines it locally via the import. This duplication will cause maintenance issues.

# In sensors/kernels.py
@wp.kernel
def concat_pos_and_quat_to_pose_kernel(...):

# Also in contact_sensor/kernels.py (line 280)
@wp.kernel  
def concat_pos_and_quat_to_pose_kernel(...):

Fix: Remove the duplicate in contact_sensor/kernels.py and import from sensors/kernels.py.

🔵 Improvement: contact_sensor.py:92-94 — Config mutation in __init__

Mutating the config object's attributes in __init__ is a side effect that could surprise callers:

if self.cfg.max_contact_data_count_per_prim is None:
    self.cfg.max_contact_data_count_per_prim = 4
if self.cfg.force_threshold is None:
    self.cfg.force_threshold = 1.0

Fix: Use local variables or @property accessors with defaults instead of mutating the config.

@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_contactsensor branch from 7981eed to 42001ea Compare May 6, 2026 12:40

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

Copy link
Copy Markdown

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 implements ContactSensor, ContactSensorCfg, and ContactSensorData for the OVPhysX backend, mirroring the PhysX implementation. The sensor reports normal contact forces using the ovphysx.ContactBinding API.

Update (f495912)

133 commits since last review. Key feature-related changes:

Final Alignment with PhysX/Newton (f495912)

  • Aligned OVPhysX ContactSensor parity with PhysX/Newton contracts
  • Fixed configclass import path for develop branch compatibility (2578b48)

Multi-Body Sensor Indexing Fix (05c4053)

  • Critical fix: Corrected flat-buffer indexing from env-major to pattern-major layout
  • OVPhysX ContactBinding returns flat_idx = sensor * num_envs + env (pattern-major)
  • PhysX uses flat_idx = env * num_sensors + sensor (env-major)
  • Added dedicated regression test (8e9783d) to catch layout mismatches

Infrastructure Improvements

  • Wired OvPhysX preset into AnymalD Flat config (9bfc8d2)
  • Auto-register PhysxSchema USD plugin in OvPhysxManager (7ee5897)
  • Fixed PhysicsManager.clear_callbacks ID-recycle bug (b1a1e59)

Code Review Findings

✅ Strengths

  1. Clean API Symmetry: The sensor mirrors PhysX ContactSensor nearly 1:1, enabling backend-agnostic user code.

  2. Proper Unsupported Feature Handling: track_contact_points and track_friction_forces raise NotImplementedError early in __init__, before USD discovery — fail-fast is correct.

  3. Pattern-Major Indexing: The update_net_forces_ovphysx_kernel correctly uses src_idx = sensor * num_envs + env for OVPhysX's flat buffer layout.

  4. Callback ID Recycling Fix: PhysicsManager.clear_callbacks() no longer resets _callback_id = 0, preventing stale callback deregistration bugs in multi-scene tests.

  5. PhysxSchema Registration: OvPhysxManager._ensure_physx_schemas_registered() handles kitless flows where omni.physx doesn't load the schema automatically.

  6. Comprehensive Test Suite: 1000+ line test file covering contact/air time, cube stack filtering, multi-body indexing, threshold validation — with proper device-lock fixture for OVPhysX's process-global device binding.

⚠️ Suggestions (Non-Blocking)

  1. Duplicate Kernel Definition (lines ~300 in contact_sensor/kernels.py and sensors/kernels.py):

    • concat_pos_and_quat_to_pose_kernel is defined in both files
    • The contact_sensor/kernels.py imports from the shared module but also has a local copy
    • Consider removing the duplicate from contact_sensor/kernels.py
  2. Force Threshold Comparison (kernels.py line ~268):

    in_contact = wp.length_sq(net_forces_w[env, sensor]) > contact_force_threshold * contact_force_threshold
    • Using squared threshold is computationally efficient ✓
    • Document that force_threshold cfg is applied as squared magnitude
  3. _num_filter_shapes Initialization (_create_buffers, line ~290):

    • The filter_count is read from binding: self._contact_binding.filter_count if self.cfg.filter_prim_paths_expr else 0
    • Consider validating that filter_count matches expected len(filter_prim_paths_expr) per sensor
  4. Debug Visualization Warning (_set_debug_vis_impl):

    • Uses module-level _warned_debug_vis_unavailable attribute via getattr
    • Consider using a class attribute with cls._warned_... for cleaner state management

📝 Documentation

  • Changelog fragments present and well-structured ✓
  • Docstrings comprehensive with parameter descriptions ✓
  • Gap spec reference (docs/superpowers/specs/2026-04-27-ovphysx-contact-api-gaps.md) properly documented for deferred features ✓

Test Coverage Analysis

Test Status Coverage
test_cube_contact_time Net forces, air/contact time, history
test_sphere_contact_time Same as cube with sphere primitive
test_cube_stack_contact_filtering Force matrix, filter patterns
test_multi_body_per_sensor_indexing Pattern-major layout validation
test_no_contact_reporting Unfiltered sensor behavior
test_sensor_print __str__ method
test_contact_sensor_threshold USD attribute verification
test_friction_reporting ⏭️ Skipped v1 limitation
test_invalid_prim_paths_config ⏭️ Skipped v1 limitation
test_invalid_max_contact_points_config ⏭️ Skipped v1 limitation

The skipped tests preserve the test bodies so they can be trivially un-skipped when ovphysx ships tensor-friendly friction/contact-point APIs.


CI Status Note

  • Check changelog fragments failing — likely needs fragment regeneration after rebase
  • Other jobs pending/running

Verdict

LGTM — The ContactSensor implementation is complete and well-tested. The pattern-major indexing fix is correct and validated. The code follows Isaac Lab conventions and maintains API symmetry with PhysX.

The PR is ready to merge once:

  1. CI passes (changelog fragment check)
  2. Parent PR #5459 merges (per stacking note)

Update (e6fc1e0): Import order fix & changelog addition.

  • Test import fix: Moved pytest.importorskip("ovphysx.types") before all isaaclab_ovphysx imports and deferred wp.init() — CI environments without the ovphysx wheel now skip cleanly instead of failing on import.
  • Changelog fragment: Added antoiner-feat-ovphysx_contactsensor.rst documenting the new ovphysx preset for locomotion velocity tasks.

Both changes are housekeeping. LGTM stands.


Update (84f6e28): randomize_rigid_body_material physics backend detection refactor.

  • events.py: Simplified manager detection logic in RandomizeRigidBodyMaterialImpl.__init__:
    • Changed from exact name matching to substring matching with priority order
    • Checks "ovphysxmanager" first (to avoid false positive from "physx" in ...)
    • Then uses "newton" in ... and "physx" in ... for Newton subclasses (NewtonMJWarpManager, NewtonKaminoManager, etc.) and PhysX variants
    • Manager name is now lowercased before comparison for case-insensitive matching
  • This unifies the dispatch pattern and handles future Newton subclasses automatically without explicit enumeration.

Minor maintenance change, no functional impact on ContactSensor. LGTM holds.

@kellyguo11 kellyguo11 moved this to In review in Isaac Lab May 11, 2026
@kellyguo11 kellyguo11 moved this from In review to In progress in Isaac Lab May 11, 2026
@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_contactsensor branch 2 times, most recently from 498e4b0 to 25e8f3c Compare May 12, 2026 11:42
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label May 12, 2026
@AntoineRichard AntoineRichard marked this pull request as ready for review May 12, 2026 15:25
@greptile-apps

greptile-apps Bot commented May 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces the ContactSensor, ContactSensorCfg, and ContactSensorData classes for the isaaclab_ovphysx backend, mirroring the PhysX implementation using the ovphysx 0.3.7 ContactBinding API with Warp-accelerated kernels. The OvPhysxManager is also updated to keep the C++ PhysX instance alive across scene resets (preventing destructor races) and to auto-register the PhysxSchema USD plugin in kitless runs.

  • Net forces, force-matrix filtering, pose tracking, and air/contact-time accumulation are all wired through pre-allocated DLPack read buffers and pattern-major index remapping in the Warp kernels.
  • track_contact_points and track_friction_forces are explicitly deferred with NotImplementedError; a maintainer-facing API-gap spec is included.
  • The test suite (9 tests) is authored but flagged as not yet adapted to the kitless run_ovphysx.sh runner — adaptation is planned as a follow-up commit.

Confidence Score: 3/5

The core data-flow is sound, but the shared result buffer between compute_first_contact and compute_first_air means a locomotion policy calling both in the same step silently gets wrong answers for the first call.

The shared _first_transition buffer is a real behavioral defect: both compute_first_contact and compute_first_air write into and return the same Warp array, so the earlier call is clobbered whenever a policy queries both signals in one control step — a common pattern in foot-contact locomotion. The duplicate kernel in contact_sensor/kernels.py causes unnecessary Warp JIT overhead. The contact_pos_w NaN-vs-zero inconsistency is latent but should be addressed before the deferred v2 feature lands. Tests are unrun in the current state, so runtime correctness of the pattern-major index remapping and the binding fan-out logic remains unverified.

contact_sensor.py (shared transition buffer), kernels.py (dead kernel copy and NaN reset mismatch)

Important Files Changed

Filename Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/contact_sensor/contact_sensor.py New ContactSensor implementation for ovphysx backend; shared _first_transition buffer between compute_first_contact/compute_first_air causes silent result-clobbering when both are called in the same step
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/contact_sensor/kernels.py Warp kernels for the contact sensor; contains a dead duplicate of concat_pos_and_quat_to_pose_kernel (already in sensors/kernels.py) and resets contact_pos_w to zeros instead of NaN in reset_contact_sensor_kernel
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/contact_sensor/contact_sensor_data.py Data container; ProxyArray wrappers look correct; contact_pos_w initialized to NaN but the corresponding reset kernel uses zeros (latent inconsistency for v2)
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/contact_sensor/contact_sensor_cfg.py Thin cfg subclass pointing class_type at the ovphysx ContactSensor; no issues
source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/kernels.py Shared 2D/1D concat_pos_and_quat_to_pose kernels; correct and clean
source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py Adds process-global device-lock tracking, PhysX USD schema auto-registration, and a soft-reset path that keeps the C++ instance alive; the HACK comment is well-documented
source/isaaclab/isaaclab/sensors/sensor_base.py Tightens prim-deletion guard from substring match to exact class-name match to avoid false-positive on OvPhysxManager; safe change
source/isaaclab_ovphysx/test/sensors/test_contact_sensor.py 9 tests mirroring the PhysX suite; device-lock autouse fixture correctly skips tests on the wrong device; tests are authored but not yet adapted to the kitless run-script flow per the PR notes

Sequence Diagram

sequenceDiagram
    participant SC as SimulationContext
    participant OPM as OvPhysxManager
    participant CS as ContactSensor
    participant CB as ContactBinding
    participant PB as PoseBinding (opt)
    participant WK as Warp Kernels

    SC->>OPM: initialize(sim_context)
    OPM->>OPM: _ensure_physx_schemas_registered()
    SC->>OPM: reset()
    OPM->>OPM: _warmup_and_load() to PhysX instance

    SC->>CS: initialize()
    CS->>CS: _initialize_impl()
    CS->>OPM: get_physx_instance()
    CS->>CS: discover sensor bodies (USD prim walk)
    CS->>CB: physx_instance.create_contact_binding(sensor_patterns, filter_patterns)
    CS->>CS: override _num_envs from binding.sensor_count
    opt track_pose
        CS->>PB: physx_instance.create_tensor_binding(RIGID_BODY_POSE)
    end
    CS->>CS: _create_buffers()

    loop Every physics step
        SC->>OPM: step_sync()
        SC->>CS: update() to _update_buffers_impl()
        CS->>CB: read_net_forces(net_forces_flat_buf)
        opt filter_prim_paths_expr
            CS->>CB: read_force_matrix(force_matrix_flat_buf)
        end
        CS->>WK: update_net_forces_kernel(pattern-major to env,sensor)
        opt track_pose
            CS->>PB: read(poses_flat_buf)
            CS->>WK: split_flat_pose_to_pos_quat()
        end
    end

    SC->>OPM: stop()
    OPM->>CS: _invalidate_initialize_callback()
    CS->>CB: destroy()
    opt track_pose
        CS->>PB: destroy()
    end
    OPM->>OPM: _release_physx() physx.reset() keep C++ instance
Loading

Reviews (1): Last reviewed commit: "Patch caller sensor _num_envs from OvPhy..." | Re-trigger Greptile

Comment on lines +420 to +454
wp.launch(
compute_first_transition_kernel,
dim=(self._num_envs, self._num_sensors),
inputs=[float(dt + abs_tol), self._data._current_contact_time],
outputs=[self._data._first_transition],
device=self._device,
)
return self._data._first_transition_ta

def compute_first_air(self, dt: float, abs_tol: float = 1.0e-8) -> ProxyArray:
"""Boolean mask (as float) of bodies that broke contact within ``dt`` [s].

Args:
dt: Time window since contact break [s].
abs_tol: Absolute tolerance for the comparison [s].

Returns:
Boolean tensor (1.0/0.0) of shape ``(num_envs, num_sensors)``.

Raises:
RuntimeError: If :attr:`ContactSensorCfg.track_air_time` is False.
"""
if not self.cfg.track_air_time:
raise RuntimeError(
"The contact sensor is not configured to track air time."
" Please enable 'track_air_time' in the sensor configuration."
)
wp.launch(
compute_first_transition_kernel,
dim=(self._num_envs, self._num_sensors),
inputs=[float(dt + abs_tol), self._data._current_air_time],
outputs=[self._data._first_transition],
device=self._device,
)
return self._data._first_transition_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.

P1 Shared _first_transition buffer clobbers results across calls

compute_first_contact and compute_first_air both write into self._data._first_transition and return the same self._data._first_transition_ta ProxyArray. In a locomotion policy that needs both signals simultaneously (e.g. rewarding foot-contact establishment while also penalizing staying airborne), the first call's result is silently overwritten when the second call runs the kernel into the same buffer. The caller has no way to detect this — both return the same object. Two separate buffers (e.g. _first_contact and _first_air) and two ProxyArray wrappers are needed to make both values usable in the same step.

Comment on lines +286 to +300
@wp.kernel
def concat_pos_and_quat_to_pose_kernel(
pos: wp.array2d(dtype=wp.vec3f),
quat: wp.array2d(dtype=wp.quatf),
pose: wp.array2d(dtype=wp.transformf),
):
"""Concatenate position and quaternion to pose.

Args:
pos: Position array. Shape is (N, B).
quat: Quaternion array. Shape is (N, B).
pose: Pose array. Shape is (N, B).
"""
env, sensor = wp.tid()
pose[env, sensor] = wp.transform(pos[env, sensor], quat[env, sensor])

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 Duplicate concat_pos_and_quat_to_pose_kernel — dead code and double Warp JIT

An identical implementation of this kernel already lives in isaaclab_ovphysx/sensors/kernels.py and is what contact_sensor_data.py actually imports (via from isaaclab_ovphysx.sensors.kernels import concat_pos_and_quat_to_pose_kernel). The copy at the bottom of this file is never imported, but Warp still JIT-compiles it as a separate kernel object on module load, doubling compile time for this kernel. The dead copy should be removed.

Comment on lines +152 to +158
if friction_forces_w:
for f in range(num_filter_objects):
friction_forces_w[env, sensor, f] = wp.vec3f(0.0)

if contact_pos_w:
for f in range(num_filter_objects):
contact_pos_w[env, sensor, f] = wp.vec3f(0.0)

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 reset_contact_sensor_kernel sets contact_pos_w to zeros, but it was initialized with NaN

ContactSensorData.create_buffers initialises _contact_pos_w to wp.vec3f(math.nan, math.nan, math.nan) (line 290-293 in contact_sensor_data.py) to signal "no contact point available". After a reset the kernel writes wp.vec3f(0.0) here, so the post-reset value diverges from the initial value. Downstream code relying on NaN to detect "no contact" would see false zeros after the first reset. This code path is currently unreachable in v1 (blocked by the NotImplementedError on track_contact_points), but should be fixed before the deferred feature lands to avoid a silent regression.

Comment on lines +196 to +209
base_glob = self.cfg.prim_path.rsplit("/", 1)[0]
base_glob = re.sub(r"\{ENV_REGEX_NS\}", "*", base_glob)
base_glob = re.sub(r"\.\*", "*", base_glob)
sensor_patterns = [f"{base_glob}/{name}" for name in body_names]

# Build filter patterns (flat: len = n_sensors * filters_per_sensor).
filter_globs = [
re.sub(r"\.\*", "*", re.sub(r"\{ENV_REGEX_NS\}", "*", expr)) for expr in self.cfg.filter_prim_paths_expr
]
filters_per_sensor = len(filter_globs)
if filters_per_sensor > 0:
filter_patterns: list[str] | None = filter_globs * self._num_sensors
else:
filter_patterns = 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 filter_patterns does not account for multi-env when sizing the binding

filter_patterns = filter_globs * self._num_sensors produces num_sensors × num_filters entries. The comment on max_count (line 219) explicitly acknowledges that at binding-creation time self._num_envs reflects only env_0 because USD-discovery sees only the template prim. If the ovphysx create_contact_binding API expects filter_patterns sized as num_sensors × num_envs × num_filters (analogous to sensor_patterns), the current list is undersized by a factor of the real env count. The binding's own expansion semantics are not documented here; a brief comment explaining that the binding handles per-clone fan-out would remove the ambiguity.

@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_contactsensor branch from d1fbcd2 to 0ef602d Compare May 13, 2026 09:57
@kellyguo11 kellyguo11 moved this from In progress to In review in Isaac Lab May 14, 2026
AntoineRichard added a commit to AntoineRichard/IsaacLab that referenced this pull request May 15, 2026
Verification agents flagged five issues against the previous two
commits and one issue-isaac-sim#876 gap that the first pass missed.

1. Newton using-kamino.rst literalinclude path was one level short
   (../../../../../ → ../../../../../../). The previous depth resolved
   to a non-existent /docs/source/isaaclab_tasks/... and would have
   broken the sphinx build.

2. PhysX supported-features.rst listed Ray Caster, Visuo-tactile, and
   Camera as PhysX-specific sensors. Ray Caster and Camera are
   implemented in isaaclab core (backend-agnostic); Visuo-tactile lives
   in isaaclab_contrib. Reframe the section to separate PhysX-implemented
   sensors from backend-agnostic ones. Drop the unverifiable
   "path-traced" qualifier on the RTX renderer claim.

3. OvPhysX stub referenced only PR isaac-sim#5426 and PR isaac-sim#5459. The actual
   in-flight set spans six PRs: isaac-sim#5426 (merged), isaac-sim#5459, isaac-sim#5422, isaac-sim#5421,
   isaac-sim#5570, isaac-sim#5589. List them all with merge state, and reword "primary
   covered surfaces" to reflect that most are still open PRs.

4. backends/index.rst feature matrix said OvPhysX sensor coverage was
   "Partial" — actually only RigidObject is merged. Replace the
   matrix rows with concrete "In-flight (PR #...)" / "Not yet" cells.

5. Issue isaac-sim#876 asked to "review the limitations list and update it." The
   previous pass only reworded the intro. Refresh the task list against
   develop's actual newton_mjwarp coverage, add Shadow Hand / Shadow
   Hand Over / cabinet / dexsuite / rough-terrain locomotion, and
   replace the rigid bullet list with a discovery recipe so the list
   stops bit-rotting.
@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_contactsensor branch from 0ef602d to d5d5dd4 Compare May 18, 2026 13:14
Discovers sensor bodies via USD prim walk + PhysxContactReportAPI schema
check, converts IsaacLab regex paths to ovphysx fnmatch globs, creates
the ContactBinding and optional RIGID_BODY_POSE tensor binding, then
pre-allocates Warp read buffers for net forces, force matrix, and poses.
Port the 9 contact sensor tests from the PhysX backend test file,
adapting them to use isaaclab_ovphysx.sensors.ContactSensorCfg
(which wires class_type to the ovphysx ContactSensor).

Three tests that depend on track_friction_forces (not yet supported
in ovphysx v1) are marked @pytest.mark.skip.  The contact-time helper
is adapted to never enable track_contact_points or track_friction_forces
so the basic contact-time tests continue to provide real coverage.
Replace try/except/pass patterns with contextlib.suppress
as required by ruff SIM105 rule. Also add missing contextlib
import.
The post-articulation upstream uses changelog fragments under
source/<pkg>/changelog.d/; restore the direct CHANGELOG.rst and
extension.toml edits and add a minor-tier fragment that the nightly
CI workflow will compile.
Drop AppLauncher and the PhysX-style setup_simulation fixture in favour
of the kitless _ovphysx_sim_context helper (matches test_rigid_object.py
shipped in isaac-sim#5459) and the autouse device-lock fixture for ovphysx<=0.3.7.

Also drop the disable_contact_processing parametrize axis: toggling the
global Carbonite '/physics/disableContactProcessing' flag requires the
settings manager which is not available in kitless mode.  test_no_contact_reporting
now exercises the same semantic via a no-filter cfg instead.

The three v2-deferred tests (track_friction_forces / track_contact_points)
keep their pytest.mark.skip but their bodies are fully kitless so they
can be un-skipped once the ovphysx wheel ships the missing per-sensor
read APIs (see docs/superpowers/specs/2026-04-27-ovphysx-contact-api-gaps.md).

Run via:
    ./scripts/run_ovphysx.sh -m pytest source/isaaclab_ovphysx/test/sensors/test_contact_sensor.py -v
The PhysX-only PRIM_DELETION callback in
isaaclab.sensors.SensorBase._register_callbacks was gated on
'physx' in physics_mgr_cls.__name__.lower(), which also matches
OvPhysxManager.  In kitless ovphysx mode the resulting
'from isaaclab_physx.physics import IsaacEvents' raises
ModuleNotFoundError because omni.physics.tensors isn't loaded.

Switch to an exact name match so the import only fires for the
PhysX backend.  Add an isaaclab changelog fragment.

Fixes the test-time import error surfaced by
test_contact_sensor.py under ./scripts/run_ovphysx.sh.
Three places matched ovphysx as physx via substring 'physx in
manager_name.lower()'.  Switch to exact name matches and explicitly
raise on unknown managers:
  - SensorBase._register_callbacks (already fixed in prior commit;
    extended changelog)
  - AssetBase.set_debug_vis (omni.kit.app import gate)
  - SceneDataProvider._get_backend (factory dispatch)

Also broaden InteractiveScene.physics_scene_path to fall back to a
bare UsdPhysics.Scene prim (TypeName='PhysicsScene') when no prim
with PhysxSceneAPI is present.  Kitless ovphysx does not load the
omni.physx schema, so the scene prim only carries the stock USD
type.  PhysX-backed flows still prefer PhysxSceneAPI.
The ovphysx ContactBinding API explicitly states that no USD schema
beyond the rigid bodies themselves is needed for contact reporting
(ovphysx/api.py:2186-2189).  The PhysxContactReportAPI check was
copied verbatim from the PhysX backend but is irrelevant for ovphysx
and silently breaks the kitless flow where the PhysxSchema USD
plugin is not registered (so AddAppliedSchema('PhysxContactReportAPI')
in activate_contact_sensors is a no-op).

Switch the discovery loop to require UsdPhysics.RigidBodyAPI (the
USD-core rigid body schema, applied by the spawner regardless of
PhysX availability).  Update the error message accordingly.
The ovphysx wheel ships its own PhysxSchema USD plugin under
ovphysx/plugins/usd/PhysxSchema, but in kitless runs it is not
auto-registered (omni.physx normally does that in Kit-based runs).
Without registration, prim.AddAppliedSchema('PhysxContactReportAPI')
silently writes the schema name to the apiSchemas listOp but the wheel's
C++ contact-binding check fails to match the schema on the prim and
emits 'Failed to find contact report API at ...' — the binding then
returns sensor_count=0.

Register the plugin once in OvPhysxManager.initialize() (idempotent;
guarded by a class-level flag).  This is the canonical bootstrap for
any sensor that depends on Physx-prefix schemas under kitless ovphysx,
not just ContactSensor.

Also switch ContactSensor._initialize_impl's discovery check from the
filtered prim.GetAppliedSchemas() to the unfiltered
prim.GetPrimTypeInfo().GetAppliedAPISchemas() so the Python-side
discovery sees the same schemas the wheel does.  The filtered API
hides Physx-prefix schemas in kitless mode because the C++ schema
type registration only happens when the omni.physx plugin loads the
PhysxSchema library — distinct from the plugInfo.json registration we
just added.
clear_callbacks() reset _callback_id back to 0, so subsequent
register_callback() calls handed out IDs that collided with old
CallbackHandle.id values still held by not-yet-garbage-collected
sensors / assets.  When the old object's __del__ eventually ran it
deregistered the NEW callback by ID — leaving the new sensor's
_initialize_callback wired up but never fired.

The symptom under kitless OVPhysX was a sensor that registered its
callback, then PHYSICS_READY dispatched, then sensor.reset()
crashed with 'has no attribute _ALL_ENV_MASK' (init never ran).
The PhysX backend hid this because its callback registry is the
Carbonite event bus rather than this Python dict.

Keep _callback_id monotonic across clear_callbacks invocations to
preserve uniqueness for the lifetime of the process.
* Add 'ovphysx' field to PhysicsCfg in AnymalD flat_env_cfg.py
* Add 'ovphysx' field to VelocityEnvContactSensorCfg in velocity_env_cfg.py
* events.py randomize_rigid_body_material: explicit OvPhysxManager branch
  (no-op + warning; PhysX impl assumes root_view.link_paths which OvPhysx's
  per-tensor-type bindings dict does not satisfy)
* ContactSensor._initialize_impl: override _num_envs from binding sensor_count
  (clone_usd=False on OvPhysX leaves env_1..N out of USD, so the parent
  class's USD walk reports num_envs=1 even with N=4096 cloned physics envs)
… layout

ovphysx ContactBinding returns sensors in pattern-major order:
the flat index for (env, sensor) is sensor*num_envs + env, NOT the
PhysX env-major env*num_sensors + sensor that update_net_forces_kernel
inherited from the PhysX port.

Symptom on AnymalD locomotion: body indices were scrambled, so foot
contacts were attributed to the wrong body (often the base), tripping
Episode_Termination/base_contact and resetting episodes after ~4 steps.

Verified by inspecting cb.sensor_paths for a 3-env, 2-body scene:
  ['/World/envs/env_0/A', '/World/envs/env_1/A', '/World/envs/env_2/A',
   '/World/envs/env_0/B', '/World/envs/env_1/B', '/World/envs/env_2/B']
i.e. each per-body sensor pattern is fully expanded across envs before
the next pattern starts.

Pass num_envs through update_net_forces_kernel and switch its src_idx
to the pattern-major formula.
Existing tests are all single-body per ContactSensor; pattern-major
vs. env-major flat-buffer indexing collapses to the same address when
there is only one body, so they couldn't catch the kernel formula bug
fixed in the previous commit.

test_multi_body_per_sensor_indexing exercises a single ContactSensor
with prim_path='{ENV_REGEX_NS}/Cube_.*' that resolves to two cubes per
env (one on the ground, one floating).  After the scene settles, the
on-ground cube reports a non-zero net force and the floating cube
reports zero.  If the kernel indexing is wrong, the floating cube
picks up a phantom contact force from another env's grounded cube and
the assertion fires with a diagnostic message pointing at the
pattern-major / env-major mismatch.

Verified to fail (sum-abs ~3.0) on the pre-fix kernel and pass after
re-applying sensor*num_envs+env.
Two small fixes needed after the rebase onto current develop:

* ``isaaclab.utils`` no longer re-exports the ``configclass`` decorator
  directly; the import has to be ``from isaaclab.utils.configclass
  import configclass``. Touched ContactSensorCfg and the contact-sensor
  test module.
* The OVPhysX wheel is now invocable through the standard Kit Python
  entrypoint (``./isaaclab.sh -p``); the kitless ``run_ovphysx.sh``
  wrapper is no longer required. Updated the test-module docstring to
  reflect that.
Three small parity fixes that came out of a code review against the
PhysX/Newton implementations:

* Rename ``update_net_forces_kernel`` to
  ``update_net_forces_ovphysx_kernel``. PhysX ships a kernel of the same
  name with a different signature (env-major vs sensor-major index
  order); the explicit suffix prevents a confusing compile error if
  someone cross-imports.
* ``ContactSensor.body_names`` now returns ``list[str]`` (never ``None``)
  and raises a clear ``RuntimeError`` if accessed before initialization,
  matching the PhysX contract.
* Add ``_set_debug_vis_impl`` and a no-op ``_debug_vis_callback`` to the
  OVPhysX ContactSensor. The kitless OVPhysX flow has no Kit renderer,
  so the implementations log a one-shot warning and otherwise no-op,
  but the hooks are wired so ``cfg.debug_vis=True`` surfaces an
  explicit message instead of silently doing nothing.
@AntoineRichard AntoineRichard force-pushed the antoiner/feat/ovphysx_contactsensor branch from d5d5dd4 to f495912 Compare May 19, 2026 08:33
The CI ``isaaclab_ov*`` filter collects the OVPhysX contact-sensor
tests in Docker images that do not yet bundle the ``ovphysx`` wheel,
causing ``ModuleNotFoundError`` at collection time and a red required
check.

Match the precedent set by
``source/isaaclab_ovphysx/test/assets/test_articulation_helpers.py``
and ``test_rigid_object_helpers.py``: move the existing
``pytest.importorskip("ovphysx.types", ...)`` guard above the
``from isaaclab_ovphysx ...`` imports so the file is skipped before
the missing wheel can break collection.

Also add the missing ``isaaclab_tasks`` changelog fragment for the
``AnymalD Flat`` and ``LocomotionVelocityRoughEnvCfg`` ovphysx-preset
wiring that PR 5422 ships.
The previous exact-name check (``manager_name == "NewtonManager"``)
broke ``NewtonMJWarpManager``, ``NewtonKaminoManager``,
``NewtonXPBDManager``, and ``NewtonFeatherstoneManager`` — all
subclasses of ``NewtonManager``. The original substring match handled
them; the exact-match check rejected them and fell into the catch-all
``ValueError`` raise.

Restore the original ``.lower()`` substring matching, but check the
``ovphysxmanager`` exact name *before* the substring branches: without
that ordering ``"physx" in "ovphysxmanager"`` would route OVPhysX to
the PhysX impl (which assumes a ``root_view`` with ``.link_paths``,
absent on OVPhysX's per-tensor-type bindings dict).

Reproduced locally by running
``test_rendering_dexsuite_kuka[newton-isaacsim_rtx-rgb]`` against the
contact-sensor branch — failed with the documented ``ValueError``
before the fix, passes the dispatch path after.
AntoineRichard added a commit that referenced this pull request May 19, 2026
…5637)

# Description

Restructures the physics-backend documentation to give each backend a
first-class home and to add the cross-backend orientation pages asked
for in
[isaac-sim/IsaacLab-Internal#876](isaac-sim/IsaacLab-Internal#876).

**Structure**

```
docs/source/overview/core-concepts/physical-backends/
├── index.rst                 ← user-facing hub + feature-support matrix
├── solver-comparison.rst     ← cross-backend behavioural differences
├── physx/
│   ├── index.rst
│   ├── installation.rst
│   ├── configuration.rst     ← PhysxCfg tuning knobs
│   └── supported-features.rst
├── newton/
│   ├── index.rst
│   ├── installation.rst
│   ├── supported-features.rst   (was experimental-features/.../limitations-and-known-bugs.rst)
│   ├── mjwarp-solver.rst        (was experimental-features/.../solver-transitioning.rst)
│   └── kamino-solver.rst        (was experimental-features/.../using-kamino.rst)
└── ovphysx/
    └── index.rst             ← stub flagged as highly experimental; tracking issue #5634
```

**Highlights**

- The Newton subdir moves wholesale out of `experimental-features/` via
`git mv`; framing changes from "experimental feature branch" to "beta
backend." The Experimental Features toctree now contains only
`bleeding-edge`.
- Per-solver Newton pages (`mjwarp-solver`, `kamino-solver`) replace the
old `solver-transitioning` / `using-kamino` files, matching #876's
"sub-sections for the Newton solvers" ask.
- New PhysX page set written from `isaaclab_physx.physics.PhysxCfg`,
mirroring the Newton structure.
- OvPhysX stub references the full in-flight PR set (#5421, #5422,
#5426, #5459, #5570, #5589) and is gated by a follow-up issue (#5634)
for expansion after those land.
- `solver-comparison.rst` covers friction, contact pipeline,
restitution, stabilization, convergence, articulation coordinates,
substepping, and GPU buffers across PhysX TGS, Newton MJWarp, and Newton
Kamino. Each cell points at the concrete config attribute that controls
the behavior, with a porting checklist at the end.
- Newton's `supported-features.rst` list refreshed against `develop`'s
actual `newton_mjwarp` coverage (adds Shadow Hand, Shadow Hand Over,
cabinet, dexsuite, rough-terrain locomotion). Replaces the bit-rotting
bullet list with a discovery recipe (`grep -rln newton_mjwarp
source/isaaclab_tasks/`).
- Cross-references updated in `docs/index.rst`,
`core-concepts/index.rst`, `multi_backend_architecture.rst`,
`features/visualization.rst`, and
`overview/reinforcement-learning/rl_existing_scripts.rst`.

Fixes isaac-sim/IsaacLab-Internal#876

## Type of change

- Documentation update

## Screenshots

N/A — docs-only restructure. New page tree visible from the
table-of-contents in `core-concepts/`.

## Checklist

- [x] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works *(N/A — docs-only)*
- [ ] I have added a changelog fragment under
`source/<pkg>/changelog.d/` for every touched package *(N/A — docs-only;
matches precedent of #5512 and 4aeb4d6 which shipped without
fragments)*
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there
@AntoineRichard AntoineRichard merged commit 3c1753b into isaac-sim:develop May 19, 2026
63 of 65 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Isaac Lab May 19, 2026
@AntoineRichard AntoineRichard mentioned this pull request May 20, 2026
7 tasks
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

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants