Skip to content
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 support for SkPathEffect to DisplayList #27155

Merged
merged 2 commits into from Jul 4, 2021

Conversation

flar
Copy link
Contributor

@flar flar commented Jul 3, 2021

Fixes the problem of not preserving the SkPathEffects in the paint used by the Text engine as seen in the golden test failures in the engine roll (flutter/flutter#85802)

This fix is related to the recent revert of enabling the DisplayList by default (#27153)

Includes tests for the new attribute and some minor edits to other attributes.

Note that SkPictureRecorder does not compute the bounds of these path effects properly in the drawPoints() methods without the workaround included in the SkCanvas rendering in the tests. The DisplayList bounds calculations are OK in this respect and worked fine without modifications.

@@ -1074,22 +1075,27 @@ void DisplayListBuilder::setBlendMode(SkBlendMode mode) {
void DisplayListBuilder::setFilterQuality(SkFilterQuality quality) {
Push<SetFilterQualityOp>(0, quality);
}
void DisplayListBuilder::setShader(sk_sp<SkShader> shader) {
void DisplayListBuilder::setShader(const sk_sp<SkShader> shader) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this use of const does anything useful. If anything, it makes the std::move(shader) resort to a copy instead of a move. Did you perhaps mean sk_sp<const SkShader> shader?

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think other than the dubious use of const, everything else looks good. Perhaps back that out and proceed with the rest to unblock enabling this?

@flar flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 4, 2021
@fluttergithubbot fluttergithubbot merged commit 12a7db6 into flutter:master Jul 4, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 4, 2021
moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
3 participants