more path is rect bugs
More edge cases found; clean up the logic a bit
to make more clear where the rectangle points
start and stop.
R=robertphillips@google.com,brianosman@google.com
Bug: 824145,skia:7792
Change-Id: Ie24dfd1519f30875f44ffac68e20d777490b00b9
Reviewed-on: https://skia-review.googlesource.com/120422
Commit-Queue: Cary Clark <caryclark@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
diff --git a/gm/pathfill.cpp b/gm/pathfill.cpp
index d96f356..af91b84 100644
--- a/gm/pathfill.cpp
+++ b/gm/pathfill.cpp
@@ -432,3 +432,70 @@
DEF_GM( return new PathFillGM; )
DEF_GM( return new PathInverseFillGM; )
+
+DEF_SIMPLE_GM(bug7792, canvas, 800, 200) {
+ // from skbug.com/7792 bug description
+ SkPaint p;
+ SkPath path;
+ path.moveTo(10, 10);
+ path.moveTo(75, 75);
+ path.lineTo(150, 75);
+ path.lineTo(150, 150);
+ path.lineTo(75, 150);
+ canvas->drawPath(path, p);
+ // from skbug.com/7792 comment 3
+ canvas->translate(200, 0);
+ path.reset();
+ path.moveTo(75, 50);
+ path.moveTo(100, 75);
+ path.lineTo(150, 75);
+ path.lineTo(150, 150);
+ path.lineTo(75, 150);
+ path.lineTo(75, 50);
+ path.close();
+ canvas->drawPath(path, p);
+ // from skbug.com/7792 comment 9
+ canvas->translate(200, 0);
+ path.reset();
+ path.moveTo(10, 10);
+ path.moveTo(75, 75);
+ path.lineTo(150, 75);
+ path.lineTo(150, 150);
+ path.lineTo(75, 150);
+ path.close();
+ canvas->drawPath(path, p);
+ // from skbug.com/7792 comment 11
+ canvas->translate(-200 * 2, 200);
+ path.reset();
+ path.moveTo(75, 150);
+ path.lineTo(75, 75);
+ path.lineTo(150, 75);
+ path.lineTo(150, 150);
+ path.lineTo(75, 150);
+ path.moveTo(75, 150);
+ canvas->drawPath(path, p);
+ // from skbug.com/7792 comment 14
+ canvas->translate(200, 0);
+ path.reset();
+ path.moveTo(250, 75);
+ path.moveTo(250, 75);
+ path.moveTo(250, 75);
+ path.moveTo(100, 75);
+ path.lineTo(150, 75);
+ path.lineTo(150, 150);
+ path.lineTo(75, 150);
+ path.lineTo(75, 75);
+ path.close();
+ path.lineTo(0, 0);
+ path.close();
+ canvas->drawPath(path, p);
+ // from skbug.com/7792 comment 15
+ canvas->translate(200, 0);
+ path.reset();
+ path.moveTo(75, 75);
+ path.lineTo(150, 75);
+ path.lineTo(150, 150);
+ path.lineTo(75, 150);
+ path.moveTo(250, 75);
+ canvas->drawPath(path, p);
+}
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index 284a6a7..9e24237 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -447,18 +447,20 @@
bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** ptsPtr,
bool* isClosed, Direction* direction, SkRect* rect) const {
int corners = 0;
- SkPoint first, last;
- const SkPoint* firstPt = nullptr;
+ SkPoint previous; // used to construct line from previous point
+ const SkPoint* firstPt = nullptr; // first point in the rect (last of first moves)
+ const SkPoint* lastPt = nullptr; // last point in the rect (last of lines or first if closed)
const SkPoint* pts = *ptsPtr;
- const SkPoint* savePts = nullptr;
- first.set(0, 0);
- last.set(0, 0);
+ const SkPoint* savePts = nullptr; // used to allow caller to iterate through a pair of rects
+ previous.set(0, 0);
int firstDirection = 0;
int lastDirection = 0;
int nextDirection = 0;
bool closedOrMoved = false;
+ bool addedLine = false;
bool autoClose = false;
bool insertClose = false;
+ bool accumulatingRect = false;
int verbCnt = fPathRef->countVerbs();
while (*currVerb < verbCnt && (!allowPartial || !autoClose)) {
uint8_t verb = insertClose ? (uint8_t) kClose_Verb : fPathRef->atVerb(*currVerb);
@@ -468,23 +470,27 @@
pts = *ptsPtr;
autoClose = true;
insertClose = false;
+ accumulatingRect = false;
case kLine_Verb: {
- SkScalar left = last.fX;
- SkScalar top = last.fY;
+ SkScalar left = previous.fX;
+ SkScalar top = previous.fY;
SkScalar right = pts->fX;
SkScalar bottom = pts->fY;
+ if (accumulatingRect) {
+ lastPt = pts;
+ }
++pts;
if (left != right && top != bottom) {
return false; // diagonal
}
+ addedLine = true;
if (left == right && top == bottom) {
break; // single point on side OK
}
nextDirection = rect_make_dir(right - left, bottom - top);
if (0 == corners) {
firstDirection = nextDirection;
- first = last;
- last = pts[-1];
+ previous = pts[-1];
corners = 1;
closedOrMoved = false;
break;
@@ -501,7 +507,7 @@
return false; // too many direction changes
}
}
- last = pts[-1];
+ previous = pts[-1];
if (lastDirection == nextDirection) {
break; // colinear segment
}
@@ -525,8 +531,13 @@
*currVerb -= 1; // try move again afterwards
goto addMissingClose;
}
- firstPt = pts;
- last = *pts++;
+ if (!addedLine) {
+ firstPt = pts;
+ accumulatingRect = true;
+ } else {
+ accumulatingRect = false;
+ }
+ previous = *pts++;
closedOrMoved = true;
break;
default:
@@ -539,10 +550,12 @@
;
}
// Success if 4 corners and first point equals last
- SkScalar closeX = first.x() - last.x();
- SkScalar closeY = first.y() - last.y();
+ if (corners < 3 || corners > 4) {
+ return false;
+ }
+ SkPoint closeXY = *firstPt - *lastPt;
// If autoClose, check if close generates diagonal
- bool result = 4 == corners && (first == last || (autoClose && (!closeX || !closeY)));
+ bool result = 4 == corners && (closeXY.isZero() || (autoClose && (!closeXY.fX || !closeXY.fY)));
if (!result) {
// check if we are just an incomplete rectangle, in which case we can
// return true, but not claim to be closed.
@@ -550,12 +563,12 @@
// 3 sided rectangle
// 4 sided but the last edge is not long enough to reach the start
//
- if (closeX && closeY) {
+ if (closeXY.fX && closeXY.fY) {
return false; // we're diagonal, abort (can we ever reach this?)
}
- int closeDirection = rect_make_dir(closeX, closeY);
+ int closeDirection = rect_make_dir(closeXY.fX, closeXY.fY);
// make sure the close-segment doesn't double-back on itself
- if (3 == corners || (4 == corners && closeDirection == lastDirection)) {
+ if (3 == corners || closeDirection == lastDirection) {
result = true;
autoClose = false; // we are not closed
}
@@ -564,7 +577,7 @@
*ptsPtr = savePts;
}
if (result && rect) {
- ptrdiff_t count = (savePts ? savePts : pts) - firstPt;
+ ptrdiff_t count = lastPt - firstPt + 1;
rect->set(firstPt, (int) count);
}
if (result && isClosed) {
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 85b307a..ae6568f 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -4901,21 +4901,75 @@
// skbug.com/7792
DEF_TEST(Path_isRect, reporter) {
- SkPath path;
- SkPoint points[] = { {10, 10}, {75, 75}, {150, 75}, {150, 150}, {75, 150} };
- for (size_t index = 0; index < SK_ARRAY_COUNT(points); ++index) {
- index < 2 ? path.moveTo(points[index]) : path.lineTo(points[index]);
- }
+ auto makePath = [](const SkPoint* points, size_t count, bool close) -> SkPath {
+ SkPath path;
+ for (size_t index = 0; index < count; ++index) {
+ index < 2 ? path.moveTo(points[index]) : path.lineTo(points[index]);
+ }
+ return path;
+ };
+ auto makePath2 = [](const SkPoint* points, const SkPath::Verb* verbs, size_t count) -> SkPath {
+ SkPath path;
+ for (size_t index = 0; index < count; ++index) {
+ switch (verbs[index]) {
+ case SkPath::kMove_Verb:
+ path.moveTo(*points++);
+ break;
+ case SkPath::kLine_Verb:
+ path.lineTo(*points++);
+ break;
+ case SkPath::kClose_Verb:
+ path.close();
+ break;
+ default:
+ SkASSERT(0);
+ }
+ }
+ return path;
+ };
+ // isolated from skbug.com/7792 bug description
SkRect rect;
+ SkPoint points[] = { {10, 10}, {75, 75}, {150, 75}, {150, 150}, {75, 150} };
+ SkPath path = makePath(points, SK_ARRAY_COUNT(points), false);
REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
SkRect compare;
compare.set(&points[1], SK_ARRAY_COUNT(points) - 1);
REPORTER_ASSERT(reporter, rect == compare);
- path.reset();
- SkPoint points2[] = { {75, 50}, {100, 75}, {150, 75}, {150, 150}, {75, 150}, {75, 50} };
- for (size_t index = 0; index < SK_ARRAY_COUNT(points); ++index) {
- index < 2 ? path.moveTo(points2[index]) : path.lineTo(points2[index]);
- }
- path.close();
+ // isolated from skbug.com/7792 comment 3
+ SkPoint points3[] = { {75, 50}, {100, 75}, {150, 75}, {150, 150}, {75, 150}, {75, 50} };
+ path = makePath(points3, SK_ARRAY_COUNT(points3), true);
REPORTER_ASSERT(reporter, !path.isRect(&rect, nullptr, nullptr));
+ // isolated from skbug.com/7792 comment 9
+ SkPoint points9[] = { {10, 10}, {75, 75}, {150, 75}, {150, 150}, {75, 150} };
+ path = makePath(points9, SK_ARRAY_COUNT(points9), true);
+ REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
+ compare.set(&points9[1], SK_ARRAY_COUNT(points9) - 1);
+ REPORTER_ASSERT(reporter, rect == compare);
+ // isolated from skbug.com/7792 comment 11
+ SkPath::Verb verbs11[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb,
+ SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kMove_Verb };
+ SkPoint points11[] = { {75, 150}, {75, 75}, {150, 75}, {150, 150}, {75, 150}, {75, 150} };
+ path = makePath2(points11, verbs11, SK_ARRAY_COUNT(verbs11));
+ REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
+ compare.set(&points11[0], SK_ARRAY_COUNT(points11));
+ REPORTER_ASSERT(reporter, rect == compare);
+ // isolated from skbug.com/7792 comment 14
+ SkPath::Verb verbs14[] = { SkPath::kMove_Verb, SkPath::kMove_Verb, SkPath::kMove_Verb,
+ SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb,
+ SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kClose_Verb,
+ SkPath::kLine_Verb, SkPath::kClose_Verb };
+ SkPoint points14[] = { {250, 75}, {250, 75}, {250, 75}, {100, 75},
+ {150, 75}, {150, 150}, {75, 150}, {75, 75}, {0, 0} };
+ path = makePath2(points14, verbs14, SK_ARRAY_COUNT(verbs14));
+ REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
+ compare.set(&points14[3], 5);
+ REPORTER_ASSERT(reporter, rect == compare);
+ // isolated from skbug.com/7792 comment 15
+ SkPath::Verb verbs15[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb,
+ SkPath::kLine_Verb, SkPath::kMove_Verb };
+ SkPoint points15[] = { {75, 75}, {150, 75}, {150, 150}, {75, 150}, {250, 75} };
+ path = makePath2(points15, verbs15, SK_ARRAY_COUNT(verbs15));
+ REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
+ compare.set(&points15[0], SK_ARRAY_COUNT(points15) - 1);
+ REPORTER_ASSERT(reporter, rect == compare);
}