Let run-time variations apply build-time effects by alexmillane · Pull Request #907 · isaac-sim/IsaacLab-Arena

alexmillane · 2026-07-14T12:48:46Z

Summary

Allow any variation (not just build-time ones) to perform build-time mutations.

Detail

To support some run-time variations, sometimes you need to set something up in the EnvironmentCfg at build time.
This makes this possible.
Rename the BuildTimeVariationBase abstract apply() to apply_build_time_effects().

Notes

This is added to support forthcoming support for camera intrinsic variations.

- Add VariationBase.apply_build_time_effects (default no-op) and have the env builder call it on every enabled variation, so a run-time variation can also mutate asset configs before the scene is built. - Rename the BuildTimeVariationBase abstract apply() to apply_build_time_effects and update the existing build-time variations and tests accordingly. Signed-off-by: alex <amillane@nvidia.com>

arena-review-bot · 2026-07-15T12:13:31Z

-                if not isinstance(variation, BuildTimeVariationBase):
-                    continue
-                variation.apply()
+                variation.apply_build_time_effects()


🟡 New build-time hook for run-time variations has no test

The tests here still only exercise BuildTimeVariationBase, which was already applied at build time before this change. Nothing covers the actual new behavior: that a RunTimeVariationBase whose apply_build_time_effects is overridden gets called during compose_manager_cfg. Could we add a small test — a run-time variation that mutates a config in apply_build_time_effects, enable it, build, and assert the mutation took effect? That would guard the mechanism MR3 depends on and catch a future regression if this loop ever re-gains a type filter.

arena-review-bot · 2026-07-15T12:13:44Z

🤖 Isaac Lab-Arena Review Bot

Summary

This PR turns the build-time apply() hook into apply_build_time_effects(), adds a default no-op on VariationBase, and drops the isinstance(BuildTimeVariationBase) filter in the builder so any enabled variation can mutate asset configs before the scene is materialised. Clean, minimal, and the default path is preserved — run-time variations inherit the no-op, so existing behavior is unchanged. Nice use of re-declaring apply_build_time_effects as @abstractmethod on BuildTimeVariationBase to keep the "must implement" contract while giving run-time variations an opt-in override.

Findings

🟡 Warning: arena_env_builder.py:160 — The one genuinely new behavior (a run-time variation getting apply_build_time_effects called at build time) has no test; the existing tests only cover BuildTimeVariationBase, which was already applied here before. A small test with a run-time variation that mutates a config in apply_build_time_effects would guard the mechanism MR3 relies on.

Test Coverage

Existing build-time tests were correctly updated to the new method name and still pass. Missing: direct coverage of the new run-time → build-time path (see the warning above). Not a blocker given this is the mechanism-only MR of a three-part stack, but worth adding here rather than deferring.

Verdict

Ship it (consider adding the run-time-path test).

alexmillane

self review 2

alexmillane · 2026-07-15T12:12:31Z

+        Calls ``apply_build_time_effects`` on each enabled variation regardless of type: a
+        :class:`BuildTimeVariationBase` realises its whole effect there (e.g. a dome light's
+        spawner texture), while a run-time variation uses it for build-time preconditions (e.g.
+        forcing its camera untiled). These mutate asset configs in place, so this must run before
+        the scene is materialised.


Restore the details section of the docstring to what it was.

alexmillane · 2026-07-15T12:13:29Z

+        Called once per env build for every enabled variation, regardless of type. Default:
+        no-op. A :class:`BuildTimeVariationBase` realises its whole effect here; a run-time
+        variation overrides this only when it needs a build-time precondition (e.g. forcing a
+        camera untiled so per-env run-time edits take effect).


remove: regardless of type

remove :class: markings.

remove "(e.g. forcing a...)

alexmillane · 2026-07-15T12:13:48Z

    Use for properties that can't change in-flight: HDR maps, USD swaps,
    spawner params baked into a config. Subclasses hold references to the
-    asset(s) they mutate.
+    asset(s) they mutate and realise the effect in :meth:`apply_build_time_effects`.


remove :meth: marker.

greptile-apps · 2026-07-15T12:14:26Z

Greptile Summary

This PR lifts the apply_build_time_effects() hook from BuildTimeVariationBase up to VariationBase as a concrete no-op, so any variation — including run-time ones — can express build-time preconditions without changing the call-site logic in ArenaEnvBuilder. The corresponding isinstance filter in _apply_build_time_variations is removed, making every enabled variation eligible for the hook.

