Refactor teleop factory

[rwiltz]

Description

Refactors the teleop factory to shift declaration of teleop devices and retargeters out of the factory and into themselves.

Fixes # (issue)

Type of change

• New feature (non-breaking change which adds functionality)

Screenshots

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[greptile-apps]

Greptile Overview

Greptile Summary

Refactors the teleop device factory to use a self-declaring config pattern, eliminating hardcoded DEVICE_MAP and RETARGETER_MAP dictionaries from the factory.

Key changes:

• Added device_type field to DeviceCfg base class and retargeter_type field to RetargeterCfg base class
• Each device and retargeter config class now declares its own implementation type (e.g., Se2KeyboardCfg.device_type = Se2Keyboard)
• Factory now reads these fields via getattr() instead of looking up in hardcoded maps
• Config classes moved after their implementations to enable forward references without circular imports
• Documentation updated to reflect new registration pattern

Benefits:

• More maintainable - new devices/retargeters self-register without modifying factory code
• Better encapsulation - each module owns its config-to-implementation binding
• Follows dependency inversion principle - factory depends on abstractions, not concrete mappings

Test issue found:

• test_create_teleop_device_unsupported_config expects old error message that no longer matches new implementation

Confidence Score: 4/5

• Safe to merge after fixing the test error message expectation
• The refactoring is well-structured and follows good design patterns. All device and retargeter implementations were correctly updated with the self-declaring pattern. However, one test needs updating to match the new error message format, preventing a score of 5.
• source/isaaclab/test/devices/test_device_constructors.py - test expects wrong error message (line 460)

Important Files Changed

File Analysis

Filename	Score	Overview
source/isaaclab/isaaclab/devices/teleop_device_factory.py	5/5	Refactored to use self-declaring device_type and retargeter_type from configs, eliminating hardcoded mappings
source/isaaclab/isaaclab/devices/device_base.py	5/5	Added device_type field to DeviceCfg and teleoperation_active_default field with documentation comments
source/isaaclab/isaaclab/devices/retargeter_base.py	5/5	Added retargeter_type field to RetargeterCfg for self-declaring retargeter implementations
docs/source/how-to/cloudxr_teleoperation.rst	5/5	Updated documentation to reflect new self-declaring config pattern instead of manual RETARGETER_MAP registration

Sequence Diagram

sequenceDiagram
    participant User
    participant Factory as create_teleop_device()
    participant DeviceCfg
    participant Device as DeviceBase
    participant RetargeterCfg
    participant Retargeter as RetargeterBase
    
    User->>Factory: create_teleop_device(name, devices_cfg)
    Factory->>DeviceCfg: Get cfg from devices_cfg[name]
    Factory->>DeviceCfg: Read device_type attribute
    DeviceCfg-->>Factory: Return device class constructor
    
    alt Has retargeters configured
        loop For each retargeter_cfg
            Factory->>RetargeterCfg: Read retargeter_type attribute
            RetargeterCfg-->>Factory: Return retargeter class constructor
            Factory->>Retargeter: Instantiate retargeter(cfg)
            Retargeter-->>Factory: Return retargeter instance
        end
    end
    
    Factory->>Device: Instantiate device(cfg, retargeters)
    Device-->>Factory: Return device instance
    
    Factory->>Device: Register callbacks (if provided)
    Factory-->>User: Return configured device
    
    Note over DeviceCfg,RetargeterCfg: Self-declaring pattern:<br/>Each config class sets its own<br/>device_type/retargeter_type field

Loading

Additional Comments (1)

1. source/isaaclab/test/devices/test_device_constructors.py, line 460 (link)

   syntax: test will fail - expected error message changed from "Unsupported device configuration type" to "does not declare device_type"

