Light intensity variation.#775
Conversation
Introduce the variation framework: a VariationBase with build-time and run-time flavors, a SamplerBase abstraction with uniform and categorical samplers, and two concrete variations -- HDR dome-light image selection (build-time) and camera extrinsic decalibration (run-time). Variations attach to any Asset (scene objects or embodiments) and are collected by ArenaEnvBuilder, which applies build-time variations before scene composition and folds run-time variations into the event manager cfg. All variations default to disabled so existing envs are unchanged. The variations package exposes its API lazily (PEP 562 __getattr__): camera_decalibration pulls in torch and isaaclab.sensors, and importing that pair before the SimulationApp launches corrupts USD's Python bindings. Lazy exports keep importing the package -- or its lightweight base/sampler submodules -- safe at module-load time (e.g. pytest collection). Signed-off-by: alex <amillane@nvidia.com>
- Rename camera_decalibration.py -> camera_decalibration_variation.py - Drop package-level re-exports; import concrete classes from submodules - Remove sampler/variation listener plumbing (deferred to a follow-up MR) - Hoist a compulsory sampler_cfg field onto VariationBaseCfg and rename the per-variation sampler field to sampler_cfg - Replace UniformSampler.event_shape with an abstract SamplerBase.shape_per_sample - Drop the camera variation's unused mode field (all variations fire on reset) - Move attribute docs onto each member and trim docstrings to one line Signed-off-by: alex <amillane@nvidia.com>
Agents drove the container with bare `docker exec` (root), while users attach via run_docker.sh as their host user (su $(id -un)). Root-run commands left root-owned files in shared caches (/tmp/Assets, ~/.config) that the host user could not read, breaking their later interactive runs. Update AGENTS.md and the dev-container/run-tests skills to route every in-container command through `su $(id -un) -c`, mirroring run_docker.sh. Signed-off-by: alex <amillane@nvidia.com>
…tion_hydra_config
alexmillane
requested review from
cvolkcvolk,
peterd-NV,
qianl-nv,
viiik-inside,
xyao-nv and
zhx06
as code owners
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge with the in-place mutation issue addressed; the test suite uses subprocess isolation so tests will pass, but the shared default config will be silently corrupted in any same-process multi-instance scenario. The set_intensity method and the refactored add_hdr both mutate self.spawner_cfg directly. Because the default spawner_cfg parameter is backed by a single class-level object, those writes propagate to every future DomeLight() instance in the same process. The old add_hdr deliberately created a fresh DomeLightCfg to avoid exactly this, and that guard has been removed. isaaclab_arena/assets/object_library.py — specifically the set_intensity method and the refactored add_hdr block around lines 508–523. Important Files Changed
Reviews (3): Last reviewed commit: "Rename test." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds a build-time variation that samples and applies dome light intensity before environment composition. The implementation follows the existing HDRImageVariation pattern well, with a clean separation between the variation class, the DomeLight.set_intensity() helper, and the test. However, there is a name mismatch between the variation registration and the test lookup that will cause test failures, a stale docstring, and a debug print left in production code.
Design Assessment
Design is sound. The approach correctly mirrors the HDRImageVariation pattern — a BuildTimeVariationBase subclass with a reference to the light asset, sampling once at build time. The refactor of add_hdr to mutate in-place rather than reconstructing the entire DomeLightCfg is a good simplification. Adding set_intensity() as a public method on DomeLight is the right level of abstraction.
Findings
See inline comments for details.
Test Coverage
- New code: Yes — dedicated test file covers both enabled and disabled paths
- Gaps: Test will fail due to name mismatch (see Critical finding); once fixed, coverage is adequate
- Quality: Good use of degenerate sampler range for deterministic assertions
CI Status
⏳ Pre-commit check is pending.
Verdict
Significant concerns
The variation name mismatch ("intensity" vs "dome_light_intensity") means the included tests cannot pass as-is, which is a blocking issue. The debug print() statement should also be removed before merge. Once these are addressed, the implementation is clean and follows established patterns well.
Update (d7888a3): All previous concerns have been addressed ✅
- 🔴 Name mismatch — Fixed. Test now uses
"intensity"consistently matching the variation's default name. - 🟡 Debug
print()statement — Removed. - 🟡 Stale docstring — Simplified; misleading range text removed.
- 🔵 Unused
DEFAULT_INTENSITYconstant — Removed.
No new issues found in the incremental changes. LGTM.
alexmillane
force-pushed
the
alex/feature/light_intensity_variation
branch
from
d7888a3 to
fa1349e
Compare
alexmillane
changed the base branch from
alex/feature/variation_hydra_config
to
main
2 participants
Summary
Add light intensity variation.
Detailed description