Skip to content

Turn off OVRTX shadows by default - #7454

Open
mataylor-nvidia wants to merge 3 commits into
isaac-sim:developfrom
mataylor-nvidia:mataylor/ovrtx-shadows-off-by-default
Open

Turn off OVRTX shadows by default#7454
mataylor-nvidia wants to merge 3 commits into
isaac-sim:developfrom
mataylor-nvidia:mataylor/ovrtx-shadows-off-by-default

Conversation

@mataylor-nvidia

@mataylor-nvidia mataylor-nvidia commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Description

Turns off shadows by default for the OVRTX renderer, in the one render mode that can turn them off.

OVRTXRendererCfg gains enable_shadows, defaulting to False. When the render product is in RTX
Minimal mode it authors omni:rtx:minimal:castShadows accordingly. Minimal mode is what the
simple_shading_constant_diffuse, simple_shading_diffuse_mdl and simple_shading_full_mdl data
types select, so those are the outputs this affects. Opt back in with
OVRTXRendererCfg(enable_shadows=True).

Screenshots

simple_shading_full_mdl, kit-less Newton scene, 640×480, single distant light at 45°, rendered
through the unmodified Camera / OVRTXRendererCfg path.

enable_shadows=True enable_shadows=False (new default)
minimal mode, shadows on minimal mode, shadows off

The pillar and sphere shadows are gone in the right-hand frame (mean abs diff 1.49, max 116,
1.3% of pixels differing by more than 8/255).

Why the path-traced modes are not covered

RTPT and PathTracing provide no shadow switch — they always trace shadows. omni:rtx:shadows:enabled
exists as a setting name and authors onto the render product without error, but it only feeds
renderConfig.shadows, which no path-tracing backend reads. It is inert in every OVRTX mode.

An earlier revision of this PR authored it alongside castShadows; that has been removed rather than
left in place advertising a control that does nothing. enable_shadows therefore does not change
rgb or the other AOV outputs, and the config docstring says so.

Testing

  • source/isaaclab_ov/test/test_ovrtx_usd.py — covers the authored attribute in Minimal mode, and
    that neither shadow setting is authored for the path-traced modes.
  • test_rendering_lift_kuka_homo_kitless[legacy-newton-ovrtx-simple_shading_full_mdl] and
    [legacy-newton-ovrtx_renderer-static] both pass against their existing golden images.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

Add OVRTXRendererCfg.enable_shadows, defaulting to False, and author it on
the OVRTX render product as omni:rtx:shadows:enabled for the path-traced
render modes and omni:rtx:minimal:castShadows for the simple_shading_* data
types, so the toggle covers every render mode the renderer selects.
@mataylor-nvidia
mataylor-nvidia requested a review from a team August 31, 2026 18:38
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Aug 31, 2026
@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds an enable_shadows option to the OVRTX renderer, defaulting to disabled, and authors the corresponding path-traced and RTX Minimal attributes on every render product.

  • Propagates the configuration through both OVRTX render-product creation paths.
  • Adds USD-generation tests for the default and opt-in behavior.
  • Documents the changed default in the package changelog.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness or security issues identified.

The new configuration is propagated through both runtime construction paths, the generated USD consistently authors both mode-specific values, and repository callers remain compatible.

Important Files Changed

Filename Overview
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py Both renderer initialization paths consistently forward the new shadow configuration.
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer_cfg.py Adds the documented opt-in shadow configuration with the intended disabled default.
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_usd.py Authors matching path-traced and Minimal shadow settings and preserves existing argument binding.
source/isaaclab_ov/test/test_ovrtx_usd.py Covers disabled defaults, explicit opt-in, both render-mode attributes, and builder forwarding.

Reviews (1): Last reviewed commit: "Turn off OVRTX shadows by default" | Re-trigger Greptile

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isaac Lab Review Bot

The change adds OVRTXRendererCfg.enable_shadows, defaulting to False, and threads it through both renderer initialization paths to author the path-traced and RTX Minimal shadow settings on the render product.

  • Design and architecture: Keeping the policy toggle in the OVRTX-specific configuration and translating it into renderer-specific USD attributes in the render-scope builder is consistent with the existing separation. Authoring both mode-specific attributes makes behavior independent of the selected render mode.
  • API: The new builder parameters are trailing and defaulted, preserving existing caller compatibility. The user-visible default change and the OVRTXRendererCfg(enable_shadows=True) migration path are documented in the package changelog and field docstring.
  • Implementation: Both legacy and ovstage initialization paths forward the configuration value, and focused tests cover the default and opt-in USD output. The remaining non-blocking risk is runtime effectiveness: the PR reports that pinned ovrtx==0.4.1.364340 produces identical pixels for both values, so this currently establishes authored intent rather than demonstrated rendered-shadow behavior.

No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.

Automated review; human maintainers own approval decisions.

Only RTX Minimal mode exposes a shadow switch. Author
omni:rtx:minimal:castShadows there and drop omni:rtx:shadows:enabled: the
setting name resolves and authors without error, but no path-tracing backend
reads it, so authoring it advertised a control that does nothing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant