🤖 Isaac Lab Review Bot

Summary

This PR fixes the RTX simple-shading modes (simple_shading_constant_diffuse, simple_shading_diffuse_mdl, simple_shading_full_mdl) which were silently falling back to Kit's default shading. The fix updates the carb setting path from /rtx/sdg/simpleShading/mode to /rtx/minimal/mode and corrects the integer values from 0/1/2 to 1/2/3 to match Kit's actual minimal render mode API. The implementation is correct and well-documented.

Architecture Impact

Self-contained change within IsaacRtxRenderer. The SIMPLE_SHADING_MODES dictionary is only consumed internally by _resolve_simple_shading_mode() and the render() method. No downstream callers depend on these integer values. Public API data type names (simple_shading_constant_diffuse, etc.) remain unchanged.

Implementation Verdict

Minor fixes needed — The core bug fix is correct and the approach is sound, but there is one improvement that would benefit the test suite.

Test Coverage

✅ Bug fix has visual regression tests — The golden images have been regenerated for all affected shading modes across multiple scenes (cartpole, dexsuite_kuka, shadow_hand). The existing test_rendering_correctness.py suite covers these modes and will catch future regressions.

✅ New SSIM gate added — The test now uses SSIM as a pass/fail gate (threshold 0.985) alongside the pixel L2 gate, which is a good addition for catching structural regressions that survive the per-pixel threshold.

🟡 Test threshold tightened — shadow_hand threshold reduced from 8.0% to 4.0%, which indicates higher confidence in the fix. However, see finding below.

CI Status

• ✅ pre-commit passed
• ✅ license-check passed
• ✅ Build Base Docker Image passed
• ⏳ isaaclab_physx, isaaclab_tasks, environments_training tests pending (awaiting runner)

No CI failures on the current HEAD commit.

Branch Status

This branch is 7 commits behind develop. No merge conflicts detected. Recommend rebasing before merge to pick up recent changes.

Findings

🔵 Improvement: test_rendering_correctness.py:713 — Test threshold explanation

The shadow_hand test threshold was reduced from 8.0% to 4.0%. While the conversation thread mentions this was addressed, a brief inline comment explaining why this specific threshold was chosen would help future maintainers understand the rationale. The other tests use varying thresholds (2.0%, 4.0%, 5.0%, 10.0%) without clear explanation of what drives each choice.

Consider adding a comment like:

# Threshold tightened after fixing simple-shading to produce correct constant-diffuse output
max_different_pixels_percentage=4.0,

🔵 Improvement: CHANGELOG.rst:4 — Date should match merge date

The changelog entry is dated 2026-04-20 but the fix wasn't finalized until 2026-04-22. Minor, but ideally the date should reflect when the version is actually released/merged rather than when work started.

Overall: Ship it after CI passes. The fix is correct, well-documented in the changelog, and the golden images confirm the modes now produce visually distinct outputs as intended.

🤖 Isaac Lab Review Bot — Re-review (new push 93e725a)

Summary

This PR fixes the RTX simple-shading modes (simple_shading_constant_diffuse, simple_shading_diffuse_mdl, simple_shading_full_mdl) which were silently falling back to Kit's default shading. The latest push adds a reasonable improvement: disabling SSIM validation for depth-based data types where SSIM metrics are unreliable due to non-uniform value distributions after normalization.

Changes Since Last Review

The new commit 93e725a5 adds:

• _SSIM_DISABLED_DATA_TYPES set for {"depth", "distance_to_camera", "distance_to_image_plane"}
• check_ssim parameter to _compare_images() function
• HTML report styling for disabled SSIM cells
• Threshold tightening for dexsuite from 10% → 5%

Implementation Verdict

Ship it — The SSIM disable logic is well-documented and correctly implemented. The rationale (SSIM assumes natural-image statistics and is unreliable on normalized depth data) is sound.

Test Coverage

✅ The SSIM disable is appropriately scoped to only depth-related data types where the metric produces misleading results. The per-pixel L2 gate remains active for all data types.

CI Status

• ✅ pre-commit passed
• ✅ license-check passed
• ⏳ isaaclab_tasks, isaaclab_physx tests pending/running
• ⚠️ Build Latest Docs failed (appears unrelated to this PR's changes)

Branch Status

⚠️ This branch is 7 commits behind develop due to the rebase with commits from develop included. No merge conflicts detected, but recommend rebasing to pick up the latest develop HEAD before merge.

Findings

No new issues found. The implementation is clean and the documentation is thorough. The changes since the previous approval are improvements that address edge cases in SSIM-based testing.

Overall: The fix is correct, well-documented in the changelog, and the new SSIM disable for depth data types is a sensible improvement. Ready to merge once CI completes.