Fix streaming_gt_types docstring and polish visualization docs hero videos - #7588
Fix streaming_gt_types docstring and polish visualization docs hero videos#7588matthewtrepte wants to merge 3 commits into
Conversation
…edback Documents the already-supported 'normals' streaming GT type and switches to American spelling (initialisation -> initialization) in two VisualizerCfg docstrings, closing the last outstanding items from review on isaac-sim#7054's visualization docs restructure. Also reworks the visualization.rst hero video CSS: crops more off the top and bottom of the 2x2 hero tile grid, adds an extra top-only crop for the top row (Viser/Newton RTX), crops more off the top of the standalone Newton GL hero clip, and restacks the Streaming Camera View videos vertically, centered at 65% width with natural aspect ratio.
There was a problem hiding this comment.
Isaac Lab Review Bot
The patch aligns the VisualizerCfg.streaming_gt_types documentation with existing "normals" support, standardizes spelling, adds the appropriate package changelog fragment, and updates the visualization page’s video presentation without changing runtime behavior.
- Design and architecture: The presentation changes remain scoped to the page-local CSS and markup. The new
.viz-stack-centeredlayout replaces the prior streaming-video grid consumer, while.viz-hero-row-topadds a targeted override without altering unrelated hero rows. - API: The public field name, type, default, validation path, and accepted runtime values are unchanged. The docstring now accurately reflects
SUPPORTED_GT_TYPES, and the changelog fragment records the user-visible documentation correction without requiring migration guidance. - Implementation: The hero-tile dimensions and offsets are internally consistent with the documented crop calculations. The former
.viz-grid-naturalstreaming-video consumer is updated alongside removal of its rules, and the replacement preserves centered videos with natural aspect ratios and captions.
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.
Greptile SummaryThis PR corrects
Confidence Score: 4/5The PR appears safe to merge, with only a non-blocking contradictory CSS maintenance comment to clarify. The runtime documentation matches the supported ground-truth types, and the new layout selectors correctly target the intended markup; the sole concern is an inaccurate explanatory comment that could confuse later maintenance. Files Needing Attention: docs/source/concepts/visualization.rst Important Files Changed
Reviews (1): Last reviewed commit: "Fix streaming_gt_types docstring and add..." | Re-trigger Greptile |
| /* Trims pixels off each hero tile on top of whatever object-position crop is already | ||
| applied, so the tile itself is shorter rather than just repositioning the existing crop. | ||
| Both classes crop to the same final 241px height so the 2x2 tile grid stays even, split | ||
| differently per tile: Kit/Rerun/Newton RTX crop 35px off the top and 10px off the bottom; | ||
| Viser crops 25px off the top and 20px off the bottom (its object-position framing already | ||
| Both classes crop to the same final 221px height so the 2x2 tile grid stays even, split | ||
| differently per tile: Kit/Rerun/Newton RTX crop 45px off the top and 20px off the bottom; | ||
| Viser crops 35px off the top and 30px off the bottom (its object-position framing already | ||
| leaves more headroom at the bottom, so it can take a heavier bottom crop). */ |
There was a problem hiding this comment.
Contradictory hero grid comment
This comment says both crop classes keep the 2x2 tile grid even at 221px, but the new top-row rules override those tiles to 206px. The contradiction could make future crop adjustments error-prone; clarify that tiles within each row have equal heights while the two rows intentionally differ.
| /* Trims pixels off each hero tile on top of whatever object-position crop is already | |
| applied, so the tile itself is shorter rather than just repositioning the existing crop. | |
| Both classes crop to the same final 241px height so the 2x2 tile grid stays even, split | |
| differently per tile: Kit/Rerun/Newton RTX crop 35px off the top and 10px off the bottom; | |
| Viser crops 25px off the top and 20px off the bottom (its object-position framing already | |
| Both classes crop to the same final 221px height so the 2x2 tile grid stays even, split | |
| differently per tile: Kit/Rerun/Newton RTX crop 45px off the top and 20px off the bottom; | |
| Viser crops 35px off the top and 30px off the bottom (its object-position framing already | |
| leaves more headroom at the bottom, so it can take a heavier bottom crop). */ | |
| /* Trims pixels off each hero tile on top of whatever object-position crop is already | |
| applied, so the tile itself is shorter rather than just repositioning the existing crop. | |
| Both classes initially crop to the same 221px height, keeping tiles within each row even. | |
| Kit/Rerun/Newton RTX crop 45px off the top and 20px off the bottom; Viser crops 35px off | |
| the top and 30px off the bottom (its object-position framing already leaves more headroom | |
| at the bottom, so it can take a heavier bottom crop). */ |
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!
The comment claimed both crop classes always produce a 221px tile, but the top-row override added afterward shrinks Viser/Newton RTX to 206px, leaving the two rows' heights undocumented and contradicting each other. Split the comment so it states tiles are only even within each row, and documents the top row's override separately.
Summary
streaming_gt_typesdocstring inVisualizerCfgwas missing the already-supported"normals"value, and used British spelling (initialisation) in two places. Both fixed, matchingSUPPORTED_GT_TYPESand the docs page which already advertised normals support.Test plan
uv run isaaclab -f(ruff, ruff format, rst checks, changelog fragment check) — all passeduv run python tools/changelog/cli.py check develop— passeddevelopTOT:SUPPORTED_GT_TYPESincludes"normals", the docs page advertises normals support, and the feature-support table correctly separates markers/live plots and shows RTX video support without contradicting Rerun/Viser's lack of--videosupportuv run --extra test make current-docsindocs/) and visually inspected the renderedvisualization.htmlfor the CSS changes