Address SQA bug: camera support from Experiment config.#953
Conversation
Read enable_cameras from the Experiment before SimulationApp starts so experiment_runner no longer requires --enable_cameras on the CLI, and move enable_cameras onto ArenaEnvironmentCfg so every environment declares it once. Signed-off-by: alex <amillane@nvidia.com>
| environment_values = run_values.get("environment") | ||
| if not isinstance(environment_values, dict): | ||
| return False | ||
| enable_cameras = environment_values.get("enable_cameras", False) |
There was a problem hiding this comment.
🔵 Reader assumes the base default stays False
This defaults an absent enable_cameras to False, which only matches composition because ArenaEnvironmentCfg.enable_cameras is False and no environment overrides it. If some environment ever defaults cameras on, this pre-startup read would disagree with the composed config and trip _assert_camera_support_enabled for a perfectly valid Experiment. Worth a short NOTE: here tying this default to the cfg default so a future maintainer keeps the two in sync?
Greptile SummaryThe PR derives Isaac Sim camera startup support from typed Experiment YAML and Hydra overrides.
Confidence Score: 3/5The unsupported embodiment-constructor path should be fixed before merging because selecting The main pre-startup camera detection is internally consistent, but the new unconditional constructor keyword breaks a currently registered embodiment on two configurable environment-building paths. Files Needing Attention: isaaclab_arena_environments/lift_object_environment.py, isaaclab_arena_environments/press_button_environment.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Experiment YAML and Hydra overrides] --> B[Pre-startup camera requirement read]
B --> C[SimulationAppContext]
C --> D[Compose typed Experiment]
D --> E[Build selected environment]
E --> F[Instantiate embodiment with enable_cameras]
Reviews (1): Last reviewed commit: "Address SQA bug: derive camera support f..." | Re-trigger Greptile |
| embodiment = self.asset_registry.get_asset_by_name(cfg.embodiment)( | ||
| enable_cameras=cfg.enable_cameras, concatenate_observation_terms=True | ||
| ) |
There was a problem hiding this comment.
Unsupported embodiment constructor keyword
When either lift_object or press_button selects the registered kuka_allegro embodiment, the factory unconditionally passes enable_cameras, but KukaAllegroEmbodiment.__init__ does not accept that keyword, causing environment construction to fail with TypeError even when cameras are disabled.
Knowledge Base Used: Environment builder flow
🤖 Isaac Lab-Arena Review BotSummaryThis fixes the SQA bug where shipped Experiment configs failed unless Findings🔵 Improvement — Test CoverageStrong. The new tests are pure-Python config-loading tests (they monkeypatch VerdictShip it |
Summary
Address issue that shipped Experiment configs failed unless
--enable_cameraswas also passed on the CLI.Detailed description
environment.enable_cameras: true, but AppLauncher needs camera support beforeSimulationAppstarts and typed YAML was only composed afterwards, so the runner asserted and told the user to repeat the flag on the CLI.typed_experiment_requires_camerasnow reads the YAML envelope and Hydra overrides (via Hydra's ownOverridesParser) without importing environment modules, soexperiment_runnerderives camera support from the config for typed and legacy Experiments alike. The former user-facing assert is now an internal invariant check.enable_camerasmoved ontoArenaEnvironmentCfginstead of being redeclared by 15 environment configs;lift_objectandpress_buttonnow honor it anddexsuite_liftrejects it, sinceKukaAllegroEmbodimenthas no camera configuration.--enable_camerasalongside--experiment_config, and Experiment YAML envelope errors now surface before Isaac Sim starts rather than after.