refactor: move Fabric/USD dispatch from FabricFrameView to FrameView factory#5673
refactor: move Fabric/USD dispatch from FabricFrameView to FrameView factory#5673pv-nvidia wants to merge 3 commits into
Conversation
8575a19 to
b51594f
Compare
Add get_service(cls) / set_service(cls, instance) — a lightweight typed singleton registry on SimulationContext, keyed by service class. This lets backend-specific caches (e.g. Fabric hierarchy handles) register themselves without polluting SimulationContext with backend-specific fields or imports. Services with a close() method are automatically closed: - On replacement via set_service() - On teardown via clear_instance() No existing behavior changes — this is purely additive.
- 7 unit tests covering get_service, set_service, replacement lifecycle, close-on-clear_instance, multiple service types, and idempotent re-registration - Changelog entry for the new service locator API
…factory
FabricFrameView had an internal _use_fabric flag that fell back to
UsdFrameView when Fabric was disabled or the device was unsupported.
This violated single-responsibility: FabricFrameView pretended to be
one class but sometimes behaved as another.
Now the FrameView factory handles all dispatch:
- PhysX + Fabric enabled + supported device → FabricFrameView
- PhysX without Fabric (or unsupported device) → UsdFrameView
- Newton → NewtonSiteFrameView
FabricFrameView no longer checks _use_fabric or _fabric_supported_devices.
It assumes Fabric is available (the factory guarantees this).
UsdFrameView is eagerly registered on the factory since it lives in
isaaclab (not a backend package), so FactoryBase's dynamic import
(isaaclab_{backend}.sim.views) can't discover it.
b51594f to
4c71366
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
PR #5673: refactor: move Fabric/USD dispatch from FabricFrameView to FrameView factory
Summary
This PR contains 3 commits that introduce a service locator pattern and refactor the FrameView backend dispatch logic:
596bedb- Add typedServiceLocatortoSimulationContextdac5a16- Add unit tests and changelog fragment4c71366- Move Fabric/USD dispatch fromFabricFrameViewtoFrameViewfactory
✅ What's Good
-
Clean Service Locator Implementation (
service_locator.py)- Well-designed subscript API (
services[cls] = instance) - Proper lifecycle management with
close()handling - Good defensive coding with
_try_close()checking callable
- Well-designed subscript API (
-
Improved Separation of Concerns
- Factory pattern in
FrameViewnow handles backend selection logic FabricFrameViewis now simpler - only handles Fabric operations- Removed conditional checks inside
FabricFrameViewmethods
- Factory pattern in
-
Comprehensive Tests
test_service_locator.pycovers edge cases (non-callable close, missing keys, etc.)- Good coverage of the service locator API
-
Proper Integration
SimulationContext.clear_instance()now callsservices.close_all()- Services property exposed for external use
🔍 Review Items
1. _FABRIC_SUPPORTED_DEVICES Duplication (Minor)
# In frame_view.py:
_FABRIC_SUPPORTED_DEVICES = ("cpu", "cuda", "cuda:0")This was previously in fabric_frame_view.py. Consider:
- Making this a module-level constant that can be imported
- Or document why the factory owns this knowledge
2. Device Argument Extraction (frame_view.py:58-60)
device = kwargs.get("device", "cpu")
if len(args) >= 2:
device = args[1]This relies on positional argument order. Consider adding a comment documenting the expected signature or using keyword-only enforcement.
3. Return Type Consistency (ServiceLocator.__getitem__)
def __getitem__(self, cls: type[_T]) -> _T | None:
return self._services.get(cls) # returns None if missingReturning None for missing keys differs from standard dict behavior (which raises KeyError). This is documented but worth noting - callers must always handle None.
4. No SettingsManager Import Removed from FabricFrameView
The import of SettingsManager was removed from fabric_frame_view.py since Fabric checks moved to the factory. 👍
📋 Checklist
- Code follows Isaac Lab style guidelines
- Type hints present and correct
- Docstrings provided for public APIs
- Unit tests added for new functionality
- Changelog fragment included
- No obvious regressions
Verdict: Approve ✅
This is a clean refactor that improves code organization by moving dispatch logic to the factory where it belongs. The service locator is a useful addition for managing backend-specific singletons. Minor suggestions above are non-blocking.
Reviewed at commit: 4c71366
Problem
FabricFrameViewhas an internal_use_fabricflag that falls back toUsdFrameViewwhen Fabric is disabled or the device is unsupported. This violates single-responsibility: the class pretends to be a Fabric-accelerated view but sometimes silently delegates everything to a different implementation. It's also a Liskov substitution violation — callers can't reason about which code path runs.Solution
Move the Fabric-enabled + device-supported check from
FabricFrameView.__init__up to theFrameViewfactory's_get_backend()method. The factory now dispatches to three implementations:physxFabricFrameViewusdUsdFrameViewnewtonNewtonSiteFrameViewUsdFrameViewis eagerly registered on the factory since it lives inisaaclab(not a backend package likeisaaclab_physx), soFactoryBase's dynamic import (isaaclab_{backend}.sim.views) can't discover it.Changes
FrameView._get_backend(): checks/physics/fabricEnabledsetting + device support, returns"physx","usd", or"newton"FabricFrameView: stripped of all_use_fabricconditionals,SettingsManagerimport,_fabric_supported_devicesconstant — it just does Fabric, nothing elseUsdFrameView: eagerly registered as"usd"backend on the factoryResult
Each FrameView implementation now has a single responsibility.
FabricFrameViewassumes Fabric is available (the factory guarantees this). No runtime behavior change for existing callers — the same code paths execute, just dispatched from a cleaner location.Merge Order
This PR is part of a stacked series for Fabric-accelerated local poses:
this depends on