skip usd cloning in pure newton path#5743
Conversation
ooctipus
requested review from
Mayankm96,
kellyguo11 and
pascal-roth
as code owners
ooctipus
changed the title
Greptile SummaryThis PR fixes Newton replicated-scene cloning by making clone-plan source paths available before sensor construction, skipping redundant USD replication for Newton physics, and rerouting sensor/view/asset resolution to use clone-plan source prims and Newton model labels instead of cloned destination USD prims.
Confidence Score: 3/5Two functional defects need attention before merge: The
Important Files Changed
Sequence DiagramsequenceDiagram
participant Scene as InteractiveScene
participant SimCtx as SimulationContext
participant Sensor as SensorBase
participant Cloner as iter_clone_plan_matches
participant Newton as NewtonManager
Scene->>SimCtx: set_clone_plan(plan)
Scene->>Sensor: construct sensor
Sensor->>SimCtx: get_clone_plan()
SimCtx-->>Sensor: ClonePlan
Sensor->>Cloner: iter_clone_plan_matches(plan, prim_path)
Cloner-->>Sensor: source_root, dest_template, source_path, env_ids
Sensor->>Newton: cl_register_site(body_pattern, xform, per_world)
Newton-->>Sensor: site_label
Scene->>Newton: newton_physics_replicate
Newton->>Newton: _cl_inject_sites returns world_sites
Newton->>Newton: _world_xforms populated
Newton-->>Sensor: PHYSICS_READY callback
Sensor->>Newton: _initialize_from_site_map
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Obtain stage handle | ||
| stage = sim_utils.get_current_stage() | ||
|
|
||
| for i in range(num_envs): | ||
| # Resolve prim path | ||
| obj_path = prim_path.replace(".*", str(i)) | ||
| sample_targets: list[tuple[str, tuple[int, ...]]] = [] |
There was a problem hiding this comment.
clone_plan is None or simulation context is missing
iter_clone_plan_matches unconditionally dereferences plan.sources, so if SimulationContext.instance() returns None or get_clone_plan() returns None (both are possible outside a Newton-replicated scene), the call at line 45 raises AttributeError. Additionally, if the clone plan has no matching entries, sample_targets is empty and points is returned without being filled for any environment — a silent data-corruption path that would produce zero-filled tensors for every env rather than a diagnostic error.
| pos, quat = sim_utils.resolve_prim_pose(prim, body_prim) | ||
| body_path = body_prim.GetPath().pathString | ||
| if source_root is not None and destination_template is not None: | ||
| assert env_ids is not None |
There was a problem hiding this comment.
assert in production code is silently stripped by -O
assert env_ids is not None is evaluated only in debug mode; running Python with -O or -OO silently skips this guard and proceeds with env_ids = None, which would later cause a crash inside the loop that iterates over env_ids. A proper if env_ids is None: raise ValueError(...) guard is needed here.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔬 Code Review: skip usd cloning in pure newton path
Thanks for this comprehensive PR addressing Newton replicated-scene cloning! The changes enable sensors and assets to resolve clone-plan source paths before construction, skipping USD replication when Newton handles physics replication directly. I've reviewed the architecture, error handling, and test coverage.
📋 Summary
Overall Assessment: Minor fixes needed — The core approach is sound and well-architected. A few issues warrant attention before merge.
🔴 Critical Findings
1. source/isaaclab_newton/isaaclab_newton/sensors/ray_caster/ray_caster.py:143-145 — Silent failure when target is outside ClonePlan
if not owner_exprs:
raise RuntimeError(f"RayCaster target '{prim_expr}\\ is not owned by the active ClonePlan.")The _resolve_target_owner_exprs method raises if no iter_clone_plan_matches results are found, but earlier at line 129-131, it already raises if plan is None. This means targets that exist in USD but aren't covered by the ClonePlan will fail, even if they're valid global meshes. Consider falling back to direct prim resolution for non-plan targets rather than requiring all targets to be in the ClonePlan.
🟡 Warnings
2. source/isaaclab/isaaclab/sensors/sensor_base.py:229-234 — Backend detection via string matching
if clone_plan is not None and sim.physics_manager.__name__.lower().startswith("newton"):
clone_plan_matches = tuple(iter_clone_plan_matches(clone_plan, self.cfg.prim_path))Using __name__.lower().startswith("newton") for backend detection is brittle. Consider using a dedicated property or method on the physics manager (e.g., physics_manager.supports_clone_plan_resolution) rather than string-matching the class name.
3. source/isaaclab/isaaclab/cloner/cloner_utils.py:66-68 — Complex sorting key may affect performance
matches.sort(key=lambda match: len(match[1].format(match[3][0])), reverse=True)
if matches:
owner_length = len(matches[0][1].format(matches[0][3][0]))
yield from (match for match in matches if len(match[1].format(match[3][0])) == owner_length)The format(match[3][0]) is called multiple times per match. For ClonePlans with many entries, pre-compute these lengths once:
matches_with_length = [(m, len(m[1].format(m[3][0]))) for m in matches]
matches_with_length.sort(key=lambda x: x[1], reverse=True)
if matches_with_length:
owner_length = matches_with_length[0][1]
yield from (m for m, length in matches_with_length if length == owner_length)4. source/isaaclab/isaaclab/sensors/camera/camera.py:515-517 — Fragile view prim fallback
view_prims = list(self._view.prims)
if not view_prims and cam_paths:
view_prims = [self.stage.GetPrimAtPath(cam_paths[0])] * self._view.countWhen self._view.prims is empty, this creates a list with the same prim reference repeated count times. If downstream code modifies these prims in-place, all "instances" are affected. Add a comment explaining this is intentional for Newton's source-only USD pattern, or consider whether the Camera prim list should track this differently.
🔵 Suggestions for Improvement
5. source/isaaclab_newton/isaaclab_newton/sim/views/newton_site_frame_view.py — Significant simplification (net -414 lines)
The refactored NewtonSiteFrameView is a substantial improvement — removing parent-relative pose tracking and switching to direct body-label resolution is cleaner. However, the docstring at line 26-34 could be expanded to explain:
- What "body-local or world-local sites" means in practice
- The relationship between
_site_specsand the ClonePlan - When
_initialize_from_site_mapvs_initialize_from_specsis called
6. Consider adding a get_clone_plan return type annotation
In interactive_scene.py, self.sim.set_clone_plan(self._clone_plan) is called, but there's no type annotation showing what get_clone_plan() returns. This would improve IDE support and type checking.
✅ Positive Observations
-
Excellent test coverage — The new
test_iter_clone_plan_matchestest thoroughly covers edge cases (heterogeneous sources, partial masks, nested templates, prefix matching). -
Clean separation of concerns — The
iter_clone_plan_matchesutility incloner_utils.pyis well-designed and reusable across sensors, ray casters, and the frame view. -
Good error messages — RuntimeErrors include context about what path expression failed and why.
-
Changelog fragments added — All four packages (
isaaclab,isaaclab_contrib,isaaclab_newton,isaaclab_tasks) have appropriate changelog entries.
📊 Test Coverage Assessment
| Component | Coverage Status |
|---|---|
iter_clone_plan_matches |
✅ Well-tested |
| Newton site frame view | ✅ Good coverage |
| Ray caster ClonePlan resolution | |
| Camera view fallback | |
| Sensor base env count |
The camera's if not view_prims and cam_paths branch (line 515) doesn't have an explicit unit test. Consider adding one to verify the Newton path works correctly when only source USD prims exist.
Verdict: This PR represents a solid architectural improvement for Newton's replicated-scene handling. The critical finding (ray caster target outside ClonePlan) should be addressed before merge; the warnings are worth considering but not blockers.
Update (e82da94): New commit adds well-structured rigid-body traversal helpers (_has_trackable_rigid_body_api, _find_rigid_body_ancestor, _iter_rigid_body_records) to base_multi_mesh_ray_caster.py. The refactored _build_mesh_records now properly finds rigid-body ancestors/descendants for ClonePlan-backed tracked targets with clear error messages. This is a clean improvement — no new issues introduced. Previous findings remain outstanding (pending author response).
Update (b1395e8): This commit improves PhysX ray-caster robustness for edge-case USD structures:
-
base_multi_mesh_ray_caster.py: Added fallback in_iter_rigid_body_recordsto yieldsource_primwhen no descendants with RigidBodyAPI are found. This handles referenced robot USDs where visual/link prims are trackable by PhysX even withoutRigidBodyAPIon the composed prim. -
ray_caster.py(isaaclab_physx): Changed error handling strategy — instead of raising early when no physics body is found, it now uses the prim itself as fallback(body or prim)and validates after creating the rigid body view withbody_view.count == 0. This is a cleaner approach that lets PhysX determine trackability.
✅ Good fix — the error is now deferred to PhysX validation rather than preemptive rejection. Previous review findings remain outstanding.
Update (6b1e164): This commit is primarily documentation and test maintenance:
-
docs/source/features/environments.rst: Reformatted environment presets table to clarify
physics=,renderer=, andpresets=categories. Addsnewton_kaminobackend support to several environments. Documentation-only change. -
docs/source/setup/quick_installation.rst: Fixed broken doc link (
/source/features/visualization→/source/overview/core-concepts/visualization). -
test/utils/test_noise.py: Changed
torch.randtotorch.onesforop="scale"tests to avoid NaN when rand returns exact 0 (fixes OMPE-94619). Good defensive fix. -
Changelog fragments: Added skip marker for test-only change, released teleop changelog for 0.5.1.
-
isaaclab_ov: Fixed OVRTX renderer log path to use cross-platform
tempfile.gettempdir()instead of hardcoded/tmp.
✅ No new code issues introduced. Previous inline comments on utils.py:47 (clone_plan None handling) and newton_site_frame_view.py:291 (assert → proper guard) remain outstanding.
Update (e8da390): This is a significant commit with extensive changes across CI, cloning, and Newton components:
CI/Infrastructure Changes
-
.github/actions/run-tests/action.yml: Addedexclude-patterninput for comma-separated test exclusion. Now properly combines filter and exclude patterns withTEST_EXCLUDE_PATTERNenv var. Added JUnit XML report upload step. -
.github/actions/run-package-tests/action.yml: Propagatedexclude-patterninput to run-tests action. -
.github/workflows/build.yaml:- Removed standalone
verify-base-non-rootandverify-curobo-non-rootjobs (folded intotest-isaaclab-ovandtest-curoborespectively) - Added
exclude-pattern: "test_rendering_"to test-isaaclab-tasks shards 1-3 - Removed rendering-correctness tests from
TESTS_TO_SKIPintools/test_settings.py
- Removed standalone
-
.github/workflows/install-ci.yml: Added JUnit XML report upload for both uv and conda install tests. -
docker/Dockerfile.base: Addedlibgmp-devfor arm64 pytetwild build.
Core Cloning Changes
-
source/isaaclab/isaaclab/cloner/clone_plan.py: Addedenv_posefield (shape[num_envs, 7]) with identity default. MadeClonePlanmutable (removedfrozen=True). -
source/isaaclab/isaaclab/cloner/cloner_utils.py:make_clone_plannow returnstuple[tuple[str, ...], tuple[str, ...], torch.Tensor]instead ofClonePlan— caller assembles theClonePlanwith pose buffer. -
source/isaaclab/isaaclab/scene/interactive_scene.py:- Merged
_default_env_originsinto 7-element_default_env_posetensor - Set clone plan immediately after building (before
_add_entities_from_cfg) - Fixed logic bug at line 255:
if hasattr(cfg, "spawn") or cfg.spawn is Noneshould beandnotor
- Merged
Newton-specific Changes
-
source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py: Added_world_xformstracking and world-site injection per environment. -
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py:- Added
per_worldparameter tocl_register_site()for env-scoped bodyless sites _cl_inject_sitesnow returns(global_sites, proto_sites, world_sites)tuple- Added
_world_xformsclass attribute for cloned-world transforms
- Added
-
source/isaaclab_newton/isaaclab_newton/sim/views/newton_site_frame_view.py: Major refactor — body patterns now resolved from ClonePlan destination templates, not USD prims. -
Asset/sensor initialization (
articulation.py,rigid_object.py,rigid_object_collection.py,ray_caster.py,joint_wrench_sensor.py): Switched from USD prim traversal to Newton body-label matching viafnmatch.
⚠️ New Issue Found
source/isaaclab/isaaclab/scene/interactive_scene.py:255 — Logic bug in condition:
if hasattr(cfg, "spawn") or cfg.spawn is None or self.env_ns not in prim_path:This should be and not or. With or, the condition passes if hasattr(cfg, "spawn") is True (which it always is for spawner configs), bypassing the intended spawn is None check. The original code (before this commit) had:
if not hasattr(cfg, "spawn") or cfg.spawn is None or self.env_ns not in prim_path:
continueThe not was dropped, inverting the logic incorrectly.
✅ Positive Changes
- Documentation update (
docs/source/how-to/cloning.rst): Clarified thatmake_clone_planreturns raw components, not aClonePlanobject. - Test updates: All tests properly updated to use the new
make_clone_plantuple return. - VBD managers (
isaaclab_contrib): All three VBD managers updated consistently for world-site handling. - Changelog fragments properly added.
Previous findings remain outstanding.
Update (e8da390): This is a significant commit with extensive changes across CI, cloning, and Newton components:
CI/Infrastructure Changes
-
.github/actions/run-tests/action.yml: Addedexclude-patterninput for comma-separated test exclusion. Now properly combines filter and exclude patterns withTEST_EXCLUDE_PATTERNenv var. Added JUnit XML report upload step. -
.github/actions/run-package-tests/action.yml: Propagatedexclude-patterninput to run-tests action. -
.github/workflows/build.yaml:- Removed standalone
verify-base-non-rootandverify-curobo-non-rootjobs (folded intotest-isaaclab-ovandtest-curoborespectively) - Added
exclude-pattern: "test_rendering_"to test-isaaclab-tasks shards 1-3 - Removed rendering-correctness tests from
TESTS_TO_SKIPintools/test_settings.py
- Removed standalone
-
.github/workflows/install-ci.yml: Added JUnit XML report upload for both uv and conda install tests. -
docker/Dockerfile.base: Addedlibgmp-devfor arm64 pytetwild build.
Core Cloning Changes
-
source/isaaclab/isaaclab/cloner/clone_plan.py: Addedenv_posefield (shape[num_envs, 7]) with identity default. MadeClonePlanmutable (removedfrozen=True). -
source/isaaclab/isaaclab/cloner/cloner_utils.py:make_clone_plannow returnstuple[tuple[str, ...], tuple[str, ...], torch.Tensor]instead ofClonePlan— caller assembles theClonePlanwith pose buffer. -
source/isaaclab/isaaclab/scene/interactive_scene.py:- Merged
_default_env_originsinto 7-element_default_env_posetensor - Set clone plan immediately after building (before
_add_entities_from_cfg) ⚠️ Logic bug at line 255:if hasattr(cfg, "spawn") or cfg.spawn is Noneshould beandnotor
- Merged
Newton-specific Changes
-
source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py: Added_world_xformstracking and world-site injection per environment. -
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py:- Added
per_worldparameter tocl_register_site()for env-scoped bodyless sites _cl_inject_sitesnow returns(global_sites, proto_sites, world_sites)tuple- Added
_world_xformsclass attribute for cloned-world transforms
- Added
-
source/isaaclab_newton/isaaclab_newton/sim/views/newton_site_frame_view.py: Major refactor — body patterns now resolved from ClonePlan destination templates, not USD prims. -
Asset/sensor initialization (
articulation.py,rigid_object.py,rigid_object_collection.py,ray_caster.py,joint_wrench_sensor.py): Switched from USD prim traversal to Newton body-label matching viafnmatch.
🔴 New Issue Found
source/isaaclab/isaaclab/scene/interactive_scene.py:255 — Logic bug in condition:
if hasattr(cfg, "spawn") or cfg.spawn is None or self.env_ns not in prim_path:This should be and not or. With or, the condition passes if hasattr(cfg, "spawn") is True (which it always is for spawner configs), bypassing the intended spawn is None check. The original code (before this commit) had:
if not hasattr(cfg, "spawn") or cfg.spawn is None or self.env_ns not in prim_path:
continueThe not was dropped, inverting the logic incorrectly.
✅ Positive Changes
- Documentation update (
docs/source/how-to/cloning.rst): Clarified thatmake_clone_planreturns raw components, not aClonePlanobject. - Test updates: All tests properly updated to use the new
make_clone_plantuple return. - VBD managers (
isaaclab_contrib): All three VBD managers updated consistently for world-site handling. - Changelog fragments properly added.
Previous findings remain outstanding.
Update (9e95f98f): This commit addresses all major outstanding findings from the previous review and represents a significant cleanup:
✅ Issues Resolved
1. Interactive scene logic bug fixed — At interactive_scene.py:255, the hasattr/spawn condition is now correct:
if not hasattr(cfg, "spawn") or cfg.spawn is None or self.env_ns not in prim_path:
continueThe not has been restored, properly skipping entries without spawners. The original logic bug from e8da3902 is fixed.
2. Multi-spawn variant guard improved — Line 258 now only processes entities with count > 0 variants:
if (count := num_variants(cfg.spawn)) > 0:
groups.append(...)This properly handles single-variant spawners without adding them to the clone plan groups.
🔍 Changes in This Commit
source/isaaclab/isaaclab/scene/interactive_scene.py: Fixed logic condition and simplified variant handlingsource/isaaclab/isaaclab/cloner/cloner_utils.py: Addediter_clone_plan_matches()utility for querying ClonePlan entries by destination path expression — well-designed helper with comprehensive test coveragesource/isaaclab_newton/isaaclab_newton/sim/views/newton_site_frame_view.py: Major refactoring (-414 lines net) — simplified to use direct body-label resolution instead of USD parent-relative tracking- Documentation: Updated
cloning.rstto explain the caller's responsibility for composingClonePlanwith pose buffer - Tests: Comprehensive new tests for
iter_clone_plan_matches, updated existing tests for the new API
🧪 Test Coverage Assessment
| Component | Status |
|---|---|
iter_clone_plan_matches |
✅ Thorough edge cases |
| Newton frame view clone-plan resolution | ✅ New tests added |
| Camera empty prims fallback | |
| Sensor base Newton path | ✅ Integration tests updated |
⚡ Performance Note
The iter_clone_plan_matches sorting key still calls format() multiple times per match (as noted in previous review). For typical clone plans this is negligible, but worth optimizing if plans grow large.
📋 Summary
This commit successfully addresses the critical logic bug and other outstanding issues. The refactored NewtonSiteFrameView is substantially cleaner. Approve recommended pending one remaining minor item: consider adding an explicit unit test for the camera's view_prims fallback path.
ooctipus
force-pushed
the
zhengyuz/newton-clone-plan
branch
2 times, most recently
from
4639a8e to
690943d
Compare
github-actions
Bot
added
the
documentation
ooctipus
force-pushed
the
zhengyuz/newton-clone-plan
branch
5 times, most recently
from
e8da390 to
ca54ca2
Compare
ooctipus
force-pushed
the
zhengyuz/newton-clone-plan
branch
from
ca54ca2 to
6c8a9fb
Compare
2 participants
Description
Fixes Newton replicated-scene cloning so clone-plan source paths are available before sensor construction and asset USD replication is skipped when Newton handles physics replication.
This also updates Newton frame views, ray caster and joint wrench sensor resolution, Newton camera preparation, deformable managers, and DexSuite point-cloud sampling to resolve against clone-plan source prims and Newton model labels instead of cloned destination USD prims.
Fixes # N/A
Type of change
Screenshots
N/A
Validation
./isaaclab.sh -fwas attempted. It passed ruff, formatting, whitespace, YAML/TOML, merge-conflict, private-key, debug-statement, codespell, license, and RST checks before failing inend-of-file-fixerwithPermissionErroron root-owned files in this checkout. The working tree remained clean after the attempt.git diff --check upstream/develop...HEADpassed.Checklist
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