Change default TileMode for blur ImageFilter objects to null #55552
Conversation
flar
force-pushed
the
ui-blur-image-filter-null-default-tile-mode
branch
from
4c5d450
to
56a2fcb
Compare
There was a problem hiding this comment.
I'm not sure that I followed the discussion on clamp for image operations vs decal. I think it can be a bit fuzzy whether or not something is an image operation, because different paint settings (like mask filter) can lead to something ending up as a saveLayer more or less.
We also need to change this for the web engine.
drawImage(Rect/Nine/Atlas) are image operations. I figure that saveLayer is a collection of rendering and rendering operations have space all around them out to infinity so they should reflect that in the choice of mode. drawImage et al, the source is a physical image that has edges. Within the bounds of the image it should be smeared from its contents. Granted it will spill over the theoretical "frame" which they might clip out. If we take some other operation and render it to a temp layer then we need to respect whatever tile mode was chosen for the content even as we perform that operation (whether it is a defaulted tile mode or whether they explicitly set it), so I don't think that case should affect the defaults. It's an implementation detail. The defaults were chosen based on the base operation itself and what is happening... |
|
Triage: This is waiting on an update to the web engine. |
flar
force-pushed
the
ui-blur-image-filter-null-default-tile-mode
branch
from
56a2fcb
to
1b20a34
Compare
|
I've plumbed most of the web stuff, but haven't yet gotten it all working. I think I got canvaskit renderer to accept the defaults, but it doesn't implement the tile mode in saveLayer so you can't really tell. web gives me some sort of "null canvas" and "width/height > 0" assertion failure. I haven't tried with skwasm as I don't have that building in my local engine case... |
flar
force-pushed
the
ui-blur-image-filter-null-default-tile-mode
branch
from
c5ca242
to
30e02d2
Compare
|
I'm hoping this last commit fixes all of the tests, but I still want to have a rendering test for the various blur tile mode values to catch specific regressions. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
| @@ -60,7 +60,7 @@ abstract class SkwasmImageFilter implements SceneImageFilter { | |||
| /// | |||
| /// The handle is deleted immediately after [borrow] returns. The [borrow] | |||
| /// function must not store the handle to avoid dangling pointer bugs. | |||
| void withRawImageFilter(ImageFilterHandleBorrow borrow); | |||
| void withRawImageFilter(ImageFilterHandleBorrow borrow, {ui.TileMode defaultMode = ui.TileMode.clamp}); | |||
There was a problem hiding this comment.
Two things: can you actually call this defaultTileMode to be a bit more clear, and secondly can you modify the doc comment to mention this parameter and what it does?
Fixes flutter/flutter#154935
Historically ImageFilter.blur supported setting a TileMode and had a default mode of
clamp, but few developers actually set the value and the default was not appropriate for some common uses like as a backdrop filter where the clamp mode produces flashing when scrolling high frequency pixel content underneath a blurred title bar.This PR removes the default tile mode instead allowing a null value as the default which will allow the engine to use an appropriate context-dependent default tile mode depending on the action being performed. Typically: