fix(scene): scale backdrop blur with element opacity - #9
Merged
Conversation
A backdrop blur painted full strength until its element unmounted, so a flyout fading to zero kept its blurred backdrop through the entire exit and dropped it in one step at unmount. Measured on a black/white stripe backdrop the step was 41 luma; on lower-contrast content it reads as the 1-3 luma pop consumers see at the end of an otherwise clean fade. Carry the element's effective opacity on the primitive and use it as the composite weight of the blurred layer. Because the composite blends over the target that already holds the unblurred backdrop, weighting its alpha crossfades blurred to sharp, which measures linear across the fade with no discontinuity. Blur radius is untouched, so the pyramid plan and the cluster identity stay opacity-independent and batching is unchanged. The primitive is skipped below half of one 8-bit step of opacity, which drops the source snapshot and blur passes for surfaces that can no longer change a pixel. The reused `pad2` field keeps the GPU layout at 112 bytes.
Cull only at exactly zero opacity rather than at a small positive threshold. A threshold reintroduces the single-frame pop the opacity work removes, just at a different point in the fade, so anything above zero now keeps painting. Add the tests that pin the contract: opacity stays out of cluster identity and plan grouping, so two surfaces mid-fade at different opacities still share one cluster, one plan group and one source snapshot; preparation copies opacity through untouched; and the HLSL structured-buffer stride matches size_of::<BackdropBlur>(), which the hand-maintained shader mirror had silently violated before this branch.
WalkthroughBackdrop blur primitives now carry element opacity through scene planning, window painting, GPU storage layouts, and Metal/WGPU compositing shaders. Tests cover opacity propagation, culling, grouping, scratch sizing, cache behavior, and cross-backend layout compatibility. ChangesBackdrop blur opacity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Window as Window::paint_backdrop_blur
participant Scene as BackdropBlur
participant Shader as backdrop_blur_fragment
Window->>Scene: Store clamped element opacity
Scene->>Shader: Upload opacity in GPU buffer
Shader->>Shader: Scale blur alpha by opacity
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gpui/src/scene.rs`:
- Around line 1746-1794: Add a GPU/visual render regression alongside backdrop
blur tests using overlapping blur surfaces with different opacities. Exercise
the fragment compositing path and validate the rendered blend output, while
retaining the existing planning/grouping/scratch assertions in
backdrop_blur_clusters, backdrop_blur_plan_groups, and prepare_backdrop_blurs
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 242241ec-ded9-4788-a49b-2bdafa33afa1
⛔ Files ignored due to path filters (1)
crates/gpui_windows/src/shaders.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (7)
crates/gpui/src/scene.rscrates/gpui/src/window.rscrates/gpui_macos/src/metal_renderer.rscrates/gpui_macos/src/shaders.metalcrates/gpui_wgpu/src/shaders.wgslcrates/gpui_wgpu/src/wgpu_renderer.rscrates/gpui_windows/src/directx_renderer.rs
Comment on lines
+1746
to
+1794
| #[test] | ||
| fn backdrop_blur_opacity_does_not_split_clusters_or_plan_groups() { | ||
| // Same bounds and radius, so everything but opacity is shared. | ||
| let mut blurs = [ | ||
| test_backdrop_blur_with_bounds(1, 0.0, 10.0, 12.0), | ||
| test_backdrop_blur_with_bounds(2, 5.0, 10.0, 12.0), | ||
| ]; | ||
| let viewport_size = size(DevicePixels(200), DevicePixels(100)); | ||
| let opaque_clusters = backdrop_blur_clusters(&blurs, viewport_size); | ||
| let opaque_groups = backdrop_blur_plan_groups(&blurs, BackdropBlurPlan::MAX_PASSES); | ||
| let opaque_scratch = backdrop_scratch_bounds(&blurs, viewport_size).unwrap(); | ||
|
|
||
| // Mid-fade the two surfaces sit at different opacities. Batching must | ||
| // not notice: opacity is a composite parameter, and letting it reach | ||
| // planning would give each surface its own cluster, plan group, blur | ||
| // texture and source snapshot. | ||
| blurs[0].opacity = 0.9; | ||
| blurs[1].opacity = 0.8; | ||
|
|
||
| let faded_clusters = backdrop_blur_clusters(&blurs, viewport_size); | ||
| let faded_groups = backdrop_blur_plan_groups(&blurs, BackdropBlurPlan::MAX_PASSES); | ||
| let faded_scratch = backdrop_scratch_bounds(&blurs, viewport_size).unwrap(); | ||
|
|
||
| assert_eq!(faded_clusters.len(), 1); | ||
| assert_eq!(faded_clusters[0].len(), 2); | ||
| assert_eq!(faded_clusters.len(), opaque_clusters.len()); | ||
| assert_eq!(faded_clusters[0].len(), opaque_clusters[0].len()); | ||
|
|
||
| // One plan group is one generated blur texture. | ||
| assert_eq!(faded_groups.len(), 1); | ||
| assert_eq!(faded_groups, opaque_groups); | ||
|
|
||
| // One source snapshot, over the same region. | ||
| assert_eq!(faded_scratch.bounds, opaque_scratch.bounds); | ||
| assert_eq!(faded_scratch.texture_size, opaque_scratch.texture_size); | ||
| } | ||
|
|
||
| #[test] | ||
| fn prepare_backdrop_blurs_preserves_opacity() { | ||
| let mut blurs = [test_backdrop_blur_with_bounds(1, 0.0, 10.0, 1.0)]; | ||
| blurs[0].opacity = 0.4; | ||
| let viewport_size = size(DevicePixels(100), DevicePixels(100)); | ||
| let scratch_bounds = backdrop_scratch_bounds(&blurs, viewport_size).unwrap(); | ||
|
|
||
| let prepared = prepare_backdrop_blurs(&blurs, scratch_bounds); | ||
|
|
||
| assert_eq!(prepared[0].opacity, 0.4); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift
Add a render-level regression for overlapping opacities.
This test verifies planning, grouping, and scratch sizing only; it does not exercise fragment compositing. Add a GPU/visual case with overlapping blurs at different opacities to validate the expected blend behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gpui/src/scene.rs` around lines 1746 - 1794, Add a GPU/visual render
regression alongside backdrop blur tests using overlapping blur surfaces with
different opacities. Exercise the fragment compositing path and validate the
rendered blend output, while retaining the existing planning/grouping/scratch
assertions in backdrop_blur_clusters, backdrop_blur_plan_groups, and
prepare_backdrop_blurs tests.
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.
Backdrop blur ignored element opacity.
paint_backdrop_blurwas the only paint method that never readelement_opacity— its neighbours (paint_quad,paint_shadow,paint_text) all do — andBackdropBlurcarried no opacity field to multiply by. A surface fading to zero kept painting a full-strength blurred backdrop until unmount, then dropped it in a single frame.Downstream (gpui-component) measured this as a 1.5–3.2 luma single-frame step at the tail of otherwise-smooth 215ms exit animations, and it made wrapper-driven fades over blurred subtrees impossible — a faded-to-zero flyout still showed its blur.
Approach
Opacity becomes a per-instance parameter on
BackdropBlur, applied only at final composite. It is deliberately invisible to blur planning, source capture, clustering and pyramid generation:backdrop_blur_clustersgroups on padded-bounds overlapbackdrop_blur_plan_groupsgroups on radius→plan equalityprepare_backdrop_blurscopies opacity through untouchedSo two surfaces mid-fade at 0.9 and 0.8 still share one cluster, one plan group, one blur texture and one source snapshot — batching is unaffected during exactly the frames where cost matters.
Opacity folds into the surface alpha rather than lerping against a separately-bound sharp texture. The composite already blends src-over against the framebuffer, which still holds the unblurred backdrop at that draw order, so
alpha *= opacityyieldsdst = blurred·w + sharp·(1−w)— identical result, no extra sample or bind.Culling happens only at exactly zero opacity. A positive threshold would reintroduce the same single-frame pop at a different point in the fade.
Why not the alternatives
Measured on a real 215ms/13-frame exit through
render_to_imageon the Metal path:Radius scaling is also structurally wrong: radius feeds
backdrop_blur_plan_for_radiusand cluster padding, so animating it re-plans and re-clusters every frame.Incidental fix
The HLSL
BackdropBlurmirror was missingpad2, giving a 108-byte shader-side stride against the 112-byteStructureByteStridefromsize_of::<BackdropBlur>(). Any Windows batch with two or more blurs read skewed data from index 1 onward. Both are now 112, and asize_ofassertion guards the hand-maintained mirror — unlike the Metal struct, which is cbindgen-generated and cannot drift.Verification
macOS:
cargo test -p gpui -p gpui_macos --features gpui/test-support→ 298 passed, 0 failed. Clippy byte-identical to baseline.On-GPU measurements (scratch probe, not committed — the fork has no image-comparison test infrastructure): opaque interpolation max error 1/255 at o = 0.25/0.5/0.75 and exactly 0 at endpoints; zero-opacity frame byte-identical to the no-blur frame with unmount step 0.000; rounded-mask intersection shows 0 difference outside the mask and in rounded-off corners at every opacity; translucent-target luma at o=0.5 measured 52.07 against a linear midpoint of 52.02, excluding an opacity-squared implementation (which would give 44.9).
Unverified: DirectX/HLSL and wgpu have not been compiled — no local toolchain for those targets. CI is their first build.
Not covered: painter order across two overlapping blurred surfaces at differing opacities (batching is covered, ordering is not).
Note
Scale backdrop blur composite weight by element opacity
opacity: f32field to theBackdropBlurstruct (replacing thepad2padding field), passed from the element's effective opacity inpaint_backdrop_blur.clamp(blur.opacity, 0.0, 1.0)during fragment shading.Macroscope summarized d2b94c1.