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

Commit 97705b8

Browse files
authored
Do not convert an open path to a closed rect. (#45903)
Fixes flutter/flutter#134816. --- Confirmed this only happens when utilizing `DisplayListBuilder`. Consider: ```cc TEST_P(DisplayListTest, CanDrawAnOpenPath) { flutter::DisplayListBuilder builder; flutter::DlPaint paint; paint.setColor(flutter::DlColor::kRed()); paint.setDrawStyle(flutter::DlDrawStyle::kStroke); paint.setStrokeWidth(10); builder.Translate(300, 300); // Move to (50, 50) and draw lines from: // 1. (50, height) // 2. (width, height) // 3. (width, 50) SkPath path; path.moveTo(50, 50); path.lineTo(50, 100); path.lineTo(100, 100); path.lineTo(100, 50); builder.DrawPath(path, paint); ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } ``` ![Screenshot 2023-09-15 at 2 06 53 PM](https://github.com/flutter/flutter/assets/168174/3a3868e1-f1d1-4e29-affa-4bd32d26ccdc) --- The bug is in `dl_dispatcher.cc`: ```cc // |flutter::DlOpReceiver| void DlDispatcher::drawPath(const SkPath& path) { SkRect rect; SkRRect rrect; SkRect oval; if (path.isRect(&rect)) { canvas_.DrawRect(skia_conversions::ToRect(rect), paint_); } else if (path.isRRect(&rrect) && rrect.isSimple()) { canvas_.DrawRRect(skia_conversions::ToRect(rrect.rect()), rrect.getSimpleRadii().fX, paint_); } else if (path.isOval(&oval) && oval.width() == oval.height()) { canvas_.DrawCircle(skia_conversions::ToPoint(oval.center()), oval.width() * 0.5, paint_); } else { canvas_.DrawPath(skia_conversions::ToPath(path), paint_); } } ``` Note the documentation for `isRect`: ```cc /** Returns true if SkPath is equivalent to SkRect when filled. If false: rect, isClosed, and direction are unchanged. If true: rect, isClosed, and direction are written to if not nullptr. rect may be smaller than the SkPath bounds. SkPath bounds may include kMove_Verb points that do not alter the area drawn by the returned rect. @param rect storage for bounds of SkRect; may be nullptr @param isClosed storage set to true if SkPath is closed; may be nullptr @param direction storage set to SkRect direction; may be nullptr @return true if SkPath contains SkRect example: https://fiddle.skia.org/c/@Path_isRect */ bool isRect(SkRect* rect, bool* isClosed = nullptr, SkPathDirection* direction = nullptr) const; ``` ... the `isClosed` is critical, without checking that we're doing an incorrect optimization. I'll send a fix and test.
1 parent 30b7e9d commit 97705b8

File tree

3 files changed

+70
-7
lines changed

3 files changed

+70
-7
lines changed

impeller/aiks/aiks_unittests.cc

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1262,6 +1262,32 @@ TEST_P(AiksTest, CanRenderDifferencePaths) {
12621262
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
12631263
}
12641264

1265+
// Regression test for https://github.com/flutter/flutter/issues/134816.
1266+
//
1267+
// It should be possible to draw 3 lines, and not have an implicit close path.
1268+
TEST_P(AiksTest, CanDrawAnOpenPath) {
1269+
Canvas canvas;
1270+
1271+
// Starting at (50, 50), draw lines from:
1272+
// 1. (50, height)
1273+
// 2. (width, height)
1274+
// 3. (width, 50)
1275+
PathBuilder builder;
1276+
builder.MoveTo({50, 50});
1277+
builder.LineTo({50, 100});
1278+
builder.LineTo({100, 100});
1279+
builder.LineTo({100, 50});
1280+
1281+
Paint paint;
1282+
paint.color = Color::Red();
1283+
paint.style = Paint::Style::kStroke;
1284+
paint.stroke_width = 10;
1285+
1286+
canvas.DrawPath(builder.TakePath(), paint);
1287+
1288+
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
1289+
}
1290+
12651291
static sk_sp<SkData> OpenFixtureAsSkData(const char* fixture_name) {
12661292
auto mapping = flutter::testing::OpenFixtureAsMapping(fixture_name);
12671293
if (!mapping) {

impeller/display_list/dl_dispatcher.cc

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -848,19 +848,29 @@ void DlDispatcher::drawDRRect(const SkRRect& outer, const SkRRect& inner) {
848848
// |flutter::DlOpReceiver|
849849
void DlDispatcher::drawPath(const SkPath& path) {
850850
SkRect rect;
851-
SkRRect rrect;
852-
SkRect oval;
853-
if (path.isRect(&rect)) {
851+
852+
// We can't "optimize" a path into a rectangle if it's open.
853+
bool closed;
854+
if (path.isRect(&rect, &closed); closed) {
854855
canvas_.DrawRect(skia_conversions::ToRect(rect), paint_);
855-
} else if (path.isRRect(&rrect) && rrect.isSimple()) {
856+
return;
857+
}
858+
859+
SkRRect rrect;
860+
if (path.isRRect(&rrect) && rrect.isSimple()) {
856861
canvas_.DrawRRect(skia_conversions::ToRect(rrect.rect()),
857862
rrect.getSimpleRadii().fX, paint_);
858-
} else if (path.isOval(&oval) && oval.width() == oval.height()) {
863+
return;
864+
}
865+
866+
SkRect oval;
867+
if (path.isOval(&oval) && oval.width() == oval.height()) {
859868
canvas_.DrawCircle(skia_conversions::ToPoint(oval.center()),
860869
oval.width() * 0.5, paint_);
861-
} else {
862-
canvas_.DrawPath(skia_conversions::ToPath(path), paint_);
870+
return;
863871
}
872+
873+
canvas_.DrawPath(skia_conversions::ToPath(path), paint_);
864874
}
865875

866876
// |flutter::DlOpReceiver|

impeller/display_list/dl_unittests.cc

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,33 @@ TEST_P(DisplayListTest, CanDrawWithOddPathWinding) {
411411
ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
412412
}
413413

414+
// Regression test for https://github.com/flutter/flutter/issues/134816.
415+
//
416+
// It should be possible to draw 3 lines, and not have an implicit close path.
417+
TEST_P(DisplayListTest, CanDrawAnOpenPath) {
418+
flutter::DisplayListBuilder builder;
419+
flutter::DlPaint paint;
420+
421+
paint.setColor(flutter::DlColor::kRed());
422+
paint.setDrawStyle(flutter::DlDrawStyle::kStroke);
423+
paint.setStrokeWidth(10);
424+
425+
builder.Translate(300, 300);
426+
427+
// Move to (50, 50) and draw lines from:
428+
// 1. (50, height)
429+
// 2. (width, height)
430+
// 3. (width, 50)
431+
SkPath path;
432+
path.moveTo(50, 50);
433+
path.lineTo(50, 100);
434+
path.lineTo(100, 100);
435+
path.lineTo(100, 50);
436+
builder.DrawPath(path, paint);
437+
438+
ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
439+
}
440+
414441
TEST_P(DisplayListTest, CanDrawWithMaskBlur) {
415442
auto texture = CreateTextureForFixture("embarcadero.jpg");
416443
flutter::DisplayListBuilder builder;

0 commit comments

Comments
 (0)