New Scene Data Provider#5128
Conversation
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR introduces a new SceneDataProvider API for converting physics backend transforms into various formats (Vec3/Quat, Transform, Matrix44, Vec3/Matrix33) with optional index remapping via Warp GPU kernels. The design is clean and the kernel coverage is comprehensive.
Design Assessment
- Architecture: ✅ Good abstraction layer between physics backends and consumers (renderers)
- Layer Placement: ✅ Correct - decouples transform format conversion from both physics and rendering
Architecture Impact
- Adds new abstract method
get_scene_data_backend()toPhysicsManagerbase class - Creates new
SceneDataBackendinterface that physics managers must implement - Newton renderer now uses the new API for transform synchronization
Implementation Verdict
Test Coverage
❌ No unit tests for the new SceneDataProvider, SceneDataBackend, or conversion kernels
CI Status
✅ Labeler passed | Branch is 10 commits behind develop
Findings
🔴 Critical: Missing get_scene_data_backend in NewtonManager
NewtonManager (in isaaclab_newton/physics/newton_manager.py) does not implement the new abstract method get_scene_data_backend(). Since Python's ABC doesn't enforce abstract classmethods at class definition time, calling it returns None.
Impact: When using Newton physics backend, SimulationContext.__init__ passes None to NewSceneDataProvider(backend), causing AttributeError on any get_transforms() call.
Fix: Add NewtonSceneDataBackend class and implement get_scene_data_backend() in NewtonManager.
🟡 Code Quality: Inconsistent attribute access (scene_data_provider.py:96)
Lines 88, 92, 115 use input._cls.vars while line 96 uses input.cls.__name__. Both work but the inconsistency is confusing. Recommend using input._cls.__name__ consistently.
🟡 Code Quality: Prefer explicit None check (scene_data_provider.py:85)
if not mapping relies on wp.array.__bool__(). Explicit if mapping is None is clearer.
🟠 Lifecycle: SceneDataProvider created before views exist (simulation_context.py:170)
NewSceneDataProvider is created in __init__ before _warmup_and_create_views() sets simulation_view. This works because the API is lazy, but the implicit dependency is fragile. Consider documenting this or deferring creation.
🟠 Error Handling: Silent print on failure (newton_warp_renderer.py:212)
print("Newton Renderer - Failed to update transforms!") should use logger.warning() instead.
🟡 Missing module export
New classes (SceneDataProvider, SceneDataFormat, SceneDataBackend) should be exported from isaaclab.scene.__init__.py or added to __all__.
🟢 Note: Stage traversal in PhysxSceneDataBackend
First get_rigid_body_view() call traverses the entire USD stage. This is cached, but worth documenting.
🟡 Missing newline at EOF
scene_data_provider.py should end with a newline.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — Detailed Code Review
Summary
This PR introduces a SceneDataProvider API that abstracts physics backend transforms and provides GPU-accelerated format conversion via Warp kernels. The Newton renderer is updated to consume transforms through this new API instead of directly coupling to the scene data provider.
Design Assessment
- Architecture: ✅ Clean separation —
SceneDataBackend(physics-side) ↔SceneDataProvider(conversion layer) ↔ consumers (renderers) - Kernel Coverage: ✅ Full 4×4 conversion matrix between all
SceneDataFormattypes - GPU Pipeline: ✅ Zero-copy passthrough when formats match, Warp kernel launch otherwise
Architecture Impact
- Adds
get_scene_data_backend()as an abstract classmethod onPhysicsManager - All physics manager subclasses must implement this or callers get
None SimulationContext.__init__eagerly constructs the provider — lifecycle coupling
Implementation Verdict
Test Coverage
❌ No unit tests. The __main__ example is a good start but should be extracted into proper tests covering:
- Each conversion kernel (round-trip correctness)
- Mapping with gaps (
-1indices) - Passthrough vs copy paths
PhysxSceneDataBackendwith mock views
CI Status
⚙️ Only labeler ran — no linting/test CI triggered yet
See inline comments for specific findings.
| @@ -583,6 +584,9 @@ def initialize_visualizers(self) -> None: | |||
| close_provider() | |||
There was a problem hiding this comment.
🟠 Eager construction before physics views exist
This runs during __init__, but PhysxSceneDataBackend.simulation_view isn't set until _warmup_and_create_views() (called during reset()). The backend handles None gracefully (returns empty data), so this won't crash — but any code that calls get_new_scene_data_provider().get_transforms() between __init__ and reset() will silently get empty results.
Consider either:
- Deferring construction to
reset()/ first access (lazy property) - Adding a docstring noting the lifecycle dependency
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR introduces a new SceneDataProvider API to abstract transform data from physics backends (PhysX) and convert between different formats (Vec3/Quat, Transform, Matrix44) for consumers like the Newton renderer. The design is reasonable, but there are critical bugs that will break the Newton physics backend and cause incorrect behavior with empty mappings.
Design Assessment
Problem being solved: Decouple transform data format conversion from specific physics backends, allowing renderers to request transforms in their preferred format without knowing which physics backend is active.
Alternative approaches considered:
- Extend existing
BaseSceneDataProvider— The codebase already has a scene data provider pattern (isaaclab.physics.BaseSceneDataProvider). This PR creates a parallel system (NewSceneDataProvider) rather than extending the existing one. This creates two similar-but-different abstractions. - Format conversion at the renderer level — Each renderer could convert from a canonical format. This would be simpler but duplicates conversion logic.
Assessment: The abstraction is reasonable — centralizing format conversion makes sense. However, introducing a new provider (NewSceneDataProvider) alongside the existing BaseSceneDataProvider creates confusion. The naming with "New" prefix suggests this is transitional. Acceptable short-term, but the long-term fix should unify these into a single SceneDataProvider API.
The PR correctly puts the abstraction at the physics layer (backend provides raw transforms) with conversion happening in a dedicated provider class. This follows the framework's ownership model.
Architecture Impact
🔴 Breaking change for Newton physics backend. The PR adds get_scene_data_backend() as an abstract method to PhysicsManager, but NewtonManager does not implement it. When SimulationContext.__init__ calls self.physics_manager.get_scene_data_backend() (line 170), it will return None from the base class's pass implementation, causing SceneDataProvider(None) to be constructed. Subsequent calls to backend.transform_count will fail with AttributeError.
Implementation Verdict
Significant concerns — Two critical bugs and missing test coverage will cause crashes with the Newton backend and incorrect behavior with empty mappings.
Test Coverage
🟡 No tests added. This is a new feature introducing:
- A new
SceneDataProviderclass with multiple code paths - 16 conversion kernels
- A new
PhysxSceneDataBackendclass - A new abstract method on
PhysicsManager
Without tests, regressions can silently return. At minimum, add:
- Unit tests for
create_mapping()with various path orderings - Unit tests for
get_transforms()with each format conversion - Unit tests for passthrough vs copy behavior
- Integration test verifying PhysX backend provides correct transforms
CI Status
Only labeler check has run and passed ✅. Full CI suite (pre-commit, tests) appears not to have triggered yet.
Branch Status
develop. Recommend rebasing to pick up recent changes before merge.
Findings
🔴 Critical: newton_manager.py — Missing get_scene_data_backend() implementation
NewtonManager extends PhysicsManager but doesn't implement the new abstract method get_scene_data_backend(). When using Newton physics, SimulationContext.__init__ will fail because physics_manager.get_scene_data_backend() returns None, and SceneDataProvider(None) cannot function.
🔴 Critical: scene_data_provider.py:86 — not mapping incorrectly treats empty arrays as None
The check if not mapping and type(input) is type(output) uses truthiness to test for None. However, an empty warp array wp.array([], dtype=wp.int32) is falsy (bool(empty_array) == False), so not empty_array == True. This means explicitly passing an empty mapping will incorrectly trigger the passthrough branch instead of using the kernel. Use if mapping is None instead.
🟡 Warning: physx_manager.py:201-206 — transforms property allocates on every call
The transforms property creates a new SceneDataFormat.Transform() instance every time it's accessed. Since this is called every render frame from get_transforms(), it creates unnecessary allocations in a hot path. Consider caching the struct instance and only updating the transforms field.
🔵 Improvement: scene/init.pyi — New classes not exported
The new classes (SceneDataProvider, SceneDataBackend, SceneDataFormat, ConversionKernels) are not added to __init__.pyi, so they won't be exported from isaaclab.scene. While direct imports work, this is inconsistent with codebase conventions.
🔵 Improvement: simulation_context.py — Naming inconsistency
The new provider is named NewSceneDataProvider and accessed via get_new_scene_data_provider(). The "New" prefix suggests this is transitional. Consider a cleaner name like TransformDataProvider or plan to replace the existing SceneDataProvider.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — Follow-up
New commits (f905ac1→e8b1fa01): Simplifies Newton renderer init (removes backend-type branching, always creates own state/mapping) and restructures render() to use SceneDataProvider directly.
One new concern below — previous comments still stand.
| raise BufferError("Newton Renderer - Failed to update transforms!") | ||
|
|
||
| def write_output(self, render_data: RenderData, output_name: str, output_data: torch.Tensor): | ||
| """Copy a specific output to the given buffer. |
There was a problem hiding this comment.
🟠 New: raise BufferError crashes the render loop on transient failures
The previous code logged a warning and continued rendering with stale transforms. This new code raises an exception, which will crash the entire simulation if get_transforms() returns False for even a single frame (e.g., during initialization before views are ready, or a transient backend hiccup).
Two issues:
- Behavioral regression: Converting a warning to a hard crash in a per-frame render call is risky. Consider
logger.warning()+return(skip the frame) instead of raising. - Wrong exception type:
BufferErroris for Python buffer-protocol violations (e.g.,memoryviewissues). UseRuntimeErrorif you do want to raise here.
| """Copy a specific output to the given buffer. | |
| else: | |
| logger.warning("Newton Renderer - Failed to update transforms, skipping frame.") |
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — Follow-up
The new commit (e8b1fa0→9695a0ba) reintroduces backend-type branching in NewtonWarpRenderer.__init__ and restructures the render flow to use SceneDataProvider.
New Critical Issue
🔴 Critical: newton_warp_renderer.py:174 — Syntax error in sensor construction
self.newton_sensor = newton.sensors.SensorTiledCamera(self._
newton_model,This is a syntax error — the line break splits self._newton_model incorrectly. The code will fail to parse at runtime. This appears to be a merge artifact or copy-paste error. Should be:
self.newton_sensor = newton.sensors.SensorTiledCamera(
self._newton_model,Previous Concerns Status
- PhysX backend stage traversal (physx_manager.py:199-203): Still present — full stage traversal on every
get_rigid_body_view()call remains a performance concern. - Missing tests: Still no test coverage for the new
SceneDataProviderAPI.
Implementation Verdict
Needs rework — the syntax error in newton_warp_renderer.py will cause immediate runtime failure.
12bd8c2 to
7a97c41
Compare
…vider-api # Conflicts: # source/isaaclab/isaaclab/sim/simulation_context.py # source/isaaclab_newton/isaaclab_newton/renderers/newton_warp_renderer.py
Greptile SummaryThis PR introduces a
Confidence Score: 3/5Three defects in hot paths need fixing before merge: duplicate marker dispatch, dropped env-filter assignment in ViserVisualizer, and zero env count on PhysX backend. The duplicate dispatch_callbacks() block fires every marker callback twice per frame; the missing self._resolved_visible_env_ids assignment makes max_visible_envs a no-op in ViserVisualizer; get_num_envs() returns 0 for the PhysX+Newton-renderer path disabling world filtering in both Rerun and Viser visualizers. simulation_context.py, viser_visualizer.py, rerun_visualizer.py, newton_manager.py Important Files Changed
Sequence DiagramsequenceDiagram
participant SC as SimulationContext
participant PM as PhysicsManager
participant SDB as SceneDataBackend
participant SDP as SceneDataProvider
participant VIZ as Visualizer
participant NM as NewtonManager
SC->>PM: initialize(sim_context)
PM->>SDB: new Backend()
SC->>PM: get_scene_data_backend()
PM-->>SC: backend
SC->>SDP: SceneDataProvider(backend)
SC->>SC: initialize_visualizers()
SC->>VIZ: initialize(scene_data_provider)
VIZ->>NM: get_model() / get_state()
loop Each render frame
SC->>SC: update_visualizers(dt)
SC->>PM: pre_render() [PhysX only]
PM->>NM: update_visualization_state()
NM->>SDP: get_transforms(scene_data, mapping)
SDP->>SDB: transforms
SDB-->>SDP: SceneDataFormat.Transform
SDP-->>NM: body_q synced
SC->>VIZ: step(dt)
VIZ->>NM: get_state()
NM-->>VIZ: state_0 (with synced body_q)
end
|
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot - Incremental Review
Reviewing changes: 1b0b5898 → 469c6bb0
📋 Summary
This is a substantial update to PR #5128 "New Scene Data Provider" with several significant additions:
✅ Positive Changes
1. OVPhysX RigidObject Support (Major)
- Added comprehensive test suite
test_rigid_object.py(1100+ lines) mirroring PhysX tests - Added
asset_kind="rigid_object"support inMockOvPhysxBindingSetfor proper tensor binding shapes - Proper handling of
RIGID_BODY_*tensor types with appropriate CPU-only type management
2. PhysX Task-Space Controller Infrastructure (Major)
- New
body_link_jacobian_w,body_com_jacobian_w,mass_matrix,gravity_compensation_forcesproperties onArticulationData - COM→link-origin shift kernel (
shift_jacobian_com_to_origin) fixes latent IK/OSC correctness bug - Thorough test coverage including IK/OSC tracking accuracy tests and gravity compensation verification
3. RSL-RL Configuration Modernization
- Migrated from deprecated
policyconfigs to separateactor/criticmodel configs with distribution configs - Consistent updates across all agent configurations (locomotion, manipulation, classic tasks)
4. Newton/MJWarp Physics Preset Support
- Dexsuite environments now support Newton MJWarp physics via
PhysicsCfgpreset system - Heterogeneous mesh-based object spawning (
MeshCuboidCfg,MeshSphereCfg, etc.)
⚠️ Observations
1. Test Timeout Increase
# tools/test_settings.py
"test_articulation.py": 3000, # Was 1500Doubled timeout for articulation tests - the new task-space controller tests add significant runtime.
2. Retry Reduction
# tools/conftest.py
TIMEOUT_RETRIES = 0 # Was 2Timeout retries disabled - may be intentional for faster CI feedback.
3. Material Property Gap (Expected)
Five tests in test_rigid_object.py are marked @pytest.mark.xfail due to missing RIGID_BODY_MATERIAL TensorType - this is documented and expected per the gap spec.
🔍 Code Quality Notes
- Consistent Documentation: All new properties have proper docstrings with cross-references to base class contracts
- Cache Invalidation:
write_*_to_sim_indexmethods properly invalidate task-space accessor timestamps - Process-Global Device Lock Handling:
test_rigid_object.pyhandles ovphysx session-pinning gracefully with skip decorators
📊 Verdict: LGTM ✅
The incremental changes are well-structured and thoroughly tested. The task-space accessor additions fix an important correctness bug in IK/OSC controllers.
Reviewed at commit 469c6bb00d864c06a3ed736a5e086c1d5becc768
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.
Description
This PR introduces the new Scene Data Provider API and adds it for the Newton Renderer and PhysX Simulation backends.