Skip to content

refactor: move Fabric/USD dispatch from FabricFrameView to FrameView factory#5673

Draft
pv-nvidia wants to merge 3 commits into
isaac-sim:developfrom
pv-nvidia:pv/fabric-view-no-fallback
Draft

refactor: move Fabric/USD dispatch from FabricFrameView to FrameView factory#5673
pv-nvidia wants to merge 3 commits into
isaac-sim:developfrom
pv-nvidia:pv/fabric-view-no-fallback

Conversation

@pv-nvidia
Copy link
Copy Markdown

@pv-nvidia pv-nvidia commented May 18, 2026

Problem

FabricFrameView has an internal _use_fabric flag that falls back to UsdFrameView when 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 the FrameView factory's _get_backend() method. The factory now dispatches to three implementations:

Condition Backend key Class
PhysX + Fabric enabled + supported device physx FabricFrameView
PhysX without Fabric or unsupported device usd UsdFrameView
Newton newton NewtonSiteFrameView

UsdFrameView is eagerly registered on the factory since it lives in isaaclab (not a backend package like isaaclab_physx), so FactoryBase's dynamic import (isaaclab_{backend}.sim.views) can't discover it.

Changes

  • FrameView._get_backend(): checks /physics/fabricEnabled setting + device support, returns "physx", "usd", or "newton"
  • FabricFrameView: stripped of all _use_fabric conditionals, SettingsManager import, _fabric_supported_devices constant — it just does Fabric, nothing else
  • UsdFrameView: eagerly registered as "usd" backend on the factory

Result

Each FrameView implementation now has a single responsibility. FabricFrameView assumes 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:

  1. feat: add typed service locator to SimulationContext #5672 — service locator ← this depends on
  2. feat: add FabricStageCache service for shared hierarchy handles #5676 — FabricStageCache
  3. feat: add indexed fabric transform kernels for local/world matrix propagation #5675 — indexed fabric kernels
  4. refactor: move Fabric/USD dispatch from FabricFrameView to FrameView factory #5673 — factory dispatch ← this PR (depends on feat: add typed service locator to SimulationContext #5672)
  5. refactor: fuse set_world_poses/set_scales into single _compose_fabric_transform #5674 — fused compose
  6. PR C+D — local poses (depends on all above)

pv-nvidia added 3 commits May 18, 2026 17:06
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.
@pv-nvidia pv-nvidia force-pushed the pv/fabric-view-no-fallback branch from b51594f to 4c71366 Compare May 18, 2026 17:10
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 596bedb - Add typed ServiceLocator to SimulationContext
  2. dac5a16 - Add unit tests and changelog fragment
  3. 4c71366 - Move Fabric/USD dispatch from FabricFrameView to FrameView factory

✅ What's Good

  1. 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
  2. Improved Separation of Concerns

    • Factory pattern in FrameView now handles backend selection logic
    • FabricFrameView is now simpler - only handles Fabric operations
    • Removed conditional checks inside FabricFrameView methods
  3. Comprehensive Tests

    • test_service_locator.py covers edge cases (non-callable close, missing keys, etc.)
    • Good coverage of the service locator API
  4. Proper Integration

    • SimulationContext.clear_instance() now calls services.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 missing

Returning 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant