Fix usd_replicate camera xforms

[ooctipus]

Summary

• preserve copied xformOpOrder entries when usd_replicate() authors transform overrides
• avoid applying env-origin positions to nested clone rows like /World/envs/env_{}/Camera, preserving camera-local offsets
• add regression coverage for copied camera xform order and nested env camera local transforms

Validation

• git diff --check refs/remotes/upstream/develop...HEAD
• python -m py_compile source/isaaclab/isaaclab/cloner/usd.py source/isaaclab/test/sim/test_cloner.py
• focused pytest attempted with ./isaaclab.sh -p -m pytest source/isaaclab/test/sim/test_cloner.py -q, but this local checkout uses /usr/bin/python3.12 without runtime deps (lazy_loader, pxr, torch), so collection fails before running the tests

Bug

• OMPE-97354

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR fixes two related bugs in usd_replicate: (1) env-origin positions were incorrectly overwriting local camera transforms on nested clone rows like /World/envs/env_{}/Camera, and (2) previously-copied xformOpOrder entries were being clobbered when authoring new transform overrides.

Design Assessment

The fix is well-structured with clear separation of concerns:

• _source_is_nested_destination_instance() — correctly identifies when a source prim is a nested child of the destination template (e.g., a camera inside an env clone root). The pattern-matching logic using partition("{}") is sound and handles edge cases (trailing slashes, root paths).
• _author_xform_op_order() — properly preserves existing xformOpOrder entries by reading the current list and only appending new ops if not already present. This is the correct USD pattern for additive authoring.
• queue_mapping() guard — the conditional row_positions = None if _source_is_nested_destination_instance(...) cleanly prevents env-origin positions from propagating to nested rows while keeping the existing behavior for root-level clones.

The approach follows established patterns in the codebase (helper functions prefixed with _, Sdf layer-level authoring, Sdf.ChangeBlock batching).

Findings

Minor (style/robustness):