_{22 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Overview

Greptile Summary

This PR refactors the teleoperation device factory to use a self-referential pattern where config classes declare their associated implementation classes, eliminating centralized mapping dictionaries.

Key changes:

• Removed DEVICE_MAP and RETARGETER_MAP from factory - these hardcoded dictionaries required factory updates for each new device/retargeter
• Added device_type field to DeviceCfg and retargeter_type field to RetargeterCfg base classes
• Each device/retargeter config now sets its own type attribute (e.g., Se3KeyboardCfg.device_type = Se3Keyboard)
• Factory now uses getattr() to dynamically resolve constructors from config instances
• Moved config class definitions after implementation classes to enable forward references
• Updated documentation showing new registration pattern
• Deleted stub dex_retargeter.py file that was incomplete
• All tests updated and passing

Benefits:

• Better encapsulation - devices/retargeters self-register rather than requiring factory updates
• Easier extensibility - new devices/retargeters don't need factory modifications
• Follows dependency inversion principle - factory depends on abstractions, not concrete mappings

Confidence Score: 5/5

• This PR is safe to merge - it's a clean architectural refactoring with comprehensive test coverage
• The refactoring is well-executed with consistent application across all device and retargeter implementations. Tests verify the new pattern works correctly, documentation is updated, and the change is backward-compatible in behavior while improving maintainability. The pattern is simple and eliminates a common source of errors (forgetting to update factory mappings).
• No files require special attention - all changes follow the same consistent pattern

Important Files Changed

File Analysis

Filename	Score	Overview
source/isaaclab/isaaclab/devices/teleop_device_factory.py	5/5	Removed hardcoded DEVICE_MAP and RETARGETER_MAP dictionaries; factory now uses device_type and retargeter_type attributes from config classes for dynamic resolution
source/isaaclab/isaaclab/devices/device_base.py	5/5	Added device_type and teleoperation_active_default fields to DeviceCfg for self-referential device construction
source/isaaclab/isaaclab/devices/retargeter_base.py	5/5	Added retargeter_type field to RetargeterCfg for self-referential retargeter construction
source/isaaclab/isaaclab/devices/keyboard/se3_keyboard.py	5/5	Moved Se3KeyboardCfg after class definition and set device_type = Se3Keyboard
source/isaaclab/isaaclab/devices/openxr/retargeters/manipulator/se3_abs_retargeter.py	5/5	Moved Se3AbsRetargeterCfg after class definition and set retargeter_type = Se3AbsRetargeter
source/isaaclab/test/devices/test_device_constructors.py	5/5	Tests updated to verify factory creates devices correctly using new self-referential config pattern

Sequence Diagram

sequenceDiagram
    participant User
    participant Factory as create_teleop_device()
    participant DeviceCfg
    participant Device as DeviceBase subclass
    participant RetargeterCfg
    participant Retargeter as RetargeterBase subclass
    
    User->>Factory: create_teleop_device(device_name, devices_cfg)
    Factory->>DeviceCfg: Get device_cfg by name
    Factory->>DeviceCfg: Read device_type attribute
    Note over DeviceCfg: device_type = Se3Keyboard<br/>(set in device module)
    
    alt Has retargeters configured
        Factory->>DeviceCfg: Get retargeters list
        loop For each retargeter_cfg
            Factory->>RetargeterCfg: Read retargeter_type attribute
            Note over RetargeterCfg: retargeter_type = Se3AbsRetargeter<br/>(set in retargeter module)
            Factory->>Retargeter: Construct retargeter(cfg)
            Retargeter-->>Factory: Retargeter instance
        end
    end
    
    Factory->>Device: Construct device(cfg, retargeters)
    Device-->>Factory: Device instance
    Factory->>Device: add_callback() for each callback
    Factory-->>User: Configured device instance

Loading

_{23 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[rwiltz]

@greptileai

[greptile-apps]

Greptile Overview

Greptile Summary

This PR successfully refactors the teleop device factory to use a self-referential pattern where device and retargeter configurations declare their own types, eliminating the need for centralized mapping dictionaries.

Key Changes:

• Added device_type field to DeviceCfg and retargeter_type field to RetargeterCfg base classes
• Removed DEVICE_MAP and RETARGETER_MAP dictionaries from the factory (eliminated ~50 lines of hardcoded mappings)
• Updated factory to introspect config objects for their type references using getattr()
• All device and retargeter modules now set their type reference in their config classes (e.g., device_type: type[DeviceBase] = Se3Keyboard)
• Standardized code organization by moving config classes to bottom of files after class definitions
• Added __future__ annotations imports to support forward references in type hints
• Updated tests to validate new factory behavior with proper error messages
• Deleted unused stub file dex_retargeter.py

Benefits:

• Better encapsulation - each module declares its own type
• Easier extensibility - new devices/retargeters don't require factory modifications
• Reduced coupling between factory and device implementations
• More maintainable codebase with clearer ownership

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The refactoring is well-executed with consistent patterns across all 20+ files. Tests have been updated to validate the new behavior, the changes are purely structural without altering runtime behavior, and the PR follows best practices with proper documentation updates and changelog entries.
• No files require special attention

Important Files Changed

File Analysis

Filename	Score	Overview
source/isaaclab/isaaclab/devices/teleop_device_factory.py	5/5	Refactored factory to use self-referential device_type and retargeter_type attributes from configs instead of hardcoded maps. Removed DEVICE_MAP and RETARGETER_MAP dictionaries.
source/isaaclab/isaaclab/devices/device_base.py	5/5	Added device_type field to DeviceCfg to enable self-referential device construction. Also added teleoperation_active_default field.
source/isaaclab/isaaclab/devices/retargeter_base.py	5/5	Added retargeter_type field to RetargeterCfg to enable self-referential retargeter construction.
source/isaaclab/isaaclab/devices/keyboard/se3_keyboard.py	5/5	Moved config class to bottom of file and set device_type = Se3Keyboard. Added future annotations import.
source/isaaclab/isaaclab/devices/openxr/retargeters/manipulator/se3_abs_retargeter.py	5/5	Moved config class to bottom of file and set retargeter_type = Se3AbsRetargeter. Added future annotations import.
source/isaaclab/test/devices/test_device_constructors.py	5/5	Updated tests to match new factory behavior with device_type validation. Improved test assertions and type annotations.

Sequence Diagram

sequenceDiagram
    participant User
    participant Factory as create_teleop_device
    participant DeviceCfg
    participant RetargeterCfg
    participant Device as DeviceBase subclass
    participant Retargeter as RetargeterBase subclass

    User->>Factory: create_teleop_device(device_name, devices_cfg)
    Factory->>DeviceCfg: getattr(device_cfg, "device_type")
    DeviceCfg-->>Factory: Return device class reference
    Factory->>Factory: Validate device_type is DeviceBase subclass
    
    alt Has retargeters configured
        loop For each retargeter_cfg
            Factory->>RetargeterCfg: getattr(retargeter_cfg, "retargeter_type")
            RetargeterCfg-->>Factory: Return retargeter class reference
            Factory->>Factory: Validate retargeter_type is RetargeterBase subclass
            Factory->>Retargeter: retargeter_constructor(retargeter_cfg)
            Retargeter-->>Factory: Return retargeter instance
        end
    end
    
    Factory->>Factory: Build constructor kwargs (cfg + retargeters if supported)
    Factory->>Device: device_constructor(**params)
    Device-->>Factory: Return device instance
    
    loop For each callback
        Factory->>Device: add_callback(key, callback)
    end
    
    Factory-->>User: Return configured device

Loading

_{23 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[hougantc-nvda]

LGTM

[greptile-apps]

Greptile Overview

Greptile Summary

This PR refactors the teleop device factory to use a self-registration pattern instead of centralized mapping dictionaries. Each device and retargeter configuration class now declares its own device_type or retargeter_type field, eliminating the need for DEVICE_MAP and RETARGETER_MAP in the factory.

Key Changes:

• Removed hardcoded DEVICE_MAP and RETARGETER_MAP from factory (eliminated ~50 lines of imports/mappings)
• Added device_type: type[DeviceBase] | None = None to DeviceCfg base class
• Added retargeter_type: type[RetargeterBase] | None = None to RetargeterCfg base class
• All 8 device config classes now set their device_type field (Se2/Se3Keyboard, Se2/Se3Gamepad, Se2/Se3SpaceMouse, OpenXRDevice, ManusVive)
• All 7 retargeter config classes now set their retargeter_type field
• Moved config class definitions after implementation classes in all files (required for forward reference resolution)
• Added from __future__ import annotations to 16 files to support forward references in type hints
• Factory now uses getattr(cfg, "device_type") and validates it's a proper DeviceBase/RetargeterBase subclass
• Comprehensive test coverage added with error case validation
• Documentation updated to show new registration pattern

Benefits:

• Better encapsulation - each device/retargeter declares itself
• Eliminates central bottleneck for adding new devices
• Reduces import dependencies in factory module
• More discoverable - config and implementation are co-located

Potential Impact:

• This is a breaking change for any external code that directly accessed DEVICE_MAP or RETARGETER_MAP
• Custom devices/retargeters must now set the device_type/retargeter_type field instead of modifying the factory maps

Confidence Score: 5/5

• This PR is safe to merge - well-architected refactoring with excellent test coverage
• The refactoring is systematic and complete across all devices/retargeters, with comprehensive test coverage including error cases. The pattern is consistent, uses proper type validation, and maintains backward compatibility for end users (only internal factory implementation changed).
• No files require special attention

Important Files Changed

File Analysis

Filename	Score	Overview
source/isaaclab/isaaclab/devices/teleop_device_factory.py	5/5	Refactored factory to use device_type and retargeter_type from configs instead of hardcoded maps - eliminates 50+ lines of import/mapping code
source/isaaclab/isaaclab/devices/device_base.py	5/5	Added device_type field to DeviceCfg for self-registration pattern, plus teleoperation_active_default field
source/isaaclab/isaaclab/devices/retargeter_base.py	5/5	Added retargeter_type field to RetargeterCfg for self-registration pattern
source/isaaclab/test/devices/test_device_constructors.py	5/5	Added comprehensive tests for factory pattern with devices, retargeters, and callbacks - tests error cases for missing device_type

Sequence Diagram

sequenceDiagram
    participant User
    participant Factory as create_teleop_device()
    participant DeviceCfg
    participant Device as DeviceBase subclass
    participant RetargeterCfg
    participant Retargeter as RetargeterBase subclass
    
    User->>Factory: create_teleop_device("device_name", devices_cfg)
    Factory->>DeviceCfg: getattr(cfg, "device_type")
    DeviceCfg-->>Factory: Return device class (e.g., Se3Keyboard)
    Factory->>Factory: Validate is DeviceBase subclass
    
    alt Has retargeters
        loop For each retargeter_cfg
            Factory->>RetargeterCfg: getattr(cfg, "retargeter_type")
            RetargeterCfg-->>Factory: Return retargeter class
            Factory->>Factory: Validate is RetargeterBase subclass
            Factory->>Retargeter: Instantiate retargeter_class(cfg)
            Retargeter-->>Factory: Return retargeter instance
        end
    end
    
    Factory->>Factory: Build constructor params
    Factory->>Device: device_type(cfg=cfg, retargeters=retargeters)
    Device-->>Factory: Return device instance
    
    Factory->>Device: Register callbacks
    Factory-->>User: Return configured device

Loading

_{23 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[peterd-NV]

Looks good, thanks for the refactor