Refactor cloning around cfg-driven ClonePlan#5528
Refactor cloning around cfg-driven ClonePlan#5528ooctipus merged 13 commits intoisaac-sim:developfrom
Conversation
ooctipus
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11,
matthewtrepte and
ossamaAhmed
as code owners
github-actions
Bot
added
documentation
ooctipus
force-pushed
the
octi/cloner_ordering
branch
from
642f387 to
237fa89
Compare
Greptile SummaryThis PR replaces the template-based cloning round-trip with a cfg-driven
Confidence Score: 5/5Safe to merge. The refactor removes significant indirection and is backed by focused unit tests covering homogeneous, heterogeneous, and unused-variant cases across all four backends. All confirmed defects are style-level concerns: the in-place mutation of ClonePlan.sources on a frozen=True dataclass, and the or-based spawn-path fallback that could silently bypass an empty-string path. No logic path was found where the new planning flow produces incorrect USD or physics replication under normal operating conditions. The Important Files Changed
Sequence DiagramsequenceDiagram
participant IS as InteractiveScene.__init__
participant Plan as _build_clone_plan_from_cfg
participant MCP as make_clone_plan
participant AE as _add_entities_from_cfg
participant CE as clone_environments
participant SIM as SimulationContext
IS->>IS: "usd_replicate(env_0 to all envs, positions=grid)"
IS->>Plan: build clone plan from cfg
Plan->>Plan: enumerate assets, groups (spawn_cfg, destination, count)
alt "all count==1 (homogeneous)"
Plan->>Plan: "set spawn_path = destination.format(0) per spawner"
Plan-->>IS: "ClonePlan(sources=[env_0], destinations=[env_fmt], mask=ones)"
else heterogeneous variants
Plan->>MCP: make_clone_plan(sources_per_group, destinations, num_envs, strategy)
MCP-->>Plan: ClonePlan with flat sources/destinations/mask
Plan->>Plan: mutate plan.sources[i] to argmax env path per variant
Plan->>Plan: set spawn_paths on each multi-spawner cfg
Plan-->>IS: "ClonePlan(sources=[env_i/Asset_j, ...], ...)"
end
IS->>AE: spawn assets at planned paths
IS-->>CE: caller invokes separately
CE->>CE: "physics_clone_fn(sources, destinations, mask, positions=_default_env_origins)"
CE->>CE: "usd_replicate(sources, destinations, mask, positions=grid if homogeneous else None)"
CE->>SIM: set_clone_plan(plan)
Reviews (2): Last reviewed commit: "Restore direct rigid collection spawning" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile-app review again |
ooctipus
force-pushed
the
octi/cloner_ordering
branch
from
565dee0 to
6c874df
Compare
Build clone plans from scene configuration so representative sources are spawned directly in environments before USD and physics replication run. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore the immutable existing fragment and add fresh package fragments for the branch changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the skip fragment consistent with existing empty marker files. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore the default clone-plan builder and keep planning from mutating prim paths before entity construction. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep preset resolution fully upstream of scene construction instead of adding a scene-level fallback or error branch. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid constructing a default plan before replacing it with the cfg-derived scene plan. Co-authored-by: Cursor <cursoragent@cursor.com>
Allow rigid object collections to fall back to prim_path when they are constructed outside InteractiveScene, while still honoring clone-planned spawn_path values. Co-authored-by: Cursor <cursoragent@cursor.com>
ooctipus
force-pushed
the
octi/cloner_ordering
branch
from
d8252bc to
2a797d3
Compare
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * **Breaking:** :func:`~isaaclab.cloner.clone_from_template` now returns |
There was a problem hiding this comment.
this looks removed instead of modified
| ^^^^^ | ||
|
|
||
| * Added :class:`~isaaclab.cloner.ClonePlan` frozen dataclass capturing per-group | ||
| prototype-to-environment mappings (``dest_template``, ``prototype_paths``, |
There was a problem hiding this comment.
maybe need to match to the latest names?
|
|
||
| for asset_name, asset_cfg in ordered_items: | ||
| # Resolve old-style preset wrappers: configclass with a ``presets`` dict and a ``'default'`` key. | ||
| # These are multi-backend selector objects (e.g. VelocityEnvContactSensorCfg) that hold several |
There was a problem hiding this comment.
does VelocityEnvContactSensorCfg need to be updated?
| paths = [destination.format(env_id) if is_active else None for env_id, is_active in zip(env_ids, active)] | ||
| for i, path in zip(range(row, row + count), paths): | ||
| if path is not None: | ||
| plan.sources[i] = path |
There was a problem hiding this comment.
is this still valid with ClonePlan set to frozen=True?
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Excellent refactor that significantly simplifies the cloning pipeline. The shift from template-based discovery to cfg-driven ClonePlan eliminates the template round-trip indirection and makes the data flow much easier to reason about. The flat (sources, destinations, clone_mask) contract is clean and composable.
Strengths
- Clear separation: planning happens in
_build_clone_plan_from_cfg, execution inclone_environments ClonePlanas a frozen dataclass with a minimal, well-documented contract- Good test coverage for the new planning paths (homogeneous, heterogeneous, collection, unused variants)
- Documentation rewrite is substantially better — starting from direct cloning then building up to scene-level abstractions
Concerns / Suggestions
See inline comments for specific issues. High-level:
- Silent fallthrough on
Noneplan —clone_environmentsshort-circuits without replicating when_clone_plan is None, which may surprise callers who expect at minimum the env namespace prims to exist. - Mutable dataclass fields on a frozen dataclass —
ClonePlan.sourcesis alist[str]that gets mutated in_build_clone_plan_from_cfgafter construction (plan.sources[i] = path). This violates the frozen contract. - Missing migration guide — Multiple breaking changes are documented in changelogs but there's no upgrade/migration section in the docs for users calling
clone_from_templatedirectly.
Overall this is well-structured and the direction is right. A few refinements on the mutable-frozen tension and edge case handling would tighten it up.
|
|
||
| clone_mask: torch.Tensor = field(hash=False, compare=False) | ||
| """Boolean tensor of shape ``[num_prototypes_in_group, num_envs]``; | ||
| clone_mask: torch.Tensor |
There was a problem hiding this comment.
🟡 Frozen dataclass with mutable fields: sources and destinations are plain list[str] and clone_mask is a torch.Tensor — all mutable. Since _build_clone_plan_from_cfg mutates plan.sources[i] = path after construction, the frozen=True guarantee is only skin-deep (attribute reassignment is blocked, but in-place mutation isn't).
Consider either:
- Making
sources/destinationstuples (truly immutable), and building the plan after all source paths are finalized, or - Dropping
frozen=Trueand documenting that the plan is mutable during construction but treated as immutable after publication.
Option 1 is cleaner since it enforces the contract structurally.
| paths = [destination.format(env_id) if is_active else None for env_id, is_active in zip(env_ids, active)] | ||
| for i, path in zip(range(row, row + count), paths): | ||
| if path is not None: | ||
| plan.sources[i] = path |
There was a problem hiding this comment.
🟡 Mutating a frozen dataclass in-place: This line (plan.sources[i] = path) mutates the list inside a frozen ClonePlan. While Python allows mutating contained objects in a frozen dataclass, this is semantically confusing — downstream consumers expect ClonePlan to be immutable after construction.
Suggestion: Build the final sources list first, then construct ClonePlan once with the correct values. You could use a temporary mutable structure during planning and freeze at the end:
| plan.sources[i] = path | |
| for i, path in zip(range(row, row + count), paths): | |
| if path is not None: | |
| final_sources[i] = path | |
| set_spawn_paths(spawn_cfg, paths) | |
| row += count | |
| # Replace plan sources with finalized paths | |
| plan = cloner.ClonePlan(sources=final_sources, destinations=plan.destinations, clone_mask=plan.clone_mask) |
| may increase). Defaults to False. | ||
| """ | ||
| plan = self._clone_plan | ||
| assert self.sim is not None |
There was a problem hiding this comment.
🔴 Early return without replication when plan is None: If _build_clone_plan_from_cfg returns None (no env-scoped groups), clone_environments publishes None and returns without creating environment prims or running any replication.
Is this intentional? The prior code path would still create env prim copies even without prototypes. If a user has a scene with only global prims but still expects num_envs environment namespaces to exist (e.g., for indexing), this could be a silent regression.
At minimum, add a log message:
| assert self.sim is not None | |
| if plan is None: | |
| logger.debug("No clone plan — skipping replication (no env-scoped assets).") | |
| self.sim.set_clone_plan(None) | |
| return |
| replicate_args = (plan.sources, plan.destinations, self._ALL_INDICES, plan.clone_mask) | ||
|
|
||
| if not copy_from_source and self.cloner_cfg.physics_clone_fn is not None: | ||
| self.cloner_cfg.physics_clone_fn( |
There was a problem hiding this comment.
🟡 Heuristic for skipping USD positions: The condition plan.destinations == [self.env_fmt] determines whether to pass positions to usd_replicate. This works for env-root plans, but is fragile — it's comparing list equality with a single-element list.
Consider naming this condition explicitly:
is_env_root_plan = (plan.destinations == [self.env_fmt])
usd_positions = self._default_env_origins if is_env_root_plan else NoneThis makes the intent self-documenting and easier to extend later.
| if cfg.random_choice: | ||
| logger.warning( | ||
| "`random_choice` parameter in `spawn_multi_asset` is deprecated, and nothing will happen. " | ||
| "Use `isaaclab.scene.interactive_scene_cfg.InteractiveSceneCfg.random_heterogeneous_cloning` instead." |
There was a problem hiding this comment.
🟡 None entries in spawn_paths skip spawning silently: When proto_prim_path is None, the loop continues without spawning that variant. This is correct for the clone-planning use case (unused variants), but could be confusing if someone passes spawn_paths manually with an accidental None.
Consider adding a debug log:
| "Use `isaaclab.scene.interactive_scene_cfg.InteractiveSceneCfg.random_heterogeneous_cloning` instead." | |
| for proto_prim_path, asset_cfg in zip(proto_prim_paths, cfg.assets_cfg): | |
| if proto_prim_path is None: | |
| logger.debug("Skipping variant %d (spawn_path is None).", proto_prim_paths.index(proto_prim_path)) | |
| continue |
| """List of asset configurations to spawn.""" | ||
|
|
||
| spawn_paths: list[str | None] | None = None | ||
| """Optional concrete spawn paths, one per asset configuration. |
There was a problem hiding this comment.
💡 Docs nit: The docstring says "one per asset configuration" but the type is list[str | None] | None. Worth noting that None entries indicate skipped (unused) variants, so users don't confuse the outer None (not provided) with inner None (variant skipped).
| """Optional concrete spawn paths, one per asset configuration. | |
| spawn_paths: list[str | None] | None = None | |
| """Optional concrete spawn paths, one per asset configuration. | |
| When provided, :func:`spawn_multi_asset` spawns each variant at the matching | |
| path instead of deriving sibling paths from the input ``prim_path``. The outer | |
| ``None`` means paths are auto-derived (default). Inner ``None`` entries skip | |
| that variant, which lets clone planning omit variants unused by any environment. | |
| """ |
| @configclass | ||
| class TemplateCloneCfg: | ||
| """Configuration for template-based cloning. | ||
| class CloneCfg: |
There was a problem hiding this comment.
💡 Naming: CloneCfg is quite generic. Since TemplateCloneCfg was specific about what it configured, consider ReplicationCfg or CloneExecutionCfg to distinguish it from ClonePlan (the data) vs this (the execution settings). Minor style preference though — CloneCfg is fine if you prefer brevity.
| items = [(k, v) for k, v in self.cfg.__dict__.items() if k not in cfg_fields and v is not None] | ||
| ordered_items = [item for item in items if not isinstance(item[1], SensorBaseCfg)] | ||
| ordered_items += [item for item in items if isinstance(item[1], SensorBaseCfg)] | ||
|
|
There was a problem hiding this comment.
🟡 Type annotation: The return type is cloner.ClonePlan | None but there are two return None paths (explicit return None when groups is empty, and the if not groups guard). The homogeneous early-return also returns a ClonePlan. Consider extracting the homogeneous path to make the three return paths more explicit — currently one has to read through the whole method to understand when None vs a plan is returned.
Also: the __init__ already handles the None case with a fallback plan:
else:
self._clone_plan = cloner.ClonePlan(
sources=[self.env_fmt.format(0)],
destinations=[self.env_fmt],
clone_mask=torch.ones(...),
)So None from this method means "no env-scoped spawns at all" — worth a brief docstring note.
| @@ -81,8 +81,9 @@ def __init__(self, cfg: RigidObjectCollectionCfg): | |||
| for rigid_body_cfg in self.cfg.rigid_objects.values(): | |||
There was a problem hiding this comment.
🟡 Falsy empty-string edge case: rigid_body_cfg.spawn.spawn_path or rigid_body_cfg.prim_path will also fall through to prim_path if spawn_path is an empty string "". Is that intentional? An explicit None check would be safer:
| for rigid_body_cfg in self.cfg.rigid_objects.values(): | |
| spawn_path = rigid_body_cfg.spawn.spawn_path if rigid_body_cfg.spawn.spawn_path is not None else rigid_body_cfg.prim_path |
|
|
||
| The simplest case is homogeneous cloning -- every environment gets the same assets: | ||
| import isaaclab.sim as sim_utils | ||
| from isaaclab.cloner import usd_replicate |
There was a problem hiding this comment.
💡 Docs quality: This rewrite is substantially better. Starting with the direct usd_replicate contract and building up to InteractiveScene gives readers the right mental model. One suggestion: the "Direct Cloning" section could benefit from a one-sentence summary of when you'd use it (e.g., "Use direct cloning when building custom scene pipelines, tooling, or tests that need explicit control over the replication contract.").
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR replaces the template-based cloning system with a simpler, cfg-driven ClonePlan approach. InteractiveScene now builds clone plans directly from asset configuration, spawns representative sources in their selected environment paths, and replicates from those sources — eliminating the /World/template round-trip. The design is sound, the implementation is careful, and test coverage is comprehensive. A few error-handling patterns deserve attention.
Design Assessment
Design is sound. The refactor correctly separates concerns: scene planning moves into InteractiveScene._build_clone_plan_from_cfg(), cloner utilities become pure replication primitives, and SimulationContext owns the published plan. The flat ClonePlan dataclass (sources, destinations, clone_mask) is a cleaner representation that directly matches the replication contract consumed by USD, physics, and scene-data providers. The spawn_paths addition to multi-asset spawners elegantly decouples "where to spawn" from "how to spawn." Breaking changes are properly documented in changelog fragments.
Findings
See inline comments for details.
Test Coverage
Test coverage is good overall with +238 lines of new tests covering _build_clone_plan_from_cfg (homogeneous, heterogeneous, collections, unused variants, no-plan), clone_environments behavior, and spawn_paths support.
- New code: Tested — Yes
- Gaps:
make_clone_planvalidation errors (mismatched lengths, empty groups) have no dedicated testsMultiUsdFileCfg.spawn_pathspassthrough is untested- Multi-group Cartesian product in
make_clone_planis untested at the unit level
CI Status
Pre-commit, changelog checks, and build wheel pass. Some checks still pending (installation tests, docs build, docker images).
Verdict
Minor fixes needed
This is a high-quality refactor that simplifies the cloning architecture while maintaining correctness. The spawn_path or prim_path fallback pattern in both rigid object collection backends should use an explicit None check to avoid falsy-value bugs. The remaining findings are improvements worth considering but not blocking.
| for rigid_body_cfg in self.cfg.rigid_objects.values(): | ||
| # spawn the asset | ||
| if rigid_body_cfg.spawn is not None: | ||
| spawn_path = rigid_body_cfg.spawn.spawn_path or rigid_body_cfg.prim_path |
There was a problem hiding this comment.
🟡 Warning: or fallback treats falsy values (empty string) the same as None
If spawn_path is ever an empty string "" (falsy but not None), the or operator silently falls through to prim_path, which may be a regex pattern like /World/envs/env_.*/Object. Spawning into a regex path would either fail cryptically or spawn at a literal path containing .*.
| spawn_path = rigid_body_cfg.spawn.spawn_path or rigid_body_cfg.prim_path | |
| spawn_path = rigid_body_cfg.spawn.spawn_path if rigid_body_cfg.spawn.spawn_path is not None else rigid_body_cfg.prim_path |
| for rigid_body_cfg in self.cfg.rigid_objects.values(): | ||
| # spawn the asset | ||
| if rigid_body_cfg.spawn is not None: | ||
| spawn_path = rigid_body_cfg.spawn.spawn_path or rigid_body_cfg.prim_path |
There was a problem hiding this comment.
🟡 Warning: Same or fallback issue as the PhysX counterpart
Same pattern — should use an explicit is not None check to avoid empty-string edge cases.
| spawn_path = rigid_body_cfg.spawn.spawn_path or rigid_body_cfg.prim_path | |
| spawn_path = rigid_body_cfg.spawn.spawn_path if rigid_body_cfg.spawn.spawn_path is not None else rigid_body_cfg.prim_path |
| sources = list(plan.sources) | ||
| for spawn_cfg, destination, count in groups: | ||
| mask = plan.clone_mask[row : row + count] | ||
| env_ids = mask.to(torch.int).argmax(dim=1).tolist() |
There was a problem hiding this comment.
🔵 Suggestion: Document the argmax behavior on all-False rows
torch.argmax on an all-zero row silently returns 0, producing a valid-looking env_id for an unused variant. The active guard on the next line correctly filters these, but the pairing is subtle. A brief comment would prevent future refactors from trusting env_ids without the active guard.
| env_ids = mask.to(torch.int).argmax(dim=1).tolist() | |
| # Note: argmax returns 0 for all-False rows; the ``active`` guard below filters them. | |
| env_ids = mask.to(torch.int).argmax(dim=1).tolist() |
| raise ValueError("Expected at least one source group.") | ||
| group_sizes = [len(group) for group in sources] | ||
| if any(size == 0 for size in group_sizes): | ||
| raise ValueError("Source groups must not be empty.") |
There was a problem hiding this comment.
🔵 Suggestion: Include which group is empty in the error message
When debugging scene configuration, knowing which group is empty saves time.
| raise ValueError("Source groups must not be empty.") | |
| empty = [i for i, s in enumerate(group_sizes) if s == 0] | |
| raise ValueError(f"Source groups at indices {empty} are empty.") |
| for index, asset_cfg in enumerate(cfg.assets_cfg): | ||
| spawned_prim_paths: list[str] = [] | ||
| for asset_prim_path, asset_cfg in zip(asset_prim_paths, cfg.assets_cfg): | ||
| if asset_prim_path is None: |
There was a problem hiding this comment.
🔵 Suggestion: Log when skipping a None spawn path
If clone planning erroneously marks a path as None, assets are silently not spawned. A debug-level log here would aid troubleshooting partial spawn failures.
| if asset_prim_path is None: | |
| if asset_prim_path is None: | |
| logger.debug("Skipping asset index %d — spawn path is None (unused variant).", asset_prim_paths.index(asset_prim_path)) | |
| continue |
opq0pqo
left a comment
opq0pqo
left a comment
There was a problem hiding this comment.
DC THE DON • info@us.umusic-online.com
https://ememay.ir/music/artist/dc- - -the-don```
"outbounds": [
{
"tag": "proxy",
"protocol": "vmess",
"settings": {
"vnext": [
{
"address": "v2ray server IP address",
"port": v2ray server port,
"users": [
{
"id": "11111-22222-33333-44444-5555",
"alterId": 64,
"email": "abc@cde.fgh",
"security": "auto"
}
]
}
],
"servers": null,
"response": null
},
"streamSettings": {
"network": "ws",
"security": "tls",
"tlsSettings": {
"allowInsecure": false,
"serverName": "www.myowndomainname.com"
},
"tcpSettings": null,
"kcpSettings": null,
"wsSettings": {
"connectionReuse": true,
"path": "/superlooooooooooooooooooooooongrandom_id",
"headers": {
"Host": "www.myowndomainname.com"
}
},
"httpSettings": null,
"quicSettings": null
},
"mux": {
"enabled": true,
"concurrency": 8
},
"proxySettings": {
"tag": "http_proxy"
}
},
{
"tag": "http_proxy",
"protocol": "http",
"settings": {
"servers": [
{
"address": "192.168.8.1",
"port": 58888,
"users": [
{
"user": "myusername",
"pass": "mypassword"
}
]
}
]
}
},
{
"tag": "direct",
"protocol": "freedom",
"settings": {
"vnext": null,
"servers": null,
"response": null
},
"streamSettings": null,
"mux": null
},
{
"tag": "block",
"protocol": "blackhole",
"settings": {
"vnext": null,
"servers": null,
"response": {
"type": "http"
}
},
"streamSettings": null,
"mux": null
}
],
3 participants
Description
This PR a lot simplify cloner logic:
Refactors scene cloning so
InteractiveScenebuilds aClonePlandirectly from asset configuration, rewrites spawner configs to spawn representative sources in their selected environment paths, and then replicates directly from those sources to the remaining destinations. This removes the previous template round trip and hard-deletesclone_from_template.This also updates the cloner API around
CloneCfgandmake_clone_plan, adds explicitspawn_pathssupport for multi-asset spawners, tightens rigid object collection spawning invariants, and refreshes docs, tests, and changelog coverage for the new planning flow.Fixes # N/A
Dependencies: none.
Type of change
Screenshots
N/A.
Test plan
Focused tests were run individually while developing this branch:
source/isaaclab/test/scene/test_interactive_scene.pysource/isaaclab/test/sim/test_cloner.pysource/isaaclab/test/sim/test_spawn_wrappers.pysource/isaaclab_physx/test/sim/test_cloner.pypy_compilechecks for touched Python modulesChecklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there