add RenderContext to docs#5489
Conversation
Greptile SummaryThis PR updates the renderer documentation to replace direct
Confidence Score: 3/5Safe to merge after fixing the missing BaseRenderer imports; broken code snippets in docs will mislead users. Two P1 findings: the RTX and OVRTX code blocks use BaseRenderer as a type annotation without importing it, meaning verbatim copy-paste from the docs will fail. Two P2 findings reduce confidence further. docs/source/overview/core-concepts/renderers.rst — all four code blocks need import corrections. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Camera sensor initialized\n(renderer_cfg set on CameraCfg)"] --> B["sim_ctx.render_context.get_renderer(cfg)"]
B --> C{Matching cfg already\nregistered?}
C -- Yes --> D["Return existing BaseRenderer\n(shared instance)"]
C -- No --> E["Renderer(cfg)\n(factory instantiates backend)"]
E --> F["Store (cfg, backend) in\nRenderContext._renderer_entries"]
F --> D
D --> G["Camera uses shared BaseRenderer\nfor rendering"]
Reviews (1): Last reviewed commit: "add RenderContext to docs" | Re-trigger Greptile |
| import isaaclab.sim as sim_utils | ||
| from isaaclab.renderers import BaseRenderer, Renderer | ||
| from isaaclab_newton.renderers import NewtonWarpRendererCfg |
There was a problem hiding this comment.
The
Renderer import is unused after switching to RenderContext-based instantiation. Leaving it in contradicts the new "Use the RenderContext" guidance directly below and will confuse readers who try to follow these examples.
| import isaaclab.sim as sim_utils | |
| from isaaclab.renderers import BaseRenderer, Renderer | |
| from isaaclab_newton.renderers import NewtonWarpRendererCfg | |
| import isaaclab.sim as sim_utils | |
| from isaaclab.renderers import BaseRenderer | |
| from isaaclab_newton.renderers import NewtonWarpRendererCfg |
| import isaaclab.sim as sim_utils | ||
| from isaaclab.renderers import Renderer | ||
| from isaaclab.renderers import IsaacRtxRendererCfg # or OVRTXRendererCfg |
There was a problem hiding this comment.
The RTX renderer block imports
Renderer (now unused) and omits BaseRenderer (used as a type annotation on the last line). Anyone copying this snippet verbatim will get a NameError: name 'BaseRenderer' is not defined.
| import isaaclab.sim as sim_utils | |
| from isaaclab.renderers import Renderer | |
| from isaaclab.renderers import IsaacRtxRendererCfg # or OVRTXRendererCfg | |
| import isaaclab.sim as sim_utils | |
| from isaaclab.renderers import BaseRenderer | |
| from isaaclab.renderers import IsaacRtxRendererCfg # or OVRTXRendererCfg |
| import isaaclab.sim as sim_utils | ||
| # Lightweight: does not import OVRTX backend dependencies | ||
| from isaaclab_ov.renderers import OVRTXRendererCfg |
There was a problem hiding this comment.
The OVRTX block annotates
renderer: BaseRenderer but never imports BaseRenderer. The snippet is missing the import, so copy-paste would fail at runtime.
| import isaaclab.sim as sim_utils | |
| # Lightweight: does not import OVRTX backend dependencies | |
| from isaaclab_ov.renderers import OVRTXRendererCfg | |
| import isaaclab.sim as sim_utils | |
| from isaaclab.renderers import BaseRenderer | |
| # Lightweight: does not import OVRTX backend dependencies | |
| from isaaclab_ov.renderers import OVRTXRendererCfg |
| - **Use the RenderContext**: Always instantiate renderers via the RenderContext with a renderer-specific config class | ||
| (e.g. ``RenderContext.get_renderer(IsaacRtxRendererCfg())``). Do not import or instantiate concrete backend classes |
There was a problem hiding this comment.
RenderContext.get_renderer(...) reads like a class-method or static-method call, but get_renderer is an instance method on RenderContext. The example should reflect the actual call path (sim_ctx.render_context.get_renderer(...)) so readers aren't misled about the API shape.
| - **Use the RenderContext**: Always instantiate renderers via the RenderContext with a renderer-specific config class | |
| (e.g. ``RenderContext.get_renderer(IsaacRtxRendererCfg())``). Do not import or instantiate concrete backend classes | |
| - **Use the RenderContext**: Always instantiate renderers via the RenderContext with a renderer-specific config class | |
| (e.g. ``sim_ctx.render_context.get_renderer(IsaacRtxRendererCfg())``). Do not import or instantiate concrete backend classes |
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!
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR updates documentation to reflect a new RenderContext API for renderer instantiation, replacing the previous Renderer factory pattern. The changes update code examples in renderers.rst and add a clarifying sentence about renderer sharing in camera.rst. The documentation changes are straightforward and improve API guidance.
Architecture Impact
Self-contained. These are pure documentation changes with no code impact. The documentation reflects an API change that presumably already exists in the codebase (transitioning from Renderer() factory to sim_ctx.render_context.get_renderer()).
Implementation Verdict
Minor fixes needed
Test Coverage
N/A for documentation-only changes. The checklist claims tests were added, but this is a docs-only PR — that checkbox appears to be incorrectly marked.
CI Status
No CI checks available yet — cannot verify documentation builds successfully.
Findings
🔵 Improvement: docs/source/overview/core-concepts/renderers.rst:78 — Incomplete code reference in guidance
The guidance says RenderContext.get_renderer(IsaacRtxRendererCfg()) but the actual usage pattern shown is sim_ctx.render_context.get_renderer(...). For consistency and copy-paste usability, consider showing the full pattern: sim_ctx.render_context.get_renderer(IsaacRtxRendererCfg()) or document that RenderContext is accessed via SimulationContext.render_context.
🔵 Improvement: docs/source/overview/core-concepts/renderers.rst:42 — Missing cross-reference to RenderContext class
The new point #5 introduces RenderContext but doesn't include a Sphinx cross-reference like the other components. Consider adding :class:\~isaaclab.renderers.RenderContext`` (or wherever the class lives) for API documentation linkage.
🔵 Improvement: docs/source/overview/core-concepts/renderers.rst:48-55 — Missing import for SimulationContext type hint
The code example uses sim_ctx but doesn't show its type. For completeness and IDE support when users copy this code, consider adding a comment or type hint showing sim_ctx: SimulationContext.
🟡 Warning: docs/source/overview/core-concepts/renderers.rst:55-56 — Removed BaseRenderer import in example
The old code imported BaseRenderer from isaaclab.renderers. The new code imports it correctly, but the assert isinstance(renderer, BaseRenderer) line still references it. This is fine, but verify this import path is still correct in the actual codebase.
🔵 Improvement: docs/source/overview/core-concepts/sensors/camera.rst:9-10 — Consider adding cross-reference
The new sentence "If multiple cameras use the same renderer type, only one renderer is instantiated and shared between them" is valuable. Consider linking to the RenderContext documentation or the renderers overview to help users understand the sharing mechanism.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This is a follow-up review on a documentation-only PR. The changes remain the same as the previous commit - no new modifications were made to address the previous findings.
Implementation Verdict
Ship it
Findings
The previous findings were all 🔵 Improvements (nice-to-haves) rather than blocking issues. The core documentation changes are correct and accurately reflect the new RenderContext API pattern. The clarification about renderer sharing in camera.rst is valuable.
The CI shows pre-commit: success and Check for Broken Links: success, which validates the documentation structure. The remaining pending checks are unrelated to docs.
Ship it — the documentation improvements are accurate and helpful. The previous suggestions for cross-references and type hints can be addressed in a follow-up if desired.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
No new changes since the previous review. The documentation updates for RenderContext remain accurate and well-structured.
Implementation Verdict
Ship it
Findings
No new issues. The previous review already approved this PR — the documentation correctly reflects the RenderContext API pattern, and CI checks (pre-commit, broken links) are passing. Ready to merge.
Description
Add RenderContext to documentation
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there