1. source/isaaclab/isaaclab/cloner/usd.py — _source_is_nested_destination_instance: The slot_value.strip(\"/\") check could theoretically allow a slot value like /0/ to pass if the source had double-separator edge cases. In practice this won't happen with well-formed USD paths, but a comment noting the assumption would be helpful.

2. source/isaaclab/isaaclab/cloner/usd.py — _author_xform_op_order: The op_name not in ordered_ops check uses linear list scan. For typical xformOpOrder sizes (2-4 ops), this is fine, but worth noting it's O(n²) for very large op lists — no action needed.

3. Quaternion handling in queue_mapping: The quaternions parameter is still passed through unconditionally to nested rows. If a nested camera has its own orient op, the env-level quaternion could still overwrite it. This may be intentional (orient is always per-env), but it's worth confirming whether the same guard should apply to quaternions as well.

Test Coverage

✅ Two well-targeted regression tests added:

• test_usd_replicate_preserves_copied_xform_order_when_authoring_position — validates that existing orient/scale ops in xformOpOrder survive a translate override.
• test_usd_replicate_preserves_nested_env_row_local_camera_xform — validates that nested camera local offsets are not overwritten by env-origin positions.

Both tests verify concrete USD attribute values (translate, orient, op order) which directly regresses the reported bug. The helper _define_ordered_camera sets up a realistic multi-op camera xform. Coverage is appropriate for a bug fix.

CI Status

⏳ CI checks are still pending (pre-commit passed, Build Wheel passed; core test suites and installation tests in progress).

Verdict

No issues found — clean, well-scoped bug fix with appropriate regression tests. The only minor consideration is whether quaternions should receive the same nested-row guard as positions, but this appears intentional given the PR scope targets camera position overwrites specifically.

[isaaclab-review-bot]

Nit: The _source_is_nested_destination_instance logic assumes well-formed USD paths (no double separators). This is a safe assumption given the context, but a brief inline comment noting this constraint could help future maintainers.

[isaaclab-review-bot]

Question: Should quaternions receive the same nested-row guard as positions? If a nested camera already has an orient op from the source prim, passing env-level quaternions could still overwrite it. If this is intentional (e.g., orient is always per-env), a brief comment documenting that would be helpful.

[greptile-apps]

Greptile Summary

This PR fixes two related bugs in usd_replicate: env-origin positions being incorrectly applied to nested clone rows (e.g. cameras attached to env prims), and the xformOpOrder being overwritten instead of extended when authoring a translate override on a prim that already has copied orient/scale ops.

• Adds _source_is_nested_destination_instance to detect rows whose source already lives inside an env slot and suppresses positions for those rows in queue_mapping.
• Replaces the inline xformOpOrder assignment with _author_xform_op_order, which reads existing ops from the copied spec and only appends new ones.
• Adds two regression tests; the quaternions-with-nested-rows path remains uncovered.

Confidence Score: 3/5

The position guard for nested rows is correct, but the same guard is missing for quaternions, meaning callers who supply env-origin orientations alongside positions will still corrupt camera-local rotations on nested rows.

The xformOpOrder preservation fix is clean and well-reasoned. However, queue_mapping suppresses positions for nested rows but unconditionally forwards quaternions — the identical problem applies to orientations. Any caller using both positions and quaternions for env-origin placement would silently overwrite nested-prim local orientations, defeating the purpose of the fix.

source/isaaclab/isaaclab/cloner/usd.py — specifically the quaternions forwarding in queue_mapping; source/isaaclab/test/sim/test_cloner.py lacks a test combining quaternions with nested source rows.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/cloner/usd.py	Introduces two helpers: _source_is_nested_destination_instance (suppresses env-origin positions on nested rows) and _author_xform_op_order (preserves copied xformOpOrder when authoring new ops). The position guard is correct, but the parallel quaternions parameter is not suppressed for nested rows — an incomplete fix.
source/isaaclab/test/sim/test_cloner.py	Adds two regression tests: one for preserved xformOpOrder after a translate override, and one for nested-row local camera offsets. Neither test exercises the quaternions-with-nested-rows path, leaving the incomplete fix uncovered.
source/isaaclab/changelog.d/zhengyuz-usd-replicate-camera-xform.rst	Changelog fragment accurately describes both fixes: env-origin placement not overwriting nested camera local transforms, and xformOpOrder preservation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["queue_mapping(sources, destinations, positions, quaternions)"] --> B["for each source/destination pair"]
    B --> C{"_source_is_nested_destination_instance(source, dest)"}
    C -- "True" --> D["row_positions = None, quaternions still forwarded"]
    C -- "False" --> E["row_positions = positions"]
    D --> F["queue(..., positions=None, quaternions=quaternions)"]
    E --> G["queue(..., positions=positions, quaternions=quaternions)"]
    F --> H["_apply_queue"]
    G --> H
    H --> I{"positions is not None?"}
    I -- Yes --> J["author xformOp:translate"]
    I -- No --> K["skip translate"]
    J --> L["_author_xform_op_order: merge into copied xformOpOrder"]

Loading

_{Reviews (1): Last reviewed commit: "Fix usd replicate camera xforms" | Re-trigger Greptile}

[greptile-apps]

Incomplete guard: quaternions still applied to nested rows

row_positions is set to None for nested rows (e.g. /Camera), but quaternions is passed through unchanged. If a caller supplies env-origin orientations alongside positions — a valid use of the existing API — nested prims will have their local rotation overwritten even though the local translation is protected. The fix should apply the same guard to both transform components.

Suggested change

	row_positions = None if _source_is_nested_destination_instance(source, destinations[i]) else positions
	self.queue(
	source,
	destinations[i],
	_select_env_ids(env_ids, mask, i),
	positions=positions,
	positions=row_positions,
	quaternions=quaternions,
	)
	is_nested = _source_is_nested_destination_instance(source, destinations[i])
	row_positions = None if is_nested else positions
	row_quaternions = None if is_nested else quaternions
	self.queue(
	source,
	destinations[i],
	_select_env_ids(env_ids, mask, i),
	positions=row_positions,
	quaternions=row_quaternions,
	)