Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

flar
Copy link
Contributor

@flar flar commented Nov 23, 2020

The recent PR to use Skia's mechanism for quick clip culling caused a problem during rasterization of cache entries in 2 ways:

  • There are 2 canvases used while rendering engine Layer objects. An N-way multiplexer that sends calls to a number of canvases, and a leaf canvas that draws to the current destination. The culling method was being called on the multiplexer instead of the actual rendering canvas. This wouldn't normally be an issue since the properties that affect clip culling should be kept in sync, but...

  • The setup for the caching code was not informing the N-way canvas of the transform being rendered under. The leaf canvas did have the right transform, but the N-way did not. Thus, queries about culling on the N-way canvas might make an incorrect assumption about the transform being used.

Either of the 2 code changes here could individually fix the problem, but I include both because I think it will be important to have the caching N-way canvas know accurate information about the rendering environment, and we should probably do the culling against the actual leaf rendering canvas because that is the canvas on which all rendering calls are executed.

This change fixes flutter/flutter#70933

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@google-cla google-cla bot added the cla: yes label Nov 23, 2020
@flar flar requested review from gaaclarke and liyuqian November 23, 2020 01:31
@flar
Copy link
Contributor Author

flar commented Nov 24, 2020

I fixed the existing unit tests that were breaking with this change. Apparently the MockCanvas implementation was absorbing all calls to set a clip and so the backing implementation was trying to do a quickReject() based on incomplete information. I modified the MockCanvas::doClip*() to pass the clips on to the underlying implementation which should fix those problems.

While I was doing that I tried to write an explicit test case for making sure that we didn't cull a layer incorrectly while trying to cache it. Unfortunately, the code that does culling during a cache render is buried so deep in the layers of the RasterCache that I was doing major surgery on the data structures to try to write a test case. Rather than make this simple fix much more complicated with the added opportunities to break things, I decided I will file an Issue and come back around to try to enhance our MockRasterCache in a future PR.

@flar
Copy link
Contributor Author

flar commented Nov 24, 2020

The task of writing a proper unit test for this fix falls under flutter/flutter#71173

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 25, 2020
chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020
Also Fix MockCanvas clipping so tests will work with new culling
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CupertinoPicker does not respect _kOverAndUnderCenterOpacity
2 participants