VariationBase gains a concrete no-op apply_build_time_effects(); BuildTimeVariationBase re-declares it as @abstractmethod, preserving the existing contract for its subclasses.
The three existing build-time variations (HDRImageVariation, LightDirectionVariation, LightIntensityVariation) and both affected test files are updated from the old apply() name to apply_build_time_effects().
No leftover .apply() callers remain in the codebase, and the BuildTimeVariationBase import is correctly removed from arena_env_builder.py (it is still needed and present elsewhere).

Confidence Score: 5/5

Safe to merge. The change is a clean upward refactor of a single hook method, with no logic mutations in existing build-time variations and a well-scoped no-op default for run-time variations.

The rename from apply() to apply_build_time_effects() is complete — no remaining .apply() callers exist anywhere in the codebase. The isinstance guard removal is correct: RunTimeVariationBase subclasses inherit the no-op from VariationBase, and BuildTimeVariationBase re-declares the method as @AbstractMethod, preserving the enforcement on its subclasses. The BuildTimeVariationBase import removal from arena_env_builder.py is correct (still imported where needed in variations_printing.py). All three variation files and both test files are updated consistently.

No files require special attention. All changed files are internally consistent and the codebase-wide grep confirms no stale .apply() callers remain.

Important Files Changed

Filename	Overview
isaaclab_arena/variations/variation_base.py	Adds a concrete no-op apply_build_time_effects() to VariationBase; BuildTimeVariationBase re-marks it @AbstractMethod, preserving the existing enforcement on its subclasses. Mechanics are correct.
isaaclab_arena/environments/arena_env_builder.py	Removes the BuildTimeVariationBase import (no longer needed) and the isinstance guard; now calls apply_build_time_effects() on every enabled variation. RunTimeVariationBase import is still used in _compose_variations_event_cfg.
isaaclab_arena/variations/hdr_image_variation.py	Renames apply() to apply_build_time_effects(). Pure rename, no logic change.
isaaclab_arena/variations/light_direction_variation.py	Renames apply() to apply_build_time_effects(). Pure rename, no logic change.
isaaclab_arena/variations/light_intensity_variation.py	Renames apply() to apply_build_time_effects(). Pure rename, no logic change.
isaaclab_arena/tests/test_build_time_variations.py	Updates the test stub's apply() to apply_build_time_effects() consistently with the rename.
isaaclab_arena/tests/test_variation_recorder.py	Updates three call-sites from apply() to apply_build_time_effects() in the recorder test; logic is unchanged.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Builder as ArenaEnvBuilder
    participant BTV as BuildTimeVariationBase subclass
    participant RTV as RunTimeVariationBase subclass
    participant VB as VariationBase

    Builder->>Builder: _apply_build_time_variations()
    loop every enabled variation
        alt BuildTimeVariationBase subclass
            Builder->>BTV: apply_build_time_effects()
            Note right of BTV: @abstractmethod — samples and mutates asset config
        else RunTimeVariationBase subclass (no override)
            Builder->>RTV: apply_build_time_effects()
            RTV->>VB: (inherits no-op from VariationBase)
            Note right of VB: Returns None — nothing mutated
        else RunTimeVariationBase subclass (with override)
            Builder->>RTV: apply_build_time_effects()
            Note right of RTV: Sets build-time precondition (e.g. untile camera)
        end
    end
    Builder->>Builder: compose scene_cfg (materialise)

