fix(canvas): confine a named shader effect to its own canvas layer#6440
Merged
Conversation
A CanvasWidget that is not the whole surface blackened everything painted
before it. `canvasSetShaderEffect(id, "crt", …)` on a small readout screen took
the entire host window with it — in Forge, the whole app chrome, the plugin
panel background and the header vanished behind one 604x96 canvas.
`save_layer_with_shader_effect` handed the RuntimeShader filter straight to
`push_layer`, whose `saveLayer(&bounds, &paint)` treats bounds as a HINT once an
image filter is attached: a runtime shader reports unbounded output, so the
layer grew to the device clip and the restore composited shader output over
content the widget never owned. Only `crt` showed it, because it is the one
curated effect that writes opaque pixels where the child is transparent (its
scanlines and aperture mask); the other five happen to preserve child alpha.
Cropping the filter to the layer rect makes containment structural instead of a
property every present and future curated effect has to remember to preserve.
`SkImageFilters::Crop` defaults to kDecal, so outside the rect is transparent
and the surrounding content shows through untouched.
The existing suite could not see this: every case painted the widget at the full
surface size (`{0,0,kW,kH}`), where there is no "outside" to observe. Three cases
close that gap, each anchored on a real pixel read rather than a smoke check —
an offset 32x32 widget over a green field must leave that field green for all
six effects; a vignette confined to a 48x48 widget must still darken that
widget's OWN corners, so an over-tight crop cannot pass; and both properties
must hold under a 2x canvas scale, since the effect samples in device space
while the layer rect is local and a crop in the wrong space would pass at 1x
only. The first case fails on origin/main (green field reads 0,0,0 under crt)
and passes here.
Verified visually as well as numerically: rendering the same offset crt widget
with and without the crop, the interior is pixel-identical across 35344 sampled
pixels (max channel delta 0) while the surrounding field goes from black back to
its own color — the effect is unchanged, only its overspill is gone.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QG7KYpNcdprcaDFrtmzUZC
Bloom is the one curated effect whose purpose is to spread light BEYOND its source, so a hard crop at the layer rect is exactly where this fix could be wrong. The containment case could not see it: it fills the widget edge-to-edge with flat grey, which produces no glow to clip. This adds the discriminating geometry — a bright bar hard against the widget's right edge — and reads both sides of that boundary. Rendered and compared, the crop is not taking anything from bloom. Inside the widget the two renders are pixel-identical (max delta 0 across the widget), and the glow is still alive right against the edge — the bar still bleeds light into the dark interior beside it. What the crop removes is entirely outside: a band ~8px wide of UNIFORM (115,255,115) that stopped abruptly rather than falling off with distance. That is not a glow, it is the bloom kernel sampling out-of-bounds coordinates within its fixed radius — the same unbounded-output artifact this change exists to fix. A layer cannot legitimately paint outside itself anyway, since saveLayer already clips layer content to its bounds. So bloom needs no headroom and must not get a widened crop; that would restore the overspill quietly, for the one effect where it would be easiest to excuse. The same comparison over all six effects at an offset widget shows the crop is a no-op everywhere except crt: interior delta 0 for every effect, and outside delta 0 for every effect except crt, whose surrounding field goes from black back to its own colour. All six were rendered and viewed, not only asserted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QG7KYpNcdprcaDFrtmzUZC
danielraffel
force-pushed
the
fix/shader-effect-layer-crop
branch
from
July 23, 2026 06:58
23c9b89 to
cd3440e
Compare
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
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.
The bug
A
CanvasWidgetthat is not the whole surface blackens everything painted before it.canvasSetShaderEffect(id, "crt", 0.55)on a small readout screen took the entire host window with it — found in Forge, where one 604×96 canvas erased the app chrome, the plugin panel background and the header.Cause
SkiaCanvas::save_layer_with_shader_effecthanded theSkImageFilters::RuntimeShaderstraight topush_layer, whosecanvas_->saveLayer(&bounds, &layer_paint)treatsboundsas a hint once an image filter is attached. A runtime shader reports unbounded output, so the layer grew to the device clip and the restore composited shader output over content the widget never owned.Only
crtexhibits it, because it is the one curated effect that writes opaque pixels where the child is transparent (scanlines + aperture mask). The other five happen to preserve child alpha. Cropping the filter to the layer rect makes containment structural rather than a property every present and future curated effect has to remember to preserve.SkImageFilters::Cropdefaults tokDecal, so outside the rect is transparent and surrounding content shows through untouched.Why the existing tests missed it
Every case in
test_canvas_shader_effects.cpppainted the widget at the full surface size ({0,0,kW,kH}, plus{0,0,64,64}) — there was no "outside" to observe. The suite could not have caught this.Tests
Three cases, each on a real pixel read rather than a smoke check:
does not paint outside its own canvas— an offset 32×32 widget over a green field must leave that field green, sampled at six points in every direction, for all six effects. UsesCHECKso a regression names every leaking effect, not just the first.still renders inside an offset canvas— a vignette confined to a 48×48 widget must still darken that widget's own corners relative to its own centre, so an over-tight crop cannot pass silently.layer crop holds under a canvas scale— both properties at 2×. The effect samples in device space while the layer rect is local, so a crop in the wrong coordinate space would pass at 1× and only break under a transform.Negative control: the first case fails on
origin/main(the green field reads(0,0,0)undercrt) and passes here.Verification
pulp-test-canvas-shader-effects17/17 (2437 assertions). Also green:pulp-test-canvas,-canvas-widget,-canvas-widget-shadow,-canvas-filter-chain,-canvas-capabilities,-canvas-widget-sanitize. Only one call site exists (CanvasWidget::paint).Verified visually as well as numerically — rendering the same offset
crtwidget with and without the crop, the interior is pixel-identical across 35,344 sampled pixels (max channel delta 0) while the surrounding field goes from black back to its own color. The effect is unchanged; only its overspill is gone.Note for consumers
canvasSetShaderEffectis already compiled into shipped Forge binaries and the installed SDK prefix, so both want a rebuild once this lands.🤖 Generated with Claude Code
https://claude.ai/code/session_01QG7KYpNcdprcaDFrtmzUZC
🔎 Provenance
claudem3/Volumes/Workshop/Code/pulp-shader-cropc5642b40-0090-4a66-9223-3bcc4f4de820Resume
Jump to this tab
Relaunch (any agent)
Restore URL — https://claude.ai/code/session_01QG7KYpNcdprcaDFrtmzUZC
stamped 2026-07-21 20:33 UTC