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

Use DlPathEffect Object #32159

Merged
JsouLiang merged 17 commits into from
May 12, 2022
Merged

Use DlPathEffect Object #32159

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a7c8b30
Define the DlPathEffect Object
JsouLiang Mar 21, 2022
9ae1876
Complete the DlPathEffect Object
JsouLiang Mar 22, 2022
360ef9b
Add DlPathEffect Unittests
JsouLiang Mar 22, 2022
c024ba9
Cut down only DLDashPathEffect
JsouLiang Mar 23, 2022
19bbde9
Create Intervals by other mode
JsouLiang Mar 24, 2022
731e605
Fix unittest issues
JsouLiang Mar 25, 2022
42710b3
Delete base_quals, check array equal first
JsouLiang Mar 29, 2022
40cbf6d
Change licenses file and deps
JsouLiang Mar 31, 2022
4e99e81
Add effect_bounds for DlPathEffect; Delete invalid test cases;
JsouLiang Apr 1, 2022
48c36d2
Change the intervals API; fix the DlUnknownPathEffect::effect_bounds …
JsouLiang Apr 7, 2022
d62c744
fix some suggests of comments
JsouLiang Apr 15, 2022
254893d
Change SetPathEffect one line like the others
JsouLiang Apr 16, 2022
81c2dcb
Change the DisplayListStreamDispatcher::setBlendMode api
JsouLiang Apr 22, 2022
8da4757
Delete licenses
JsouLiang Apr 23, 2022
0d2a021
change the display_list_dispatcher setPathEffect api
JsouLiang Apr 28, 2022
938ca82
Update Licenses
JsouLiang Apr 29, 2022
563d368
fix conflict
JsouLiang May 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Change licenses file and deps
  • Loading branch information
@JsouLiang
JsouLiang committed May 9, 2022
commit 40cbf6d8139da103bb705e32f5a0a845a3d162e5
2 changes: 2 additions & 0 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ FILE: ../../../flutter/display_list/display_list_ops.h
FILE: ../../../flutter/display_list/display_list_paint.cc
FILE: ../../../flutter/display_list/display_list_paint.h
FILE: ../../../flutter/display_list/display_list_paint_unittests.cc
FILE: ../../../flutter/display_list/display_list_path_effect.cc
FILE: ../../../flutter/display_list/display_list_path_effect.h
FILE: ../../../flutter/display_list/display_list_test_utils.cc
FILE: ../../../flutter/display_list/display_list_test_utils.h
FILE: ../../../flutter/display_list/display_list_tile_mode.h
Expand Down
3 changes: 1 addition & 2 deletions display_list/display_list_dispatcher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@
#include "flutter/display_list/display_list_image.h"
#include "flutter/display_list/display_list_image_filter.h"
#include "flutter/display_list/display_list_mask_filter.h"
#include "flutter/display_list/display_list_paint.h"
#include "flutter/display_list/display_list_vertices.h"
#include "flutter/display_list/display_list_path_effect.h"
#include "flutter/display_list/display_list_vertices.h"

