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 publishing the clone plan to
Confidence Score: 3/5Not safe to merge as-is: two newly introduced code paths crash with AttributeError when SimulationContext.instance() returns None, and the public cloner stub exports a symbol (cfg_source_path) that does not exist in the implementation. The new resolve_matching_prims_from_source utility is called from every sensor and asset initializer added in this PR; without the instance() null check it will crash in unit-test environments and any headless setup that exercises sensors before a SimulationContext is created. The same unguarded pattern is present in NewtonSiteFrameView._resolve_site_specs. Additionally, cfg_source_path is declared in the public stub and wired through lazy_export, but the function body is absent from cloner_utils.py, so any consumer of the published API who tries to use it will hit an ImportError. source/isaaclab/isaaclab/sim/utils/queries.py (missing None guard on line 394), source/isaaclab/isaaclab/cloner/init.pyi (undefined cfg_source_path export), and source/isaaclab_newton/isaaclab_newton/sim/views/newton_site_frame_view.py (same missing None guard in _resolve_site_specs). Important Files Changed
Sequence DiagramsequenceDiagram
participant IS as InteractiveScene
participant SC as SimulationContext
participant S as Sensor / Asset
participant CU as cloner_utils
participant NM as NewtonManager
participant QU as queries.py
IS->>IS: _build_clone_plan_from_cfg()
IS->>SC: set_clone_plan(plan)
Note over IS,SC: plan published BEFORE sensor construction
IS->>S: _add_entities_from_cfg()
S->>QU: resolve_matching_prims_from_source(prim_path)
QU->>SC: instance().get_clone_plan()
QU->>CU: path_source_path(prim_path, plan)
CU-->>QU: (source_path, dest_glob, suffix)
QU-->>S: [(source_prim, dest_expr), ...]
alt "Newton backend and clone_usd=False"
IS->>NM: newton_physics_replicate(...)
NM->>NM: _cl_inject_sites(builder, protos)
NM->>NM: store _world_xforms
NM->>NM: _cl_site_index_map populated
S->>NM: _on_physics_ready then _initialize_from_site_map()
else USD/PhysX backend
IS->>IS: usd_replicate(stage, ...)
S->>S: _initialize_from_specs(model)
end
|
![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 Update: skip usd cloning in pure newton path
Re-review of commits 87c949f2...f49038c4 — This increment addresses several important issues and includes significant modernization of the build system.
📋 Incremental Changes Summary
1. Issue #4970 Fix: Stale sensor data after scene.reset(env_ids)
The core fix across joint_wrench_sensor.py, pva.py, and their respective kernels adds timestamp guards:
# Skip envs that have not been stepped since their last reset: PhysX's incoming joint
# wrench still holds pre-reset values, so reading it now would inject stale data (#4970).
if timestamp[env] == 0.0:
return✅ Well-implemented — The timestamp guard at the Warp kernel level is the correct place to catch stale buffers before they propagate. The new regression tests in test_contact_sensor.py, test_imu.py, test_joint_wrench_sensor.py, and test_pva.py thoroughly validate the fix.
2. Build System Modernization: setup.py → pyproject.toml
Migrated 8 packages to pure pyproject.toml:
isaaclab_physxisaaclab_ppispisaaclab_rlisaaclab_tasksisaaclab_tasks_experimentalisaaclab_teleopisaaclab_visualizers
✅ Clean modernization — Removes toml build dependency, uses importlib.metadata for version retrieval. The isaaclab_rl changelog note about normalized extra names (rsl_rl → rsl-rl, rl_games → rl-games) is appropriate PEP 685 compliance.
3. Runtime Compatibility Validation
sim_launcher.py now validates incompatible backend combinations:
def validate_runtime_compatibility(env_cfg, launcher_args):
"""Validate that the resolved physics, renderer, and visualizer combination is supported.
The OVRTX renderer is a kitless renderer that cannot run together with Isaac Sim / Kit
runtimes (PhysxCfg physics or the Kit visualizer).
"""✅ Good UX improvement — Clear error messages guide users to the correct preset (isaacsim_rtx_renderer) instead of failing with opaque runtime errors. The new test_runtime_compatibility.py covers all valid/invalid combinations.
4. DexSuite Point-Cloud Sampling Fix
In isaaclab_tasks/.../mdp/utils.py:
from isaaclab.cloner.cloner_utils import iter_clone_plan_matches
sample_targets: list[tuple[str, tuple[int, ...]]] = []
clone_plan = sim_utils.SimulationContext.instance().get_clone_plan()
for _, _, source_path, env_ids in iter_clone_plan_matches(clone_plan, prim_path):
sample_targets.append((source_path, env_ids))✅ Correctly addresses Newton replicated scenes — Instead of iterating num_envs and string-replacing .* with indices (which fails for Newton's source-only USD), this now properly resolves against ClonePlan sources and broadcasts samples to all destination env_ids.
5. Sensor Resolution Refactoring
Multiple sensors (joint_wrench_sensor.py, pva.py, ray_caster.py) now use:
resolve_matching_prims_from_source()instead offind_first_matching_prim()- Proper ClonePlan-aware source/destination resolution
✅ Consistent pattern — All sensors now follow the same ClonePlan-aware resolution pattern established earlier in the PR.
6. Visualizer Test Refactoring
The monolithic test_visualizer_cartpole_integration.py is split into:
test_visualizer_integration_newton.pytest_visualizer_integration_physx.pytest_visualizer_tiled_integration_newton.pytest_visualizer_tiled_integration_physx.pyvisualizer_integration_utils.py(shared utilities)
✅ Good test hygiene — Separating by physics backend improves CI parallelization and failure isolation.
🟡 Minor Observations
1. source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/dexsuite/mdp/observations.py:237-238
Deprecation warning fix for matplotlib colormap access:
import matplotlib
turbo = matplotlib.colormaps["turbo"]✅ Good fix, though unrelated to the main PR topic.
2. setup.py still exists in isaaclab_visualizers/
The file was not deleted despite the new pyproject.toml. This appears intentional (the setup.py has different extras), but worth confirming if both are needed.
✅ Previous Findings Status
| Finding | Status |
|---|---|
| Interactive scene logic bug | ✅ Fixed in 9e95f98f |
iter_clone_plan_matches sorting optimization |
|
| Camera empty prims fallback test | |
| Newton site frame view docstring expansion |
📊 Test Coverage in This Increment
| Component | Coverage |
|---|---|
| #4970 stale data fix | ✅ 4 new regression tests |
| Runtime compatibility | ✅ 7 new tests |
| Visualizer integration | ✅ Refactored, same coverage |
| DexSuite point-cloud |
📋 Verdict
LGTM with minor notes — This increment successfully:
- Fixes a meaningful bug (#4970 stale sensor data after partial reset)
- Modernizes the build system to pure
pyproject.toml - Adds user-friendly validation for incompatible runtime combinations
- Ensures DexSuite point-cloud sampling works with Newton replicated scenes
The previous critical findings have been addressed. No new blocking issues found.
Reviewed commits: 87c949f2...f49038c4
Previous review SHA: 87c949f27ae97cc7fff8f7b922d42ccc9169b530
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
2 times, most recently
from
6c8a9fb to
87c949f
Compare
There was a problem hiding this comment.
Update (8374e25): This commit batch includes lint fixes and several merged PRs since the last review.
Changes in This Update
1. Newton Clone Plan Skip USD Cloning — The core feature continues to evolve:
newton_manager.py: Addedworld_sitestracking via_cl_inject_sitesreturning a third element and storing_world_xformsper cloned worldnewton_site_frame_view.py: Major refactoring to use clone-plan-aware site resolution with_resolve_site_specsand_initialize_from_specsray_caster.py: Refactored to use clone plan for site registration withper_worldflag supportjoint_wrench_sensor.py: Switched toresolve_matching_prims_from_sourcefor articulation root discovery
2. Fixed tendons support (PR #5522) — Added passive fixed tendon support for Newton/mjwarp:
- New
robot_tendon_propertiesevent term for tendon parameter randomization - Shadow hand Newton scene uses MJCF version with fixed tendons
3. Various Fixes — GearAssembly env count, skrl MARL observation spaces, doc build, changelog tooling, OvPhysX Kit visualizer guard.
Assessment
✅ Good progress — The clone plan integration is more complete with proper world-site tracking for per-environment transforms.
✅ Tests updated — The test_site_injection.py and test_views_xform_prim_newton.py tests have been updated to cover the new functionality.
Outstanding Items (from previous review)
Still Open:
-
utils.py:48—clone_planNone handling still needed:clone_plan = sim_utils.SimulationContext.instance().get_clone_plan() for _, _, source_path, env_ids in iter_clone_plan_matches(clone_plan, prim_path):
If
clone_planisNone, this will crash. Should guard withif clone_plan is not None:or use the pattern from other call sites. -
newton_site_frame_view.py:290—assertshould be properifguard:assert env_ids is not None
Production code should use explicit
ifchecks instead ofassert(which can be disabled with-O).
No new blocking issues identified in this commit batch.
ooctipus
force-pushed
the
zhengyuz/newton-clone-plan
branch
2 times, most recently
from
7e3381f to
f49038c
Compare
|
@greptile-apps review again |
| from isaaclab.cloner.cloner_utils import path_source_path # noqa: PLC0415 | ||
| from isaaclab.sim.simulation_context import SimulationContext # noqa: PLC0415 | ||
|
|
||
| plan = SimulationContext.instance().get_clone_plan() |
There was a problem hiding this comment.
SimulationContext.instance() not null-checked before attribute access
SimulationContext.instance() returns None when no simulation has been created (unit tests, headless scripts, or any code path that calls a sensor's init before the simulation context is alive). Chaining .get_clone_plan() directly on the result will raise AttributeError: 'NoneType' object has no attribute 'get_clone_plan'. Because resolve_matching_prims_from_source is called from every sensor's initialization path in this PR (Articulation, RigidObjectCollection, JointWrenchSensor, Camera, etc.), this is a broad regression affecting all those callers.
ooctipus
force-pushed
the
zhengyuz/newton-clone-plan
branch
from
29f145f to
9eb3175
Compare
The camera spawn block was nested under the physics-body probe check, so a camera whose prim_path has no existing prim yet (the common case) hit probe_prim is None and skipped spawning entirely, leaving the camera view with zero prims (and the verify that would have raised was skipped too). Move the spawn out of the probe conditional so it always runs when a spawner is configured; only the body-redirect stays gated on the probe.
ooctipus
force-pushed
the
zhengyuz/newton-clone-plan
branch
from
a30d973 to
74c43eb
Compare
🤖 Follow-up Review (74c43eb)Reviewed the incremental changes from Previously reported issues still present:
New changes look good:
No new issues introduced in this commit. |
| # holds in practice; revert to ``not startswith("ovphysx")`` if | ||
| # ``physx.clone()`` errors on already-populated targets. | ||
| clone_usd=True, | ||
| clone_usd=not is_newton_replicated_scene or has_kit(), |
There was a problem hiding this comment.
Can this just be not is_newton_replicated_scene? Since non-newton implies kit will be there?
There was a problem hiding this comment.
we could also have ovphysx path which won't use kit
There was a problem hiding this comment.
removed in follow up decouple cloner pr
| first_env_root_prims = get_all_matching_child_prims( | ||
| first_env_matching_prim_path, | ||
| predicate=lambda prim: prim.HasAPI(UsdPhysics.ArticulationRootAPI) | ||
| and prim.GetAttribute("physxArticulation:articulationEnabled").Get() is not False, |
There was a problem hiding this comment.
the and check for articulation enabled is removed completely, is this no longer needed?
There was a problem hiding this comment.
UsdPhysics.ArticulationRootAPI needs to exist but "physxArticulation:articulationEnabled" is currently a bit too strict for some usd, we can fix those usd and turn them on if thats better long run. but enable now will break them
| def has_rigid_body_api(prim) -> bool: | ||
| return bool(prim.HasAPI(UsdPhysics.RigidBodyAPI)) | ||
|
|
||
| matches = resolve_matching_prims_from_source(self.cfg.prim_path) |
There was a problem hiding this comment.
I see this block of code in many places, can we make a clean helper function like:
resolve_single_descendant_expr(
cfg_path,
predicate=...,
api_name="RigidBodyAPI",
)
and use it everywhere
There was a problem hiding this comment.
Ill clean up in follow up pr
|
|
||
| """ | ||
|
|
||
| def _first_RigidObject_child_path(self): |
There was a problem hiding this comment.
Similar to other comment made, something similar to this helper could be used almost everywhere in the code
| if suffix and sensor_expr.endswith(suffix): | ||
| return sensor_expr[: -len(suffix)] | ||
| return body_path | ||
| def _has_rigid_body_api(prim) -> bool: |
There was a problem hiding this comment.
This function is defined in too many places!
There was a problem hiding this comment.
will clean up in follow up pr
3 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