Repository navigation
Conversation
|
run-ci |
github-actions
Bot
added
documentation
isaaclab-bot
Bot
added
ci:run-docker
 [Medium impact] The changes reach camera rendering and Kit visualization across several simulation backends. The PR appears safe to merge, with a non-blocking marker ownership issue for markers created before the simulation. Findings
SummaryThe PR uses declared clone roots for camera bindings, RTX partitions, Kit visibility, and markers.
Reviews (1) · Last reviewed commit: "Use declared clone paths for camera part..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| plan = self._sim.get_clone_plan() if self._sim is not None else None | ||
| if plan is None: | ||
| return False |
There was a problem hiding this comment.
Markers created before SimulationContext keep _sim=None, even when the later simulation adopts their stage. _ensure_kit_backend(sim) retains the existing backend without updating its owner. This new check therefore always returns False, so visualize(environment_ids=...) no longer assigns those markers to their environments after RTX prepares the stage.
Bind an existing ownerless backend when the simulation adopts its stage.
![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
Needs attention: Scope and over-engineering (1), Test quality (1). Other reviewed categories look good.
- PR description: The body matches the implementation and provides adequate performance evidence, but should explicitly note that the lifecycle section is replaced and retain its unchanged PhysicsManager contracts.
- Design and architecture: Architecture looks good.
- Scope and over-engineering: Minimal implementation: extend Camera, OVRTXRenderer, IsaacRtxRenderer, KitVisualizer, KitVisualizationMarkers, and VisualizationMarkers; no source modules or layers were added. Keep the clone-path documentation and changelogs, but restore the unchanged PhysicsManager lifecycle contract instead of replacing it. Scope is cohesive; no split recommended.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: Implementation looks good.
- Style consistency: Style consistency looks good.
- Test quality: The new backend and ownership regressions are necessary, but environment visibility is asserted twice under an unrelated spectator-setting parameter; move it to one focused test.
Minor fixes needed. Posted 2 inline findings.
Automated review; human maintainers own approval decisions.
| configuration's ``class_type``. Their selection is independent of physics; | ||
| renderer instances are shared through the simulation registry described below. | ||
|
|
||
| Physics manager lifecycle |
There was a problem hiding this comment.
🟡 Warning · Scope Complexity — Unrelated rewrite drops lifecycle contract docs
This rewrite deletes the PhysicsManager lifecycle section — forward/step/reset/close and PhysicsEvent MODEL_INIT/PHYSICS_READY/STOP, including close() dispatching STOP — which this diff does not change. Keep that section alongside the new construction text.
| else: | ||
| assert partition_attr.Get() == expected_partition | ||
|
|
||
| visualizer._apply_env_visibility(stage, num_envs=4, visible_env_ids=[2]) |
There was a problem hiding this comment.
🟡 Warning · Test Quality — Visibility contract replayed across unrelated axis
_apply_env_visibility is a separate contract that the show_global_view axis does not branch on, so these assertions run identically in both rows under a name promising only partition behavior. Move them to their own test.
ooctipus
changed the title
|
run-ci |
isaaclab-bot
Bot
added
the
ci:run-docker
isaaclab-bot
Bot
removed
the
ci:run-docker
Custom clone templates such as
/Lab/Cells/Cell{}were accepted, but scene consumers still assumed/World/envs/env_0. This could miss camera clones, assign the wrong partitions, misorder frame-transformer rows, and break task or teleop lookups.Use the existing clone plan, bound asset paths, and native world IDs throughout cameras/renderers, PhysX/OVPhysX frame views, couplers, task configurations, and XR.
{ENV_REGEX_NS}follows the declared template; the default layout stays unchanged. Remove the old path-rewriting helpers and duplicate discovery paths.Coverage: existing tests now exercise custom roots, native-only clones, external prototypes, numeric ordering past environment 9, missing/reordered camera rows, shared parents, markers, and XR anchors. A repository-wide regression check rejects hard-coded
/World/envspaths in production code outside the central default declaration. Restoring the old assumptions reproduces the failures.Validation: 463 focused checks and 18 renderer cases passed on CPU/GPU0, including 65 image comparisons with unchanged goldens and tolerances. Both frame-view benchmark scripts completed smoke runs; formatting and changelog checks passed. Full CI is requested on the pushed commit.
Camera-path performance measurements
Measured the initial camera-path change (
d7b8595), before the broader namespace audit. GPU0 RTX 5090; 4,096 environments, RGB cameras, no viewer. Median step time across two processes × two 100-step batches after 10 warmup steps. Cartpole cameras: 96×96; Kuka: 64×64. Startup excluded because cache/disk conditions differed.