Restore RT2 RenderCfg fields removed in PhysicsManager refactor#5167
Restore RT2 RenderCfg fields removed in PhysicsManager refactor#5167nblauch wants to merge 1 commit intoisaac-sim:developfrom
Conversation
PR isaac-sim#4142 added 10 RT2 typed fields to RenderCfg (max_bounces, split_glass, split_clearcoat, split_rough_reflection, ambient_light_intensity, ambient_occlusion_denoiser_mode, subpixel_mode, enable_cached_raytracing, max_samples_per_launch, view_tile_limit) and their field_to_setting mappings. These were accidentally removed in the PhysicsManager refactor (PR isaac-sim#4564, commit 0ba9c5c). This restores them along with test coverage.
nblauch
requested review from
kellyguo11,
matthewtrepte and
ossamaAhmed
as code owners
Greptile SummaryThis PR restores 10 RT2 ( Two minor issues to be aware of: the docstrings for Confidence Score: 5/5Safe to merge; the core field restoration and carb-path wiring are correct, and all remaining findings are P2. All findings are P2: two docstring/test default inconsistencies and skipped tests. The restored cfg fields and field_to_setting mappings are correct, matching the original PR #4142 intent and the kit preset files. test_simulation_render_config.py — the RT2-specific tests are skipped and the default values used in test_render_cfg_defaults contradict two docstrings. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[RenderCfg field set\ne.g. max_bounces=4] --> B{value is None?}
B -- Yes --> C[Skip field]
B -- No --> D{in excluded keys?\nrendering_mode / carb_settings / antialiasing_mode}
D -- Yes --> E[Handled separately]
D -- No --> F[field_to_setting lookup]
F --> G{mapping found?}
G -- No --> H[Unknown field, skipped]
G -- Yes --> I[set_setting carb path\ne.g. /rtx/rtpt/maxBounces]
I --> J[SettingsHelper.set\nauto-types bool/int/float/str]
J --> K[SettingsManager writes\nto carb runtime]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| split_rough_reflection: bool | None = None | ||
| """Enables separate rough reflection ray splitting (RT2). Default is True. | ||
|
|
||
| Enabling this can reduce noise on rough reflective materials at the cost of performance. | ||
|
|
||
| This is set by the variable: ``/rtx/rtpt/splitRoughReflection``. | ||
| """ |
There was a problem hiding this comment.
split_rough_reflection is documented as "Default is True", but test_render_cfg_defaults (line 213) uses False as the expected default value under the # RT2 defaults comment. Similarly, enable_cached_raytracing is documented as "Default is True" (line 215) while the test uses False (line 216). One of these is wrong — either the carb-setting system default changed, or the docstrings are incorrect. If the actual carb defaults are False, the docstrings should read "Default is False".
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good restoration PR — the fields, carb path mappings, and test coverage all look correct. One docstring was changed from the original PR #4142 (not a faithful restoration). CI still running (pre-commit, license-check in progress).
| Enabling this can reduce noise on rough reflective materials at the cost of performance. | ||
|
|
||
| This is set by the variable: ``/rtx/rtpt/splitRoughReflection``. | ||
| """ |
There was a problem hiding this comment.
The original PR #4142 had this as "Default is False", and test_render_cfg_defaults also uses False. All three .kit presets (performance, balanced, quality) explicitly set splitRoughReflection = true, which strongly implies the carb-level default is false (otherwise the presets wouldn't need to set it).
This should be restored to match the original:
| """ | |
| """Enables separate rough reflection ray splitting (RT2). Default is False. |
2 participants
Description
PR #4142 added 10 typed RT2 (
RealTimePathTracing) fields toRenderCfgto expose the carb settings that actually control rendering quality under the RT2 mode that has been default since Isaac Sim 4.5. The PhysicsManager refactor in PR #4564 (commit0ba9c5cb) accidentally removed all 10 of these fields during a largesimulation_cfg.pyrewrite, while the.kitpreset files that reference the same carb paths were left intact.This PR restores the removed fields and their
field_to_settingmappings, along with test coverage.Restored fields:
max_bounces/rtx/rtpt/maxBouncessplit_glass/rtx/rtpt/splitGlasssplit_clearcoat/rtx/rtpt/splitClearcoatsplit_rough_reflection/rtx/rtpt/splitRoughReflectionambient_light_intensity/rtx/sceneDb/ambientLightIntensityambient_occlusion_denoiser_mode/rtx/ambientOcclusion/denoiserModesubpixel_mode/rtx/raytracing/subpixel/modeenable_cached_raytracing/rtx/raytracing/cached/enabledmax_samples_per_launch/rtx/pathtracing/maxSamplesPerLaunchview_tile_limit/rtx/viewTile/limitWithout these fields, users can only reach RT2 quality knobs through the
carb_settingsescape-hatch dict, while the existing named fields (samples_per_pixel,enable_translucency,enable_reflections,enable_direct_lighting) map to RT1 Legacy carb paths that RT2 ignores entirely.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there