Add core Arena environment factory

[cvolkcvolk]

Summary

Move the shared environment construction class ExampleEnvironmentBase into core (as ArenaEnvironmentFactory) because it applies to every Arena environment, not just examples. ArenaEnvironmentFactory now handles registry setup and provides the typed build(cfg) entry point.

[greptile-apps]

Greptile Summary

This PR extracts the core factory infrastructure from ExampleEnvironmentBase into a new ArenaEnvironmentFactoryBase in the isaaclab_arena core package, and updates all 17 registered environments to inherit from it directly. ExampleEnvironmentBase is retained as a deprecated compatibility shim for external argparse-based factories.

• New core base: ArenaEnvironmentFactoryBase in isaaclab_arena/environments/arena_environment_factory.py owns registry initialization (AssetRegistry, DeviceRegistry, HDRImageRegistry) and declares the single abstract build(cfg) entrypoint; all environment files are mechanically updated to import and extend this class instead of the examples-package shim.
• Compatibility shim: ExampleEnvironmentBase now inherits from ArenaEnvironmentFactoryBase, overrides build() to raise NotImplementedError, and retains the abstract get_env/add_cli_args contract for legacy adapters — though it has no runtime deprecation warning.
• Test update: test_environment_cfgs.py drops the __orig_bases__ reflection helper and switches type annotations to ArenaEnvironmentFactoryBase, but the loop still calls add_cli_args and get_env on every registered environment, which will break for any future build()-only factory.

Confidence Score: 4/5

Safe to merge for the current environment set; the structural migration is clean, but the test will silently break for the first new environment that implements only build() without legacy adapters.

All 17 environment parent-class swaps are mechanical and correct, and ArenaEnvironmentFactoryBase itself is a straightforward extraction with no logic changes. The one concrete defect is in test_environment_cfgs.py: the loop over all registered environments unconditionally calls add_cli_args and get_env, which are no longer part of the base contract.

isaaclab_arena/tests/test_environment_cfgs.py needs a guard so it only exercises environments that expose the legacy adapter methods.

Important Files Changed

Filename	Overview
isaaclab_arena/environments/arena_environment_factory.py	New core base class ArenaEnvironmentFactoryBase extracted from the examples package; clean ABC with registry initialization in __init__ and a single abstract build(cfg) method.
isaaclab_arena_environments/example_environment_base.py	Reduced to a thin compatibility shim over ArenaEnvironmentFactoryBase; build() raises NotImplementedError for legacy adapters, but there is no runtime deprecation warning when new subclasses are created.
isaaclab_arena/tests/test_environment_cfgs.py	Updated to import ArenaEnvironmentFactoryBase instead of ExampleEnvironmentBase, but the loop unconditionally calls add_cli_args and get_env on every registered environment — methods not required by the new base class, causing AttributeError for any future build()-only registration.
isaaclab_arena_environments/pick_and_place_maple_table_environment.py	Parent class updated to ArenaEnvironmentFactoryBase; retains full get_env/add_cli_args/build implementation unchanged.
isaaclab_arena_environments/galileo_g1_locomanip_pick_and_place_environment.py	Mechanical parent-class swap from ExampleEnvironmentBase to ArenaEnvironmentFactoryBase; no logic changes.

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class ArenaEnvironmentFactoryBase {
        <<abstract, Generic[CfgT]>>
        +name: str | None
        +asset_registry: AssetRegistry
        +device_registry: DeviceRegistry
        +hdr_registry: HDRImageRegistry
        +__init__()
        +build(cfg: CfgT) IsaacLabArenaEnvironment*
    }

    class ExampleEnvironmentBase {
        <<deprecated shim, Generic[CfgT]>>
        +get_env(args_cli: Namespace) IsaacLabArenaEnvironment*
        +add_cli_args(parser: ArgumentParser) None*
        +build(cfg: CfgT) raises NotImplementedError
    }

    class PickAndPlaceMapleTableEnvironment {
        +name = "pick_and_place_maple_table"
        +get_env(args_cli) IsaacLabArenaEnvironment
        +build(cfg) IsaacLabArenaEnvironment
        +add_cli_args(parser) None
    }

    class OtherMigratedEnvironments {
        +get_env(args_cli) IsaacLabArenaEnvironment
        +build(cfg) IsaacLabArenaEnvironment
        +add_cli_args(parser) None
    }

    class FutureDirectFactories {
        +build(cfg) IsaacLabArenaEnvironment
    }

    ArenaEnvironmentFactoryBase <|-- ExampleEnvironmentBase : deprecated shim
    ArenaEnvironmentFactoryBase <|-- PickAndPlaceMapleTableEnvironment
    ArenaEnvironmentFactoryBase <|-- OtherMigratedEnvironments
    ArenaEnvironmentFactoryBase <|-- FutureDirectFactories : intended new pattern

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
classDiagram
    class ArenaEnvironmentFactoryBase {
        <<abstract, Generic[CfgT]>>
        +name: str | None
        +asset_registry: AssetRegistry
        +device_registry: DeviceRegistry
        +hdr_registry: HDRImageRegistry
        +__init__()
        +build(cfg: CfgT) IsaacLabArenaEnvironment*
    }

    class ExampleEnvironmentBase {
        <<deprecated shim, Generic[CfgT]>>
        +get_env(args_cli: Namespace) IsaacLabArenaEnvironment*
        +add_cli_args(parser: ArgumentParser) None*
        +build(cfg: CfgT) raises NotImplementedError
    }

    class PickAndPlaceMapleTableEnvironment {
        +name = "pick_and_place_maple_table"
        +get_env(args_cli) IsaacLabArenaEnvironment
        +build(cfg) IsaacLabArenaEnvironment
        +add_cli_args(parser) None
    }

    class OtherMigratedEnvironments {
        +get_env(args_cli) IsaacLabArenaEnvironment
        +build(cfg) IsaacLabArenaEnvironment
        +add_cli_args(parser) None
    }

    class FutureDirectFactories {
        +build(cfg) IsaacLabArenaEnvironment
    }

    ArenaEnvironmentFactoryBase <|-- ExampleEnvironmentBase : deprecated shim
    ArenaEnvironmentFactoryBase <|-- PickAndPlaceMapleTableEnvironment
    ArenaEnvironmentFactoryBase <|-- OtherMigratedEnvironments
    ArenaEnvironmentFactoryBase <|-- FutureDirectFactories : intended new pattern

