Route clone backends through ClonePlan - #7462
Conversation
|
Too many files changed for review (295 files, 100 file limit). Bypass the limit by tagging |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The flattened ClonePlan/ReplicateSession lifecycle is coherent overall, but cfg discovery can incorrectly turn sensor visualizer configs into conflicting plan rows, camera spawning removes established redirect/existence behavior, the RayCasterCfg.spawn default change lacks migration guidance, and startup profiling still targets the removed dispatch name.
- Design and architecture: Publishing one immutable plan before construction and dispatching it to registered backends provides a single description of clone ownership. However,
_plan_cfgsalso classifies visualizer marker configs as prim authors; sensors sharing a default marker path can therefore fail planning with duplicate ownership. Visualizer-only configs should be excluded from clone-plan rows. - API: The major changelog covers the removed cloner entry points and most cfg/spawner field removals. It does not cover the
RayCasterCfg.spawndefault changing fromSensorFrameCfg()toNone. Camera construction also drops the previous physics-body-to-child redirect and existing-prim guard, changing behavior for publicCameraCfgusages that target bodies or pre-authored cameras. - Implementation: Plan construction, source-path resolution, and backend dispatch were traced. The remaining implementation integration issue is the startup profiler whitelist still matching
cloner.*:replicateafter dispatch was renamed to_replicate, causing clone-dispatch measurements to be reported as zero placeholders.
Significant concerns. Posted 4 actionable findings inline.
The full PR diff was reviewed; some supplemental surrounding file context was omitted.
Automated review; human maintainers own approval decisions.
ooctipus
changed the title
5c3ba34 to
5f7619e
Compare
ooctipus
changed the title
|
PR 3 has now been split at the execution/lifecycle boundary. This PR contains only the existing Accordingly, the earlier automated inline findings on |
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
isaaclab-bot
Bot
removed
the
ci:run-docker
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
| contexts = ( | ||
| (() if physics_context is None else (physics_context,)) | ||
| if references is None | ||
| else tuple(string_to_callable(value) if isinstance(value, str) else value for value in references) |
There was a problem hiding this comment.
Optional: I'm not a fan of the usage of string_to_callable, you can create a strictly types dictionary instead that maps the string to specific functions.
There was a problem hiding this comment.
Built-in USD and physics contexts are concrete types now. I kept string_to_callable only for the documented user-configurable AssetBaseCfg.cloning_contexts string boundary; a closed core dictionary would make core enumerate backend packages and would exclude custom context classes.
| mujoco~=3.11.0 | ||
| mujoco-warp~=3.11.0 | ||
| newton[sim]==1.5.1 | ||
| newton[sim] @ git+https://github.com/newton-physics/newton.git@24bd863528d6b91137408930d0fbe8fa216ad962 |
There was a problem hiding this comment.
This PR is mixing a newton bump as well?
There was a problem hiding this comment.
This is temporary stack overlap with #7453, which is still open. That predecessor uses the new ModelBuilder.replicate(..., label_prefixes=...) API and therefore carries the Newton 1.6 development pin. The pin and fast-path files disappear from this PR after #7453 merges and this branch is restacked.
|
|
||
| label_groups = _label_groups(source_builder) | ||
| original_labels = {name: list(labels) for name, labels in label_groups.items()} | ||
| try: |
There was a problem hiding this comment.
why try, finally? Are we expecting possible failure?
There was a problem hiding this comment.
_rebase_labels temporarily mutates the retained source builder and can itself fail after a partial mutation; builder.replicate can also fail. The finally restores the snapshot on both paths. Restoration also matters on success because these source builders are retained in NewtonManager._cl_protos for later consumers.
bc71259 to
987f764
Compare
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
987f764 to
91288ba
Compare
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
# Description Revives #6308 on current `develop` and supersedes #3728 with a single backend-neutral contract for kinematic rigid-object rendering. ## Architecture `source/isaaclab/test/renderers/rigid_object_rendering_contract.py` is the composition root. It owns the cloned scene, kinematic pose sequence, depth measurements, and assertions. Package-local adapters own only availability checks, simulation/renderer selection, and backend cleanup: - Isaac RTX + PhysX on CPU and CUDA, with and without a coexisting articulation; - Newton Warp + PhysX on CUDA; - OVRTX + OVPhysX on CUDA through both legacy and OVStage scene ownership (OVStage runs when installed). The dependency direction is adapter -> shared test contract -> public Isaac Lab APIs. An AST architecture gate rejects backend imports in the shared contract and rejects scene, asset, sensor, or class ownership in adapters. The contract creates two cloned instanceable DexCubes with root-level nonuniform scale, verifies their depth silhouettes, moves both kinematic bodies through the public rigid-object tensor API, verifies the physics poses, and requires opposite rendered centroid displacement. ## Current-develop audit Most production changes in the old PR have since landed through newer ownership boundaries: Isaac RTX render-product lifetime in #6729, Newton shadow-state copying in #6773, OVRTX scale-aware transform writes in #7010, and Newton Fabric scale preservation in #7481. This revival removes those stale patches rather than carrying duplicate implementations. The revived contract exposed one remaining OVRTX bug: composed scale was captured only for clone-plan source paths, while OVRTX creates non-source destinations after exporting the host USD stage. Those destinations therefore defaulted to unit scale. As a deliberately temporary bridge, this PR projects only captured non-unit scales through the existing `isaaclab.cloner.query.path_env_ids` and `path_to_clone` boundary using the already-validated `ClonePlan`; real destination scales take precedence. It adds no plan fields, query APIs, renderer configuration, or per-body fallback, and the bridge can be deleted as one unit when SDP supplies composed scale aligned with canonical rigid-body paths. Historical context: [Isaac Sim forum report](https://forums.developer.nvidia.com/t/rigidbody-prim-is-not-updated-in-rendering-pipeline-if-set-to-kinematic/346608). ## Type of change - Bug fix - Shared regression coverage ## Testing - Isaac RTX contract: 4 passed (CUDA/CPU x articulation absent/present). - Newton Warp contract: 1 passed. - OVRTX contract: 2 passed (legacy and OVStage). - OVRTX renderer unit surface: 167 passed. - Core architecture and Newton visualization suites: 25 passed. - OVRTX clone-plan suite: 17 passed. - Cloner query and rendering-contract architecture suites: 87 passed. - Controlled OVRTX regression: failed before the production fix with clone silhouettes of 264 vs. 36 pixels; passed after the fix. - Incoming #7462 NumPy-backed `ClonePlan` query-boundary smoke check: passed unchanged, including non-dense environment ids. - `uv run isaaclab -f`: all hooks passed against the exact upstream `develop` base, including changelog validation. ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the pre-commit checks - [x] Documentation changes are not applicable - [x] I have added unit and integration regression coverage - [x] I have added changelog fragments for every touched package - [x] My name is already present in `CONTRIBUTORS.md`
# Description Restores the missing `torch` import in `test_ovrtx_clone_plan.py`. #7462 converted the existing clone-plan test inputs from Torch tensors to NumPy arrays and removed the then-unused import. #6308 subsequently added a new object-scale test using `torch.ones` and `torch.arange` without restoring the import, causing the repository-wide Ruff pre-commit check to fail with `F821 Undefined name torch`. This PR contains only the import repair and an `isaaclab_ov` `.skip` changelog fragment. It unblocks #7579 and other changes based on the current `develop` branch. ## Type of change - Bug fix (non-breaking test fix) ## Release backport - [ ] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` No release backport is required: `release/3.0.0` already imports `torch` in this test. ## Screenshots Not applicable. ## Testing - `uv run --no-project --with pre-commit python -m pre_commit run --all-files` - `uv run --no-project python tools/changelog/cli.py check --include-worktree` - `git diff --check upstream/develop...HEAD` ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the full pre-commit checks - [x] Documentation changes are not required because no public API changed - [x] My changes generate no new warnings - [x] The fix directly covers the Ruff `F821` failure - [x] I have added an `isaaclab_ov` changelog fragment - [x] My name already exists in `CONTRIBUTORS.md`
2 participants
Description
This is a narrow follow-up to merged #7453. It keeps that Newton fast path and makes the existing
ClonePlanthe single mapping consumed by USD, Newton, PhysX, and OvPhysX replication.Planning records
context_rows, mapping each simulation-owned clone-context type to the plan rows it consumes.cloner.replicate(plan)retrieves those already-registered contexts fromSimulationContext, orders them by replication priority, and passes each the same plan. Backends no longer rebuild the mapping through localqueue(...)orqueue_mapping(...)state.Clone planning is a NumPy host-side control plane.
ClonePlan, its constructors, strategies, queries, and raw clone APIs use NumPy arrays; backend code no longer performs Torch device synchronization ordetach().cpu().tolist()round trips. Runtime owners convert once when they need a device tensor.InteractiveScenederives its environment roots and device-side origins from the same plan instead of calculating a second grid.The public structure stays small:
ClonePlan.global_pathsremains the explicit declaration for shared scene prims.ClonePlan.env_idsandpositionsremain optional for query-only plans; execution validates what it needs.cloning_contexts,cfg_rows,ReplicateSession, and the existing scene-construction lifecycle remain.Migration
Custom clone contexts now implement
replicate(plan)and must be registered withSimulationContext.get_or_create_backend(...)before dispatch. Remove context-localqueue(...)/queue_mapping(...)calls and the former backendPHYSICS_CONTEXTaliases; raw standalone replication functions remain available.The high-level
stage=argument is removed fromcloner.replicate(...)andReplicateSession; each simulation-owned context already owns its stage.Clone arrays and raw clone API arrays are now NumPy. The unused public
devicearguments are removed fromCloneCfg, plan constructors,grid_transforms,ReplicateSession, clone strategies, and raw backend replication functions. Custom clone strategies now implement(combinations: np.ndarray, num_clones: int) -> np.ndarray.Size
These counts are for this PR alone against current
develop; merged #7453 code and dependency changes are no longer duplicated in the diff.Performance
The targeted benchmark warms with 128 worlds, then times replication and required label publication for one 48-body/48-shape Newton prototype at 4,096 worlds. Measurements taken while #7453 was under review used physical GPU 0 and the same Newton revision:
The restack removed the duplicated #7453 implementation rather than reimplementing it. A file-by-file audit confirms this PR no longer changes #7453's label-prefix fast-path files, and the one-source/all-true Newton route still delegates to
ModelBuilder.replicate(...).Type of change
Validation
84 passed— clone-plan algebra7 passed— replicate-session lifecycle15 passed— focused Newton label-prefix/root-name coverage from [Newton] Delegate homogeneous world prefix generation toModelBuilder.replicate()#7453upstream/developgit diff --checkpassedRelease backport
developChecklist
CONTRIBUTORS.md