Repository navigation
Add Clip enum and default to not clip #18057
Description
The following should just show a rotated black small box:
new Center(
child: new RepaintBoundary(
child: new Container(
color: Colors.white,
child: new Padding(
padding: const EdgeInsets.all(100.0),
child: new SizedBox(
height: 100.0,
width: 100.0,
child: new Transform.rotate(
angle: 1.0, // radians
child: new ClipRect(
child: new Container(
color: Colors.red,
child: new Container(
color: Colors.white,
child: new RepaintBoundary(
child: new Center(
child: new Container(
color: Colors.black,
height: 10.0,
width: 10.0,
),
),
),
),
),
),
),
),
),
),
),
),Instead it renders like this:
Activity
Hixie commentedon May 31, 2018
cc @chinmaygarde @abarth @liyuqian
See also the bugs and PRs linked to above.
tvolkert commentedon May 31, 2018
Can we add a golden file test that covers this?
liyuqian commentedon May 31, 2018
Oh, I see: the saveLayer should be applied after clipPath to prevent the artifacts. In the code that I removed in flutter-team-archive/engine#5420 , the saveLayer is applied before clipPath. That only pays the performance cost without solving the artifacts:
https://fiddle.skia.org/c/71acea8432e19235d836b6e4c9c71b42
reed-at-google commentedon May 31, 2018
liyuqian commentedon May 31, 2018
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.
liyuqian commentedon May 31, 2018
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?
Hixie commentedon May 31, 2018
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
Cardwidget will clip because it may have a background image, but might not. IMHO we should consider removing all explicit clips in the framework that aren't demonstrably always necessary, and instead suggest that developers who want clipping should opt into it. So for example, we could add aclipproperty toCardthat turns on clipping for cards.This would obviously be a major breaking change, but if the performance benefits are as serious as we suspect, then it's worth it.
liyuqian commentedon May 31, 2018
Avoiding unnecessary clip (especially a complex one) is always a good thing to do regardless of whether we have saveLayer or not.
The artifacts are not specific to clipPath. It's universal to all AA paths: https://fiddle.skia.org/c/3e08883e8a1fbe97155f39e04ff0ad68
Therefore, I don't see a particular reason to treat clipPath specially by adding a saveLayer. We probably should always notify our users that there would be such artifacts if they try to render paths with shared edges in AA mode. That said, if clipPath without a saveLayer is the wrong thing to do, then we also did the drawPath wrong; if we think it's OK to drawPath without saveLayer, then it's probably Ok to clipPath without saveLayer.
Hixie commentedon Jun 1, 2018
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-team-archive/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).
liyuqian commentedon Jun 1, 2018
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 ?
liyuqian commentedon Jun 1, 2018
We decided to
Shall we close this issue after implementing all these enums?
10 remaining items
Remove saveLayer after clip from dart (#18616)
[-]Clips aren't saving layers correctly[/-][+]Add Clip enum and default to not clip[/+]Add antiAlias and saveCount to clipPath and restore
Add antiAlias to clipPath/clipRRect and restoreToCount
Add antiAlias and saveCount to clipPath and restore (#5638)
Add antiAlias and saveCount to clipPath and restore (#5638)
Add ClipMode to ClipPath/ClipRRect and PhysicalShape layers (#5647)
Add Clip enum to Material and related widgets (#18576)
Update golden files of PhysicalModel and PhysicalShape
Call drawPaint instead of drawPath if there's clip (#5937)
github-actions commentedon Aug 15, 2021
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
flutter doctor -vand a minimal reproduction of the issue.Set default clipBehavior to Clip.none and update tests (#20205)
Set default clipBehavior to Clip.none and update tests (#20205)