-
Notifications
You must be signed in to change notification settings - Fork 26.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Clip enum and default to not clip #18057
Comments
cc @chinmaygarde @abarth @liyuqian |
Can we add a golden file test that covers this? |
Oh, I see: the saveLayer should be applied after clipPath to prevent the artifacts. In the code that I removed in flutter/engine#5420 , the saveLayer is applied before clipPath. That only pays the performance cost without solving the artifacts: |
If you clip before the layer, you should be good ... this is because we
deliberately do NOT apply complex clips inside the layer, but we defer them
until the restore, so we don't apply the soft-edged clip for each element.
…On Thu, May 31, 2018 at 2:00 PM liyuqian ***@***.***> wrote:
Oh, I see: the saveLayer should be applied after clipPath to prevent the
artifacts. In the code that I removed in flutter/engine#5420
<flutter/engine#5420> , the saveLayer is applied
before clipPath. That only pays the performance cost without solving the
artifacts:
https://fiddle.skia.org/c/71acea8432e19235d836b6e4c9c71b42
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18057 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMa7osADh_MDEETmMA9dtaFlWILigX2qks5t4C-tgaJpZM4UUdrG>
.
|
I think that saveLayer is so costly and the artifact is such an edge case that we shouldn't make the saveLayer the default solution inside our ClipPathLayer or PhysicalShapeLayer. If we have clients that do want to avoid such artifacts, maybe we should let them explicitly state that (e.g., add a SaveLayerWidget beneath ClipPathLayer) and tell them that such widgets are costly in performance. |
Thanks @reed-at-google for clarifying that. I've just also reproduced it in https://fiddle.skia.org/c/36043f79c796cdbdae85ffa68fca5274 BTW, do you know what do Android and Chrome do with those artifacts? Do they insert a saveLayer after each clipPath? |
Ah, yes, this is something that we got wrong early in the project. I fixed it for the cases where we did it wrong in Dart but I forgot to check the C++ code for similar errors. I strongly feel that our clip primitives should do the right thing. There's no point us having APIs if they give bad output. I agree that doing the right thing for clipping is expensive. I think we should therefore consider if we could redesign the framework to avoid clipping entirely unless explicitly requested. Right now we clip speculatively to "be on the safe side", e.g. the This would obviously be a major breaking change, but if the performance benefits are as serious as we suspect, then it's worth it. |
|
I don't think #2 above is accurate. We were previously not doing the saveLayer logic right, but when done right, for clipping, there's no artifacts. I think we should revert flutter/engine#5420, fix the saveLayer/clip logic to actually do the right thing, and then we should go through the framework and change it so that it never clips unless it's definitely necessary (e.g. a viewport) or someone explicitly opts in (e.g. with "clip: true" on a Material or DecoratedBox, or anything that uses those under the hood and where we want to support clipping, like Card, Container, etc). |
Ok, this is a complicated issue with a big tradeoff between performance and quality. Let's have an offline discussion with those who're interested in this topic. Before that, I arranged a meeting with Skia team to see how Android/Chrome's compositor handles this and what their recommendations are. Please let me know who I should add to our offline discussion. @Hixie , @tvolkert , @chinmaygarde , @cbracken , @xster , or maybe @abarth , @eseideGooglel ? |
We decided to
Shall we close this issue after implementing all these enums? |
Note that “AA without saveLayer” and “AA with saveLayer” have semantically different behaviors so we can’t make decisions for our clients (i.e., automatically adding or removing saveLayer is not a choice for us). Below are 2 fiddles that illustrate “without saveLayer” and “saveLayer” could have different semantics:
Note that the difference depends on the blend mode. For many blend modes, they’re associative (i.e., A blend B blend C = A blend (B blend C)) so adding a saveLayer may not be immediately problematic. But there’s no guarantee that all blend modes are associative (at least there’s no guarantee from Skia’s documentation). Moreover, each draw may have a different blend mode which could break the associative property even if every blend mode by itself is associative (see, e.g., the fiddles above). Therefore, we shouldn’t automatically add saveLayer for our clients. That’s both costly in performance and error-prone. |
Sounds good. |
This is a follow up on flutter/engine#5420 and flutter#18057 As you can see from the diff, we also mistakenly saveLayer before the clip at some places previously.
This is a follow up on flutter/engine#5420 and #18057 As you can see from the diff, we also mistakenly saveLayer before the clip at some places previously.
This would enable us to solve flutter/flutter#18057
This would enable us to solve flutter/flutter#18057
This would enable us to solve flutter/flutter#18057
This would enable us to solve flutter/flutter#18057
See details in our proposal for this breaking API change and #18057. This PR setup all code paths to allow the change but doesn't change the clip behavior by itself. We'll change `defaultClipBehavior` from `Clip.antiAlias` to `Clip.none` in the following PR to change the clip behavior and update tests.
The new golden files reflects Clip enum and will check the bleeding edge artifact as mentioned in: flutter/flutter#18057 (comment)
If we want to avoid the bleeding edge artifact (flutter/flutter#18057 (comment)) using saveLayer, we have to call drawPaint instead of drawPath as anti-aliased drawPath will always have such artifacts. This is discovered when I try to add golden tests for such bleeding artifacts using our new Clip enum. Here's the updated golden files: flutter/goldens@cb1fa8a?short_path=57b30ce#diff-57b30cea9b10b7ca689009854e12d70e
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
The following should just show a rotated black small box:
Instead it renders like this:
The text was updated successfully, but these errors were encountered: