Use DlPathEffect Object #32159

JsouLiang · 2022-03-21T16:15:30Z

Use DlPathEffect Object to replace SkPathEffect Object

Change the impeller DLDispatcher SkPathEffect to flutter::DlPathEffect

flutter-dashboard · 2022-03-21T16:15:44Z

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

JsouLiang · 2022-03-22T07:35:24Z

@flar Hey, this is about the DlPathEffect , please give me a review, Thank you! :)

flar

Some of the information in this review is now covered under a new comment on the "graphics package neutral" issue.

In the end, implementing this particular may end up being eliminated by upcoming Impeller work. It may be useful in the short term (for weeks if not months) for "Flutter on DisplayList on Skia", but that is something to consider with respect to how much time to spend on this object.

Minimally, it implements way more flavors of PathEffect than we will ever see and so it should be cut down to only those sub-classes that we need (Dash + Unknown?)

If you want to finish this work off, then consider the comments below about trimming it down and I will review again in a more focused implementation.

display_list/display_list_path_effect.cc

display_list/display_list_path_effect.h

JsouLiang · 2022-03-23T08:52:01Z

@flar As you say, I have cut down other PathEffect,reserved only DlDashPathEffect and DlUnknownPathEffect

flar

I've only done a light read through as I'd like to see the DlDashPathEffect use inline storage for the intervals to be bulk-compare compatible for the DisplayList buffer.

display_list/display_list_path_effect.h

display_list/display_list_ops.h

display_list/display_list_path_effect.h

JsouLiang · 2022-03-25T03:09:40Z

@flar hi, I have fixed the comments please help me take a look. Thanks

display_list/display_list_path_effect.h

JsouLiang · 2022-03-31T03:43:21Z

@flar hey, I have fix the comments, please give me a review, thanks. And the impeller version has some issue, case the latest version display_list_dispatcher DlVertices has changed, but the engine doesn't have this class

display_list/display_list_canvas_unittests.cc

display_list/display_list.h

display_list/display_list_path_effect.h

display_list/display_list_path_effect_unittests.cc

display_list/display_list_unittests.cc

display_list/display_list_utils.cc

display_list/display_list_path_effect.h

display_list/display_list_path_effect_unittests.cc

flar · 2022-04-06T10:44:50Z

display_list/display_list_path_effect_unittests.cc