namespace flutter {

Expand Down
36 changes: 16 additions & 20 deletions display_list/display_list_path_effect_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@
namespace flutter {
namespace testing {

const SkScalar TestDashes1[] = {4.0, 2.0};
const SkScalar TestDashes2[] = {1.0, 1.5};

TEST(DisplayListPathEffect, BuilderSetGet) {
const SkScalar TestDashes1[] = {4.0, 2.0};
auto dash_path_effect = DlDashPathEffect::Make(TestDashes1, 2, 0.0);
DisplayListBuilder builder;
ASSERT_EQ(builder.getPathEffect(), nullptr);
Expand All @@ -35,84 +33,80 @@ TEST(DisplayListPathEffect, FromSkiaNullPathEffect) {
}

TEST(DisplayListPathEffect, FromSkiaPathEffect) {
const SkScalar TestDashes2[] = {1.0, 1.5};
sk_sp<SkPathEffect> sk_path_effect =
SkDashPathEffect::Make(TestDashes2, 2, 0.0);
std::shared_ptr<DlPathEffect> dl_path_effect =
DlPathEffect::From(sk_path_effect);

ASSERT_EQ(dl_path_effect->type(), DlPathEffectType::kDash);
// We cannot recapture the dash parameters from an SkDashPathEffect
ASSERT_EQ(dl_path_effect->asDash(), dl_path_effect.get());
ASSERT_TRUE(
Equals(dl_path_effect, DlDashPathEffect::Make(TestDashes2, 2, 0.0)));
SkScalar s1[]{0.0, 0.0};
SkPathEffect::DashInfo info1(s1, 2, 0);
sk_path_effect->asADash(&info1);

SkScalar s2[]{0.0, 0.0};
SkPathEffect::DashInfo info2(s2, 2, 0);
dl_path_effect->skia_object()->asADash(&info2);
// check interval values is equal
for (int i = 0; i < 2; i++) {
ASSERT_EQ(s1[i], s2[i]);
}
ASSERT_EQ(info1.fCount, info2.fCount);
ASSERT_EQ(info1.fPhase, info2.fPhase);
}

TEST(DisplayListPathEffect, EffectShared) {
const SkScalar TestDashes2[] = {1.0, 1.5};
auto effect = DlDashPathEffect::Make(TestDashes2, 2, 0.0);
ASSERT_TRUE(Equals(effect->shared(), effect));
}

TEST(DisplayListPathEffect, DashEffectAsDash) {
const SkScalar TestDashes2[] = {1.0, 1.5};
auto effect = DlDashPathEffect::Make(TestDashes2, 2, 0.0);
ASSERT_NE(effect->asDash(), nullptr);
ASSERT_EQ(effect->asDash(), effect.get());
}

TEST(DisplayListPathEffect, DashEffectEquals) {
const SkScalar TestDashes2[] = {1.0, 1.5};
auto effect1 = DlDashPathEffect::Make(TestDashes2, 2, 0.0);
auto effect2 = DlDashPathEffect::Make(TestDashes2, 2, 0.0);
ASSERT_TRUE(Equals(effect1, effect2));
ASSERT_TRUE(Equals(effect1->shared(), effect2->shared()));
}

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

@flar flar Mar 31, 2022

Choose a reason for hiding this comment

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

If you had a test where the first dash was equal and the second dash was not, you'd have discovered that your memcmp was using the wrong byte count.

Please add new dash arrays to test "interval 1 is different" and "interval 2 is different" and another that has 3 values (the first 2 of which match TestDashes1, but the 3rd is additional) so that you can test a difference in the interval count.

Copy link
Contributor

@flar flar Apr 6, 2022

Choose a reason for hiding this comment

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

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");

auto effect1 = DlDashPathEffect::Make(TestDashes1, 2, 0.0);
auto effect2 = DlDashPathEffect::Make(TestDashes2, 2, 0.0);
auto effect3 = DlDashPathEffect::Make(TestDashes2, 3, 0.0);
auto effect3 = DlDashPathEffect::Make(TestDashes2, 2, 1.0);
ASSERT_NE(effect1, effect2);
ASSERT_NE(effect2, effect3);
ASSERT_NE(effect1->shared(), effect2->shared());
ASSERT_NE(effect2->shared(), effect3->shared());
}

TEST(DisplayListPathEffect, UnknownConstructor) {
const SkScalar TestDashes1[] = {4.0, 2.0};
DlUnknownPathEffect path_effect(SkDashPathEffect::Make(TestDashes1, 2, 0.0));
}

TEST(DisplayListPathEffect, UnknownShared) {
const SkScalar TestDashes1[] = {4.0, 2.0};
DlUnknownPathEffect path_effect(SkDashPathEffect::Make(TestDashes1, 2, 0.0));
ASSERT_NE(path_effect.shared().get(), &path_effect);
ASSERT_EQ(*path_effect.shared(), path_effect);
}

Copy link
Contributor

@flar flar May 12, 2022

Choose a reason for hiding this comment

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

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

TEST(DisplayListPathEffect, UnknownContents) {
const SkScalar TestDashes1[] = {4.0, 2.0};
sk_sp<SkPathEffect> sk_effect = SkDashPathEffect::Make(TestDashes1, 2, 0.0);
DlUnknownPathEffect effect(sk_effect);
ASSERT_EQ(effect.skia_object(), sk_effect);
ASSERT_EQ(effect.skia_object().get(), sk_effect.get());
}

TEST(DisplayListPathEffect, UnknownEquals) {
const SkScalar TestDashes1[] = {4.0, 2.0};
sk_sp<SkPathEffect> sk_effect = SkDashPathEffect::Make(TestDashes1, 2, 0.0);
DlUnknownPathEffect effect1(sk_effect);
DlUnknownPathEffect effect2(sk_effect);
TestEquals(effect1, effect1);
}

TEST(DisplayListPathEffect, UnknownNotEquals) {
const SkScalar TestDashes1[] = {4.0, 2.0};
// Even though the effect is the same, it is a different instance
// and we cannot currently tell them apart because the Skia
// DashEffect::Make objects do not implement ==
Expand Down Expand Up @@ -173,9 +167,11 @@ void testNotEquals(std::shared_ptr<const DlPathEffect> a,
}

TEST(DisplayListPathEffect, ComparableTemplates) {
const SkScalar TestDashes1[] = {4.0, 2.0};
const SkScalar TestDashes2[] = {1.0, 1.5};
auto effect1 = DlDashPathEffect::Make(TestDashes1, 2, 0.0);
auto effect2 = DlDashPathEffect::Make(TestDashes1, 2, 0.0);
auto effect3 = DlDashPathEffect::Make(TestDashes2, 3, 0.0);
auto effect3 = DlDashPathEffect::Make(TestDashes2, 2, 1.0);
std::shared_ptr<DlPathEffect> shared_null;

// null to null
Expand Down