Add RenderingModeCfg for setting Rendering Settings#4674
Add RenderingModeCfg for setting Rendering Settings#4674matthewtrepte wants to merge 36 commits intoisaac-sim:developfrom
Conversation
matthewtrepte
requested review from
Mayankm96,
hhansen-bdai,
jtigue-bdai,
kellyguo11 and
ossamaAhmed
as code owners
github-actions
bot
added
documentation
Greptile SummaryThis PR introduces a new Key Changes:
Testing:
Documentation:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AppLauncher --rendering_quality CLI] -->|Stores in carb settings| B[SimulationContext]
C[SimulationCfg.rendering_quality_cfgs] -->|Named profiles| B
D[VisualizerCfg.rendering_quality] -->|Profile name| B
B -->|Resolves quality name| E{Quality Profile Resolution}
E -->|CLI explicit| F[Use CLI value]
E -->|No CLI| G[Use VisualizerCfg value]
F --> H[Lookup in rendering_quality_cfgs]
G --> H
H --> I[RenderingQualityCfg]
I -->|kit_rendering_preset| J[get_kit_rendering_preset]
I -->|kit_* fields| K[Direct carb settings]
I -->|newton_* fields| L[Newton visualizer properties]
J --> M[Apply RTX settings]
K --> M
L --> N[Apply Newton renderer settings]
M --> O[Kit Visualizer]
N --> P[Newton Visualizer]
Last reviewed commit: 6bfdc53 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @pytest.mark.skip(reason="Timeline not stopped") | ||
| @pytest.mark.isaacsim_ci | ||
| def test_render_cfg_defaults(): | ||
| """Test that the simulation context is created with the correct render cfg.""" | ||
| enable_translucency = False | ||
| enable_reflections = False | ||
| enable_global_illumination = False | ||
| antialiasing_mode = "DLSS" | ||
| enable_dlssg = False | ||
| enable_dl_denoiser = False | ||
| dlss_mode = 2 | ||
| enable_direct_lighting = False | ||
| samples_per_pixel = 1 | ||
| enable_shadows = False | ||
| enable_ambient_occlusion = False | ||
|
|
||
| render_cfg = RenderCfg( | ||
| enable_translucency=enable_translucency, | ||
| enable_reflections=enable_reflections, | ||
| enable_global_illumination=enable_global_illumination, | ||
| antialiasing_mode=antialiasing_mode, | ||
| enable_dlssg=enable_dlssg, | ||
| enable_dl_denoiser=enable_dl_denoiser, | ||
| dlss_mode=dlss_mode, | ||
| enable_direct_lighting=enable_direct_lighting, | ||
| samples_per_pixel=samples_per_pixel, | ||
| enable_shadows=enable_shadows, | ||
| enable_ambient_occlusion=enable_ambient_occlusion, | ||
| def test_rendering_quality_cfg_field_overrides(): |
There was a problem hiding this comment.
Check that this skipped test either gets fixed or documented why it should remain skipped
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
thanks! can you run linter? @matthewtrepte |
matthewtrepte
changed the title
matthewtrepte
force-pushed
the
mtrepte/add_rendering_quality_cfg
branch
from
a8d81c4 to
6b2c026
Compare
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add RenderingModeCfg for Setting Rendering Settings
Overall this is a well-structured refactor that replaces the monolithic RenderCfg with a more flexible named-profile system (RenderingModeCfg). The architecture of separating kit/newton/newton-warp fields into a single config with prefixed names, resolved through SimulationCfg.rendering_mode_cfgs, is a solid design that scales to multiple backends. Good work deleting the old .kit preset files and inlining the values as Python dicts.
Here are some concerns and suggestions:
Breaking Change: RenderCfg Removal & carb_settings Drop
The old RenderCfg is removed entirely with no deprecation shim. Users who have RenderCfg(rendering_mode="quality", carb_settings={...}) will get immediate errors. This is fine as a breaking change if intended, but:
-
No
carb_settingsequivalent —RenderCfg.carb_settingsallowed arbitrary carb key/value overrides.RenderingModeCfghas only explicitly listed fields plus a TODO comment. This is a feature regression for users who relied on arbitrary overrides (e.g./rtx/aovConverter/disocclusionScale). The docs acknowledge this. Consider adding akit_extra_carb_settings: dict[str, Any] | None = Nonefield. -
Migration guide — There's no deprecation warning or migration documentation beyond the updated RST. A short section ("Migrating from RenderCfg") would help.
RendererCfg.renderer_type Default Changed from "default" to None
This could break downstream code. apply_mode_profile_to_renderer_cfg checks rtype in ("default", "isaac_rtx", "rtx") — existing code relying on default "default" will now get None and skip Kit profile application entirely. Was this intentional?
newton_* Defaults Break None-means-no-override Pattern
Most fields default to None, but several newton fields have concrete defaults (True, color tuples). Any RenderingModeCfg() — even one intended only for Kit presets — will override Newton defaults. These should probably default to None.
Unrelated Changes
physx_scene_data_provider.pyadds two new methods never called in this diff- Viser
sys.pathmanipulation is unrelated to rendering config
See inline comments for details.
|
|
||
| newton_enable_wireframe: bool | None = False | ||
| """Overrides Newton visualizer wireframe rendering.""" | ||
|
|
There was a problem hiding this comment.
These Newton defaults (True, True, False, color tuples) break the None-means-no-override convention used by all kit_* fields. Any RenderingModeCfg — even one intended only for Kit preset tuning — will silently override Newton visualizer defaults.
Suggest defaulting all fields to None and moving these concrete values to NewtonVisualizerCfg where they already live as defaults.
| """Configuration for a renderer.""" | ||
|
|
||
| renderer_type: str = "default" | ||
| renderer_type: str | None = None |
There was a problem hiding this comment.
Changing renderer_type default from "default" to None is a subtle breaking change. apply_mode_profile_to_renderer_cfg dispatches on rtype in ("default", "isaac_rtx", "rtx") — existing RendererCfg() instances will now skip Kit profile application. If intentional, call this out in the PR description.
| except ValueError: | ||
| continue | ||
| for p in reversed(ordered): | ||
| sys.path.insert(0, p) |
There was a problem hiding this comment.
This sys.path manipulation and module-dropping at import time is a heavy side effect affecting the entire process. Consider:
- Moving to lazy init inside
ViserVisualizer.__init__orinitialize() - Gating behind an env var
- Splitting into a separate PR (unrelated to
RenderingModeCfg)
| self._filtered_newton_state = None | ||
| self._filtered_body_indices = [] | ||
|
|
||
| def _extract_model_path_list(self, model: Any, attr_candidates: list[str]) -> list[str]: |
There was a problem hiding this comment.
_extract_model_path_list and _discover_physics_paths_from_stage appear unrelated to rendering mode configuration and are never called in this diff. Consider a separate PR.
| ------------------- | ||
|
|
||
| For performance reasons, we default to using DLSS for denoising, which generally provides better performance. | ||
| This may result in renders of lower quality, which may be especially evident at lower resolutions. |
There was a problem hiding this comment.
The docs note that carb_settings equivalent is missing but don't provide a workaround. Consider either adding a kit_extra_carb_settings field to RenderingModeCfg, or documenting sim.set_setting() as the escape hatch for arbitrary carb overrides.
There was a problem hiding this comment.
Follow-up Review (371becd)
The new commit is mostly documentation cleanup plus one meaningful behavioral change. Good improvements overall.
What's Improved
- Docs streamlined:
configure_rendering.rstis much more concise — removed verbose notes, redundant import blocks, and unnecessary caveats. Reads better now. renderer_cfg.pydocstring fixed: Old docstring confusingly listed renderer type values (default,isaac_rtx,rtx) for a field about rendering mode profile names. New docstring correctly describes it.visualizer_cfg.pydocstring improved: Now documents CLI override behavior and that only Kit backends apply it.- Redundant
RTXRendererCfg.rendering_moderemoved: Was shadowing the inherited field with the same default — clean removal. KitVisualizerCfgnow defaults to"performance": Sensible — interactive viewport gets a reasonable default rather than requiring explicit opt-in.
New Issue: Missing Space in Help Text
# app_launcher.py — two string literals without a trailing space on the first:
"When passed, selects RenderingModeCfg presets for overwriting rendering settings of all Renderers"
"and Visualizers."
# Produces: "...all Renderersand Visualizers."Needs a space at the end of the first string or start of the second.
Previous Concerns — Status
| Concern | Status |
|---|---|
| Newton defaults breaking None-means-no-override | ✅ Addressed (prior push) |
RendererCfg.renderer_type default None vs "default" |
⬜ Unchanged — downstream dispatch concern stands |
Viser sys.path manipulation |
⬜ Unchanged |
Unrelated physx_scene_data_provider methods |
⬜ Unchanged |
carb_settings equivalent (kit_extra_carb_settings) |
⬜ Still TODO in code |
The doc improvements and Kit default are solid. The help-text typo is a quick fix.
Co-authored-by: isaaclab-review-bot[bot] <270793704+isaaclab-review-bot[bot]@users.noreply.github.com> Signed-off-by: matthewtrepte <mtrepte@nvidia.com>
| from typing import Any | ||
|
|
||
| # Latest preset values sourced from apps/rendering_modes/*.kit. | ||
| _KIT_PRESETS: dict[str, dict[str, Any]] = { |
There was a problem hiding this comment.
I think this doesn't belong here. This is something very very specific for Isaac Lab RTX Renderer?
There was a problem hiding this comment.
these dictionaries define the preset rendering mode for KitVisualizer and RTX based renderers. before these values were stored in app kit files under apps/rendering_modes
| __all__ = [ | ||
| "CLI_RENDERING_MODE_PROFILE_PATH", | ||
| "RenderingModeCfg", | ||
| "get_kit_rendering_preset", |
There was a problem hiding this comment.
This should be all abstract, kit doesn't belong here in my opinion.
| kit_enable_global_illumination: bool | None = None | ||
| """Enables diffuse global illumination at the cost of some performance. Default is often ``False``. | ||
|
|
||
| Carb path: ``/rtx/indirectDiffuse/enabled``. |
There was a problem hiding this comment.
Those are all implementation details that renderer backend should initialize? This configuration should be abstract.
| ' Can be "performance", "balanced", or "quality".' | ||
| " Individual settings can be overwritten by using the RenderCfg class." | ||
| "When passed, selects RenderingModeCfg presets for overwriting rendering settings of all Renderers" | ||
| "When passed, selects RenderingModeCfg presets for overwriting rendering settings of all Renderers " |
There was a problem hiding this comment.
The help text fix introduced a duplicate line — both the old and new strings are present:
help=(
"When passed, selects RenderingModeCfg presets for overwriting rendering settings of all Renderers"
"When passed, selects RenderingModeCfg presets for overwriting rendering settings of all Renderers "
),This concatenates to: "...all RenderersWhen passed, selects...". Should be just:
help="When passed, selects RenderingModeCfg presets for overwriting rendering settings of all Renderers."
|
How does this line up with: #5167? |
5 participants
Description
Adding a new config class called RenderingModeCfg for specifically storing and applying rendering settings, which are consumed by both Renderers and Visualizers.
Currently supports KitVisualizer and RTX based renderers. Can be extended to support other modules and settings.
Maintains the existing quality / balanced / performance presets, replacing the old app kit files with new RenderingModeCfg preset classes.
Updated rendering mode docs and expanded unit tests.
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there