[OVPHYSX] FrameTransformer sensor#5703
Conversation
AntoineRichard
requested review from
Mayankm96,
jtigue-bdai and
kellyguo11
as code owners
github-actions
Bot
added
documentation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Thank you for this well-structured PR implementing the FrameTransformer sensor for the OVPhysX backend! The implementation closely mirrors the PhysX backend while correctly adapting the pose-source layer for OVPhysX's tensor binding approach.
Summary
This PR adds FrameTransformer and FrameTransformerData for the OVPhysX backend, satisfying the BaseFrameTransformer / BaseFrameTransformerData contracts. The implementation correctly:
- Uses
TT.RIGID_BODY_POSEtensor bindings per unique tracked body - Reuses the PhysX
frame_transformer_update_kernelverbatim - Provides comprehensive test coverage mirroring the PhysX test suite
📋 Findings
1. Potential Memory Leak in _invalidate_initialize_callback (Medium) — ✅ Resolved
File: frame_transformer.py, lines 518-525
The callback clears Python lists which is sufficient for GC of tensor bindings. No action required.
2. Env Count Override Warning Could Be More Actionable (Low) — ✅ Acknowledged
The current warning is adequate. Low priority suggestion for future improvement.
🔍 Test Coverage Assessment
The test suite is comprehensive:
- ✅ Factory dispatch smoke test
- ✅ Feet w.r.t. base (4 target frames with offsets)
- ✅ Feet w.r.t. thigh (source != robot base)
- ✅ Robot body to external cube (cross-asset tracking)
- ✅ Offset frames (source frame with targets)
- ✅ All bodies wildcard pattern
- ✅ Sensor print
- ✅ Duplicate body names (multi-robot scene)
All tests are parametrized for both cpu and cuda:0, matching PhysX test conventions.
✅ Overall Assessment
This is a high-quality implementation that:
- Correctly adapts the PhysX FrameTransformer architecture for OVPhysX
- Maintains API compatibility via the factory pattern
- Provides thorough test coverage
- Follows Isaac Lab coding conventions
📝 Change History
| Commit | Summary | Status |
|---|---|---|
9558ce0c |
Polish OVPhysX FrameTransformer port | ✅ Reviewed |
28385d5c |
Add missing FrameTransformer changelog fragment | ✅ Reviewed |
Update (28385d5): ✅ CI Blocker resolved — Changelog fragment for the isaaclab package has been added (source/isaaclab/changelog.d/antoiner-feat-ovphysx-frametransformer.rst). This documents the OVPhysX backend dispatch typing addition to the public FrameTransformer factory.
All changelog fragments are now in place:
source/isaaclab/changelog.d/antoiner-feat-ovphysx-frametransformer.rst✅source/isaaclab_ovphysx/changelog.d/antoiner-feat-ovphysx_frametransformer.minor.rst✅
All issues from previous review are addressed. LGTM — ready for merge. 🚀
Update (2f4a54f): Reviewed incremental changes pushed since last review (28385d5 → 2f4a54f).
FrameTransformer-specific change:
_initialize_implnow usesresolve_matching_prims_from_source()with ahas_rigid_body_apipredicate instead offind_matching_prims(). This makes frame discovery plan-aware for OVPhysX physics-clone scenarios where USD prims may not be authored for every environment. Good improvement — aligns with the same pattern used elsewhere in the codebase.
Other changes in this push (broader fixes bundled into the branch):
- Fabric notice suspension gracefully no-ops when
usdrtis unavailable (kitless runtimes) - Sensor base ancestor expression trimming now uses
endswith()to avoid false substring matches build_simulation_contextexplicitdevicekwarg now correctly overridessim_cfg.device- OVPhysX assets stop enqueueing redundant USD replication during cloning
- Newton visualizer gains particle color/visibility options
All changes are well-tested. No new issues found. LGTM 🚀
Update (9ba47a0): Reviewed incremental changes pushed since last review (2f4a54f → 9ba47a0).
Changes in this push:
- Removed
@pytest.mark.isaacsim_cimarkers from ~50+ tests across:test_articulation.pytest_rigid_object.pytest_rigid_object_collection.pytest_contact_sensor.py
This is a CI infrastructure cleanup (removing test markers) — no functional changes to the test logic. The FrameTransformer implementation remains unchanged.
LGTM 🚀
Greptile SummaryThis PR adds
Confidence Score: 3/5Safe to merge for all tested paths, but two issues in the new OVPhysX-specific code will surface at runtime under conditions not covered by current tests. The core kernel and index-computation logic is well-structured and the 22-test suite gives strong coverage of the happy path. However, source/isaaclab_ovphysx/isaaclab_ovphysx/sensors/frame_transformer/frame_transformer.py — both the debug-viz device issue and the binding-loop mutation edge case live here. Important Files Changed
Sequence DiagramsequenceDiagram
participant Scene as InteractiveScene
participant FT as FrameTransformer (OVPhysX)
participant Binding as TT.RIGID_BODY_POSE Binding ×B
participant Gather as gather_body_pose_kernel
participant Update as frame_transformer_update_kernel
participant Data as FrameTransformerData
Scene->>FT: _initialize_impl()
FT->>Binding: create_tensor_binding(pattern, RIGID_BODY_POSE) ×B
FT->>FT: allocate _raw_transforms (N×B) transformf
FT->>FT: pre-compute _source_raw_indices, _target_raw_indices
loop Each simulation step
Scene->>FT: _update_buffers_impl(env_mask)
loop For each body binding b
FT->>Binding: binding.read(read_buf) shape (N,7) float32
FT->>Gather: "wp.launch dim=N copy pose_buf to _raw_transforms"
end
FT->>Update: "wp.launch dim=NxM compute world+relative transforms"
Update->>Data: write source/target pos_w quat_w pos_source quat_source
end
Reviews (1): Last reviewed commit: "Clean up OVPhysX-side state in _invalida..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if binding.count != self._num_envs: | ||
| # OVPhysX's InteractiveScene defaults to clone_usd=True on develop, so this branch is | ||
| # unexpected in current flows. Mirror ContactSensor's clone_usd=False fallback so the | ||
| # sensor stays correct if a future scene runs with clone_usd=False. | ||
| logger.warning( | ||
| "FrameTransformer: binding.count=%d for pattern %r differs from self._num_envs=%d;" | ||
| " overriding env count from binding (clone_usd=False scene).", | ||
| binding.count, | ||
| pattern, | ||
| self._num_envs, | ||
| ) | ||
| self._num_envs = binding.count | ||
| self._ALL_ENV_MASK = wp.ones((self._num_envs,), dtype=wp.bool, device=self._device) | ||
| self._reset_mask = wp.zeros((self._num_envs,), dtype=wp.bool, device=self._device) | ||
| self._reset_mask_torch = wp.to_torch(self._reset_mask) | ||
| self._is_outdated = wp.ones(self._num_envs, dtype=wp.bool, device=self._device) | ||
| self._timestamp = wp.zeros(self._num_envs, dtype=wp.float32, device=self._device) | ||
| self._timestamp_last_update = wp.zeros_like(self._timestamp) | ||
|
|
||
| read_buf = wp.zeros((self._num_envs, 7), dtype=wp.float32, device=self._device) | ||
| dst_torch = torch.tensor( | ||
| [env_id * num_unique_bodies + body_slot for env_id in range(self._num_envs)], | ||
| dtype=torch.int32, | ||
| device=self._device, | ||
| ) | ||
| self._body_bindings.append(binding) | ||
| self._body_read_bufs.append(read_buf) | ||
| self._body_dst_flat_indices.append(wp.from_torch(dst_torch.contiguous(), dtype=wp.int32)) |
There was a problem hiding this comment.
_num_envs mid-binding-loop can leave earlier read_buf under-sized
If the very first binding's count already equals self._num_envs (no mutation on body_slot=0) but a later binding triggers the mutation to a larger new_num_envs, the first body's read_buf was already allocated with the original (smaller) count. When _update_buffers_impl subsequently runs, it launches gather_body_pose_kernel with dim=self._num_envs (the new, larger value) for all bodies, including body 0 — causing an out-of-bounds read on pose_buf_tf (which has only the original number of elements).
The scenario is documented as "unexpected in current flows", but the mutation logic does not guard against this inconsistency when count grows after the first body is processed. At minimum a post-loop sanity check would surface the mismatch explicitly.
| marker_scales = torch.ones(frames_pos.size(0) + lines_pos.size(0), 3) | ||
| marker_indices = torch.zeros(marker_scales.size(0)) |
There was a problem hiding this comment.
debug_vis crash on GPU: marker_scales created on CPU, lines_length on CUDA
torch.ones(...) defaults to CPU. On GPU runs lines_length (derived from start_pos/end_pos which are on self.device) is a CUDA tensor. The in-place assignment at line 474 will raise RuntimeError: Expected all tensors to be on the same device whenever debug visualization is enabled.
| marker_scales = torch.ones(frames_pos.size(0) + lines_pos.size(0), 3) | |
| marker_indices = torch.zeros(marker_scales.size(0)) | |
| marker_scales = torch.ones(frames_pos.size(0) + lines_pos.size(0), 3, device=frames_pos.device) | |
| marker_indices = torch.zeros(marker_scales.size(0), device=frames_pos.device) |
| @@ -23,6 +24,8 @@ class FrameTransformer(FactoryBase, BaseFrameTransformer): | |||
|
|
|||
| data: BaseFrameTransformerData | PhysXFrameTransformerData | |||
There was a problem hiding this comment.
data annotation does not include OvPhysxFrameTransformerData
__new__ was correctly extended with the OVPhysX return-type union, but the data class-level annotation still only lists BaseFrameTransformerData | PhysXFrameTransformerData. When users call .data on an OVPhysX instance, type-checkers will not resolve to OvPhysxFrameTransformerData, losing access to the OVPhysX-specific API surface.
|
The ovphysx FrameTransformer resolves source/target prims with Please mirror the current PhysX resolver before creating the ovphysx body bindings. One more consistency point: the ovphysx backend builds |
Extends __all__ in isaaclab_ovphysx/sensors/__init__.pyi so the FrameTransformer / FrameTransformerData symbols are visible to lazy_export once their module files land in subsequent commits.
Empty package with lazy_export wiring. Submodule contents land in subsequent commits.
frame_transformer_update_kernel is copied verbatim from isaaclab_physx. gather_body_pose_kernel is new — copies one RIGID_BODY_POSE binding's per-env pose into the flat raw_transforms layout the update kernel expects.
Backend-agnostic copy of isaaclab_physx's FrameTransformerData; only the sensors-shared kernel import path differs. Properties, create_buffers, and ProxyArray caching unchanged.
Class declaration, __init__, __str__, deprecated num_bodies/body_names, reset, and _get_relative_body_path. _initialize_impl and _update_buffers_impl raise NotImplementedError; bodies land in the next two commits.
Adds _set_debug_vis_impl, _debug_vis_callback, _invalidate_initialize_callback, and _get_connecting_lines, copied verbatim from isaaclab_physx. These are backend-agnostic visualization helpers called by the base class when cfg.debug_vis=True.
Mirrors PhysX's prologue for prim resolution, RigidBodyAPI validation, body-name-to-frame mapping, source-is-target handling, and offset packing. Replaces RigidBodyView with one TT.RIGID_BODY_POSE binding per unique tracked body — the same primitive isaaclab_ovphysx ContactSensor uses for pose tracking. Falls back to overriding _num_envs from binding.count for clone_usd=False scenes, mirroring ContactSensor.
It does not mirror ContactSensor — it extends ContactSensor's two-substitution pattern with a third substitution for concrete env_<N> paths that come out of sim_utils.find_matching_prims. Docstring now lists all three substitutions and notes the assumption about scene layout.
Refreshes each per-body RIGID_BODY_POSE binding via binding.read(), reinterprets the (num_envs, 7) float32 buffer as (num_envs,) transformf via .view(wp.transformf), and runs gather_body_pose_kernel to fill the flat _raw_transforms buffer. Then runs the same frame_transformer_update_kernel as PhysX to compute source/target world poses (with offsets) and target poses relative to source.
Adds TYPE_CHECKING import so IDEs see the OVPhysX backend as a possible return type from FrameTransformer(cfg). Runtime dispatch is unchanged — FactoryBase already imports the active backend dynamically.
Kitless harness modeled on test_contact_sensor.py (device-lock autouse fixture, _ovphysx_sim_context helper, pytest.importorskip guard) plus a single parametrized smoke test that the factory dispatches to the OVPhysX backend. Full cases land in the next commit.
Ports the 7 PhysX FrameTransformer tests (feet_wrt_base, feet_wrt_thigh, robot_body_to_external_cube, offset_frames, all_bodies, sensor_print, duplicate_body_names) to the OVPhysX kitless harness. Test logic (assertion thresholds, scene step counts, ground-truth derivation) stays 1:1 with PhysX; only the sim-fixture wiring (build_simulation_context with OvPhysxCfg + parametrized device) differs.
The verbatim PhysX assertions used torch.tensor([...]) defaulting to CPU. PhysX runs CPU-only so that works there; the OVPhysX port parametrizes on cpu and cuda:0, so the CUDA pass would fail with a device-mismatch when adding CPU + CUDA tensors. Pin the literal tensors to the parametrized device.
Minor bump — new sensor, additive feature. CI compiles the fragment into source/isaaclab_ovphysx/docs/CHANGELOG.rst and bumps source/isaaclab_ovphysx/config/extension.toml.
PhysX writes self._frame_physx_view = None here. The OVPhysX backend has no such attribute — instead it holds the per-body ovphysx bindings, the read buffers, the dst-index arrays, and the flat _raw_transforms buffer. Drop the stale assignment and clear those instead, so a subsequent _initialize_impl rebuilds from a clean slate.
Mirrors the PresetCfg pattern used by cartpole / ant / humanoid / allegro_hand. Adds default / physx / newton_mjwarp / ovphysx fields so the env can dispatch to any of the three backends via the preset CLI. End-to-end exercises the OVPhysX FrameTransformer plumbing landing in this PR (sensors sub-package, articulation bindings, factory dispatch). Env body is unchanged -- it was already backend-portable; the only PhysX assumption was the bare SimulationCfg with no presets.
Mark the OVPhysX FrameTransformer tests for device-split CI collection and gate them on the tensor types required by the runtime. Tighten changelog fragments to user-facing entries.
AntoineRichard
force-pushed
the
antoiner/feat/ovphysx_frametransformer
branch
from
deff6a9 to
9558ce0
Compare
Add an isaaclab fragment for the public FrameTransformer factory typing change so the changelog gate sees every touched managed package.
…ysx_frametransformer
Use the same plan-aware source prim resolver as the PhysX FrameTransformer so source and target discovery stays correct when clone plans synthesize destination envs.
OVPhysX tests cannot run in the IsaacSim CI path, so drop the stale isaacsim_ci decorators from the OVPhysX asset and contact sensor suites.
…ysx_frametransformer
|
This PR currently conflicts with the latest Reproduced with |
marcodiiga
left a comment
marcodiiga
left a comment
There was a problem hiding this comment.
Approving after re-review. Existing review comments still stand where applicable.
2 participants
Description
Implements
FrameTransformerandFrameTransformerDatafor the OVPhysX backend, satisfying theBaseFrameTransformer/BaseFrameTransformerDatacontracts. Mirrors the PhysXFrameTransformer1:1 outside the pose-source layer — USD walking,RigidBodyAPIvalidation, body-name mapping, source-is-target handling, offset packing,_source_raw_indices/_target_raw_indices, debug-viz callbacks, and the offset + relative-transform Warp kernel are all copied verbatim from PhysX.OVPhysX-specific delta: one
TT.RIGID_BODY_POSEtensor binding per unique tracked body (the same primitiveisaaclab_ovphysx.sensors.ContactSensoruses for itstrack_pose=Truepath). Each binding reads into a(num_envs, 7)float32 buffer;.view(wp.transformf)reinterprets it, then agather_body_pose_kernellaunch copies rows into a flat_raw_transformsbuffer whose layout matches what PhysX produces. The PhysX update kernel then runs unchanged.Tests are a 1:1 port of the 7 PhysX FrameTransformer test cases (
feet_wrt_base,feet_wrt_thigh,robot_body_to_external_cube,offset_frames,all_bodies,sensor_print,duplicate_body_names) plus a factory-dispatch smoke test, adapted to the kitless harness already used bytest_contact_sensor.py. All 22 tests pass on both CPU and CUDA.Also wires the OVPhysX preset into
Isaac-Franka-Cabinet-Direct-v0(mirroring the cartpole / ant / humanoid / allegro_hand pattern) so the env can dispatch to OVPhysX viaphysics=ovphysx. Throughput parity is good; policy convergence on this env shows a physics-parity gap that is independent of this PR — see below.End-to-end demo:
Isaac-Franka-Cabinet-Direct-v0— 500 PPO iter,num_envs=4096,seed=42Throughput is on par. Convergence gap is a pre-existing physics-parity issue (likely in implicit-actuator joint-target integration or contact friction handling on the drawer joint) that surfaces on this manipulation reward landscape; not specific to this PR's surface area.
Fixes #5320
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there