Fixed usd_replicate() authoring environment grid positions on nested prims (e.g. cameras), overwriting their local transforms.

[huidongc]

Description

Fixed usd_replicate() authoring environment grid positions on nested prims (e.g. cameras), overwriting their local transforms.

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]

🤖 Isaac Lab Review Bot

Summary

This PR fixes a bug where usd_replicate() was incorrectly authoring environment grid positions on all replicated prims, including nested assets like cameras. This overwrote local transforms, breaking relative offsets. The fix correctly restricts position/quaternion authoring to env-instance root destinations only (env_{}).

Design Assessment

Design is sound. The fix uses a simple and effective pattern:

is_env_root = tmpl.rstrip("/").endswith("env_{}")

This cleanly separates grid transforms (env roots) from local transforms (nested prims). The approach is:

• Non-breaking: No API changes; behavior correction only
• Self-contained: Isolated to the replication logic
• Well-documented: Docstrings updated to clarify authoring behavior

Findings

No critical or warning-level issues found. The implementation is correct and follows Isaac Lab conventions.

🔵 Observation: The env root detection pattern matches any template ending in env_{}, not strictly /World/envs/env_{}. This is fine for current usage since Isaac Lab consistently uses the env_N naming convention, but worth noting if custom env naming is ever introduced.

Test Coverage

• Bug fix: ✅ Has regression test
• Test quality: Good — test_usd_replicate_nested_asset_preserves_local_offset_with_positions verifies that camera offsets are preserved after replication with positions, directly testing the fix
• Gaps: None identified

CI Status

Most checks are pending (recently pushed). Pre-commit, license-check, and changelog validation have passed.

Verdict

No issues found

Clean, well-scoped bug fix with proper regression testing and documentation. The implementation correctly addresses the root cause without side effects.

---

Update (commit d977750): The env root detection heuristic has been improved. Instead of tmpl.rstrip("/").endswith("env_{}"), it now uses str.partition("{}") to check that the template placeholder is at the end of the path:

_, slot, remainder = tmpl.rstrip("/").partition("{}")
is_env_root = slot == "{}" and remainder == ""

This is a strict improvement — it correctly identifies env root templates regardless of naming convention (e.g., scene_{}, inst_{}) while still excluding nested prim paths (e.g., env_{}/Camera). This addresses the observation from the original review about coupling to the env_{} naming convention.

✅ Logic is correct and more robust. No new issues introduced.

---

Update (commit 0e7e857): Test improved to cover both the env-root authoring path and the nested-prim preservation path. The test now:

1. Replicates env roots with positions and asserts grid positions are authored on env prims ✅
2. Replicates nested cameras and asserts local offsets are preserved ✅

This addresses the previous P2 finding about missing env-root authoring coverage. No new issues introduced.

---

Update (commit 327c771): The template-parsing logic has been refactored into a dedicated split_instance_template() utility in cloner_utils.py, centralizing the "{}" detection that was previously scattered across multiple files. All consumers (usd.py, clone_plan.py, ray_caster.py, newton_site_frame_view.py, and both physx/ovphysx replicate.py) now use this helper with proper None-guards for templates without an instance slot.

✅ Previous inline suggestion about naming-convention coupling is fully resolved — the centralized helper is convention-agnostic.
✅ Unit test (test_split_instance_template) covers valid templates, suffix templates, and missing-slot cases.
✅ No new issues introduced. Clean, well-structured refactoring.

---

Update (commit d908a67): Minor refinements to the refactoring from the previous commit:

• Function renamed from split_instance_template() to split_clone_template() — clearer naming.
• Now raises ValueError for templates missing {} instead of returning None, with callers like get_suffix() handling the exception. This is a cleaner contract (fail-fast for programming errors).
• Docstring wording updates: "env-instance root" → "instance-root" throughout.
• Test renamed to test_split_clone_template with a pytest.raises assertion for the error case.
• __init__.pyi updated to export the new symbol.

✅ All changes are consistent with the previous refactoring commit. No new issues introduced.

---

Update (commit d7d556e): Branch was rebased/squashed. The current HEAD consolidates the split_clone_template() refactoring previously reviewed in commits 327c771 and d908a67. Incremental diff confirms no substantive changes beyond what was already reviewed — the refactor, ValueError contract, docstring updates, and cross-module consistency are all as previously assessed. No new issues.

---

