Add per-env omni:scenePartition authoring in InteractiveScene#5445
Conversation
![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
Summary
This PR adds per-environment scene partitioning for RTX rendering isolation by authoring primvars:omni:scenePartition on env roots and omni:scenePartition on cameras. The implementation is clean and correctly placed after cloning but before physics tensor view creation. The tests are well-documented regression tests that track known RTX limitations.
Architecture Impact
Self-contained with proper integration point. The author_scene_partitions() method is called at the end of clone_environments(), which is the correct location — after USD/physics cloning completes but before sim.reset() creates tensor views. Both ManagerBasedEnv (via InteractiveScene.__init__) and Direct envs (via manual clone_environments() calls) will get partitioning automatically. The method only touches USD layer attributes and doesn't interact with physics state.
Implementation Verdict
Ship it — Minor documentation improvement suggested but not blocking.
Test Coverage
Thorough and well-reasoned. The tests are regression tests for RTX behavior rather than unit tests of the authoring logic itself. The extensive docstring explains why minimal scenes don't work (RTX requires articulation presence) and documents exact measured thresholds. The xfail pattern for the articulation test is appropriate — it will surface when the upstream RTX fix lands. However, there's no direct unit test verifying that the USD attributes are actually authored correctly (checking that prims have the expected attributes after author_scene_partitions()).
CI Status
No CI checks available yet — should verify tests pass before merge.
Findings
🔵 Improvement: source/isaaclab/isaaclab/scene/interactive_scene.py:277-279 — Missing primvar interpolation metadata
For primvars, RTX typically expects the interpolation metadata to be set. While the default may work, explicitly setting it would be more robust:
# After creating the primvar attribute, consider:
# primvar_api = UsdGeom.PrimvarsAPI(env_prim)
# primvar = primvar_api.CreatePrimvar("omni:scenePartition", Sdf.ValueTypeNames.Token)
# primvar.SetInterpolation(UsdGeom.Tokens.constant)That said, the current Sdf.JustCreatePrimAttributeInLayer approach is valid and may be intentional for performance (avoiding schema traversal). Clarify in comments if intentional.
🔵 Improvement: source/isaaclab/test/scene/test_scene_partitioning.py:116 — Env not closed on build failure
If gym.make() succeeds but the subsequent hide_prims loop fails (e.g., due to a prim path typo), the env won't be closed:
def _build_dexsuite_env(hide_prims: list[str], scale_object_to: float | None = None):
...
env = gym.make("Isaac-Dexsuite-Kuka-Allegro-Lift-v0", cfg=env_cfg)
# If this section throws, env leaks
stage = env.unwrapped.scene.stage
for env_idx in range(NUM_ENVS):
...Consider wrapping in try/except to close on error, or document that callers must handle this.
🔵 Improvement: source/isaaclab/test/scene/test_scene_partitioning.py — Missing direct unit test for attribute authoring
The tests verify RTX rendering behavior but don't directly verify that author_scene_partitions() creates the expected USD attributes. A simple unit test could verify:
def test_scene_partition_attributes_authored():
# Create minimal scene, call clone_environments
# Verify env_0 has primvars:omni:scenePartition = "env_0"
# Verify cameras have omni:scenePartition = "env_N"This would catch authoring bugs independently of RTX behavior.
🔵 Improvement: source/isaaclab/isaaclab/scene/interactive_scene.py:285 — Consider logging partition authoring
Given this is a new feature that affects rendering behavior, a debug-level log message would help users understand when partitioning is active:
logger.debug(f"Authored scene partitions for {len(self.env_prim_paths)} environments")🔵 Improvement: source/isaaclab/test/scene/test_scene_partitioning.py:42-51 — Shim could be module-level constant
The _shim_omni_physics_tensors_api() function is called at module load but the shim pattern is duplicated across test files. Consider centralizing this in a test utilities module if it's needed elsewhere.
🟡 Warning: source/isaaclab/test/scene/test_scene_partitioning.py:134 — Action shape may be incorrect for batched envs
action_shape = env.action_space.shape
for _ in range(WARMUP_STEPS):
actions = (torch.rand(action_shape, ...) * 2.0 - 1.0) * ACTION_SCALEFor vectorized envs, action_space.shape typically doesn't include the batch dimension. Verify this works correctly with NUM_ENVS=4 — if action_space.shape is (action_dim,), the actions tensor should be (num_envs, action_dim). The test may work by accident if gym auto-broadcasts, but explicit shape (NUM_ENVS, *action_shape) would be clearer.
Greptile SummaryThis PR adds Confidence Score: 4/5Safe to merge; all findings are non-blocking style/best-practice suggestions with no impact on correctness for the common case. Only P2 findings: source/isaaclab/isaaclab/scene/interactive_scene.py — review the Important Files Changed
Sequence DiagramsequenceDiagram
participant Init as InteractiveScene.__init__
participant Clone as clone_environments()
participant Author as author_scene_partitions()
participant USD as USD Stage (Sdf layer)
participant RTX as RTX Renderer
Init->>Clone: clone_environments(copy_from_source)
Clone->>USD: physics_clone_fn / usd_replicate (per env prims)
Clone->>Author: author_scene_partitions()
loop for each env_path in env_prim_paths
Author->>USD: Sdf.ChangeBlock
Author->>USD: JustCreatePrimAttributeInLayer(root, primvars:omni:scenePartition = env_i)
Author->>USD: JustCreatePrimAttributeInLayer(camera, omni:scenePartition = env_i)
end
Note over Author,USD: Runs before sim.reset() so physics tensor view is not invalidated
RTX->>USD: reads primvars:omni:scenePartition on env root
RTX->>RTX: culls geometry to matching partition per camera
Reviews (1): Last reviewed commit: "Add per-env omni:scenePartition authorin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| """ | ||
| root_layer = self.stage.GetRootLayer() | ||
| token_type = Sdf.ValueTypeNames.Token | ||
| variability = Sdf.VariabilityVarying |
There was a problem hiding this comment.
VariabilityVarying is semantically incorrect for a static partition token
Sdf.VariabilityVarying signals that the attribute can carry time-sampled values and should be interpolated per the stage's time-code. A scene-partition token is a permanent, non-animated assignment — Sdf.VariabilityUniform is the correct choice. USD renderers (and RTX) are supposed to treat Uniform attributes as constant across time, which matches the intent here. Using Varying wastes a small amount of metadata overhead and could confuse downstream tools that inspect attribute variability.
| variability = Sdf.VariabilityVarying | |
| variability = Sdf.VariabilityUniform |
|
|
||
| """Launch Isaac Sim Simulator first.""" |
There was a problem hiding this comment.
Python treats only the first string literal in a module as the module __doc__. The """Launch Isaac Sim Simulator first.""" string is unreachable as a docstring and becomes a no-op string constant. Other Isaac Lab test files typically use a plain # Launch Isaac Sim Simulator first. comment here instead.
| """Launch Isaac Sim Simulator first.""" | |
| # Launch Isaac Sim Simulator first. |
| for prim in Usd.PrimRange(env_prim): | ||
| if prim == env_prim: | ||
| attr_path = prim.GetPath().AppendProperty("primvars:omni:scenePartition") | ||
| elif prim.IsA(UsdGeom.Camera): | ||
| attr_path = prim.GetPath().AppendProperty("omni:scenePartition") | ||
| else: | ||
| continue | ||
| Sdf.JustCreatePrimAttributeInLayer(root_layer, attr_path, token_type, variability, False) | ||
| root_layer.GetAttributeAtPath(attr_path).default = token |
There was a problem hiding this comment.
Usd.PrimRange skips cameras inside USD instances (instance proxies)
Usd.PrimRange(env_prim) uses the default traversal predicate, which does not descend into instance prototypes — it only visits instance proxy prims at the top level, not their children. If any env contains instanced geometry (e.g. an instanced sensor rig), cameras nested inside those instances will be invisible to this traversal and will not receive the omni:scenePartition attribute. To make the traversal instance-aware, pass Usd.TraverseInstanceProxies(): for prim in Usd.PrimRange(env_prim, Usd.TraverseInstanceProxies()):
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
e840880 to
a852485
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commits appear to be a rebase/refresh — the diff and implementation are identical to the previous review. No code changes were made to address the previous findings.
Previous findings still apply:
- The action shape concern (🟡 Warning) should be verified
- The suggested improvements (unit test for attribute authoring, env cleanup on build failure, logging) remain valid but are non-blocking
CI Status: pre-commit is failing — this needs to be fixed before merge.
Verdict: The implementation is correct. Fix the pre-commit failure and consider the previous improvement suggestions, but the core functionality is ready to ship.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commit (587b425) addresses the previous CI failure — pre-commit now passes ✅. The implementation remains unchanged and correct.
Previous findings status:
- The action shape concern was minor and the implementation is correct as-is
- Suggested improvements (unit test for attribute authoring, logging) remain valid but non-blocking
Verdict: Ship it. The core functionality is correct, tests are comprehensive with appropriate xfail handling for the known RTX articulation limitation, and CI is green.
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
db053ea to
da3e021
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commit (da3e021) adds a monkeypatch for author_scene_partitions in the test_clone_environments_non_cfg_invokes_visualizer_clone_fn unit test. This is a correct fix — the test mocks stage with a bare object(), so the USD prim walk in author_scene_partitions would fail without the patch.
Change is correct and necessary. The test now properly isolates its concern (verifying clone callback invocation) without triggering the real USD traversal code, which is separately exercised by the end-to-end test_partitioning_isolates_* tests.
Verdict: Ship it.
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
da3e021 to
019ca00
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
Previous concern about the unit test crashing due to author_scene_partitions being called on a mocked stage has been addressed — the monkeypatch is now in place.
The new commit (019ca00) adds a comment clarifying why the monkeypatch exists. This is a documentation-only change and is correct.
Verdict: Ship it.
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
019ca00 to
1a0e549
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commit (1a0e549) adds a clarifying comment in test_clone_environments_non_cfg_invokes_visualizer_clone_fn explaining that the author_scene_partitions monkeypatch exists because the test mocks stage with a bare object(), and that the partition USD walk is exercised end-to-end by the test_partitioning_isolates_* tests below.
This is a documentation-only change that improves code clarity. Previous concerns have been addressed.
Verdict: Ship it.
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
1a0e549 to
acaba41
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commit (acaba41) makes no functional changes to the PR. Reviewing the diff between the previous commit and HEAD shows identical code — this appears to be a rebase or no-op commit.
Previous concerns have been addressed. The implementation is correct, test coverage is thorough (with appropriate xfail for the known RTX articulation limitation), and the changelog accurately describes the feature.
Verdict: Ship it.
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
acaba41 to
76f8fb4
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds per-environment omni:scenePartition USD attribute authoring to InteractiveScene for RTX cull-by-env rendering. The implementation writes inheriting primvars on env roots and explicit tokens on cameras, enabling RTX renderers to isolate geometry per-environment. The approach is clean, well-documented, and includes regression tests with appropriate xfail markers for known RTX limitations.
Design Assessment
Architecture: ✅ Good
- File placement is appropriate — the scene-level partition authoring belongs in
InteractiveScene. - The method is called at the end of
clone_environments(), ensuring both ManagerBasedEnv and Direct envs benefit automatically. - Using
Sdf.ChangeBlock()for batched USD edits is the correct pattern for performance. - The O(N + cameras) complexity is acceptable; avoiding per-descendant authoring is the right call given RTX primvar inheritance.
Cross-module impact: ✅ Low risk
- The partition attributes are RTX-specific and ignored by renderers that don't support them.
- No changes to public APIs beyond the new
author_scene_partitions()method. - Physics tensor views are not affected since authoring happens before
sim.reset().
Findings
🔵 Suggestion — interactive_scene.py:378-379: Consider adding a docstring comment clarifying why the token is extracted via rsplit (to handle nested paths correctly).
🔵 Suggestion — test_interactive_scene.py:499,559: The magic threshold values (3.0 and 5.0) for tile diff assertions could benefit from brief inline comments explaining their derivation.
🔵 Suggestion — The existing clone-callback test now requires a monkeypatch for author_scene_partitions. This is properly handled, but consider whether the mock stage in that test should eventually be upgraded to support the full partition flow.
Test Coverage
- Feature PR: ✅ Coverage adequate
- Two regression tests added (
test_partitioning_isolates_rigid_object,test_partitioning_isolates_articulation) - The xfail pattern for the articulation test is a good approach — it will automatically signal when the RTX fix lands
- Existing tests properly monkeypatched to avoid new dependency
- Gaps: None critical — the tests exercise the key rendering-isolation behavior
CI Status
- Core checks passed (labeler, broken links, detect changes)
- Several checks still pending (pre-commit, license-check, installation tests, docker builds)
- Recommend waiting for full CI green before merge
Verdict
Ship it ✅
This is a well-designed, non-breaking feature addition that provides automatic scene partitioning for RTX rendering isolation. The implementation follows USD best practices, has appropriate test coverage including forward-looking regression hooks, and the code is clean and well-documented. The minor suggestions above are all optional polish items.
| the per-env USD prims are cloned and before the physics backend takes its tensor view | ||
| of the stage in ``sim.reset`` — authoring USD attributes after the view exists | ||
| invalidates it. | ||
| """ |
There was a problem hiding this comment.
🔵 Style suggestion: Consider adding a brief comment explaining why rsplit("/", 1)[-1] is used here — it correctly handles arbitrarily nested env paths and extracts just the leaf name (e.g., env_3 from /World/envs/env_3).
| """ | |
| token = env_path.rsplit("/", 1)[-1] # leaf name, e.g. "env_3" |
| cube.write_root_pose_to_sim_index( | ||
| root_pose=wp.from_torch(root_pose.contiguous(), dtype=wp.transformf), | ||
| env_ids=wp.from_torch(torch.arange(scene.num_envs, device=device, dtype=torch.int32)), | ||
| ) |
There was a problem hiding this comment.
🔵 Minor: Consider adding a brief comment explaining the threshold choice (3.0 for RGB mean diff). For future maintainers, it helps to know this was derived from empirical testing where partitioned envs with offset cubes showed diffs well above rendering noise.
| ) | ||
| robot.write_joint_position_to_sim_index(position=target_pos) | ||
| robot.set_joint_position_target_index(target=target_pos) | ||
| for _ in range(4): |
There was a problem hiding this comment.
🔵 Minor: Same suggestion — a brief comment on why 5.0 was chosen as the threshold would help future maintainers understand the expected visual difference when articulation partitioning works correctly.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds automatic per-environment omni:scenePartition authoring in InteractiveScene to enable RTX cull-by-env rendering. The implementation writes inheriting primvars at env roots that propagate to descendant geometry via RTX primvar inheritance, plus explicit attributes on cameras. Two regression tests are included, one of which is expected to xfail until an RTX articulation fix lands.
Design Assessment
Architecture: ✅ Sound approach
The design correctly leverages RTX's primvar inheritance for scene partitioning, which is the spec'd mechanism. Key design decisions are well-reasoned:
-
Placement in
_aggregate_scene_data_requirements()— Called after USD cloning but beforesim.reset()takes tensor views. This timing is correct and well-documented. -
O(N + cameras) complexity — Intentionally avoids per-descendant authoring since RTX honors primvar inheritance. This is the right performance trade-off.
-
Unconditional authoring — Non-RTX renderers ignore the primvar, so authoring is safe across all backends.
-
Camera handling — Cameras need the non-primvar
omni:scenePartitiontoken explicitly (not via inheritance), which the code correctly handles.
One architectural concern: The method is called from _aggregate_scene_data_requirements(), which semantically aggregates requirements — scene partition authoring is a side effect that arguably doesn't belong there. However, the timing constraint (after cloning, before tensor views) limits placement options, so this is acceptable.
Findings
🔵 Suggestion interactive_scene.py:376-377 — Consider adding a brief comment explaining why cameras need the non-primvar attribute vs. the primvar used for geometry.
# Cameras need explicit omni:scenePartition (not primvar) per RTX spec
elif prim.IsA(UsdGeom.Camera):🔵 Suggestion interactive_scene.py:373 — The token extraction env_path.rsplit("/", 1)[-1] is clean, but consider using Sdf.Path(env_path).name for consistency with USD API conventions elsewhere in the codebase.
🔵 Suggestion test_interactive_scene.py:515-517 — The randomized cube positions could theoretically result in similar positions across environments (unlikely but possible with only 4 envs). Consider using deterministic, spread-apart positions for more robust test isolation:
# Deterministic spread positions instead of random
root_pose[:, 1] = torch.tensor([-0.4, -0.1, 0.1, 0.4], device=device) # Y spread
root_pose[:, 2] = torch.tensor([0.8, 1.0, 1.2, 1.4], device=device) # Z spread🔵 Suggestion test_interactive_scene.py:473 — The monkeypatch for author_scene_partitions is correct and well-documented. Good defensive testing pattern.
🟡 Warning test_interactive_scene.py — The tests depend on isaaclab_assets.KUKA_ALLEGRO_CFG and build_simulation_context with device="cuda:0". Ensure CI runners have GPU access and the asset is available. The @pytest.mark.isaacsim_ci marker suggests this is handled, but worth verifying.
Test Coverage
- Feature PR: Yes — Adequate coverage with two complementary tests
test_partitioning_isolates_rigid_object: Tests the happy path (RigidObject partitioning)test_partitioning_isolates_articulation: Regression hook for known RTX limitation (xfail)- Gaps: No negative tests (e.g., verifying behavior when stage is invalid), but the
if not env_prim.IsValid(): continueguard handles this gracefully
CI Status
- ❌
Check changelog fragments— Missing changelog entry (required for feature PRs) - ⏳ Several checks still pending (pre-commit, license-check, Docker builds)
- ✅ Build Wheel, Broken Links, Labeler passing
Verdict
Minor fixes needed
The implementation is architecturally sound and follows RTX partitioning specs correctly. The code is well-documented with clear docstrings explaining timing constraints and design rationale. Tests are thoughtful, including an xfail marker as a regression hook for a known upstream RTX issue.
Required before merge:
- Add changelog fragment (CI is failing on this)
Optional improvements:
- Minor code suggestions above (comments, USD API consistency)
- Consider deterministic test positions for robustness
Nice work on the documentation and test strategy! 🎉
Update (6f4895b):
Reviewed incremental changes from cbc7ee7 → 6f4895b. The new commits include many unrelated changes (import path updates from isaaclab.utils to isaaclab.utils.configclass, new RenderCfg RT2 settings, camera data warp dtype changes, etc.), but the scene partitioning code in interactive_scene.py and its tests are unchanged.
Previous inline comments still apply:
- P2:
Sdf.VariabilityVarying→VariabilityUniform(not addressed) - P2:
Usd.TraverseInstanceProxies()for camera traversal (not addressed) - Minor suggestions (comments, Sdf.Path usage) — optional, not blocking
No new issues introduced by this commit.
| if requirements != current_req: | ||
| self.sim.update_scene_data_requirements(requirements) | ||
|
|
||
| self.author_scene_partitions() |
There was a problem hiding this comment.
🔵 Architectural note: Calling author_scene_partitions() from _aggregate_scene_data_requirements() is a timing necessity (after cloning, before tensor views), but semantically it's a side effect in a method focused on requirement aggregation. Consider adding a comment explaining this coupling, or in a future refactor, creating a dedicated post-clone hook point.
| of the stage in ``sim.reset`` — authoring USD attributes after the view exists | ||
| invalidates it. | ||
| """ | ||
| root_layer = self.stage.GetRootLayer() |
There was a problem hiding this comment.
🔵 Minor: Consider using Sdf.Path(env_path).name instead of env_path.rsplit("/", 1)[-1] for consistency with USD API conventions used elsewhere in the codebase.
| ) | ||
| # Avoid binding this unit test to global SimulationContext singleton state. | ||
| monkeypatch.setattr(InteractiveScene, "device", property(lambda self: "cpu")) | ||
| # This test mocks ``stage`` with a bare ``object()``; the partition USD walk needs a real |
There was a problem hiding this comment.
👍 Good defensive testing pattern — monkeypatching out author_scene_partitions for the mock-stage test prevents crashes while the real functionality is exercised by the dedicated test_partitioning_isolates_* tests below.
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
cbc7ee7 to
6f4895b
Compare
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
6f4895b to
8bd2846
Compare
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
3 times, most recently
from
07ac6af to
2cfa75d
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — Follow-up Review
Summary
Branch was force-pushed/squashed to a single commit (2cfa75d) with message "fix scene partition", rebasing onto latest develop.
Changes from Previous Review
The diff content is functionally identical to the previously reviewed changes. This is a clean rebase/squash — no new code changes introduced.
Key components unchanged:
InteractiveScene.author_scene_partitions()method with correctSdf.ChangeBlock()batching- Camera partition token authoring in
camera_view.py - RTX scene partitioning enabled in all Kit app files
- Isolated
test_scene_partitioning.pywith proper test isolation for RTX state - Monkeypatching in
test_interactive_scene.pyfor mock-stage tests - Golden images regenerated for new partition behavior
CI Status
- ✅
pre-commit: success - ✅
Build Wheel: success - ⏳ Several checks still in progress (Installation Tests, Docs, license-check)
- ❌
Check changelog fragments: failure — the changelog entry exists (zhengyuz-scene-partitioning.rst), this may be a CI config issue
Previous Findings
All minor suggestions from prior reviews remain applicable but are non-blocking:
- Silent
continueon invalid env prim (could add warning log) - Camera path split assumption comment (minor readability)
Sdf.PathAPI consistency (style nit)
Verdict
Still ready for merge. ✅
The squash/rebase is clean. Human approval from @kellyguo11 already obtained. Once CI stabilizes and the changelog check is resolved, this PR is good to go.
Update (commit ccb384a): The only change since the last review is the addition of source/isaaclab_ov/changelog.d/zhengyuz-scene-partitioning.skip — a changelog skip marker that should resolve the previously-failing "Check changelog fragments" CI check. No functional code changes. Previous inline suggestions (VariabilityUniform, instance proxy traversal, style nits) remain as-is — they are non-blocking and the original comments are still visible. ✅ No new issues.---
Update (commit cb62aef): New push includes two concerns:
-
Scene partitioning improvements:
- ✅
author_scene_partitions()now called directly fromclone_environments()instead of from_aggregate_scene_data_requirements()— addresses the architectural coupling concern from previous review. - Camera partition authoring added to
camera_view.py(correctly setsomni:scenePartitiontoken on spawned visualizer cameras). - OvRTX renderer's
create_scene_partition_attributescommented out (sinceInteractiveScenenow handles it centrally). Minor nit: would be cleaner to remove the line + unused import rather than comment-out.
- ✅
-
Deferred
pxrimports (separate contributor:jichuanh): Moves allfrom pxr import …statements from module-level to function bodies /TYPE_CHECKINGblocks across ~10 files. This fixes Kit's USD binding registration whenpxris imported beforeAppLauncher. Well-structured change.
Status of previous inline findings:
- ✅
author_scene_partitions()placement: Fixed (moved toclone_environments()). - ⬜
VariabilityVarying→VariabilityUniform: Not addressed (non-blocking). - ⬜ Instance proxy traversal (
Usd.TraverseInstanceProxies()): Not addressed (non-blocking). - ⬜ Style nits (
rsplitcomment,Sdf.PathAPI): Not addressed (non-blocking).
No new blocking issues. ✅
Update (commit 504a5ed): ✅ VariabilityVarying → VariabilityUniform fix applied — the attribute is now correctly authored as uniform with isCustom=True. This was the main semantic correctness issue from my earlier reviews.
Status of previous findings:
- ✅
VariabilityUniform: Fixed in this commit. - ⬜ Instance proxy traversal (
Usd.TraverseInstanceProxies()): Not addressed (non-blocking). - ⬜ Style nits (orphaned docstring,
rsplitcomment,Sdf.PathAPI): Not addressed (non-blocking).
No new issues introduced. ✅
Update (commit 30e6ef5): Significant architectural refactoring — scene partitioning logic has been moved from InteractiveScene into IsaacRtxRenderer.prepare_stage(). This is the right layering: the renderer now owns its own scene-preparation concerns rather than requiring the scene to know about renderer internals.
Key changes reviewed:
-
isaac_rtx_renderer.py—prepare_stage()is no longer a no-op:- Iterates
/World/envs/env_{i}prims usingUsd.PrimRange - Writes
primvars:omni:scenePartition(token) on each env root for primvar inheritance - Writes
omni:scenePartition(token) onUsdGeom.Cameradescendants only - Uses
Sdf.ChangeBlock()for batched notification — ✅ correct - Uses
Sdf.VariabilityUniformwithisCustom=True— ✅ correct (addresses earlier review feedback) - Properly skips non-env-root, non-Camera prims via
continue— efficient
- Iterates
-
camera_view.py— visualizer camera partition authoring:- Sets
omni:scenePartitionon spawned visualizer cameras using the standardUsdAPI (not low-levelSdf) - Token derived from
path.split("/")[-2](the env name) — works correctly for the expected/World/envs/env_N/CameraNamepattern
- Sets
-
Kit app files (6 files): All correctly add
renderer.scenePartitioning.enabled = truebefore RTX init -
New test:
test_isaac_rtx_renderer_scene_partitioning.py:- Well-motivated isolation (RTX partition state persists across scenes in one Kit app session)
- Tests both
RigidObjectandArticulationscenarios - Uses
env_spacing=0.0+ distinct poses to verify partitioning isolation — good test design - Asserts tile RGB difference thresholds (>3.0 rigid, >5.0 articulation)
-
Golden images: Regenerated as expected from partition-correct rendering. LFS pointers updated.
-
Changelog: Proper
.rstentry +.skipmarkers for packages without user-facing changes.
Remaining non-blocking observations:
- ⬜ Instance proxy traversal:
Usd.PrimRange(env_prim)won't traverse instance proxies. Ifreplicate_physics=True(USD native instancing), cameras inside instances won't get the attribute. However, the test usesreplicate_physics=Falseand the currentInteractiveSceneworkflow callsprepare_stageafter cloning — so this is acceptable for the current use case. Worth a follow-up if native instancing is supported later. - ⬜
camera_view.pyuses high-level API vs renderer uses low-levelSdfAPI: Minor style inconsistency but both are correct. The renderer path is performance-sensitive (many envs), so low-levelSdfis justified there.
No blocking issues. ✅ Ready for merge.
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
ccb384a to
cb62aef
Compare
| attr_path = prim.GetPath().AppendProperty("omni:scenePartition") | ||
| else: | ||
| continue | ||
| Sdf.JustCreatePrimAttributeInLayer(root_layer, attr_path, token_type, variability, False) |
There was a problem hiding this comment.
Could you try this change on your local machine?
Sdf.JustCreatePrimAttributeInLayer(root_layer, attr_path, token_type, Sdf.VariabilityUniform, True)
There was a problem hiding this comment.
Re-Review After Latest Commits (504a5ed)
Summary
This PR adds per-environment omni:scenePartition authoring in InteractiveScene for RTX cull-by-env rendering. The implementation moves partition attribute creation from the OvRTX renderer to the core scene infrastructure, ensuring all Kit apps benefit from partitioning automatically.
Latest Change: change to variability uniform
The most recent commit (504a5ed) changes the attribute variability to Sdf.VariabilityUniform. This is the correct approach:
Sdf.JustCreatePrimAttributeInLayer(root_layer, attr_path, token_type, Sdf.VariabilityUniform, True)✅ Good: Using VariabilityUniform is semantically correct for scene partition tokens since they:
- Don't change over time during the simulation
- Are configuration/topology data, not animated values
- Match USD best practices for attributes that describe scene structure
Architecture Review
Strengths:
- Clean separation - Partitioning is now handled at scene construction time, not at renderer preparation
- Kit app enablement - All 6 Kit files now enable
renderer.scenePartitioning.enabledbefore RTX init - Comprehensive test coverage - New
test_scene_partitioning.pywith both RigidObject and Articulation isolation tests - Primvar inheritance - Using
primvars:omni:scenePartitionon env roots propagates correctly to descendant geometry - Camera handling - Cameras get explicit
omni:scenePartitionattributes (not primvars) as required by RTX
The implementation pattern:
- Env roots get
primvars:omni:scenePartition(inherited by geometry) UsdGeom.Cameradescendants getomni:scenePartition(direct attribute)- Uses
Sdf.ChangeBlock()for efficient batch USD edits
Minor Observations
-
Commented-out code in ovrtx_renderer.py:
# create_scene_partition_attributes(stage, num_envs, self._use_ovrtx_cloning)Consider removing the import and commented call entirely if this function is no longer used anywhere, to reduce dead code.
-
camera_view.py duplication: The camera partition logic in
create_visualizer_camera()partially duplicates whatauthor_scene_partitions()does for cameras. Since visualizer cameras are created after scene cloning, this is likely intentional, but a comment clarifying the relationship would help future maintainers. -
Test isolation note: The docstring explaining why these tests live in a separate file (renderer state pollution) is excellent documentation. 👍
Testing
The test design is solid:
test_partitioning_isolates_rigid_object()- validates geometry partitioningtest_partitioning_isolates_articulation()- validates articulated asset partitioning- Both use tile difference thresholds (3.0 and 5.0) to detect rendering isolation
The golden images have been regenerated to reflect the new partitioning behavior.
Automated review by isaaclab-review-bot 🤖
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
504a5ed to
30e6ef5
Compare
There was a problem hiding this comment.
Re-Review After Latest Commit (30e6ef5)
Summary
Great work addressing the reviewer feedback! This commit properly moves the scene partition authoring logic into IsaacRtxRenderer.prepare_stage(), following the guidance from @huidongc, @pbarejko, and @ooctipus that renderer-specific implementation details belong in the renderer layer.
Key Changes in This Commit
1. Architecture Refactor (✅ Approved Approach)
- Scene partition authoring now lives in
IsaacRtxRenderer.prepare_stage()instead ofInteractiveScene - This is the correct separation of concerns — RTX-specific attributes are authored by the RTX renderer
- The
prepare_stage()method now properly handles:primvars:omni:scenePartitionon env roots (inherited by descendant geometry)omni:scenePartitiononUsdGeom.Cameradescendants (direct attribute)
2. Tests Relocated
- Tests moved from
source/isaaclab/test/scene/tosource/isaaclab_physx/test/renderers/ - Makes sense given the partitioning is now renderer-specific
3. Kit App Files (✅ Consistent)
- All 6 Kit files have
renderer.scenePartitioning.enabled = trueadded - Consistent comment:
# Enable before RTX initialization so env-root partitions are honored
Code Review Notes
isaac_rtx_renderer.py - Implementation looks solid:
for prim in Usd.PrimRange(env_prim):
if prim == env_prim:
attr_path = prim.GetPath().AppendProperty("primvars:omni:scenePartition")
elif prim.IsA(UsdGeom.Camera):
attr_path = prim.GetPath().AppendProperty("omni:scenePartition")
else:
continue- Uses
Sdf.ChangeBlock()for efficient batch USD edits ✅ - Correctly uses
Sdf.VariabilityUniformfor static partition tokens ✅ - Handles env root primvar vs camera direct attribute correctly ✅
camera_view.py - Visualizer camera handling:
attr = cam_prim.GetAttribute("omni:scenePartition")
if not attr.IsValid():
attr = cam_prim.CreateAttribute("omni:scenePartition", Sdf.ValueTypeNames.Token)
attr.Set(path.split("/")[-2])- Necessary because visualizer cameras are created after
prepare_stage()is called - Uses
path.split("/")[-2]which extracts the env name (e.g.,env_0from/World/envs/env_0/Camera)
Minor Observations
-
Usd.PrimRange traversal predicate: The default traversal predicate in
Usd.PrimRangedoes not descend into USD instance proxies. If env prims use USD instancing for cameras, they would be missed. However, since the comment in the tests mentionsreplicate_physics=False, this is likely not an issue for the current use case. Worth documenting if this assumption matters. -
OvRTX renderer: Does the OvRTX renderer (in
isaaclab_ov) need similarprepare_stage()logic, or does it handle partitioning differently? The skip file suggests no changes needed, but confirming the OvRTX path works would be good. -
Changelog wording: The changelog says "Fixed" but this is really a new feature (enabling partitioning). Consider "Added" for accuracy.
Testing
The relocated tests exercise both RigidObject and Articulation partitioning scenarios with appropriate tile-difference thresholds. The test isolation rationale in the docstring is excellent documentation.
Automated review by isaaclab-review-bot 🤖
Update (b685361):
Reviewed the incremental changes from 504a5edb → b685361e. This is a large commit that combines scene-partitioning refinements with the broader core/contrib task restructure.
Previous concerns status:
- ✅
Sdf.VariabilityVarying→Sdf.VariabilityUniform— Fixed. The code now correctly usesSdf.VariabilityUniformfor the static partition token. - ✅ Architectural placement —
author_scene_partitions()is now called fromclone_environments()directly (not as a side effect of_aggregate_scene_data_requirements()). Cleaner call site.
Notable design change: Scene partition authoring moved from IsaacRtxRenderer.prepare_stage() back to InteractiveScene.author_scene_partitions(). This is a reasonable architectural choice — the USD primvar is renderer-agnostic metadata that both RTX and OvRTX consume, so placing it at the scene level avoids duplication and correctly separates "what to partition" (scene) from "how to render partitions" (renderer). The OvRTX renderer's create_scene_partition_attributes call is now commented out accordingly.
No new issues found in the scene-partitioning changes. The core/contrib restructure is mechanical (import path renames) and looks consistent.
Update (065ac79): Reviewed incremental changes from b685361e → 065ac79c. This commit refines the architecture and adds an orthogonal event manager feature.
Previous concerns status:
- ✅
Sdf.VariabilityUniform— Still correctly used inIsaacRtxRenderer.prepare_stage() - ✅ Architecture — Scene partition authoring is now cleanly in
IsaacRtxRenderer.prepare_stage()usingSdf.ChangeBlock()and directSdflayer edits. Good separation of concerns. - ℹ️
Usd.PrimRangeinstance proxy traversal — Still uses default predicate, but sincereplicate_physics=Falseis required for partitioning to work, this is acceptable for current use cases. - ℹ️ Previous inline comments on
interactive_scene.pyandtest_scene_partitioning.pyare now moot — those files no longer contain partition logic (moved to renderer layer).
New in this commit:
resample_interval_on_resetadded toEventTermCfg— well-documented, well-tested, clean implementation in both standard and experimental (Warp) event managers.camera_view.pynow authorsomni:scenePartitionon visualizer cameras created afterprepare_stage()— necessary since these cameras are spawned dynamically.- Test file
test_isaac_rtx_renderer_scene_partitioning.pyhas excellent isolation rationale in its module docstring explaining why it needs its own file. - Golden images regenerated to match new partitioning behavior.
No new issues found. The code is well-structured and the test coverage is thorough.
Update (4290e40): Reviewed incremental changes from 065ac79c → 4290e40e. This is a large commit combining the scene-partitioning feature (unchanged from previous review) with a significant cartpole task restructure.
Previous concerns status:
- ✅ All previous concerns addressed — no regressions in scene-partitioning code.
- ✅
IsaacRtxRenderer.prepare_stage()unchanged — still usesSdf.VariabilityUniform,Sdf.ChangeBlock(), correct primvar/camera attribute split. - ✅
camera_view.pypartition authoring for visualizer cameras unchanged.
Cartpole restructure (orthogonal to scene partitioning):
- Merged
direct_cartpole+manager_cartpole→ singlecore/cartpolepackage with_direct_/_manager_infixes. - Dropped
-v0suffixes from task IDs; manager-based workflow is default (no suffix), direct uses-Direct. - Removed ~35 deprecated per-variant alias registrations — clean consolidation via
presets=CLI. - Documentation, scripts, tests, benchmarks all updated consistently.
- Changelogs added for breaking changes.
No new issues found. The scene-partitioning implementation remains solid and the cartpole restructure is mechanical and consistent.
Update (3bcfa8d): Reviewed incremental changes from 4290e40e → 3bcfa8dc. This is a large release-prep commit that bumps versions across all packages, generates CHANGELOGs, and adds several orthogonal features.
Previous concerns status:
- ✅ All previous concerns remain addressed — no regressions in scene-partitioning code.
- ✅
IsaacRtxRenderer.prepare_stage()unchanged — still usesSdf.VariabilityUniform,Sdf.ChangeBlock(), correct primvar/camera attribute split. - ✅
camera_view.pyvisualizer camera partition authoring unchanged. - ✅ Kit app files still have
renderer.scenePartitioning.enabled = truein all 6 files. - ✅ Test file
test_isaac_rtx_renderer_scene_partitioning.pyunchanged.
What's new in this commit (relevant to this PR):
- Version bumps & changelog generation —
isaaclab6.1.0→6.2.0,isaaclab_tasks1.10.1→2.0.0,isaaclab_newton0.13.0→0.14.0,isaaclab_physx1.1.0→1.1.1, plus all other subpackages. The scene-partitioning changelog entry correctly appears underisaaclab6.2.0 Fixed section: "Enabled RTX scene partitioning in Isaac Lab Kit app files and added top-level scene partitioning regression coverage." get_scales()→ ProxyArray (orthogonal, by @jichuanh) —BaseFrameView.get_scales(),UsdFrameView.get_scales(),FabricFrameView.get_scales(),OvPhysxFrameView.get_scales(), andNewtonSiteFrameView.get_scales()now returnProxyArrayfor API consistency. Breaking change documented in changelogs.reshape_data_to_view_3dtorch support (orthogonal, by @antoiner) — Newton, OvPhysX, and PhysXRigidObjectCollectionimplementations now accepttorch.Tensorin addition towp.array.- Math docstring correction —
combine_frame_transformsandsubtract_frame_transformsdocs corrected: "from frame A to B" → "from frame B to A" (correct for T_AB convention). - Golden images regenerated — Cartpole golden images updated to match per-env partitioning (expected).
- .skip files added —
zhengyuz-scene-partitioning.skipfiles inisaaclab_physx,isaaclab_ov,isaaclab_taskscorrectly suppress changelog entries for test-only changes in those packages.
No new issues found. The scene-partitioning feature is complete and well-integrated into the release. All version bumps and changelog entries are consistent.
Update (e183198): Reviewed incremental changes from 3bcfa8dc → e1831989. This commit is a rebase/cleanup that brings the PR to its final form.
Previous concerns status:
- ✅ All previous concerns remain addressed — no regressions in scene-partitioning code.
- ✅
IsaacRtxRenderer.prepare_stage()unchanged — still usesSdf.VariabilityUniform,Sdf.ChangeBlock(), correct primvar/camera attribute split. - ✅
camera_view.pyvisualizer camera partition authoring unchanged. - ✅ Kit app files still have
renderer.scenePartitioning.enabled = truein all 6 files. - ✅ Test file
test_isaac_rtx_renderer_scene_partitioning.pyunchanged. - ✅
kit_visualizer.pyviewport camera scene partition tagging unchanged.
Summary of files in this version (22 files changed):
- 6 Kit app files — All consistently add
renderer.scenePartitioning.enabled = truebefore RTX initialization. isaac_rtx_renderer.py—prepare_stage()authorsprimvars:omni:scenePartitionon env roots andomni:scenePartitiononUsdGeom.Cameradescendants using efficientSdf.ChangeBlock()batch edits.camera_view.py— Authorsomni:scenePartitionon dynamically-created visualizer cameras.kit_visualizer.py— Tags viewport camera with first visible env partition so RTX renders correctly in interactive mode.test_isaac_rtx_renderer_scene_partitioning.py(new, 187 lines) — Two regression tests: rigid object and articulation partitioning isolation verified via per-tile RGB difference thresholds.- Changelog entries and .skip files — Properly placed under
isaaclabandisaaclab_visualizersFixed sections; other packages have skip files. - Golden images regenerated — 6 cartpole golden images updated to match new partitioning behavior.
No new issues found. The implementation is complete, well-tested, and ready for merge.
Update (10e9cc0): Reviewed new commit 10e9cc06 — "Update cartpole rendering golden images". This commit only updates 3 LFS golden image pointers for Isaac-Cartpole-Camera-Direct test renderings (constant_diffuse, diffuse_mdl, full_mdl). No logic changes — these appear to be expected regenerated test baselines due to the scene partitioning changes. No new concerns.
Update (2566931): Reviewed incremental change from 10e9cc06 → 25669312. Single file modified: isaac_rtx_renderer.py.
Change: Made omni:scenePartition attribute creation idempotent. Previously, Sdf.JustCreatePrimAttributeInLayer() was called unconditionally which raises if the attribute spec already exists (e.g., when multiple renderer backends share the same stage). Now it:
- Checks if
root_layer.GetAttributeAtPath(attr_path)returns an existing spec - Only creates the attribute when absent (
attr_spec is None) - Always (re)assigns the per-env partition token regardless
Assessment: ✅ Good defensive fix. This addresses a real multi-renderer scenario where two backends could both call prepare_stage() on the same USD stage. The idempotent guard prevents Sdf spec-creation errors while still ensuring correct token values. Comment explains the rationale clearly.
No new issues found. Clean, minimal, well-commented fix.
| if len(sim_utils.find_matching_prims(path)) == 0: | ||
| spawn.func(path, spawn, translation=(0.0, 0.0, 0.0), orientation=(0.0, 0.0, 0.0, 1.0)) | ||
|
|
||
| stage = sim_utils.get_current_stage() |
There was a problem hiding this comment.
Could you add some comment in the code to explain what the code block here is intended for? It is not as obvious to me. :)
There was a problem hiding this comment.
One thing I don't follow: the visualization camera is supposed to show all the envs, how do we need to modify the env cameras for visualization purpose?
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
30e6ef5 to
b685361
Compare
|
|
||
| logger.info("Preparing stage for export (%d envs, cloning=%s)...", num_envs, self._use_ovrtx_cloning) | ||
| create_scene_partition_attributes(stage, num_envs, self._use_ovrtx_cloning) | ||
| # create_scene_partition_attributes(stage, num_envs, self._use_ovrtx_cloning) |
There was a problem hiding this comment.
Please restore this call.
| @@ -0,0 +1 @@ | |||
| No user-facing isaaclab_ov changelog entry needed for scene partitioning test changes. | |||
There was a problem hiding this comment.
This file can be reverted.
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
3 times, most recently
from
4290e40 to
3bcfa8d
Compare
ooctipus
force-pushed
the
zhengyuz/scene-partitioning
branch
from
3bcfa8d to
e183198
Compare
When a scene registers cameras with multiple distinct renderer configs, RenderContext creates more than one IsaacRtxRenderer backend, and each runs prepare_stage() on the same USD stage. Sdf.JustCreatePrimAttributeInLayer raises if the omni:scenePartition attribute spec was already authored by a prior backend, crashing env creation with a pxr.Tf.ErrorException. Only create the attribute spec when absent, then assign the per-env token either way, so repeated prepare_stage calls on a shared stage are safe.
4 participants
Description
Adds
InteractiveScene.author_scene_partitions()that, for each env root inenv_prim_paths, writes the inheriting primvarprimvars:omni:scenePartitionon the root and the matching non-primvaromni:scenePartitiontoken on every per-envUsdGeom.Camera. RTX (and OvRTX) honor primvar inheritance, so the env-root primvar propagates to all descendant geometry; renderers that don't recognize the primvar simply ignore it, so authoring is unconditional.The method is called at the end of
clone_environments(), so bothManagerBasedEnv(which callsInteractiveScene.__init__to auto-clone) and Direct envs that drive cloning manually from_setup_scene(e.g.anymal_c_env) get partitioning for free. Authoring runs after USD/physics cloning but before the physics backend takes its tensor view insim.reset, so PhysX and Newton tensor views are not invalidated. Cost isO(N + cameras); per-descendant authoring is intentionally not done because RTX is spec'd to honor primvar inheritance here.Type of change
Tests
Two regression tests under
source/isaaclab/test/scene/test_scene_partitioning.py:test_partitioning_isolates_rigid_objecttest_partitioning_isolates_articulationConfirmed via ablation against
Isaac-Dexsuite-Kuka-Allegro-Lift-v0atnum_envs=4,env_spacing=0,replicate_physics=False, with selected per-env prims hidden viaUsdGeom.Imageable.MakeInvisible(visibility-only — preserves PhysX tensor view validity).The articulation-side bug is confirmed by Nathan Cournia; Steven Bloemer / FSD has the RTX-side fix in flight. The xfail test is the regression hook for that fix landing.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there