[OVPHYSX] SceneDataProvider#5589
Conversation
AntoineRichard
requested review from
Mayankm96,
jtigue-bdai and
kellyguo11
as code owners
github-actions
Bot
added
documentation
Greptile SummaryThis PR introduces
Confidence Score: 3/5The core pose-sync pipeline is solid, but two edge-case logic bugs in get_camera_transforms and _wildcard_env_paths can silently produce wrong data or a permanent no-op with no WARNING-level signal. The core pose-sync path (TensorBinding to Warp kernel to Newton body_q) is architecturally sound and well-tested for the primary cartpole and Allegro-hand scenes. The two flagged issues are narrow edge cases unlikely to affect those workloads today, but both fail silently once triggered: get_camera_transforms produces structurally misaligned arrays for global cameras, and _wildcard_env_paths permanently excludes non-env_0 rigid bodies from binding when a mixed scene is used. source/isaaclab_ovphysx/isaaclab_ovphysx/scene_data_providers/ovphysx_scene_data_provider.py — specifically get_camera_transforms (num_envs initialisation) and _wildcard_env_paths (mixed-path handling) Important Files Changed
Sequence DiagramsequenceDiagram
participant Sim as SimulationContext
participant Factory as SceneDataProvider (factory)
participant OvSDP as OvPhysxSceneDataProvider
participant PhysX as OvPhysX TensorBinding
participant Warp as Warp kernel
participant Newton as Newton body_q
Sim->>Factory: create(stage, sim_ctx)
Factory->>Factory: "_get_backend() -> ovphysx"
Factory->>OvSDP: __init__(stage, sim_ctx)
OvSDP->>OvSDP: _build_newton_model_from_clone_plan()
OvSDP->>OvSDP: _setup_scene_bindings()
OvSDP->>PhysX: create_tensor_binding(pattern, RIGID_BODY_POSE)
PhysX-->>OvSDP: pose_binding
OvSDP->>PhysX: create_tensor_binding(pattern, RIGID_BODY_VELOCITY)
PhysX-->>OvSDP: vel_binding
loop Each simulation step
Sim->>OvSDP: update()
OvSDP->>OvSDP: _refresh_newton_model_if_needed()
OvSDP->>OvSDP: _read_poses_from_best_source()
OvSDP->>PhysX: pose_binding.read(pose_buf)
PhysX-->>OvSDP: poses [Nx7]
OvSDP->>OvSDP: _apply_xform_poses() [FrameView fallback]
OvSDP->>Warp: launch _set_body_q_kernel(positions, orientations, body_q)
Warp->>Newton: body_q updated
end
Reviews (1): Last reviewed commit: "Refactor OVPhysX SDP to mirror PhysX sin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| num_envs = -1 | ||
|
|
||
| # Breadth-first walk so we discover camera prims across the full stage. | ||
| stage_prims = deque([self._stage.GetPseudoRoot()]) | ||
| while stage_prims: | ||
| prim = stage_prims.popleft() | ||
| prim_path = prim.GetPath().pathString | ||
|
|
||
| world_id = 0 | ||
| template_path = prim_path | ||
| if match := env_pattern.match(prim_path): | ||
| # Normalize per-env path to a shared template key (env_%d/...) so | ||
| # visualizers can query one camera path for all env instances. | ||
| world_id = int(match.group("id")) | ||
| template_path = match.group("root") + "%d" + match.group("path") | ||
| if world_id > num_envs: | ||
| num_envs = world_id | ||
|
|
||
| imageable = UsdGeom.Imageable(prim) | ||
| if imageable and imageable.ComputeVisibility() == UsdGeom.Tokens.invisible: | ||
| continue | ||
|
|
||
| if prim.IsA(UsdGeom.Camera): | ||
| if template_path not in instances: | ||
| instances[template_path] = [] | ||
| instances[template_path].append((world_id, prim_path)) | ||
| if template_path not in shared_paths: | ||
| shared_paths.append(template_path) | ||
|
|
||
| if hasattr(UsdGeom, "TraverseInstanceProxies"): | ||
| child_prims = prim.GetFilteredChildren(UsdGeom.TraverseInstanceProxies()) | ||
| else: | ||
| child_prims = prim.GetChildren() | ||
| if child_prims: | ||
| stage_prims.extend(child_prims) | ||
|
|
||
| num_envs += 1 | ||
| positions: list[list[list[float] | None]] = [] | ||
| orientations: list[list[list[float] | None]] = [] | ||
|
|
||
| for template_path in shared_paths: | ||
| per_world_pos: list[list[float] | None] = [None] * num_envs | ||
| per_world_ori: list[list[float] | None] = [None] * num_envs | ||
| for world_id, prim_path in instances.get(template_path, []): | ||
| if world_id < 0 or world_id >= num_envs: | ||
| continue | ||
| prim = self._stage.GetPrimAtPath(prim_path) | ||
| if not prim.IsValid(): | ||
| continue | ||
| pos, ori = isaaclab_sim.resolve_prim_pose(prim) | ||
| per_world_pos[world_id] = [float(pos[0]), float(pos[1]), float(pos[2])] | ||
| per_world_ori[world_id] = [float(ori[0]), float(ori[1]), float(ori[2]), float(ori[3])] | ||
| positions.append(per_world_pos) | ||
| orientations.append(per_world_ori) |
There was a problem hiding this comment.
num_envs is initialised to -1 and incremented only when a camera prim matches the env pattern (/World/envs/env_<id>/...). When only global cameras exist, num_envs stays 0 after += 1. Any such camera gets world_id = 0 and template_path = prim_path (no env match), is added to shared_paths and instances, but then at line 775 the guard world_id >= num_envs (i.e. 0 >= 0) skips its pose. The result is a return value where order has the camera path but the corresponding positions[i] / orientations[i] are [] — structurally misaligned with callers that zip order with the pose arrays and expect num_envs-length sub-arrays. Even when env cameras exist, a global camera writes its pose at per_world_pos[0], corrupting the env-0 slot for that template path.
| Deduplicated wildcard-form paths, in input order. | ||
| """ | ||
| wildcard = [p.replace("/World/envs/env_0", "/World/envs/env_*") for p in paths if "/World/envs/env_0" in p] | ||
| return list(dict.fromkeys(wildcard)) if wildcard else list(paths) |
There was a problem hiding this comment.
_wildcard_env_paths only converts paths containing /World/envs/env_0. When wildcard is non-empty (any env_0 paths present), the return value is the deduplicated wildcard list — which entirely omits any non-env path. A rigid body at /World/Pedestal that passes the UsdPhysics.RigidBodyAPI filter in _setup_scene_bindings is stripped here, receives no binding, and silently falls through to FrameView fallback. If FrameView also fails for that prim, _read_poses_from_best_source returns None and update() is a silent no-op for every subsequent frame.
| def update(self) -> None: | ||
| """Sync OVPhysX body transforms into the full Newton state. | ||
|
|
||
| Reads body poses from the rigid + articulation bindings (with FrameView | ||
| fallback) and writes them into ``newton_state.body_q`` via a single | ||
| Warp kernel launch. | ||
| """ | ||
| if not self._needs_newton_sync or self._newton_state is None: | ||
| return | ||
| try: | ||
| self._refresh_newton_model_if_needed() | ||
| # Bindings may be deferred if the ovphysx runtime was not ready at __init__. | ||
| if not self._rigid_bindings: | ||
| self._setup_scene_bindings() | ||
| result = self._read_poses_from_best_source() | ||
| if result is None: | ||
| return | ||
| positions, orientations, _, _ = result | ||
| positions_wp = wp.from_torch(positions.reshape(-1, 3), dtype=wp.vec3) | ||
| orientations_wp = wp.from_torch(orientations.reshape(-1, 4), dtype=wp.quatf) | ||
| if positions_wp.shape[0] != self._newton_state.body_q.shape[0]: | ||
| return |
There was a problem hiding this comment.
_apply_xform_poses hides permanent FrameView failures
The except Exception: continue swallows all errors from FrameView(...) construction and from get_world_poses(). Only self._xform_view_failures is updated, and the warn-once log fires at DEBUG level. If the FrameView fails permanently for a body, the covered mask for that body is never set to True, _read_poses_from_best_source returns None on every frame, and update() is a no-op forever with no WARNING-level log after the first attempt.
| return None | ||
|
|
||
| def get_velocities(self) -> dict[str, Any] | None: | ||
| """Return linear/angular velocities concatenated across per-pattern bindings. | ||
|
|
There was a problem hiding this comment.
_setup_scene_bindings retried every step after permanent failure
In update(), if not self._rigid_bindings: self._setup_scene_bindings() is meant to defer binding creation until the OVPhysX runtime is ready. But if binding creation fails permanently, _rigid_bindings stays empty and _setup_scene_bindings() is called on every simulation step, walking every body path against the stage repeatedly. A flag distinguishing "deferred because physx not ready" from "tried and failed" would allow the hot path to skip the retry.
|
|
||
| def update(self) -> None: | ||
| """Sync OVPhysX body transforms into the full Newton state. | ||
|
|
There was a problem hiding this comment.
torch.where(~covered) with a single argument returns a tuple of index tensors; [0] is correct for the 1-D case here but torch.nonzero(~covered, as_tuple=True)[0] (already used in _apply_rigid_binding at line 549) is the conventional idiom and makes the 1-D assumption explicit.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
AntoineRichard
force-pushed
the
antoiner/feat/ovphysx_scene_data_provider
branch
from
4384bfb to
41f8784
Compare
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔄 Follow-up Review (post-push update)
Reviewed incremental changes: 4384bfb → 41f8784
✅ Improvements in this push
- Added
replace_newton_shape_colors()for material parity with PhysX/Newton - Enhanced docstrings throughout
- Added warning when no Newton body paths available
- Fixed
get_velocities()to returnNoneon partial read failure (prevents silent misalignment)
📋 Previous review items
The original P1/P2 items from the prior review remain as-is:
- P1 issues (global cameras, non-env paths) — these are edge cases that may be acceptable for the initial implementation
- P2 issues (exception handling, retry logic, idiom consistency) — minor improvements that could be addressed in follow-up PRs
Since the PR has already been approved, these can be tracked as potential follow-up work if they cause issues in practice.
Update (1d70296): Reviewed new commits 41f8784 → 1d70296 which add OvPhysxSceneDataBackend for the new SceneDataProvider API (#5128 integration). Changes look solid:
- ✅ New
OvPhysxSceneDataBackendclass properly implements theSceneDataBackendinterface - ✅ Comprehensive unit tests (8 tests) cover transform count/paths, setup, and read logic
- ✅ Backend constructed eagerly in
OvPhysxManager.initializematching PhysX pattern - ✅ Changelog fragment added
No new issues introduced. The implementation correctly mirrors the existing one-pattern-per-binding approach from the original SceneDataProvider.
Update (49a64ed): Reviewed 1d70296 → 49a64ed:
- ✅ Performance optimization:
pose_buf_transformfview now cached at setup time (eliminates per-step Python allocations) - ✅ Improved docstrings explaining the zero-copy reinterpretation strategy
- ✅ Type annotation fix (
list[dict[str, Any]]) - ✅ Two new tests for error handling paths (
test_setup_continues_when_create_tensor_binding_raises,test_transforms_logs_warning_when_a_binding_read_fails)
Good refinements, no new issues.
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.
Implements the SceneDataBackend interface introduced in develop's PR isaac-sim#5128 ("New Scene Data Provider") for the kitless OVPhysX backend. The backend adapts the wheel's one-pattern-per-binding API by bucketing UsdPhysics.RigidBodyAPI prims by their env-wildcard form (cartpole -> 2 bindings, Allegro hand -> ~17), pre-allocating per-binding read buffers plus a single merged wp.transformf buffer, and concatenating reads on each transforms-property access. OvPhysxManager exposes the backend via the new get_scene_data_backend() classmethod, initialized at warmup.
Pre-commit reformats dict-literal and SimpleNamespace(...) constructions in the new test file to multi-line form. Mechanical change with no semantic effect; tests still pass 9/9.
Bumps isaaclab_ovphysx by a minor tier. Describes the new OvPhysxSceneDataBackend and OvPhysxManager.get_scene_data_backend introduced in the previous commit and the per-binding/merged-buffer strategy used to bridge the wheel's one-pattern-per-binding API to the central SceneDataProvider.
AntoineRichard
force-pushed
the
antoiner/feat/ovphysx_scene_data_provider
branch
from
41f8784 to
d4855e4
Compare
SimulationContext.__init__ wires up the central SceneDataProvider with the result of physics_manager.get_scene_data_backend() — when the backend was lazy-constructed in _warmup_and_load, that capture happened before warmup and the SDP held backend=None forever, breaking NewtonManager.update_visualization_state on the first call to visualizer.initialize(). Construct the backend instance in initialize() (matches PhysX's pattern) so SimulationContext gets a real reference. setup() still runs in _warmup_and_load when the wheel and USD stage are live; the backend reports empty transform_paths until then, which create_mapping already handles (returns None). Also clear the singleton in close() so cached TensorBinding handles don't survive a physx.reset() across SimulationContext restarts. Matches Newton's lifecycle.
Folds in the polish items the post-implementation code review surfaced: * Cache the per-binding wp.transformf view at setup time (entry pose_buf_transformf) instead of reconstructing on every transforms access. Removes Python allocation churn on hot rendering paths. * Widen _rigid_bindings annotation from list[dict[str, object]] to list[dict[str, Any]]. Adds an inline comment documenting the per-entry shape so future readers don't have to dig. * SI-unit note in the transforms property docstring per AGENTS.md: position [m], quaternion (xyzw, unit). * Two new tests covering the previously unverified defensive paths: test_setup_continues_when_create_tensor_binding_raises and test_transforms_logs_warning_when_a_binding_read_fails. Both assert the surviving bindings still produce correct output, so the fault-isolation isn't just decorative. Tests: 11/11 pass (was 9). Pre-commit clean.
2 participants
Description
Adds
OvPhysxSceneDataProvider, the OVPhysX-backend implementation ofBaseSceneDataProvider. Enables Newton-based visualizers (Rerun, Viser, native Newton viewport) to render OVPhysX simulations. Also fixes theSceneDataProviderfactory dispatch (and adds a regression test) so the substring check"physx" in "ovphysxmanager"no longer routes ovphysx to PhysX.Fixes #5327
Important
Stacked on #5459 (
[OVPHYSX] Articulation rewrite). The base of this PR isdevelop, so the diff includes every commit from #5459 — review only the 16 SDP-specific commits on top (git log antoiner/feat/ovphysx_articulation..antoiner/feat/ovphysx_scene_data_provider). Once #5459 merges, this PR will rebase cleanly.Architectural notes
Mirrors
PhysxSceneDataProviderend-to-end: build a Newton model from the scene'sClonePlanat init (gated onSceneDataRequirement.requires_newton_model), then on everyupdate()read body poses through ovphysxTensorBindings and write them into the Newton state'sbody_qvia a single Warp kernel.The interesting OVPhysX-specific surface:
PhysX.create_tensor_bindingaccepts a single glob-style pattern per binding (not a list, unlike PhysX'sRigidBodyView). We bucket Newton body paths by their env-wildcard form (/World/envs/env_*/Robot/cart,/World/envs/env_*/Robot/pole, ...), one pose+velocity binding per distinct relative path. Cartpole produces 2 bindings; Allegro hand ~17 — each covering all envs.RIGID_BODY_POSEcovers everything: confirmed via standalone probe + wheel docs that the matcher accepts any prim withUsdPhysics.RigidBodyAPI, including articulation links. No separateLINK_POSEplumbing is needed. Paths flow through aRigidBodyAPIfilter (mirroringPhysxSceneDataProvider._setup_rigid_body_viewline ~281) so joints and articulation-root xforms are excluded from binding patterns.binding.read(dst)API: pre-allocated warp destination buffers, so per-step reads are allocation-free.prim_pathsis matched against the Newtonbody_labelordering to build a per-binding reorder tensor with-1sentinels for rows owned by other bindings. The pose merge respects partial coverage across bindings, withFrameViewfallback for anything that lacksRigidBodyAPI(cameras, decorative xforms).SceneDataProvider._get_backendnow checks"ovphysx"explicitly before"physx"so the substring overlap doesn't route the wrong backend.Files changed (16 commits since
antoiner/feat/ovphysx_articulation)source/isaaclab/isaaclab/physics/scene_data_provider.py— factory dispatch fix (+9/-1).source/isaaclab/test/sim/test_scene_data_provider_factory.py— new parametrized regression test (44 lines).source/isaaclab_ovphysx/isaaclab_ovphysx/scene_data_providers/__init__.py,__init__.pyi— package scaffolding.source/isaaclab_ovphysx/isaaclab_ovphysx/scene_data_providers/ovphysx_scene_data_provider.py— the provider (~760 lines).source/isaaclab_ovphysx/test/scene_data_providers/test_ovphysx_scene_data_provider.py— 18 stub-based unit tests covering factory dispatch, metadata, env discovery, Newton-model build, binding setup, pose-merge orchestration, transform/velocity getters, view-order reorder, camera-transform stage walk.docs/source/api/index.rst+docs/source/api/lab_ovphysx/isaaclab_ovphysx.scene_data_providers.rst— firstisaaclab_ovphysxsection in the API reference.source/isaaclab/changelog.d/antoiner-feat-ovphysx-scene-data-provider.rst— patch fragment for the factory fix.source/isaaclab_ovphysx/changelog.d/antoiner-feat-ovphysx-scene-data-provider.minor.rst— minor fragment for the new provider.Type of change
Validation
./isaaclab.sh -p -m pytest source/isaaclab/test/sim/test_scene_data_provider_factory.py source/isaaclab_ovphysx/test/scene_data_providers/→ 23 passed../isaaclab.sh -p scripts/reinforcement_learning/rsl_rl/train.py --task=Isaac-Cartpole-Direct-v0 --visualizer newton --num_envs 4096 presets=ovphysxreaches the simulation loop; visualizer renders the cartpole scene../isaaclab.sh -fclean./tmp/probe_rigid_body_pose_on_articulation.pyconfirmedRIGID_BODY_POSEaccepts articulation-link patterns. Reproducible against the wheel's bundled minimal articulation USD.Known follow-ups (not in this PR)
_build_newton_model_from_clone_planinto a shared helper used by both PhysX and OVPhysX providers (currently a verbatim copy).FrameViewfactory could grow an explicit"ovphysx"registration entry (today's dispatch falls through to the"physx"branch, which works becauseFabricFrameViewreads from the shared USD stage).test_ovphysx_scene_data_provider_visualizer_contract.pymirroringtest_physx_scene_data_provider_visualizer_contract.py.Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdor my name already exists there