Add Clip enum and default to not clip

[Hixie]

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:

[Hixie]

cc @chinmaygarde @abarth @liyuqian See also the bugs and PRs linked to above.

[tvolkert]

Can we add a golden file test that covers this?

[liyuqian]

Oh, I see: the saveLayer should be applied after clipPath to prevent the artifacts. In the code that I removed in https://github.com/flutter/engine/pull/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]

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 notifications@github.com 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 https://github.com/flutter/engine/pull/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 https://github.com/flutter/flutter/issues/18057#issuecomment-393620544, or mute the thread https://github.com/notifications/unsubscribe-auth/AMa7osADh_MDEETmMA9dtaFlWILigX2qks5t4C-tgaJpZM4UUdrG .

[liyuqian]

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]

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]

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 Card widget 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 a clip property to Card that 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]

1. Avoiding unnecessary clip (especially a complex one) is always a good thing to do regardless of whether we have saveLayer or not.

2. 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]

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 https://github.com/flutter/engine/pull/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]

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]

We decided to

1. Remove clip as much as possible and make no-clip the default behavior
2. Expose the following 3 enums when there’s a clip:
   1. No AA
   2. AA without saveLayer
   3. AA with saveLayer

Shall we close this issue after implementing all these enums?

[liyuqian]

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:

1. https://fiddle.skia.org/c/83ed46ceadaf90f36a4df3b98cbe1c35
2. https://fiddle.skia.org/c/704acfa049a7e99fbe685232c45d1582

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.

[Hixie]

Sounds good.

[github-actions[bot]]

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 -v and a minimal reproduction of the issue.