%%{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"}}}%%
sequenceDiagram
    participant Builder as ArenaEnvBuilder
    participant BTV as BuildTimeVariationBase subclass
    participant RTV as RunTimeVariationBase subclass
    participant VB as VariationBase

    Builder->>Builder: _apply_build_time_variations()
    loop every enabled variation
        alt BuildTimeVariationBase subclass
            Builder->>BTV: apply_build_time_effects()
            Note right of BTV: @abstractmethod — samples and mutates asset config
        else RunTimeVariationBase subclass (no override)
            Builder->>RTV: apply_build_time_effects()
            RTV->>VB: (inherits no-op from VariationBase)
            Note right of VB: Returns None — nothing mutated
        else RunTimeVariationBase subclass (with override)
            Builder->>RTV: apply_build_time_effects()
            Note right of RTV: Sets build-time precondition (e.g. untile camera)
        end
    end
    Builder->>Builder: compose scene_cfg (materialise)

_{Reviews (1): Last reviewed commit: "Merge branch 'main' into alex/feature/va..." | Re-trigger Greptile}

alexmillane

self review 3

alexmillane · 2026-07-15T14:21:11Z

            self._sampler.add_listener(listener)

+    def apply_build_time_effects(self) -> None:
+        """Sample and mutate the bound asset config(s) in place before the scene is materialised.


Update: Sample and mutate the bound assets in place in the ArenaEnvBuilder before the environment cfg is composed.

Signed-off-by: alex <amillane@nvidia.com>

The apply() method was not renamed to apply_build_time_effects() during the main merge, leaving LightColorVariation with an unimplemented abstract method so it could not be instantiated. This broke every test that builds a scene containing a light. Signed-off-by: alex <amillane@nvidia.com>

cvolkcvolk · 2026-07-16T08:12:13Z

        if self._sampler is not None:
            self._sampler.add_listener(listener)

+    def apply_build_time_effects(self) -> None:


Is "effect" a bit vaque here? I think we are doing build-time realization and preparation for later run-time realization here. So this function serves two different purposes: a BuildTimeVariationBase realizes its sampled variation, whereas a run-time variation only prepares configuration required by its later event.

Only a suggestion, could we make the distinction explicit while leaving the builder with one call with something like

class VariationBase(ABC): def prepare_for_build(self) -> None: """Configure prerequisites required before environment construction.""" pass def realize_at_build_time(self) -> None: """Realize this variation during environment construction.""" pass def configure_at_build_time(self) -> None: """Run this variation's build-time preparation and realization.""" self.prepare_for_build() self.realize_at_build_time()

BuildTimeVariationBase makes self.realize_at_build_time() mandatory:

class BuildTimeVariationBase(VariationBase): @abstractmethod def realize_at_build_time(self) -> None: """Sample and apply this variation to its target configuration.""" ...

A run-time variation overrides self.prepare_for_build() when necessary:

class CameraIntrinsicsRunTimeVariation(RunTimeVariationBase): def prepare_for_build(self) -> None: """Configure the camera for per-environment run-time updates.""" if self._camera_rig is not None: self._camera_rig.set_use_tiled_camera(False)

and the arena_env_builder.py retains one call:

variation.configure_at_build_time()

cvolkcvolk

Great!

alexmillane marked this pull request as ready for review July 15, 2026 12:11

alexmillane requested review from cvolkcvolk, peterd-NV, qianl-nv, viiik-inside, xyao-nv and zhx06 as code owners July 15, 2026 12:11

Merge branch 'main' into alex/feature/variation_build_time_effects

f95ec24

arena-review-bot Bot reviewed Jul 15, 2026

View reviewed changes

alexmillane commented Jul 15, 2026

View reviewed changes

Address self review.

54429e3

alexmillane mentioned this pull request Jul 15, 2026

DRAFT: Camera intrinsics variation #913

Draft

Merge branch 'main' into alex/feature/variation_build_time_effects

65afa59

alexmillane commented Jul 15, 2026

View reviewed changes

alexmillane added 2 commits July 15, 2026 16:23

Clarify apply_build_time_effects docstring summary

b3d8043

Signed-off-by: alex <amillane@nvidia.com>

cvolkcvolk reviewed Jul 16, 2026

View reviewed changes

cvolkcvolk approved these changes Jul 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Let run-time variations apply build-time effects#907

Let run-time variations apply build-time effects#907
alexmillane wants to merge 6 commits into
mainfrom
alex/feature/variation_build_time_effects

alexmillane commented Jul 14, 2026 •

edited

Loading

Uh oh!

arena-review-bot Bot Jul 15, 2026

Uh oh!

arena-review-bot Bot commented Jul 15, 2026

Uh oh!

alexmillane left a comment

Uh oh!

alexmillane Jul 15, 2026

Uh oh!

alexmillane Jul 15, 2026

Uh oh!

alexmillane Jul 15, 2026

Uh oh!

greptile-apps Bot commented Jul 15, 2026

Uh oh!

alexmillane left a comment

Uh oh!

alexmillane Jul 15, 2026

Uh oh!

cvolkcvolk Jul 16, 2026

Uh oh!

cvolkcvolk left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

alexmillane commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Detail

Notes

Uh oh!

arena-review-bot Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

arena-review-bot Bot commented Jul 15, 2026

🤖 Isaac Lab-Arena Review Bot

Summary

Findings

Test Coverage

Verdict

Uh oh!

alexmillane left a comment

Choose a reason for hiding this comment

Uh oh!

alexmillane Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

alexmillane Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

alexmillane Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented Jul 15, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

alexmillane left a comment

Choose a reason for hiding this comment

Uh oh!

alexmillane Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

cvolkcvolk Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

cvolkcvolk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexmillane commented Jul 14, 2026 •

edited

Loading