Record Newton markers and overlays in videos#6280
Conversation
ooctipus
requested review from
Mayankm96,
Toni-SM,
jtigue-bdai and
kellyguo11
as code owners
github-actions
Bot
added
documentation
Greptile SummaryThis PR wires Newton visualizer state — selected environments, world spacing, and marker overlays — into the Newton GL video capture, so recorded clips now mirror the live viewer exactly. It also fixes the env-major marker filtering bug where the old block-major loop was sampling markers from the wrong indices.
Confidence Score: 3/5The recording pipeline change is safe, but The env-major filter rewrite is correct and the new capture APIs are well-designed with good test coverage. The open question is in
Important Files Changed
Reviews (1): Last reviewed commit: "Record Newton visualizer overlays in vid..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| spacing = getattr(viewer, "_user_spacing", None) | ||
| if spacing is not None and all(float(spacing[axis]) == 0.0 for axis in range(3)): | ||
| return translations | ||
|
|
||
| world_offsets = getattr(viewer, "world_offsets", None) | ||
| if world_offsets is None: | ||
| return translations | ||
|
|
||
| if isinstance(world_offsets, torch.Tensor): | ||
| offsets = world_offsets | ||
| elif isinstance(world_offsets, np.ndarray): | ||
| offsets = torch.as_tensor(world_offsets) | ||
| else: | ||
| try: | ||
| offsets = wp.to_torch(world_offsets) | ||
| except (AttributeError, RuntimeError, TypeError): | ||
| return translations | ||
| if offsets.ndim != 2 or offsets.shape[1] != 3: | ||
| return translations | ||
|
|
||
| if world_indices is None: | ||
| if num_envs <= 0 or translations.shape[0] % num_envs != 0: | ||
| return translations | ||
| repeat_count = translations.shape[0] // num_envs | ||
| world_indices = torch.arange(num_envs, device=translations.device).repeat_interleave(repeat_count) | ||
|
|
||
| indices = world_indices.to(device=offsets.device) | ||
| selected_offsets = offsets.index_select(0, indices).to(device=translations.device, dtype=translations.dtype) | ||
| return translations + selected_offsets |
There was a problem hiding this comment.
world_offsets is indexed by env-id
_apply_marker_world_offsets feeds world_indices (original env IDs, e.g. [1, 1, 3, 3]) directly into offsets.index_select(0, indices). This is correct only if Newton's world_offsets array is shaped (num_envs, 3) and indexed by env-id. If Newton allocates world_offsets only for the currently visible worlds and indexes them by position, then with visible_env_ids=[1, 3], world_offsets would have 2 rows. Indexing into it with env-id 3 would trigger an IndexError, and env-id 1 would receive the wrong offset. All tests set world_offsets manually on a _FakeNewtonMarkerViewer so they pass regardless of Newton's actual layout. A bounds guard or a confirmed guarantee from Newton's API would resolve the ambiguity. Does Newton's ViewerGL.world_offsets always have num_envs rows indexed by env-id, or does it only allocate rows for the currently visible worlds (indexed by position within the visible list)?
| spacing = getattr(viewer, "_user_spacing", None) | ||
| if spacing is not None and all(float(spacing[axis]) == 0.0 for axis in range(3)): | ||
| return translations |
There was a problem hiding this comment.
viewer._user_spacing is an undocumented internal attribute of Newton's ViewerGL. The getattr fall-through means a missing attribute only loses the short-circuit optimization. However, the early-exit is logically redundant: if spacing is zero, Newton sets all rows of world_offsets to zero, and translations + selected_offsets returns the same result anyway. Removing the _user_spacing check and relying solely on world_offsets would make the function independent of Newton internals.
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!
| set_frame_overlay_callback = getattr(self._capture, "set_frame_overlay_callback", None) | ||
| render_markers = getattr(self._live_visualizer, "render_markers", None) | ||
| if callable(set_frame_overlay_callback): | ||
| enable_markers = bool(getattr(self._live_visualizer.cfg, "enable_markers", False)) | ||
| callback = render_markers if enable_markers and callable(render_markers) else None | ||
| set_frame_overlay_callback(callback) |
There was a problem hiding this comment.
set_frame_overlay_callback called every frame without deduplication
set_visible_worlds and set_world_offsets both guard against redundant Newton calls via equality checks, but set_frame_overlay_callback is reset unconditionally on every sync. The callback is a bound method whose identity is stable across frames, so a simple is not identity check would mirror the deduplication pattern used elsewhere.
| set_frame_overlay_callback = getattr(self._capture, "set_frame_overlay_callback", None) | |
| render_markers = getattr(self._live_visualizer, "render_markers", None) | |
| if callable(set_frame_overlay_callback): | |
| enable_markers = bool(getattr(self._live_visualizer.cfg, "enable_markers", False)) | |
| callback = render_markers if enable_markers and callable(render_markers) else None | |
| set_frame_overlay_callback(callback) | |
| set_frame_overlay_callback = getattr(self._capture, "set_frame_overlay_callback", None) | |
| render_markers = getattr(self._live_visualizer, "render_markers", None) | |
| if callable(set_frame_overlay_callback): | |
| enable_markers = bool(getattr(self._live_visualizer.cfg, "enable_markers", False)) | |
| callback = render_markers if enable_markers and callable(render_markers) else None | |
| if getattr(self._capture, "_frame_overlay_callback", ...) is not callback: | |
| set_frame_overlay_callback(callback) |
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!
1 participant
Summary
Testing
75 passed:test_video_recorder.py,test_newton_gl_perspective_video.py, andtest_newton_adapter.py./isaaclab.sh -d(documentation build succeeded)./isaaclab.sh -fpassed every hook exceptcheck-git-lfs-pointers, which reports unrelated golden-image pointers already present onupstream/developtest_visualization_markers.pywas not run locally because the available environment is kitless and does not include Isaac Sim; its added regression cases are included for Isaac Sim CI.