Add PPISP camera QA demos and discovery fallback#5753
Conversation
github-actions
Bot
added
documentation
|
@kellyguo11 for viz. |
![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 - PR #5753
Summary
This PR adds QA-oriented PPISP camera demo workflows for validating USD-authored Gaussian scenes and fixes a discovery edge case in auto_camera_ppisp_cfg(). This is a cherry-pick to release/3.0.0-beta2.
✅ Overall Assessment: Well-Structured QA Addition
The PR introduces two comprehensive QA demos (Kit-based and kit-less OVRTX) that follow Isaac Lab patterns well, along with a legitimate bug fix backed by a proper unit test.
🏗️ Architecture & Design
| Aspect | Status | Notes |
|---|---|---|
| Multi-renderer support | ✅ | Separate scripts for Kit (newton/isaac_rtx) and kit-less OVRTX |
| Environment duplication | ✅ | Proper {ENV_REGEX_NS} pattern for multi-env tiled rendering |
| Resource cleanup | ✅ | OVRTX script uses try/finally for proper cleanup |
| Scene configuration | ✅ | Clean InteractiveSceneCfg subclasses with minimal anchor pattern |
🐛 Bug Fix Analysis
Fixed issue in cfg.py:
# BEFORE (buggy): Early return blocked discovery of later PPISP RenderProducts
if shader_prim and shader_prim.IsValid():
return ppisp_cfg_from_usd_shader(...)
return None # ← Premature exit if first matching RP had no PPISP
# AFTER (fixed): Continue iteration to find PPISP-enabled RenderProducts
if shader_prim and shader_prim.IsValid():
return ppisp_cfg_from_usd_shader(...)
# (no early return - loop continues)This correctly handles the case where Isaac RTX generates transient RenderProducts targeting the same camera as authored scene RenderProducts.
🛡️ Error Handling & Edge Cases
| Check | Status |
|---|---|
| CLI argument validation | ✅ All inputs validated before launch |
| Missing PPISP bindings | ✅ Clear RuntimeError with available cameras listed |
| Division by zero protection | ✅ clamp_min(1.0) in brightness ratio calculation |
| Stage traversal |
🧪 Test Coverage
| Test | Purpose |
|---|---|
test_auto_camera_ppisp_cfg_skips_matching_render_product_without_ppisp |
✅ Directly validates the bug fix |
| Updated docstrings | ✅ Accurately describe HDR→LDR PPISP mapping tests |
| Multi-tile tests | ✅ Already covered in existing test suite |
📝 Documentation
- ✅ Changelog fragment added for
isaaclab_ppisp - ✅ README.md scaffolding for the extension
- ✅ Script docstrings with clear usage examples
💡 Minor Suggestions (Non-blocking)
-
Consistency: The Kit-based demo (
ppisp_camera.py) usesdel sceneoutside try/finally, while the OVRTX version properly uses try/finally for cleanup. Consider aligning for consistency. -
Diagnostic clarity: The
corner_center_ratiometric returnscorners/1.0for extremely dark images - this is fine for QA but could log a warning ifcenter < 1.0.
CI Status
Pre-commit and initial checks passing. Docker builds and extended tests pending.
Review generated by isaaclab-review-bot using ensemble multi-model analysis
Update (1191792): This commit updates test docstrings to use consistent "PPISP" terminology (instead of "ISP") and clarifies what invariants each test asserts. No functional changes to the demo scripts or the bug fix. The original assessment remains valid.
Update (commit b1adb9f): Reviewed incremental changes since 1191792.
Status of previous findings:
- ⏳ Stage traversed twice in
main()(inline comment) — remains unaddressed. The original inline comment stands. - ✅ All other observations from the initial review remain valid.
New changes in this push:
- ✅ Documentation reorganization: moved
visualization.rstto core-concepts, addedrlinf_vla_posttraining.rst, updated references. Clean restructuring. - ✅ Presets column formatting improvements in
environments.rst— clearer physics=/renderer=/presets= labeling. - ✅
test_noise.pyfix for NaN edge case whentorch.randreturns exactly 0 withop="scale"(OMPE-94619). - ✅ Skip changelog entries for test-only and docstring-only changes.
No new issues found in the incremental diff. Ready for merge once CI passes.
Greptile SummaryThis PR adds two QA-oriented demo scripts for PPISP camera validation (
Confidence Score: 4/5Safe to merge; the core bug fix is a single-line removal that is well-covered by a new unit test, and the demo scripts are additive with no impact on production paths. The bug fix and its test are correct. The demo scripts work as described but duplicate a large block of utility logic that could diverge silently over time, and the auto_camera_ppisp_cfg docstring still describes the old behavior after the fix. The auto_camera_ppisp_cfg docstring in cfg.py needs a one-line update to reflect the new behavior. The two demo scripts (ppisp_camera.py and ppisp_camera_ovrtx.py) share ~250 lines of identical utility code that would benefit from extraction into a shared module. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[stage.Traverse] --> B{prim type == RenderProduct?}
B -- No --> A
B -- Yes --> C{camera rel targets camera_prim_path?}
C -- No --> A
C -- Yes --> D{PPISP shader child exists?}
D -- Yes --> E[return ppisp_cfg_from_usd_shader]
D -- No --> F["(old) return None — BUG REMOVED"]
D -- No --> G[continue to next prim — FIX]
G --> A
A -- exhausted --> H[return None]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ppisp_cfg = make_ppisp_cfg(shader_prim, len(find_ppisp_camera_bindings(source_stage))) | ||
| camera_prim_path = source_camera_path_to_env_regex(source_stage, source_camera_prim_path) | ||
| width, height = resolve_image_shape(render_product_prim) | ||
|
|
||
| sim_utils.create_new_stage() | ||
| sim_cfg = make_sim_cfg() | ||
| sim = sim_utils.SimulationContext(sim_cfg) | ||
| sim.set_camera_view(eye=[2.5, 2.5, 2.5], target=[0.0, 0.0, 0.0]) |
There was a problem hiding this comment.
main()
find_ppisp_camera_bindings(source_stage) is traversed once inside resolve_source_camera_binding() and again on the next line to compute num_ppisp_bindings for the log message. For large USD scenes this can be noticeable; resolve_source_camera_binding could return the full binding list alongside the selected tuple, or the count could be stored as len(ppisp_bindings) from the first call. The same pattern exists in ppisp_camera_ovrtx.py at the equivalent main() block.
moennen
force-pushed
the
nicolasm/ppisp-demo-scripts-release
branch
2 times, most recently
from
1191792 to
b1adb9f
Compare
🔄 Review Update (b1adb9f)Reviewed incremental changes since 1191792. The new commits add:
Previous inline comment status:
✅ No new issues introduced in the incremental changes. |
moennen
force-pushed
the
nicolasm/ppisp-demo-scripts-release
branch
2 times, most recently
from
cc19adb to
828ea5d
Compare
moennen
requested review from
Mayankm96,
hhansen-bdai,
jtigue-bdai,
kellyguo11 and
pascal-roth
as code owners
moennen
force-pushed
the
nicolasm/ppisp-demo-scripts-release
branch
2 times, most recently
from
2cee9a7 to
9d21585
Compare
|
Review Update (9d21585): Reviewed the incremental changes from b1adb9f...9d21585. New commits include:
No new issues found. Previous inline comment (double stage traversal in `main()`) remains unaddressed but is low priority (P2). |
moennen
force-pushed
the
nicolasm/ppisp-demo-scripts-release
branch
from
9d21585 to
a053f9a
Compare
🔄 Review Update (a053f9a)Reviewed incremental changes since initial review at 9d21585:
Previous Concerns
SummaryNo new issues introduced. LGTM for these incremental changes. |
1 participant
Description
This PR adds QA-oriented PPISP camera demo workflows for USD-authored Gaussian scenes and fixes a PPISP auto-discovery edge case.
Summary:
scripts/demos/sensors/ppisp_camera.pyfor Kit-based PPISP validation withnewtonandisaac_rtxrenderers.scripts/demos/sensors/ppisp_camera_ovrtx.pyfor kit-less OVRTX PPISP validation.num_envs > 1.auto_camera_ppisp_cfg()so a matching RenderProduct without aPPISPchild does not prevent discovery of a later matching RenderProduct that does contain PPISP.isaaclab_ppispdocs/changelog scaffolding and a changelog fragment for the QA workflows.Motivation/context:
Existing unit tests cover PPISP renderer integration, but QA needed a user-facing workflow that validates PPISP through CameraSensor-style rendering on a real USD-authored Gaussian scene. These
demos provide that workflow without requiring training jobs or task-specific environment changes.
Dependencies:
isaaclab_ov/ovrtxstack installed.OVRTX_rtx_rtpt_gaussian_skipTonemapping_enabled=0Fixes #
Type of change
Screenshots
The demo scripts generate baseline / PPISP / diff image grids under the selected
--output_dir. Attach representative generated images here if desired.Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there