+
+TEST(DisplayListPathEffect, BlurNotEquals) {
+  const SkScalar TestDashes1[] = {4.0, 2.0};
+  const SkScalar TestDashes2[] = {1.0, 1.5};


You misread or misunderstood my comment.

Use the TestNotEquals helper methods in display_list_attributes_testing.h
Test 4 potential sources of inequality as below:

This is what I was asking for:

const SkScalar TestDashes1[] = {4.0, 2.0}; const SkScalar TestDashes2[] = {5.0, 2.0}; const SkScalar TestDashes3[] = {4.0, 3.0}; const SkScalar TestDashes4[] = {4.0, 2.0, 6.0}; auto effect1 = DlDashPathEffect::Make(TestDashes1, 2, 0.0); auto effect2 = DlDashPathEffect::Make(TestDashes2, 2, 0.0); auto effect3 = DlDashPathEffect::Make(TestDashes3, 2, 0.0); auto effect4 = DlDashPathEffect::Make(TestDashes4, 3, 0.0); auto effect5 = DlDashPathEffect::Make(TestDashes1, 2, 1.0); TestNotEquals(*effect1, *effect2, "Interval 1 differs"); TestNotEquals(*effect1, *effect3, "Interval 2 differs"); TestNotEquals(*effect1, *effect4, "Dash count differs"); TestNotEquals(*effect1, *effect5, "Dash phase differs");

display_list/display_list_path_effect.cc

JsouLiang · 2022-05-06T05:07:45Z

This needs a rebase on ToT.

@chinmaygarde done.
Thanks

…bug; update test cases

flar

Really all just nits - include file over-including in most places.

Upgrading the DlStreamDispatcher::setPathEffect doesn't really fix/break anything, but it would be nice for new test writers to see the contents - and this PR is all about making it easy to see the contents.

(To see what the output looks like, I usually go to the dl_unittest that constructs a giant DL of every op (see

engine/display_list/display_list_unittests.cc

Line 1028 in 0673b78

sk_sp<DisplayList> default_dl = Build(allGroups.size(), 0);

) and add a temporary FML_LOG(ERROR) << default_dl; line and that shows how all of the various DL ops show up in an output stream. Then just run the display_list_unittests and adjust the output until I'm happy.)

flar · 2022-05-12T01:11:43Z

display_list/display_list_paint.h

@@ -5,12 +5,14 @@
 #ifndef FLUTTER_DISPLAY_LIST_DISPLAY_LIST_PAINT_H_
 #define FLUTTER_DISPLAY_LIST_DISPLAY_LIST_PAINT_H_

+#include <memory>


Is this used?

flar · 2022-05-12T01:15:10Z

display_list/display_list_path_effect.h

+#ifndef FLUTTER_DISPLAY_LIST_DISPLAY_LIST_PATH_EFFECT_H_
+#define FLUTTER_DISPLAY_LIST_DISPLAY_LIST_PATH_EFFECT_H_
+
+#include <cstring>


Is this used?

flar · 2022-05-12T01:22:36Z

display_list/display_list_path_effect_unittests.cc

+  ASSERT_NE(path_effect.shared().get(), &path_effect);
+  ASSERT_EQ(*path_effect.shared(), path_effect);
+}
+


nit - other files have an "UnknownAsNone" test that verifies that all of the "asFoo()" methods return nullptr...

flar · 2022-05-12T01:24:31Z

display_list/types.h

@@ -25,7 +25,9 @@
 #include "third_party/skia/include/core/SkShader.h"
 #include "third_party/skia/include/core/SkTextBlob.h"
 #include "third_party/skia/include/core/SkVertices.h"
+#include "third_party/skia/include/effects/SkCornerPathEffect.h"


I don't think this is used any more.

flar · 2022-05-12T01:25:38Z

impeller/display_list/display_list_dispatcher.h

@@ -4,6 +4,7 @@

 #pragma once

+#include "display_list/display_list_path_effect.h"


Shouldn't this be relative to "flutter/display_list" as the others?

flar · 2022-05-12T01:26:40Z

impeller/display_list/display_list_dispatcher.cc

@@ -6,6 +6,7 @@

 #include <optional>

+#include "display_list/display_list_path_effect.h"


This shouldn't be needed as it will be included in the impeller/dl_dispatcher.h above.

flar · 2022-05-12T01:27:35Z

testing/display_list_testing.cc

@@ -479,7 +479,7 @@ void DisplayListStreamDispatcher::setBlendMode(DlBlendMode mode) {
 void DisplayListStreamDispatcher::setBlender(sk_sp<SkBlender> blender) {
  startl() << "setBlender(" << blender << ");" << std::endl;
 }
-void DisplayListStreamDispatcher::setPathEffect(sk_sp<SkPathEffect> effect) {
+void DisplayListStreamDispatcher::setPathEffect(const DlPathEffect* effect) {


This could be expanded to be more descriptive now that we can inspect the DlPathEffect.

flar · 2022-05-12T01:28:38Z

testing/display_list_testing.h

@@ -9,6 +9,7 @@

 #include "flutter/display_list/display_list.h"
 #include "flutter/display_list/display_list_dispatcher.h"
+#include "flutter/display_list/display_list_path_effect.h"


This should come for free from the dl_dispatcher include.

This reverts commit b8ca6a5.

JsouLiang changed the title ~~Use DlPathEffect Object~~ [WIP] Use DlPathEffect Object Mar 21, 2022

flutter-dashboard bot added the needs tests label Mar 21, 2022

JsouLiang mentioned this pull request Mar 21, 2022

Make the display list interface graphics package neutral. flutter/flutter#95434

Closed

JsouLiang changed the title ~~[WIP] Use DlPathEffect Object~~ Use DlPathEffect Object Mar 22, 2022

JsouLiang force-pushed the Use-DlPathEffect-Object branch from 3b53044 to eb221b8 Compare March 22, 2022 07:34

flar suggested changes Mar 22, 2022

View reviewed changes

display_list/display_list_path_effect.cc Show resolved Hide resolved

display_list/display_list_path_effect.h Outdated Show resolved Hide resolved

flar reviewed Mar 22, 2022

View reviewed changes

display_list/display_list_path_effect.h Outdated Show resolved Hide resolved

flar reviewed Mar 22, 2022

View reviewed changes

display_list/display_list_path_effect.h Outdated Show resolved Hide resolved

JsouLiang mentioned this pull request Mar 23, 2022

Use New DlPathEffect Object flutter/impeller#98

Merged

flar suggested changes Mar 23, 2022

View reviewed changes

display_list/display_list_path_effect.h Outdated Show resolved Hide resolved

display_list/display_list_ops.h Show resolved Hide resolved

display_list/display_list_path_effect.h Outdated Show resolved Hide resolved

flar reviewed Mar 29, 2022

View reviewed changes

display_list/display_list_path_effect.h Outdated Show resolved Hide resolved

flar reviewed Mar 29, 2022

View reviewed changes

display_list/display_list_path_effect.h Outdated Show resolved Hide resolved

JsouLiang force-pushed the Use-DlPathEffect-Object branch 8 times, most recently from e494718 to 1639133 Compare March 31, 2022 03:12

JsouLiang force-pushed the Use-DlPathEffect-Object branch from 1639133 to 9aaea6a Compare March 31, 2022 15:51

zanderso requested a review from flar March 31, 2022 19:58

flar suggested changes Mar 31, 2022

View reviewed changes

JsouLiang force-pushed the Use-DlPathEffect-Object branch from ee2e487 to adad369 Compare April 1, 2022 06:56

flar suggested changes Apr 6, 2022

View reviewed changes

JsouLiang force-pushed the Use-DlPathEffect-Object branch from d727302 to ab0a620 Compare May 6, 2022 05:01

JsouLiang requested a review from flar May 6, 2022 05:08

JsouLiang force-pushed the Use-DlPathEffect-Object branch from ab0a620 to 417c862 Compare May 6, 2022 05:47

JsouLiang and others added 17 commits May 9, 2022 20:34

Define the DlPathEffect Object

a7c8b30

Complete the DlPathEffect Object

9ae1876

Add DlPathEffect Unittests

360ef9b

Cut down only DLDashPathEffect

c024ba9

Create Intervals by other mode

19bbde9

Fix unittest issues

731e605

Delete base_quals, check array equal first

42710b3

Change licenses file and deps

40cbf6d

Add effect_bounds for DlPathEffect; Delete invalid test cases;

4e99e81

Change the intervals API; fix the DlUnknownPathEffect::effect_bounds …

48c36d2

…bug; update test cases

fix some suggests of comments

d62c744

Change SetPathEffect one line like the others

254893d

Change the DisplayListStreamDispatcher::setBlendMode api

81c2dcb

Delete licenses

8da4757

change the display_list_dispatcher setPathEffect api

0d2a021

Update Licenses

938ca82

fix conflict

563d368

JsouLiang force-pushed the Use-DlPathEffect-Object branch from 417c862 to 563d368 Compare May 9, 2022 12:40

chinmaygarde removed their request for review May 10, 2022 17:23

JsouLiang requested a review from chinmaygarde May 11, 2022 02:52

eggfly removed the request for review from chinmaygarde May 11, 2022 02:54

flar approved these changes May 12, 2022

View reviewed changes

JsouLiang merged commit b8ca6a5 into flutter:main May 12, 2022

JsouLiang added a commit that referenced this pull request May 12, 2022

Revert "Use DlPathEffect Object (#32159)"

8123fa5

This reverts commit b8ca6a5.

engine-flutter-autoroll mentioned this pull request May 12, 2022

Roll Flutter Engine from 0673b781d244 to b8ca6a5e0ecf (1 revision) flutter/flutter#103567

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 12, 2022

b8ca6a5 Use DlPathEffect Object (flutter/engine#32159)

3b805fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DlPathEffect Object #32159

Use DlPathEffect Object #32159

JsouLiang commented Mar 21, 2022 •

edited

Loading

flutter-dashboard bot commented Mar 21, 2022

JsouLiang commented Mar 22, 2022

flar left a comment

JsouLiang commented Mar 23, 2022 •

edited

Loading

flar left a comment

JsouLiang commented Mar 25, 2022 •

edited

Loading

JsouLiang commented Mar 31, 2022

flar Apr 6, 2022

JsouLiang commented May 6, 2022

flar left a comment •

edited

Loading

flar May 12, 2022

flar May 12, 2022

flar May 12, 2022

flar May 12, 2022

flar May 12, 2022

flar May 12, 2022

flar May 12, 2022

flar May 12, 2022

		@@ -4,6 +4,7 @@

		#pragma once

		#include "display_list/display_list_path_effect.h"

		@@ -6,6 +6,7 @@

		#include <optional>

		#include "display_list/display_list_path_effect.h"

Use DlPathEffect Object #32159

Use DlPathEffect Object #32159

Conversation

JsouLiang commented Mar 21, 2022 • edited Loading

flutter-dashboard bot commented Mar 21, 2022

JsouLiang commented Mar 22, 2022

flar left a comment

Choose a reason for hiding this comment

JsouLiang commented Mar 23, 2022 • edited Loading

flar left a comment

Choose a reason for hiding this comment

JsouLiang commented Mar 25, 2022 • edited Loading

JsouLiang commented Mar 31, 2022

Choose a reason for hiding this comment

JsouLiang commented May 6, 2022

flar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JsouLiang commented Mar 21, 2022 •

edited

Loading

JsouLiang commented Mar 23, 2022 •

edited

Loading

JsouLiang commented Mar 25, 2022 •

edited

Loading

flar left a comment •

edited

Loading