Update (commit 9a1d6cc): PR rebased onto current develop (base now includes merged PRs #6068, #5881, #6042, #6072, #6078, #6087, #6073). The PR itself remains a single squashed commit with identical logic to what was previously reviewed at d7d556e. No substantive code changes to this PR's own diff — split_clone_template(), the is_instance_root guard in usd.py, cross-module consumer updates, changelog entries, and regression tests are all unchanged.

✅ Rebase is clean. No new issues introduced.

[greptile-apps]

Greptile Summary

This PR fixes a bug in usd_replicate() where grid positions (and quaternions) were being authored on all replicated destination prims, including nested children like cameras, overwriting their local transform offsets. The fix introduces an is_env_root flag that restricts position/orientation authoring to destination templates whose path ends with env_{}.

• usd.py: Adds is_env_root = tmpl.rstrip(\"/\").endswith(\"env_{}\") check inside _apply_queue; position/quaternion Sdf attribute authoring is now gated on that flag, leaving nested prims' copied local transforms untouched.
• test_cloner.py: Adds test_usd_replicate_nested_asset_preserves_local_offset_with_positions, which replicates a camera with a known local offset while supplying grid positions and asserts both env_0 and env_1 cameras retain the original offset.
• changelog.d/: Adds the corresponding .rst changelog entry describing the fix.

Confidence Score: 4/5

Safe to merge for codebases using the standard env_{} root naming; the fix correctly resolves the camera transform overwrite.

The core logic change is small and well-targeted: it stops grid positions from being written onto nested prims while leaving the env-root authoring path untouched. The new test validates the regression scenario directly.

usd.py (the is_env_root string check) and test_cloner.py (missing complementary env-root positioning assertion) deserve a second look before shipping to users with custom env root naming conventions.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/cloner/usd.py	Adds is_env_root guard to skip authoring grid positions/quaternions for non-env-root destination templates; logic is correct for the intended env_{} naming convention but relies on a string-suffix heuristic with no fallback for custom root names.
source/isaaclab/test/sim/test_cloner.py	Adds regression test proving nested prims retain their local offsets; does not cover the complementary case where env-root positions are still correctly authored after the fix.
source/isaaclab/changelog.d/fix-incorrect-camera-xform-in-usd-replicate.rst	Changelog entry accurately describes the bug and the fix; no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["_apply_queue(): iterate queued items"] --> B["Compute is_env_root\ntmpl.rstrip('/').endswith('env_{}')"]
    B --> C{"src == dp?"}
    C -- "Yes (env_0 self-copy)" --> D["Skip Sdf.CopySpec"]
    C -- No --> E["Sdf.CopySpec(src to dp)\n(copies local transform)"]
    D --> F{"is_env_root AND\npositions/quaternions?"}
    E --> F
    F -- "Yes (env root)" --> G["Author xformOp:translate\nand/or xformOp:orient"]
    F -- "No (nested prim)" --> H["Skip: preserve copied\nlocal transform as-is"]
    G --> I["Update xformOpOrder"]
    H --> I

Loading

_{Reviews (1): Last reviewed commit: "Fixed usd_replicate authoring environmen..." | Re-trigger Greptile}

[isaaclab-review-bot]

🤖 Follow-up Review Update (commit d7d556e)

Force-push/rebase incorporating the same refactoring changes as before, plus:

• Added empty .skip changelog files for isaaclab_newton, isaaclab_ovphysx, and isaaclab_physx extensions (indicating they don't require separate changelog entries since the primary fix is in isaaclab)

✅ No functional changes compared to previous review. Changelog organization is correct. LGTM.

[pbarejko]

This isn't robust enough, we could make this more bullet proof by allowing only one {} occurrence :

d = '/world/env_{}/Camera_{}'
d.partition("{}")
('/world/env_', '{}', '/Camera_{}')

[huidongc]

@ooctipus for vis

Do we need to consider a clone template like /world/env_{}/Camera_{}??

[pbarejko]

I wasn't suggesting to support, I was suggesting to reject.

[huidongc]

it could probably be an ill-formed template in my opinion, but I'd let Octi comment on this.

[ooctipus]

I think lets reserve {} only for world id,
/world/env_{}/Camera_.* -> ok
/world/env_.*/Camera_.* -> ok
/world/env_{}/Camera_{} -> NOT ok

[huidongc]

I wasn't suggesting to support, I was suggesting to reject.

I can do this in my second pr. I want to let this pr run thru CI sooner.

[huidongc]

a small validation (e.g. destination.count("{}") == 1) in ClonePlan construction or UsdReplicateContext.queue would catch this earlier