Avoid continuous rendering between video captures - #7642
Conversation
Greptile SummaryThis PR prevents capture-only headless visualizers from forcing rendering and PhysX Fabric transform synchronization throughout a training run. It:
Confidence Score: 5/5The PR appears safe to merge, with the on-demand capture flow synchronized without disrupting continuous RTX camera rendering. The changed rendering classification, Fabric-setting lifecycle, and capture-time synchronization are mutually consistent and covered by focused regression tests; no actionable correctness, security, or repository-rule issue remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant Training as Training loop
participant Sim as SimulationContext
participant Recorder as VideoRecorder
participant Physics as PhysicsManager
participant Viz as Headless visualizer
Note over Sim: Headless capture-only visualizer<br/>does not enable continuous rendering
loop Between capture windows
Training->>Sim: Step physics
Note over Sim,Physics: Continuous Fabric transform<br/>synchronization remains disabled
end
rect rgb(235, 245, 255)
Note over Recorder,Viz: Scheduled capture window
Training->>Recorder: Request frame
Recorder->>Sim: Check is_rendering
Sim-->>Recorder: false
Recorder->>Sim: forward()
Sim->>Physics: Update kinematics
Physics-->>Sim: Transforms synchronized
Recorder->>Viz: render_rgb_array()
Viz-->>Recorder: Current frame
end
Reviews (1): Last reviewed commit: "Avoid continuous rendering between video..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
Reviewed the change that classifies headless capture-only visualizers as on-demand, disables continuous PhysX Fabric synchronization for non-rendering simulations, restores it for Isaac RTX cameras, and synchronizes immediately before visualizer capture. The supplied findings do not establish a broken rendering path from the available context.
- Design and architecture: The continuous-versus-on-demand rendering split is coherent for periodic headless video capture. Synchronization is intentionally performed at the visualizer capture boundary, while continuously rendered configurations retain per-step updates. The remaining coupling between SimulationContext settings, camera initialization, and VideoRecorder capture is a maintenance tradeoff but is not shown here to cause a concrete defect.
- API: No public signature changed. The narrowed is_rendering behavior is described as continuous rendering, and its docstring already distinguishes headless offscreen, on-demand rendering from per-step rendering, so the proposed documentation finding is not supported as a contradiction.
- Implementation: Traced the Fabric setting from SimulationContext initialization through the Isaac RTX camera override and VideoRecorder's guarded sim.forward() call. The claim that OVRTX or generic rgb_array rendering now receives stale transforms lacks the renderer and render-path context needed to demonstrate that those consumers depend on this PhysX Fabric setting or lack their own synchronization.
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.
kellyguo11
left a comment
There was a problem hiding this comment.
@matthewtrepte for review
|
|
||
| settings = get_settings_manager() | ||
| settings.set_bool("/isaaclab/render/rtx_sensors", True) | ||
| settings.set_bool("/physics/fabricUpdateTransformations", True) |
There was a problem hiding this comment.
why is this needed here?
There was a problem hiding this comment.
SimulationContext sets /physics/fabricUpdateTransformations during initialization, before scene-owned Camera objects are constructed. An Isaac RTX camera subsequently makes is_rendering true by setting the adjacent rtx_sensors flag, but that property does not mutate the Fabric setting. Without this line, Fabric remains disabled and camera transforms can be stale. Camera already owns the RTX sensor flag for the same lifecycle reason; the TODO above tracks moving both settings to a renderer pre-reset hook. test_camera_init verifies this path on the RTX 5090.
|
|
||
| viz = candidates[0] | ||
| if not sim.is_rendering: | ||
| sim.forward() |
There was a problem hiding this comment.
should this be the responsibility of the video recorder?
There was a problem hiding this comment.
Yes—the current placement in VideoRecorder is intentional. It is the component that knows an on-demand frame is about to be requested. Moving the synchronization into KitVisualizer would couple the visualizer to the active physics backend and would also affect non-recorder consumers. sim.forward() delegates to the configured PhysicsManager and only runs here when continuous rendering is disabled. The dual-recorder integration test added in d319ed5 exercises this boundary with two simultaneous capture requests.
|
ah nice find Antoine. for an edge case, can you test if we launch multiple headless kit video recordings at once (with multiple VideoRecordingCfg files), if the on demand method doesn't cause any issues. |
|
@matthewtrepte I tested one explicit |
Description
--videoenabled a headless Kit visualizer and continuous PhysX Fabric transform updates for the full training run, even though frames are only needed during capture windows. This caused a substantial FPS drop between recordings.This change treats headless visualizers as on-demand rather than continuous renderers, disables continuous Fabric synchronization after PhysX initialization, and synchronizes transforms immediately before each visualizer frame capture. RTX camera sensors explicitly retain continuous Fabric updates.
RTX 5090 benchmark
Isaac-Humanoid-Direct, PhysX, 4,096 environments, RSL-RL, 16 iterations. Idle results exclude iterations 0 and 10, which contain the two scheduled 32-frame capture windows.--videowith this fixThe unfixed periodic-video run reproduced approximately 214,300 FPS between captures, about 34% below its plain-headless control. With this fix, most of that avoidable idle-window regression is removed. Capture iterations still drop as expected; the fixed periodic run measured 76,127 and 135,151 FPS during its two capture iterations, with 289,675 FPS overall including both captures.
No new dependencies are required.
Type of change
Release backport
developScreenshots
Not applicable; this is a performance fix. A frame probe on the RTX 5090 verified that all 20 captured frames were non-black and changed over time (
temporal_std=72.21, mean consecutive delta12.46).Checklist
Docker and GPU tests run on demand. Push the commits you want tested, then
comment
run-cion the pull request.pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists thereValidation
uv run --frozen --extra test --extra rerun python -m pytest source/isaaclab/test/sim/test_simulation_context_visualizers.py source/isaaclab/test/envs/test_video_recorder.py -q— 74 passeduv run --frozen isaaclab -f— all hooks passedsource/isaaclab/test/sensors/test_camera.py::test_camera_init— passed on RTX 5090