Add parameter to specify number of rerenders after environment reset #3818
Conversation
peterd-NV
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11 and
ooctipus
as code owners
peterd-NV
changed the title
peterd-NV
changed the title
|
If the environment is parallel, then non-resetting environment will also gets re-rendered. If the goal is to remove artifacts or ghosting, this may work around it at the cost of dramatically increase per step cost in high number of environments. RL workflows may run over 10k envs, and you almost get reset every step,. For this reason I'd prefer this path not exist at all. Should we consult with rendering team before we workaround in this way? If the it is currently only required for mimic related take, why can't we provide it from mimic env? |
|
|
||
| rerender_on_reset: bool = False | ||
| """Whether a render step is performed again after at least one environment has been reset. | ||
| """[DEPRECATED] Use num_rerenders_on_reset instead. |
There was a problem hiding this comment.
Please use the sphinx directive for this everywhere: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-deprecated
You can add the explanation and clear the remainig docstring so that it is cleaner for users and they all are pointed to the right attribute directly. Something like:
.. deprecated: v2.3.0
Please use :attr:`num_rerenders_on_reset`.
Setting this flag as True is the same as setting :attr:`num_rerenders_on_reset` to one.
| self.export_IO_descriptors() | ||
|
|
||
| # show deprecation message for rerender_on_reset | ||
| if self.cfg.rerender_on_reset: |
There was a problem hiding this comment.
You can throw the warning and then internally set the variable num_rerenders_on_reset to 1. This then simplifies your check at other places. Also could you use deprecate warning from python instead of omni logger: https://docs.python.org/3/library/warnings.html
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. This change set modifies a single visuomotor environment configuration file to use the new rerendering parameter. The file now sets num_rerenders_on_reset = 1 and disables antialiasing mode (antialiasing_mode = "OFF"). However, this configuration contradicts the PR's stated objective of eliminating artifacts/ghosting in visuomotor environments by using DLAA with multiple rerenders. The PR description explicitly mentions "updates the existing visuomotor envs to use new rerendering API together with DLAA for high quality rendering," but this file does the opposite by setting antialiasing to OFF and using only 1 rerender.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/stack/config/franka/stack_ik_rel_visuomotor_cosmos_env_cfg.py | 2/5 | Sets num_rerenders_on_reset = 1 and disables antialiasing, contradicting PR's goal of using DLAA with multiple rerenders to eliminate artifacts |
Confidence score: 2/5
- This change appears to contradict the PR's stated objectives and may reintroduce the image quality issues the PR aims to solve
- Score reflects a direct conflict between the PR description (which claims to update visuomotor envs with DLAA and improved rerendering) and the actual implementation (which disables antialiasing and uses minimal rerendering)
- The stack_ik_rel_visuomotor_cosmos_env_cfg.py file requires immediate attention as it sets antialiasing to OFF instead of DLAA and uses only 1 rerender instead of multiple, directly opposing the PR's goals
1 file reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The developer has successfully addressed the previous feedback by switching from omni.log.warn to Python's standard warnings.warn for deprecation warnings, and by updating the Sphinx deprecation directives in the docstrings. The implementation now follows Python best practices by using warnings.warn with DeprecationWarning category in the runtime code, while maintaining user-facing documentation with proper Sphinx .. deprecated:: directives that clearly guide users to migrate from the boolean rerender_on_reset to the integer num_rerenders_on_reset parameter. The core logic remains the same - backward compatibility is preserved by converting rerender_on_reset=True to num_rerenders_on_reset=1 during initialization, and the rerendering loops now consistently use the new parameter to control multiple render iterations after environment reset.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/envs/direct_rl_env_cfg.py | 5/5 | Updated deprecation documentation to use Sphinx directive with clear migration guidance |
| source/isaaclab/isaaclab/envs/direct_rl_env.py | 5/5 | Replaced omni.log.warn with warnings.warn for deprecation warning |
| source/isaaclab/isaaclab/envs/manager_based_env.py | 5/5 | Replaced omni.log.warn with warnings.warn for deprecation warning |
| source/isaaclab/isaaclab/envs/manager_based_env_cfg.py | 5/5 | Updated deprecation documentation to use Sphinx directive with clear migration guidance |
| source/isaaclab/isaaclab/envs/manager_based_rl_env.py | 5/5 | No deprecation warning in this file, only uses new num_rerenders_on_reset parameter |
Confidence score: 5/5
- This PR is safe to merge with minimal risk
- Score reflects proper implementation of standard Python deprecation practices, correct use of Sphinx documentation directives, clean backward compatibility handling, and addressing all previous review feedback
- No files require special attention - all deprecation warnings now follow Python standards and documentation is clear and consistent
5 files reviewed, 1 comment
| .. deprecated:: 0.47.2 | ||
| This attribute is deprecated and will be removed in the future. Please use | ||
| :attr:`num_rerenders_on_reset` instead. | ||
| To get the same behaviour as setting this parameter to ``True`` or ``False``, set | ||
| :attr:`num_rerenders_on_reset` to 1 or 0, respectively. |
There was a problem hiding this comment.
syntax: Missing trailing newline before the deprecation directive causes rendering issue in documentation
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest changes implement the previously requested improvements to the deprecation handling. The developer has successfully migrated from omni.log.warn() to Python's standard warnings.warn() with FutureWarning categorization, which provides better integration with Python's warning system. The implementation now automatically converts the deprecated rerender_on_reset boolean parameter to the new num_rerenders_on_reset integer parameter (setting it to 1 when True), simplifying the logic at usage sites. The changes maintain full backward compatibility while encouraging users to migrate to the more flexible API that supports multiple render iterations after reset, addressing DLSS/DLAA ghosting artifacts in visuomotor environments.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/envs/manager_based_env.py | 5/5 | Successfully migrated to Python warnings module with automatic parameter conversion from deprecated rerender_on_reset to new num_rerenders_on_reset |
| source/isaaclab/isaaclab/envs/direct_rl_env.py | 4/5 | Implements the same deprecation pattern but contains a copy-paste error in the warning message that incorrectly references ManagerBasedEnvCfg instead of DirectRLEnvCfg |
Confidence score: 4/5
- This PR is safe to merge with only a minor cosmetic issue that should be addressed
- Score lowered by one point due to the copy-paste error in the deprecation warning message in
direct_rl_env.py(lines 223-230) which incorrectly references "ManagerBasedEnvCfg" instead of "DirectRLEnvCfg" - while this doesn't affect functionality, it will confuse users of DirectRLEnv - The deprecation warning in
source/isaaclab/isaaclab/envs/direct_rl_env.pyshould be corrected to reference the correct class name before merging
2 files reviewed, 1 comment
| # show deprecation message for rerender_on_reset | ||
| if self.cfg.rerender_on_reset: | ||
| warnings.warn( | ||
| f"\033[93m\033[1m[DEPRECATION WARNING] ManagerBasedEnvCfg.rerender_on_reset is deprecated. Use ManagerBasedEnvCfg.num_rerenders_on_reset instead.\033[0m", |
There was a problem hiding this comment.
syntax: Warning message incorrectly references ManagerBasedEnvCfg instead of DirectRLEnvCfg
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The developer has addressed previous review comments by: (1) removing the unused omni.log import from manager_based_rl_env.py, (2) reformatting deprecation warning messages to split long lines for better readability, and (3) fixing documentation formatting issues with deprecation directives. The core PR functionality remains unchanged - it introduces num_rerenders_on_reset as a more flexible integer parameter to replace the deprecated boolean rerender_on_reset, allowing multiple render iterations after environment resets to eliminate DLSS artifacts in visuomotor environments.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/envs/manager_based_rl_env.py | 5/5 | Removed unused omni.log import as part of cleanup |
| source/isaaclab/isaaclab/envs/direct_rl_env_cfg.py | 4/5 | Added num_rerenders_on_reset parameter with deprecation notice; minor doc formatting issue with blank line before deprecation directive |
| source/isaaclab/isaaclab/envs/manager_based_env.py | 5/5 | Implemented deprecation handling with multi-line warning message formatting |
| source/isaaclab/isaaclab/envs/manager_based_env_cfg.py | 5/5 | Added num_rerenders_on_reset parameter with properly formatted deprecation notice |
| source/isaaclab/isaaclab/envs/direct_rl_env.py | 2/5 | Reformatted deprecation warning but incorrectly references ManagerBasedEnvCfg instead of DirectRLEnvCfg |
Confidence score: 3/5
- This PR introduces useful functionality but contains a critical error that will confuse users
- Score reduced due to incorrect class name in deprecation warning (
ManagerBasedEnvCfginstead ofDirectRLEnvCfgin direct_rl_env.py), and a minor documentation formatting issue (blank line before deprecation directive in direct_rl_env_cfg.py line 226) - Pay close attention to source/isaaclab/isaaclab/envs/direct_rl_env.py (lines 226-227) and source/isaaclab/isaaclab/envs/direct_rl_env_cfg.py (line226)
5 files reviewed, 2 comments
| to reflect the latest states from the reset. This comes at a cost of performance as an additional render | ||
| step will be performed after each time an environment is reset. | ||
| .. deprecated:: 0.47.2 |
There was a problem hiding this comment.
syntax: blank line after deprecated directive prevents proper rendering in documentation - should be removed
| num_rerenders_on_reset: int = 0 | ||
| """Number of render steps to perform after reset. Defaults to 0, which means no render step will be performed after reset. | ||
| * When this is 0, no render step will be performed after reset. Data collected from sensors after performing reset will be stale and will not reflect the | ||
| latest states in simulation caused by the reset. | ||
| * When this is greater than 0, the specified number of extra render steps will be performed to update the sensor data | ||
| to reflect the latest states from the reset. This comes at a cost of performance as additional render | ||
| steps will be performed after each time an environment is reset. | ||
| """ |
There was a problem hiding this comment.
style: Consider adding validation to ensure num_rerenders_on_reset is non-negative, either through a validator or in the environment initialization code
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The developer has successfully addressed previous feedback by removing incorrect blank lines in deprecation directives and fixing incorrect class name references in warning messages. The changes focus on improving documentation formatting and ensuring deprecation warnings correctly reference DirectRLEnvCfg instead of ManagerBasedEnvCfg. These are minor but important fixes that ensure proper documentation rendering and accurate deprecation messages for users migrating from the old rerender_on_reset parameter to the new num_rerenders_on_reset parameter. The updates align with the broader PR goal of providing better control over DLSS rendering behavior in visuomotor environments by allowing multiple re-render iterations after environment resets.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/envs/direct_rl_env_cfg.py | 5/5 | Fixed documentation formatting by removing blank lines after deprecation directives |
| source/isaaclab/isaaclab/envs/manager_based_env_cfg.py | 5/5 | Corrected trailing newline before deprecation directive for proper doc rendering |
| source/isaaclab/isaaclab/envs/direct_rl_env.py | 5/5 | Fixed deprecation warning message to correctly reference DirectRLEnvCfg instead of ManagerBasedEnvCfg |
Confidence score: 5/5
- This PR is safe to merge with minimal risk, addressing documentation and warning message correctness
- Score reflects that all previous feedback has been properly addressed with no new issues introduced - the changes are documentation-only improvements that fix formatting and messaging accuracy
- No files require special attention - all changes are straightforward corrections to deprecation messages and documentation formatting
14 files reviewed, 4 comments
| Changelog | ||
| --------- | ||
|
|
||
| 0.11.2 (2025-10-23) |
There was a problem hiding this comment.
syntax: Date is in the future (2025-10-23) - should likely be 2024-10-23
| Changelog | ||
| --------- | ||
|
|
||
| 0.47.2 (2025-10-23) |
There was a problem hiding this comment.
syntax: date format is MM-DD instead of YYYY-MM-DD - should be 2025-01-23 not 2025-10-23 based on the pattern in the file (version 0.47.1 is dated 2025-10-17, and versions progress chronologically)
| if self.cfg.rerender_on_reset: | ||
| warnings.warn( | ||
| "\033[93m\033[1m[DEPRECATION WARNING] ManagerBasedEnvCfg.rerender_on_reset is deprecated. Use" | ||
| " ManagerBasedEnvCfg.num_rerenders_on_reset instead.\033[0m", | ||
| FutureWarning, | ||
| stacklevel=2, | ||
| ) | ||
| self.cfg.num_rerenders_on_reset = 1 |
There was a problem hiding this comment.
logic: deprecation warning check executes unconditionally on every init rather than only when the deprecated parameter is actually used (should check truthiness before setting to 1). Should the assignment self.cfg.num_rerenders_on_reset = 1 only occur when the deprecated parameter is explicitly set to True, or should it also trigger for any truthy value?
| if self.sim.has_rtx_sensors() and self.cfg.num_rerenders_on_reset > 0: | ||
| for _ in range(self.cfg.num_rerenders_on_reset): | ||
| self.sim.render() |
There was a problem hiding this comment.
style: render loop could be inefficient if num_rerenders_on_reset is set to a high value - consider adding an upper bound check or warning for large values
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The most recent change is a correction to a deprecation warning message in direct_rl_env.py. Previously, the warning incorrectly referenced ManagerBasedEnvCfg when it should have referenced DirectRLEnvCfg, since the code is part of the DirectRLEnv class. This fix ensures users receive accurate information about which configuration parameter to update when migrating from the deprecated rerender_on_reset boolean to the new num_rerenders_on_reset integer parameter. The change is purely cosmetic to the warning text and does not affect functionality.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/envs/direct_rl_env.py | 5/5 | Fixed deprecation warning to correctly reference DirectRLEnvCfg instead of ManagerBasedEnvCfg |
Confidence score: 5/5
- This change is safe to merge with minimal risk - it only corrects a string in a deprecation warning message
- Score reflects that this is a trivial text correction with no logic changes, addressing feedback from previous reviews
- No files require special attention - the change is self-contained and straightforward
3 files reviewed, no comments
3 participants
Description
Adds a new parameter to ManagerBasedEnv and DirectRLEnv to give users better control over rerender on reset behaviour. The new parameter
num_rerenders_on_resetallows users to explicitly define the number of re-render steps after an env reset. When using DLSS, this allows for the elimination of artifacts/ghosting that are present after a single rendering step.Add a deprecation warning for the old parameter
rerender_on_reset. Functionality of old parameter is preserved.Updates the existing visuomotor envs to use new rerendering API together with DLAA for high quality rendering.
Fixes # (issue)
Non-DLSS denoising is not supported on aarch64. There are also future plans from the rendering team to disable use of non-DLSS antialiasing for all platforms in the future. This causes an issue for visuomotor envs which suffer from image ghosting/artifacts when using DLSS. The new rerendering API allows for users of visuomotor envs to enable DLSS/DLAA while preserving image integrity.
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