-
Notifications
You must be signed in to change notification settings - Fork 6k
Further filter/clear <SkPaint>.setDither(true)
, this time in DlSkPaintDispatchHelper
#44912
Further filter/clear <SkPaint>.setDither(true)
, this time in DlSkPaintDispatchHelper
#44912
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM
This doesn't actually work as written :( It's possible for code to run the following (otherwise valid) branch:
With this PR, Let me try to understand the lifecycle of this class better, maybe there is a better place to do this kind of check? |
Ahh that makes sense. You'd probably need to intercept the dithering call when it is being converted into an SkPaint. |
I think const SkPaint& paint() { return paint_; } ... and could be used to clear dithering if a color source of a gradient was not provided. |
8a5b0cb
to
e6a5b7b
Compare
Appears to be all good now! Yay! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
… in `DlSkPaintDispatchHelper` (flutter/engine#44912)
… in `DlSkPaintDispatchHelper` (flutter/engine#44912)
… in `DlSkPaintDispatchHelper` (flutter/engine#44912)
… in `DlSkPaintDispatchHelper` (flutter/engine#44912)
… in `DlSkPaintDispatchHelper` (flutter/engine#44912)
… in `DlSkPaintDispatchHelper` (flutter/engine#44912)
… in `DlSkPaintDispatchHelper` (flutter/engine#44912)
…sions) (#133106) Manual roll requested by zra@google.com flutter/engine@b190f90...7d56840 2023-08-23 zanderso@users.noreply.github.com Revert "Roll Dart SDK from ab417bc74bb1 to c162b4979562 (1 revision)" (flutter/engine#44989) 2023-08-23 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from G25oJMO5jbUi-UN4F... to DoQ8KUxSk-5EU6VQ1... (flutter/engine#44988) 2023-08-23 zanderso@users.noreply.github.com Revert "Make `FontWeight` an enum, Remove unused text classes" (flutter/engine#44987) 2023-08-23 skia-flutter-autoroll@skia.org Roll Dart SDK from ab417bc74bb1 to c162b4979562 (1 revision) (flutter/engine#44986) 2023-08-23 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from kKI09su99b0AKs8b3... to VSvpNFoFjqXIQTcs6... (flutter/engine#44984) 2023-08-23 matanlurey@users.noreply.github.com Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param` (flutter/engine#44936) 2023-08-22 john@johnmccutchan.com Restore old SurfaceTextureExternal drawing code (flutter/engine#44979) 2023-08-22 skia-flutter-autoroll@skia.org Roll Skia from d0918de21c1a to aa208c8a2d60 (2 revisions) (flutter/engine#44981) 2023-08-22 jason-simmons@users.noreply.github.com Initialize the texture destruction callback in the Metal embedder test harness (flutter/engine#44973) 2023-08-22 matanlurey@users.noreply.github.com Further filter/clear `<SkPaint>.setDither(true)`, this time in `DlSkPaintDispatchHelper` (flutter/engine#44912) 2023-08-22 skia-flutter-autoroll@skia.org Roll Dart SDK from 3ebf0fedfceb to ab417bc74bb1 (1 revision) (flutter/engine#44977) 2023-08-22 skia-flutter-autoroll@skia.org Roll Skia from bf6019be75ef to d0918de21c1a (3 revisions) (flutter/engine#44975) 2023-08-22 skia-flutter-autoroll@skia.org Roll Skia from c675298ddeda to bf6019be75ef (3 revisions) (flutter/engine#44974) 2023-08-22 31859944+LongCatIsLooong@users.noreply.github.com Make `FontWeight` an enum, Remove unused text classes (flutter/engine#44960) 2023-08-22 skia-flutter-autoroll@skia.org Roll Skia from 9f4b81aac175 to c675298ddeda (2 revisions) (flutter/engine#44971) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from kKI09su99b0A to VSvpNFoFjqXI fuchsia/sdk/core/mac-amd64 from G25oJMO5jbUi to DoQ8KUxSk-5E If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
… in `DlSkPaintDispatchHelper` (flutter/engine#44912)
…aintDispatchHelper` (flutter#44912) Closes flutter/flutter#132860. - If `setDither(true)` is called, and an existing `setColorSource` is a gradient, it is ignored. - If `setColorSource(...)` is called, and it is a gradient, and dithering was previously set, it is cleared. I'm not sure this is fool proof.
…enableDithering. (#44705) Update: Blocked on #44912 landing, and merging into google3. --- Partial work towards flutter/flutter#112498. _**tl;dr**: In Impeller's backend we intend to _always_ dither gradients, and never allow any changes long-term (i.e., write your own shader if you want different behavior) with the assumption that dithered gradients look better most of the time, and don't typically hurt elsewhere._ Note that, at the time of this writing, I couldn't find a single case of this being set explicitly to `true` inside Google, and there are between [100 and 200 publicly accessible on GitHub](https://github.com/search?q=%22Paint.enableDithering%22+language%3ADart&type=code&l=Dart), of which virtually all are setting it explicitly to `true`. There are some (valid) concerns this would cause a lot of golden-file image diffs, so I'm going to seek explicit approval from the Google testing team as well as someone from the framework team before landing this commit.
Closes flutter/flutter#132860.
setDither(true)
is called, and an existingsetColorSource
is a gradient, it is ignored.setColorSource(...)
is called, and it is a gradient, and dithering was previously set, it is cleared.I'm not sure this is fool proof.