diff --git a/source/isaaclab/docs/CHANGELOG.rst b/source/isaaclab/docs/CHANGELOG.rst index 507f9ba07a2..0cd7ce812bd 100644 --- a/source/isaaclab/docs/CHANGELOG.rst +++ b/source/isaaclab/docs/CHANGELOG.rst @@ -58,6 +58,17 @@ Changed module directory before resolution. +4.2.3 (2026-02-25) +~~~~~~~~~~~~~~~~~~ + +Fixed +^^^^^ + +* Fixed :func:`~isaaclab.cloner.usd_replicate` and :func:`~isaaclab.cloner.physx_replicate` + skipping ``Sdf.CopySpec`` when the source and destination paths are identical (self-copy), + avoiding a redundant and potentially destructive USD spec overwrite. + + 4.2.2 (2026-02-26) ~~~~~~~~~~~~~~~~~~ diff --git a/source/isaaclab/isaaclab/cloner/cloner_utils.py b/source/isaaclab/isaaclab/cloner/cloner_utils.py index 96d13d38de4..3bc9ea70dc5 100644 --- a/source/isaaclab/isaaclab/cloner/cloner_utils.py +++ b/source/isaaclab/isaaclab/cloner/cloner_utils.py @@ -185,7 +185,10 @@ def dp_depth(template: str) -> int: for wid in target_envs.tolist(): dp = tmpl.format(wid) Sdf.CreatePrimInLayer(rl, dp) - Sdf.CopySpec(rl, Sdf.Path(src), rl, Sdf.Path(dp)) + if src == dp: + pass # self-copy: CreatePrimInLayer already ensures it exists; CopySpec would be destructive + else: + Sdf.CopySpec(rl, Sdf.Path(src), rl, Sdf.Path(dp)) if positions is not None or quaternions is not None: ps = rl.GetPrimAtPath(dp) diff --git a/source/isaaclab/isaaclab/sim/utils/prims.py b/source/isaaclab/isaaclab/sim/utils/prims.py index 34cfc63e52c..ef5e0aca77e 100644 --- a/source/isaaclab/isaaclab/sim/utils/prims.py +++ b/source/isaaclab/isaaclab/sim/utils/prims.py @@ -669,8 +669,21 @@ def wrapper(prim_path: str | Sdf.Path, cfg: SpawnerCfg, *args, **kwargs): else: source_prim_paths = [root_path] - # resolve prim paths for spawning - prim_spawn_path = prim_path.replace(".*", "0") + # Build a prototype prim path to spawn once, then copy to ALL matching parents. + # + # Octi: Leaf note wild card and root not wild card should be treated differently: + # (A) ".*" in root_path e.g. /World/Origin_0.*/CameraSensor + # source_prim_paths holds ALL matching parent prims already in the stage. + # We spawn the child once at source_prim_paths[0] as the prototype, then + # Sdf.CopySpec it to every remaining parent so every parent ends up with + # the child prim. + # + # (B) ".*" in asset_path only e.g. /World/template/Object/proto_asset_.* + # No matching prims exist yet; source_prim_paths == [root_path] (one entry). + # Replacing ".*" → "0" in asset_path gives the intended name proto_asset_0. + # No copy step runs because there is only one parent. + # + prim_spawn_path = f"{source_prim_paths[0]}/{asset_path.replace('.*', '0')}" # spawn single instance prim = func(prim_spawn_path, cfg, *args, **kwargs) # set the prim visibility @@ -698,16 +711,13 @@ def wrapper(prim_path: str | Sdf.Path, cfg: SpawnerCfg, *args, **kwargs): _schemas.activate_contact_sensors(prim_spawn_path) # clone asset using cloner API if len(source_prim_paths) > 1: - # lazy import to avoid circular import - from isaaclab.cloner import usd_replicate - - formattable_path = f"{root_path.replace('.*', '{}')}/{asset_path}" - usd_replicate( - stage=stage, - sources=[formattable_path.format(0)], - destinations=[formattable_path], - env_ids=torch.arange(len(source_prim_paths)), - ) + sanitized_asset = asset_path.replace(".*", "0") + rl = stage.GetRootLayer() + with Sdf.ChangeBlock(): + for src_parent in source_prim_paths[1:]: + dest_path = f"{src_parent}/{sanitized_asset}" + Sdf.CreatePrimInLayer(rl, dest_path) + Sdf.CopySpec(rl, Sdf.Path(prim_spawn_path), rl, Sdf.Path(dest_path)) # return the source prim return prim diff --git a/source/isaaclab/test/sim/test_cloner.py b/source/isaaclab/test/sim/test_cloner.py index fda276931fe..8e3b898fe65 100644 --- a/source/isaaclab/test/sim/test_cloner.py +++ b/source/isaaclab/test/sim/test_cloner.py @@ -126,6 +126,234 @@ def test_physx_replicate_no_error(sim): ) +def _make_mock_physx_rep(): + """Return (mock_rep, replicate_calls) where replicate_calls accumulates num_worlds per call. + + ``mock_rep.register_replicator`` immediately invokes attach_fn + attach_end_fn so the callbacks + fire synchronously inside ``physx_replicate``, making the calls observable in tests. + """ + from unittest.mock import MagicMock + + replicate_calls: list[int] = [] + mock_rep = MagicMock() + mock_rep.replicate.side_effect = lambda _sid, _src, num_worlds, **kw: replicate_calls.append(num_worlds) + + def _fake_register(_stage_id, attach_fn, attach_end_fn, rename_fn): + attach_fn(_stage_id) + attach_end_fn(_stage_id) + + mock_rep.register_replicator.side_effect = _fake_register + return mock_rep, replicate_calls + + +def _make_mock_physx_rep_detailed(): + """Return (mock_rep, replicate_calls, attach_excluded) for fine-grained inspection. + + ``replicate_calls`` is a list of ``(src, num_worlds)`` tuples — one entry per + ``rep.replicate`` invocation, preserving the source path for heterogeneous checks. + ``attach_excluded`` is the list of paths returned by ``attach_fn`` (i.e. the paths + that the replicator will exclude from its USD stage parse). + """ + from unittest.mock import MagicMock + + replicate_calls: list[tuple[str, int]] = [] + attach_excluded: list[str] = [] + mock_rep = MagicMock() + mock_rep.replicate.side_effect = lambda _sid, src, num_worlds, **kw: replicate_calls.append((src, num_worlds)) + + def _fake_register(_stage_id, attach_fn, attach_end_fn, rename_fn): + excluded = attach_fn(_stage_id) or [] + attach_excluded.extend(excluded) + attach_end_fn(_stage_id) + + mock_rep.register_replicator.side_effect = _fake_register + return mock_rep, replicate_calls, attach_excluded + + +@pytest.mark.parametrize( + "num_envs,src,expected_worlds", + [ + # source == env_0; 3 envs → self-world excluded, replicate to env_1 and env_2 only (2 worlds) + (3, "/World/envs/env_0", [2]), + # source == env_0; 1 env → self-excluded → worlds=[] → rep.replicate skipped. + # env_0 was already registered by the replicator's stage parse (attach_fn returns only + # the template path, so env_0 is not excluded and is parsed as a simulation body). + (1, "/World/envs/env_0", []), + # source path does not match template (external template) → all envs included + (3, "/World/template/Robot", [3]), + ], +) +def test_physx_replicate_excludes_self_world(sim, num_envs, src, expected_worlds): + """physx_replicate skips rep.replicate when all worlds are self-copies. + + attach_fn returns only the template path so the replicator's stage parse registers all + existing env prims as simulation bodies. rep.replicate is only called for the non-self + replica worlds. When filtering removes all worlds (isolated single-env case, or global + num_envs=1), rep.replicate is skipped entirely — the source prim is already registered + by the stage parse and no replication is needed. + """ + from unittest.mock import patch + + stage = sim_utils.get_current_stage() + sim_utils.create_prim("/World/envs", "Xform") + sim_utils.create_prim("/World/template", "Xform") + sim_utils.create_prim("/World/template/Robot", "Xform") + for i in range(num_envs): + sim_utils.create_prim(f"/World/envs/env_{i}", "Xform") + + mock_rep, replicate_calls = _make_mock_physx_rep() + with patch("isaaclab_physx.cloner.physx_replicate.get_physx_replicator_interface", return_value=mock_rep): + physx_replicate( + stage, + sources=[src], + destinations=["/World/envs/env_{}"], + env_ids=torch.arange(num_envs, dtype=torch.long), + mapping=torch.ones((1, num_envs), dtype=torch.bool), + ) + + assert replicate_calls == expected_worlds, ( + f"Expected replicate world counts {expected_worlds}, got {replicate_calls}" + ) + + +@pytest.mark.parametrize("device", ["cpu", "cuda"]) +def test_physx_replicate_isolated_source_loaded_without_replication(sim, device): + """An isolated source (worlds=[self]) is registered by the stage parse, not rep.replicate. + + When physx_replicate encounters a source whose only world is itself, it skips rep.replicate + (self-exclusion → empty worlds list → continue). The prim must already be in the stage + (put there by proto_mask usd_replicate) so the replicator's stage parse picks it up as a + simulation body. This test verifies: + 1. rep.replicate is NOT called for the isolated source (no self-copy). + 2. After sim.reset(), PhysX can find the rigid body at the isolated env path — i.e. the + first instance IS loaded even though rep.replicate was never invoked for it. + """ + stage = sim_utils.get_current_stage() + + # Spawn an isolated rigid body at env_0 (the only env — self is the sole destination). + sim_utils.create_prim("/World/envs", "Xform") + sim_utils.create_prim("/World/template", "Xform") + sphere_cfg = sim_utils.SphereCfg( + radius=0.1, + rigid_props=sim_utils.RigidBodyPropertiesCfg(), + mass_props=sim_utils.MassPropertiesCfg(mass=1.0), + collision_props=sim_utils.CollisionPropertiesCfg(), + ) + sphere_cfg.func("/World/envs/env_0/Sphere", sphere_cfg) + + physx_replicate( + stage, + sources=["/World/envs/env_0/Sphere"], + destinations=["/World/envs/env_{}/Sphere"], + env_ids=torch.tensor([0], dtype=torch.long), + mapping=torch.ones((1, 1), dtype=torch.bool), + device=device, + ) + + # Start simulation — PhysX builds its internal scene from the USD stage. + sim.reset() + + # The rigid body at env_0/Sphere must be findable (first instance was loaded by stage parse). + physics_sim_view = sim.physics_manager.get_physics_sim_view() + physx_view = physics_sim_view.create_rigid_body_view("/World/envs/env_*/Sphere") + assert physx_view is not None and physx_view.count == 1, ( + f"Expected 1 rigid body at /World/envs/env_0/Sphere, got " + f"{'None (prim not found by PhysX)' if physx_view is None else physx_view.count}. " + "Isolated source (worlds=[self]) must be registered by the stage parse when " + "rep.replicate is skipped — verify attach_fn does not exclude env prim paths." + ) + + +@pytest.mark.parametrize("device", ["cpu", "cuda"]) +def test_physx_replicate_heterogeneous_isolated_sources(sim, device): + """physx_replicate handles heterogeneous sources where some map only to themselves. + + This is the Dexsuite scenario: multiple object types, each with a designated proto-env. + Some types are assigned to only one environment (themselves), making them 'isolated'. + Isolated sources must be skipped by rep.replicate — they are already registered by the + replicator's stage parse (attach_fn does not exclude env paths). Multi-world sources + must replicate only to their non-self worlds. + + Sources and expected behaviour: + env_0/Object → worlds [0, 2, 4] → self-excluded → replicate to [2, 4] (2 worlds) + env_5/Object → worlds [5] → self-excluded → skip (isolated) (0 worlds) + env_7/Object → worlds [7, 11] → self-excluded → replicate to [11] (1 world) + """ + from unittest.mock import patch + + num_envs = 16 + stage = sim_utils.get_current_stage() + sim_utils.create_prim("/World/template", "Xform") + for i in range(num_envs): + sim_utils.create_prim(f"/World/envs/env_{i}", "Xform") + + mapping = torch.zeros((3, num_envs), dtype=torch.bool) + mapping[0, [0, 2, 4]] = True # env_0/Object: multi-world (includes self) + mapping[1, [5]] = True # env_5/Object: isolated (only self) + mapping[2, [7, 11]] = True # env_7/Object: multi-world (includes self) + + mock_rep, replicate_calls, _ = _make_mock_physx_rep_detailed() + with patch("isaaclab_physx.cloner.physx_replicate.get_physx_replicator_interface", return_value=mock_rep): + physx_replicate( + stage, + sources=["/World/envs/env_0/Object", "/World/envs/env_5/Object", "/World/envs/env_7/Object"], + destinations=["/World/envs/env_{}/Object"] * 3, + env_ids=torch.arange(num_envs, dtype=torch.long), + mapping=mapping, + device=device, + ) + + # env_5 is isolated → skipped; env_0 → 2 worlds; env_7 → 1 world + expected = [ + ("/World/envs/env_0/Object", 2), + ("/World/envs/env_7/Object", 1), + ] + assert replicate_calls == expected, ( + f"Expected {expected}, got {replicate_calls}. " + "env_5/Object (isolated, worlds=[5]) must be skipped — it is registered by the " + "replicator's stage parse, not by rep.replicate." + ) + + +def test_usd_replicate_self_copy_skips_copy_spec(sim): + """usd_replicate must not call Sdf.CopySpec when source and destination paths are identical. + + Sdf.CopySpec(src, src) is a no-op in the current USD version so it does not corrupt children, + but the call is still wasteful. The guard ensures it is skipped entirely. This test mocks + Sdf.CopySpec to verify it is called exactly once (for env_1) and never for the self case (env_0). + """ + from unittest.mock import patch + + import isaaclab.cloner.cloner_utils as _cloner_mod + + stage = sim_utils.get_current_stage() + sim_utils.create_prim("/World/envs", "Xform") + sim_utils.create_prim("/World/envs/env_0", "Xform") + sim_utils.create_prim("/World/envs/env_0/Robot", "Xform") + sim_utils.create_prim("/World/envs/env_0/Robot/base_link", "Xform") + sim_utils.create_prim("/World/envs/env_1", "Xform") + + copy_calls: list[tuple[str, str]] = [] + real_copy_spec = _cloner_mod.Sdf.CopySpec + + def capturing_copy_spec(src_layer, src_path, dst_layer, dst_path): + copy_calls.append((str(src_path), str(dst_path))) + return real_copy_spec(src_layer, src_path, dst_layer, dst_path) + + with patch.object(_cloner_mod.Sdf, "CopySpec", capturing_copy_spec): + usd_replicate( + stage, + sources=["/World/envs/env_0"], + destinations=["/World/envs/env_{}"], + env_ids=torch.tensor([0, 1], dtype=torch.long), + mask=torch.ones((1, 2), dtype=torch.bool), + ) + + # CopySpec must be called for env_1 but never for env_0 (self-copy) + assert all(src != dst for src, dst in copy_calls), f"Self-copy detected in CopySpec calls: {copy_calls}" + assert any(dst == "/World/envs/env_1" for _, dst in copy_calls), "CopySpec was not called for env_1" + + def test_clone_from_template(sim): """Clone prototypes via TemplateCloneCfg and clone_from_template and exercise both USD and PhysX. @@ -194,6 +422,95 @@ def test_clone_from_template(sim): assert physx_view is not None +@pytest.mark.parametrize( + "parent_paths, spawn_pattern, expected_child_paths, bad_path, match_expr", + [ + # Case A – single wildcard in root_path. + # The child must appear under every matching parent (not just the first). + # Old code spawned at the wrong synthetic path /World/rig_00/Sensor. + ( + ["/World/rig_0_alpha", "/World/rig_0_beta", "/World/rig_0_gamma"], + "/World/rig_0_.*/Sensor", + ["/World/rig_0_alpha/Sensor", "/World/rig_0_beta/Sensor", "/World/rig_0_gamma/Sensor"], + "/World/rig_00/Sensor", + "/World/rig_0_.*", + ), + # Case A – double wildcard in root_path (multiple ".*" levels). + # Old code replaced BOTH ".*" with 0, giving the wrong path /World/group_0/slot_0/Sensor. + ( + [ + "/World/group_a/slot_0", + "/World/group_a/slot_1", + "/World/group_b/slot_0", + "/World/group_b/slot_1", + ], + "/World/group_.*/slot_.*/Sensor", + [ + "/World/group_a/slot_0/Sensor", + "/World/group_a/slot_1/Sensor", + "/World/group_b/slot_0/Sensor", + "/World/group_b/slot_1/Sensor", + ], + "/World/group_0/slot_0/Sensor", + "/World/group_.*/slot_.*", + ), + # Case B – wildcard only in asset_path (leaf), no wildcard in root_path. + # This is the proto_asset_* pattern: no matching prims exist yet; the decorator + # must create a single prototype named proto_0 under the one real parent. + # root_path has no regex so source_prim_paths == [root_path] and no copy step runs. + ( + ["/World/template/Object"], + "/World/template/Object/proto_.*", + ["/World/template/Object/proto_0"], + "/World/template/Object0/proto_0", # spurious parent that must NOT be created + "/World/template/Object", + ), + ], +) +def test_clone_decorator_wildcard_patterns( + sim, parent_paths, spawn_pattern, expected_child_paths, bad_path, match_expr +): + """The @clone decorator handles two distinct wildcard patterns correctly. + + Case A – ``.*`` in root_path (parent is a regex): the child prim is spawned at + ``source_prim_paths[0]`` as a prototype and then copied to every other matching + parent via ``Sdf.CopySpec``, so **all** parents end up with the child. The old + ``prim_path.replace(".*", "0")`` approach created spurious intermediate prims + that inflated ``find_matching_prims`` counts and broke tiled-camera initialization. + + Case B – ``.*`` only in asset_path (leaf): no parent regex, so + ``source_prim_paths == [root_path]`` (one entry, no copy step). Replacing + ``".*"`` → ``"0"`` in the asset name gives the intended prototype name + (e.g. ``proto_asset_0``) under the single real parent. + """ + for path in parent_paths: + sim_utils.create_prim(path, "Xform") + + cfg = sim_utils.ConeCfg(radius=0.1, height=0.2) + cfg.func(spawn_pattern, cfg) + + stage = sim_utils.get_current_stage() + + # Every expected child path must exist + for child_path in expected_child_paths: + assert stage.GetPrimAtPath(child_path).IsValid(), ( + f"Prim was not spawned at '{child_path}'. The @clone decorator may have used the wrong spawn path." + ) + + # The spurious path from the old replace(".*", "0") must NOT exist + assert not stage.GetPrimAtPath(bad_path).IsValid(), ( + f"Spurious prim found at '{bad_path}'. " + "The @clone decorator incorrectly derived the spawn path by replacing '.*' with '0'." + ) + + # find_matching_prims must see exactly the original parents — no spurious extras + all_matching = sim_utils.find_matching_prims(match_expr) + assert len(all_matching) == len(parent_paths), ( + f"Expected {len(parent_paths)} matching prims, got {len(all_matching)}. " + "Spurious parent prims were likely created by the @clone decorator." + ) + + def _run_colocation_collision_filter(sim, asset_cfg, expected_types, assert_count=False): """Shared harness for colocated collision filter checks across devices.""" num_clones = 32 @@ -204,7 +521,8 @@ def _run_colocation_collision_filter(sim, asset_cfg, expected_types, assert_coun for i in range(num_clones): sim_utils.create_prim(f"/World/envs/env_{i}", "Xform", translation=(0, 0, 0)) - # Use _.* pattern - the @clone decorator replaces .* with 0 for single-asset spawners + # ".*" is in the asset leaf name only (Case B): no parent regex, so the decorator + # creates a single prototype named proto_asset_0 under the one real parent. prim = asset_cfg.func(f"{clone_cfg.template_root}/Object/{clone_cfg.template_prototype_identifier}_.*", asset_cfg) assert prim.IsValid() diff --git a/source/isaaclab_physx/isaaclab_physx/cloner/physx_replicate.py b/source/isaaclab_physx/isaaclab_physx/cloner/physx_replicate.py index 9f389698796..cbec0feaa98 100644 --- a/source/isaaclab_physx/isaaclab_physx/cloner/physx_replicate.py +++ b/source/isaaclab_physx/isaaclab_physx/cloner/physx_replicate.py @@ -52,7 +52,7 @@ def physx_replicate( num_envs = mapping.size(1) def attach_fn(_stage_id: int): - return ["/World/envs", *sources] + return ["/World/template"] def rename_fn(_replicate_path: str, i: int): return current_template.format(current_worlds[i]) @@ -61,13 +61,21 @@ def attach_end_fn(_stage_id: int): nonlocal current_template rep = get_physx_replicator_interface() for i, src in enumerate(sources): - current_worlds[:] = env_ids[mapping[i]].tolist() current_template = destinations[i] + worlds = env_ids[mapping[i]].tolist() + # Exclude the world whose destination resolves to the source itself — replicating + # a prim onto itself creates a duplicate PhysX body that conflicts with the + # USD-parsed original, causing erratic joint behaviour. + pre, _, suf = current_template.partition("{}") + self_id = src.removeprefix(pre).removesuffix(suf) + current_worlds[:] = [w for w in worlds if w != int(self_id)] if self_id.isdigit() else worlds + if not current_worlds: + continue rep.replicate( _stage_id, src, len(current_worlds), - useEnvIds=(len(current_worlds) == num_envs) and device != "cpu", + useEnvIds=(len(worlds) == num_envs) and device != "cpu", useFabricForReplication=use_fabric, ) # unregister only AFTER all replicate() calls completed