New Scene Data Provider

[daniela-hase]

Description

This PR introduces the new Scene Data Provider API and adds it for the Newton Renderer and PhysX Simulation backends.

[isaaclab-review-bot]

🤖 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() to PhysicsManager base class
• Creates new SceneDataBackend interface that physics managers must implement
• Newton renderer now uses the new API for transform synchronization

Implementation Verdict

⚠️ Needs fixes before merge - see findings below

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.

[isaaclab-review-bot]

🤖 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 SceneDataFormat types
• GPU Pipeline: ✅ Zero-copy passthrough when formats match, Warp kernel launch otherwise

Architecture Impact

• Adds get_scene_data_backend() as an abstract classmethod on PhysicsManager
• All physics manager subclasses must implement this or callers get None
• SimulationContext.__init__ eagerly constructs the provider — lifecycle coupling

Implementation Verdict

⚠️ Needs fixes before merge — 1 critical bug, 2 moderate issues

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 (-1 indices)
• Passthrough vs copy paths
• PhysxSceneDataBackend with mock views

CI Status

⚙️ Only labeler ran — no linting/test CI triggered yet

---

See inline comments for specific findings.

[isaaclab-review-bot]

🟠 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

[isaaclab-review-bot]

🤖 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:

1. 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.
2. 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 SceneDataProvider class with multiple code paths
• 16 conversion kernels
• A new PhysxSceneDataBackend class
• A new abstract method on PhysicsManager

Without tests, regressions can silently return. At minimum, add:

1. Unit tests for create_mapping() with various path orderings
2. Unit tests for get_transforms() with each format conversion
3. Unit tests for passthrough vs copy behavior
4. 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

⚠️ This branch is 10 commits behind 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.

[isaaclab-review-bot]

🤖 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.

[isaaclab-review-bot]

🟠 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:

1. 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.
2. Wrong exception type: BufferError is for Python buffer-protocol violations (e.g., memoryview issues). Use RuntimeError if you do want to raise here.

Suggested change

	"""Copy a specific output to the given buffer.
	else:
	logger.warning("Newton Renderer - Failed to update transforms, skipping frame.")