Enable OVRTX cloning by default#5591
Conversation
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR fixes cloned environments disappearing from tiled camera output when use_cloning=True by correcting scene-partition attribute creation. It also changes the default of use_cloning to True, yielding significant startup speedups (~45% faster on large-scale setups).
Design Assessment
Architecture: Sound ✅
The refactor simplifies create_cloning_attributes → create_scene_partition_attributes with cleaner semantics:
- Removed unnecessary complexity: No longer iterates over non-camera prims to set attributes (the old code set
primvars:omni:scenePartitionon every prim, but the new approach correctly sets it only on env roots and cameras) - Better separation of concerns: Env roots get
primvars:omni:scenePartition, cameras getomni:scenePartition(different attribute names per USD convention) - Improved logging: Added debug logging for attribution and warning for missing env prims
- Return type change: Function no longer returns object count (which was unused/misleading since it counted objects that don't need the attribute)
Findings
🔵 Info ovrtx_usd.py:165 - Function rename and docstring update clearly explain the new behavior. Good documentation.
🔵 Info ovrtx_usd.py:183-185 - The key fix: Previously the loop skipped the env root prim but set primvars:omni:scenePartition on all descendant prims. Now it correctly sets the attribute on the env root and only omni:scenePartition on cameras. This matches OVRTX's expected attribute schema.
🔵 Info ovrtx_renderer_cfg.py:37 - Default changed from False to True. This is a behavioral change for users who relied on the default. The changelog correctly documents this, and the performance benefit (78s → 43s) justifies the change.
🟡 Minor ovrtx_usd.py:180 - The warning log on missing env prim is good, but consider whether this should be an error in strict mode, as a missing env_0 is likely a configuration bug.
Test Coverage
No new tests are included in this PR. Given that:
- This is a bug fix for an existing feature
- The change is in rendering infrastructure that typically requires GPU/Isaac Sim runtime
- The fix was validated empirically (1024 envs, Isaac-Dexsuite benchmark)
Manual testing appears sufficient for this change, though unit tests for the attribute-setting logic (with a mock stage) would be a nice-to-have.
Verdict
Ship it ✅
Clean bug fix with clear performance benefits. The code simplification is appropriate, and the changelog correctly documents both the fix and the default change.
Update (6722131): ✅ Reviewed incremental changes from de1b728 → 6722131. The new commits are merge commits from develop branch (24 commits) — no changes to the actual PR content (OVRTX cloning fix). The original review findings remain valid.
Update (d546776): ✅ Reviewed incremental changes from 6722131 → d546776. The new commits are a v5.2.0 release merge (version bumps and changelog consolidation across isaaclab, isaaclab_ov, isaaclab_newton, isaaclab_physx, isaaclab_ovphysx, isaaclab_mimic, isaaclab_tasks). No changes to the actual PR content (OVRTX cloning fix). The original review findings remain valid.
Update (ab783f0): ✅ Good improvement! Added Sdf.ChangeBlock() context manager around attribute creation loop in create_scene_partition_attributes. This batches USD layer edits, reducing overhead from repeated change notifications — a solid micro-optimization for scenes with many environments. Original review findings remain valid.
Update (2eb503c): ✅ Nice backward-compatibility addition! This commit adds OVRTX 0.2.0 support:
- Gates cloning on
_IS_OVRTX_0_3_0_OR_NEWER(older versions fallback gracefully) - New
enable_scene_partition_workaroundparameter setsprimvars:omni:scenePartitionon ALL prims for OVRTX 0.2.0 (which lacks primvar inheritance) - Parameter renamed
use_cloning→use_ovrtx_cloningfor clarity - Updated docstrings to document version requirements
Note: The Sdf.ChangeBlock() from the previous commit was removed — presumably the per-prim workaround iteration for 0.2.0 makes batching less beneficial. Original review verdict stands: Ship it ✅
Update (cfd1272): ✅ Reviewed incremental changes from 2eb503c9 → cfd12727. Changes include:
- Documentation improvements: Installation instructions now use
./isaaclab.sh -p -m pip install leappfor consistency - Tutorial restructuring for LEAPP export workflow with clearer steps
- Changelog fixes (proper RST bullet formatting, added
is_homogeneousutility function documentation) - Minor docstring wording improvements (
iff→if) - New changelog validation: Rejects orphan paragraphs that would cause Sphinx build failures
No changes to the OVRTX cloning core logic. Original review verdict stands: Ship it ✅
Update (c4dd368): ✅ Reviewed incremental changes from cfd12727 → c4dd368f. Test infrastructure update: increased timeout for test_rsl_rl_export_flow.py from 4000s to 10000s (test runs 54 tasks with 600s timeout each). Reasonable adjustment to prevent CI flakiness. No changes to OVRTX cloning logic. Original review verdict stands: Ship it ✅
Update (ab7057f): ✅ Reverted the previous timeout change for test_rsl_rl_export_flow.py back to 4000s (from 10000s). No changes to OVRTX cloning logic. Original review verdict stands: Ship it ✅
Greptile SummaryThis PR fixes a bug where cloned environments disappeared from tiled camera output when
Confidence Score: 4/5The fix is internally consistent and the camera disappearance bug is addressed by correctly placing The attribute placement logic is a targeted, well-understood correction, and the renderer import rename is purely mechanical. The only risks are the implicit default change for downstream users who never set
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[prepare_stage called] --> B{use_cloning?}
B -- True --> C[env_indices = 0]
B -- False --> D[env_indices = 0..num_envs-1]
C --> E[create_scene_partition_attributes]
D --> E
E --> F[For each env_idx]
F --> G{env_prim valid?}
G -- No --> H[log warning, skip]
G -- Yes --> I[Set primvars:omni:scenePartition on env root]
I --> J[PrimRange walk under env root]
J --> K{prim IsA Camera?}
K -- Yes --> L[Set omni:scenePartition on camera]
K -- No --> M[skip]
L --> N[Next prim]
M --> N
N --> J
I --> O[export_stage_for_ovrtx]
O --> P{use_cloning AND num_envs > 1?}
P -- Yes --> Q[Deactivate env_1..env_N-1]
Q --> R[Export stage with only env_0]
R --> S[Reactivate all envs]
P -- No --> T[Export full stage]
Reviews (1): Last reviewed commit: "add note related to drop of the total st..." | Re-trigger Greptile |
# Description * Added :func:`~isaaclab.cloner.cloner_utils.is_homogeneous` to detect whether a :class:`~isaaclab.cloner.ClonePlan` assigns every environment from every source (a homogeneous clone mask). * Fixed cloned environments disappearing from tiled camera output if :attr:`~isaaclab_ov.renderers.OVRTXRendererCfg.use_ovrtx_cloning` is set to ``True``, by correcting scene-partition attribute creation on env roots and cameras. * Renamed the ``use_cloning`` field on :class:`~isaaclab_ov.renderers.OVRTXRendererCfg` to ``use_ovrtx_cloning``. Changed its default value to ``True``. This will bring notable speedup for the total startup time (Launch to Train), esp. for large-scale env setups. On Isaac-Dexsuite-Kuka-Allegro-Lift-v0 with 1024 env clones, the total startup time dropped from ~78s to ~43s. Note that if ``use_ovrtx_cloning`` is enabled but the env setup is heterogeneous, the OVRTX renderer will disable the internal cloning path and logs a warning, exporting the full multi-environment stage instead (same effect as setting ``use_ovrtx_cloning`` to ``False`` for that run). ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Co-authored-by: Kelly Guo <kellyg@nvidia.com>
Description
Added :func:
~isaaclab.cloner.cloner_utils.is_homogeneousto detect whether a :class:~isaaclab.cloner.ClonePlanassigns every environment from every source (a homogeneous clone mask).
Fixed cloned environments disappearing from tiled camera output if
:attr:
~isaaclab_ov.renderers.OVRTXRendererCfg.use_ovrtx_cloningis set toTrue,by correcting scene-partition attribute creation on env roots and cameras.
Renamed the
use_cloningfield on :class:~isaaclab_ov.renderers.OVRTXRendererCfgtouse_ovrtx_cloning.Changed its default value to
True. This will bring notable speedup for the total startup time (Launch to Train),esp. for large-scale env setups. On Isaac-Dexsuite-Kuka-Allegro-Lift-v0 with 1024 env clones, the total startup time
dropped from ~78s to ~43s. Note that if
use_ovrtx_cloningis enabled but the env setup is heterogeneous, theOVRTX renderer will disable the internal cloning path and logs a warning, exporting the full multi-environment stage
instead (same effect as setting
use_ovrtx_cloningtoFalsefor that run).Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there