fix(compositor): let zooms overflow the preview padding - #186
Merged
Conversation
Closes #179 When a zoom region is active, the screen content was confined to the padded screen rect, so the zoomed viewport stopped at the padded boundary instead of reaching the edges of the output frame. Expand s_dst / s_dst_prev to the full output frame while p.zoom > 1.0 so the zoomed content overflows the padding as expected. The shadow and rounded corners currently follow the expansion too (drawn into the same rect via the pixel shader) — noted as a follow-up TODO in the comment.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
added 2 commits
July 28, 2026 13:26
…m overflow The previous fix extended s_dst to the full output frame whenever p.zoom > 1.0 so the zoomed viewport overflows the preview padding (issue #179). But s_min_px and s_dst now both refer to the full output frame, so the existing shadow and rounded-corner code scaled up too: * shadow: drawn around [0,0,1,1] with its 40px spread — reads as a black band against the frame edges, not as a shadow. * rounded corners: s_min_px collapses to min(rw, rh) and the two radius formulas (block preset, roundness slider) coincide on that value, so * min(rw, rh) rounded the entire output frame — 43px on 1080p. Neutralize both when p.zoom > 1.0: no shadow, no radius. The content reaches the edges unframed, which matches the issue's " reach the edges of the
…nary Follow-up to the original issue #179 fix and the f7c4317 bandaid. The two earlier commits each made the wrong call: the original swap to [0,0,1,1] in one step (abrupt padding disappearance), and f7c4317 kept the same switch but neutralized the shadow and radius to mask the regression (still abrupt on the padding ring, and now also abruptly loses the frame on zoom engage). The right behavior: the "zoom frame" (s_dst) grows continuously with the zoom, from the padded size up to the full frame. At zoom = 1 it's the padded area. As the zoom ramps up, the frame expands until it hits the frame edge at zoom = 1 / padding_scale (= frame / padded_size). Past that the frame stays put and only the source rect keeps shrinking — the GPU upscales further. The source texture itself is never touched, so full resolution is preserved all the way; only the mapping changes. s_radius and the shadow follow s_dst naturally now (s_min_px grows with the zoom frame), so the f7c4317 gates on those are removed. Result: no more binary switch anywhere. The padding smoothly fades as the zoom ramps up, the frame "follows" the zoom, and by the time the content hits the edges the frame is already at full size — visually consistent. Single TODO kept, pointing at the still-pending "frame rect" separation: if you want the shadow and corners to stay anchored at the padded box while the content overflows (the "window" effect), split the frame rect from the content rect and apply shadow+corners to the frame alone.
Review follow-up on the three earlier #179 commits. They all grew the screen box (s_dst) while STILL passing the full p.zoom to screen_source_rect, so the two multiplied: with padding 50% (padding_scale 0.8, cap 1.25) a depth-1 region authored at 1.25x rendered at 1.56x, a depth-4 at 2.75x. The reference does not do that — applyZoomTransform (TS) scales the camera container by exactly zoomScale and never crops the source on top. Give the box a share g of the zoom and take that same share OFF the source cut (p.zoom / g). The product stays zoom, so: * the magnification is exactly what the region asks for; * (box, cut) is one affine image->screen map and splitting it does not move that map — the focus lands on the same pixel it did before. Everything riding the map (cursor layer, 3D tilt, motion blur) is therefore unchanged by construction. Only the drawn EXTENT grows, which is precisely what the issue asks for. Two bounds on g, both derived from the geometry rather than from padding_scale: * cover — the size that covers the frame. Past it, growing adds nothing (the rasterizer clips). Taken from the real rect, so a letterboxed crop (16:9 in a 9:16 output) also eats its bars; the old 1/padding_scale cap never reached them. * focus — the source cut must stay centreable on the target, else screen_source_rect pins it to the crop edge and the zoom stops aiming. Same constraint getFocusBoundsForScale (TS) already applies upstream, so it never blocks growth for an app-authored focus. Also drops the [0,1] clamp on the grown box: it slid off-centre rects (block presets) sideways as they grew, and pinned the rounded corners and shadow to the frame edge instead of letting them leave it — the artifact the second commit had neutralized the radius and shadow to hide. Uncapped matches the reference ("No stage clamping", frameRenderer.cameraAwareMaskRect); the rasterizer clips the overflow, as it already does for the blurred background. screen_zoom_growth / grow_around_center are free functions next to screen_source_rect so they can be tested; the previous closure could not. Three tests, the first of which fails on any of the three earlier versions: the map-invariance sweep (magnification and focus position unchanged across zooms and focus positions), the frame-edge reach, and the focus-limit arbitration. cargo test -p openscreen-compositor --lib --tests: 79 + 3 pass. Bench note: the fixture (zoom 1.8, focus [0.5, 0.32], 5% margin) now grows its box at deep zoom, so bench frames change there by design.
Fixes the regression in f2c1faa, caught in manual testing: the padding still constrained the zoom. That commit capped the box growth by a "focus budget" — g = min(zoom, cover, zoom * 2*min(c, 1-c)) — meant to keep the source cut centreable on the focus. But the zoom focus follows the cursor and is almost never centred, so the third term dominated: focus 0.3 at zoom 1.5 gives 0.9, clamped back to 1.0, i.e. no growth at all. The fix was inert in the common case. The tests missed it because they swept centred focuses and focuses pinned exactly on their bound, never an ordinary off-centre one. Root cause: that commit refused to TRANSLATE the box (to avoid re-deriving ease() in regions.rs). Without translation the only lever left to preserve framing was refusing to grow — so it protected the framing by giving up the point of the PR. So translate. The box now takes the whole zoom and the drawn cut goes back to the bare crop, which is applyZoomTransform's geometry. The box placement is not a formula to get right: remap_box() carries the new cut through the old `cut_ref -> base` mapping, so the framing is preserved by construction — crop, edge clamp and cover are all already baked into the two cuts. Nothing left to protect, hence nothing to refuse. Consequences, all of them wanted: * the box grows by exactly `zoom` for every focus, so the padding is eaten from the first frame of the ramp; * src == src_prev now, and the zoom velocity the motion blur reads lives in dst vs dst_prev instead of in the cut; * an edge-focused zoom keeps the padding on the side where the image runs out — there is no content to put there, and the reference shows background in the same spot. Tests rewritten around the two things that pull against each other: handing_the_zoom_to_the_box_moves_no_pixel (no point of the image shifts) and any_zoom_overflows_the_padding (the box always takes the zoom). Both sweep off-centre focuses including the [0.0, 1.0] corner; the second fails on f2c1faa. cargo test -p openscreen-compositor --lib --tests: 78 + 3 pass. Verified in the app with a rebuilt addon.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #179
Summary
The zoom effect was confined to the padded screen rect, so the zoomed viewport stopped at the padded boundary instead of reaching the edges of the output frame. Expand
s_dst(ands_dst_prevfor motion blur) to the full output frame wheneverp.zoom > 1.0, so the zoomed content overflows the padding as expected.This covers both the web preview (whose visible pixels come from the native D3D canvas) and the exported video — same compositor path.
Type of change
Release impact
Desktop impact
Testing
npx tsc --noEmit— cleannpm run lint— no new warnings (the 7 reported are pre-existing in unrelated files)npm run test— 1144/1144 passcrates/compositor/tests/output_geometry_golden.rssetzoomRegions: []andcfg.zoom = false, so they are unaffected by this change. The Rust test suite could not be run locally in this worktree (ffmpeg prebuilt headers incrates/thirdparty/are gitignored and absent here); a real build with the full toolchain is the next gate.Follow-up (noted in
ponytail:TODO)The shadow and rounded corners also follow the expansion because they are drawn into the same
s_dstrect via the pixel shader. A correct fix would split the content render from the frame render (two draw passes, or a shader flag to disable frame effects for the content). Out of scope here — the issue asks for the zoom to overflow the padding, which this delivers.