Loading

Comments Outside Diff (1)

1. isaaclab_arena/tests/test_environment_cfgs.py, line 63-83 (link)

   Test will AttributeError on the first build()-only environment

   _parse_legacy_arguments calls environment_type.add_cli_args(parser) (line 30) and _capture_typed_cfg calls factory.get_env(legacy_arguments) (line 57) for every entry in EnvironmentRegistry. ArenaEnvironmentFactoryBase only requires build(), so the first environment that implements the new factory pattern without a legacy adapter will raise AttributeError here — exactly the case this PR is meant to enable. The test should skip (or separately collect) environments that don't expose add_cli_args/get_env, for example with if not hasattr(environment_type, "get_env"): continue.

_{Reviews (1): Last reviewed commit: "Co-locate environment factory contracts" | Re-trigger Greptile}

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

This PR promotes the core factory behavior (build(cfg) + registry initialization) out of the examples package into a new core base, ArenaEnvironmentFactoryBase in isaaclab_arena/environments/, re-parents all 20 first-party environments onto it, and keeps ExampleEnvironmentBase as a thin argparse-compat shim for external factories. The direction is sound — the generic build contract now lives in core while robot/task-specific factories stay in the extension package — and the mechanical migration looks correct and consistent. Copyright year on the new file (2026) is right for a newly added file.

Design, Boundaries & Scope

One thing worth a look: the first-party environments still expose get_env() / add_cli_args(), and cli.py calls both on every registered environment — but after this migration those methods are no longer declared on any shared base (only build is abstract on ArenaEnvironmentFactoryBase; the argparse surface lives on the now-bypassed ExampleEnvironmentBase). So the interface the CLI depends on is enforced only by test_every_registered_legacy_adapter_translates_cli_defaults_to_its_typed_cfg, not by the type system. That test does cover it, so this is not a bug today — but is it worth keeping the CLI adapter surface expressed as a shared contract (Protocol/ABC) until the CLI itself moves to build(cfg), so a new env that forgets add_cli_args fails at definition rather than only in that one test?

Findings

🔵 Improvement: isaaclab_arena/tests/test_environment_cfgs.py:75 — The old test verified get_env() builds the exact config type declared in the generic parameter; cfg_type = type(cfg) drops that declared-vs-built check (see inline). Restoring it against ArenaEnvironmentFactoryBase is a one-liner if the loss was unintentional.

Test Coverage

The refactor is exercised by the existing test_environment_cfgs.py suite (Phase 1, no sim), which round-trips every registered adapter’s CLI defaults through get_env()→build(). No new sim behavior is introduced, so no inner/outer sim test is needed. The only gap is the narrowed assertion noted above.

Verdict

Ship it — clean refactor; the two notes above are optional polish, not blockers.

[alexmillane]

LGTM. One tiny comment.