Complete OVRTX cloning

[huidongc]

Description

Complete OVRTX cloning

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[isaaclab-review-bot]

Review Summary

This PR extends OVRTX cloning to support heterogeneous scenes by leveraging the ClonePlan from SimulationContext. The approach is well-designed—switching from the previous "deactivate/export/reactivate" pattern to a cleaner layer-pruning strategy that doesn't mutate the live stage.

Findings

🔴 Bug: num_envs shadowed in _clone_from_plan

In _clone_from_plan(), the parameter num_envs is passed but immediately overwritten:

def _clone_from_plan(self, num_envs: int, clone_plan: ClonePlan) -> None:
    """Replicate each clone-plan source row into its destination paths via ``clone_usd``."""
    num_envs = clone_plan.clone_mask.shape[1]  # shadows the parameter

The parameter becomes unused. Either remove the parameter and retrieve num_envs from clone_plan.clone_mask.shape[1] inline, or use the parameter value and add an assertion that it matches the clone plan dimensions.

File: source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py (around line 352)

---

🟡 Potential Issue: Missing OVRTX version guard

The original code had a version guard:

self._use_ovrtx_cloning = self.cfg.use_ovrtx_cloning and _IS_OVRTX_0_3_0_OR_NEWER

This was removed. While _initialize_from_spec still checks _IS_OVRTX_0_3_0_OR_NEWER for the clone path, prepare_stage() now always calls export_stage_to_string with use_ovrtx_cloning from config regardless of the OVRTX version. For pre-0.3.0 OVRTX, this could export a pruned stage that then fails because cloning isn't supported.

Consider adding the version check back into prepare_stage(), or document this behavior change in the changelog.

File: source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py (around line 230)

---

🟡 Missing None check in layer pruning

In _prune_layer_to_clone_sources, the code does:

envs_spec = layer.GetPrimAtPath("/World/envs")
if envs_spec is None or not envs_spec:
    return

The not envs_spec check relies on Sdf.PrimSpec's truthiness. However, if GetPrimAtPath returns an invalid spec (rather than None), the behavior varies by USD version. Consider using if not envs_spec or not envs_spec.valid for robustness, or simply if not layer.GetPrimAtPath("/World/envs").

File: source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_usd.py (around line 258)

---

🟢 Suggestion: Log when falling back to homogeneous cloning

When clone_plan is None, the code silently falls back to exporting only /World/envs/env_0:

# Homogeneous fallback when the plan is not published yet.
return _export_stage_scoped_to_sources(stage, ("/World/envs/env_0",))

Adding a debug/info log here would help diagnose situations where users expect heterogeneous cloning but the plan wasn't available.

File: source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_usd.py (around line 338)

---

CI Status

The pre-commit and Check for Broken Links checks are failing. Please address those before merge.

Testing

The new unit tests (test_prune_layer_keeps_heterogeneous_source_prototypes, test_prune_layer_keeps_whole_env_root_source) provide good coverage for the pruning logic. Consider adding a test case for when clone_plan is None to verify the homogeneous fallback path.

---

Overall, this is a solid improvement that removes the previous limitation around heterogeneous scenes. The layer-pruning approach is cleaner than mutating the live stage. Please address the shadowed parameter bug and consider the version guard concern.

---

Update (9e80b43): Reviewed the new commits. Changes are formatting/style only (single-line function call, any() refactor in _should_keep_prim_path, docstring indentation fixes, import reordering). No functional changes. Previous findings above still apply.

---

Update (3a93b20): Good improvement! Path handling now uses proper Sdf.Path objects and USD APIs (HasPrefix, AppendChild) instead of string manipulation. This is more robust and follows USD best practices. Previous findings still apply—no changes to the renderer file where the main issues were noted.

---

Update (4831a82): New commits add documentation improvements (OS-specific tabs for Linux/Windows commands), new PPISP camera demo scripts, the isaaclab-ppisp package, CI workflow updates (ovphysx added to pip packages), and various test/docstring cleanups. These are all additive changes unrelated to the core OVRTX cloning logic. Previous findings above still apply—the num_envs shadowing bug in _clone_from_plan remains unaddressed.

---

Update (ebada0d): This commit significantly refactors ovrtx_usd.py:

• ✅ Addressed: The envs_spec None check now properly raises a RuntimeError if the prim spec is invalid, instead of silently returning.
• ✅ Addressed: Logging now shows the export scope (source paths and env indices) after pruning.
• 🔄 Refactored: Inlined helper functions into export_stage_to_string, making the code more self-contained. The pruning logic in _prune_child_prim_specs is now clearer with explicit comments.

Previous findings in ovrtx_renderer.py still apply:

• 🔴 The num_envs parameter shadowing bug in _clone_from_plan remains unaddressed.
• 🟡 The OVRTX version guard removal concern still stands.---

Update (4f42347): This commit simplifies the layer pruning logic in ovrtx_usd.py:

• Refactored _prune_child_prim_specs: Now accepts an optional include_child callable to filter which children to process, making the function more generic and reusable.
• Consolidated env-pruning logic: Replaced the multi-loop env-index extraction (_source_env_indices, _ENV_CHILD_NAME_RE, _SOURCE_ENV_PATH_RE) with a single recursive call using the include_child filter with regex matching. Much cleaner approach.
• Removed internal test coverage: Tests for the now-inlined _prune_layer_to_clone_sources were removed since it's no longer exposed.

This is a solid refactoring that reduces code complexity. No new issues introduced.

Previous findings in ovrtx_renderer.py still apply:

• 🔴 The num_envs parameter shadowing bug in _clone_from_plan remains unaddressed.
• 🟡 The OVRTX version guard removal concern still stands.

[ooctipus]

Thanks @huidongc

[isaaclab-review-bot]

Update (91c3691, 71d4d5a): Reviewed the latest commits:

• 🔄 Refactored: _prune_child_prim_specs signature changed to take prim_path first, then prim_spec, with an optional include_child filter function. The function is more generalized now.
• 🔄 Simplified: The pruning logic in export_stage_to_string is now cleaner—using the refactored _prune_child_prim_specs directly with a regex filter for env children.
• 🗑️ Removed: Unit tests test_prune_layer_keeps_heterogeneous_source_prototypes and test_prune_layer_keeps_whole_env_root_source were deleted along with the _prune_layer_to_clone_sources helper they tested. This reduces test coverage for the pruning logic.

Concern: Removing the pruning unit tests reduces confidence in the layer pruning behavior. Consider adding integration tests that exercise export_stage_to_string directly with various clone plan configurations.

Previous findings in ovrtx_renderer.py still apply:

• 🔴 The num_envs parameter shadowing bug in _clone_from_plan remains unaddressed.
• 🟡 The OVRTX version guard removal concern still stands.