Skip to content

Commit

Permalink
fix pathops bug 8380
Browse files Browse the repository at this point in the history 
Paths to intersect have two nearly coincident cubics. Where they
cross, the intersection error makes the curves start at slightly
different points. To sort the intersection, one curve is translated
to the start of the opposite point, moving it from one side to the
other, introducing a winding error.

The fix looks for that error in a very tiny range (enlarging that
range causes other tests that now pass to fail). This fix is very
fragile and points to the need for a better approach than sorting
angles to find winding values, as documented in the bug.

Also renamed some angle functions to show that they operate only
on lines and not general curves.

All tests pass with this fix:
./out/release/pathops_unittest -V -x
./out/debug/pathops_unittest -V -x

[email protected]
Bug: skia:8380
Change-Id: I04e53d4c6a96035f661a4c9f31a17055ce13e3eb
Reviewed-on: https://skia-review.googlesource.com/c/179241
Commit-Queue: Cary Clark <[email protected]>
Reviewed-by: Cary Clark <[email protected]>
  • Loading branch information
Cary Clark authored and Skia Commit-Bot committed Dec 21, 2018
1 parent fb45594 commit 9d6049a
Show file tree
Hide file tree
Showing 6 changed files with 619 additions and 1,045 deletions.
74 changes: 54 additions & 20 deletions src/pathops/SkOpAngle.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,21 +180,21 @@ bool SkOpAngle::after(SkOpAngle* test) {
// if only one pair are the same, the third point touches neither of the pair
if (ltShare + lrShare + trShare == 1) {
if (lrShare) {
int ltOOrder = lh->allOnOriginalSide(this);
int rtOOrder = rh->allOnOriginalSide(this);
int ltOOrder = lh->linesOnOriginalSide(this);
int rtOOrder = rh->linesOnOriginalSide(this);
if ((rtOOrder ^ ltOOrder) == 1) {
return ltOOrder;
}
} else if (trShare) {
int tlOOrder = this->allOnOriginalSide(lh);
int rlOOrder = rh->allOnOriginalSide(lh);
int tlOOrder = this->linesOnOriginalSide(lh);
int rlOOrder = rh->linesOnOriginalSide(lh);
if ((tlOOrder ^ rlOOrder) == 1) {
return rlOOrder;
}
} else {
SkASSERT(ltShare);
int trOOrder = rh->allOnOriginalSide(this);
int lrOOrder = lh->allOnOriginalSide(rh);
int trOOrder = rh->linesOnOriginalSide(this);
int lrOOrder = lh->linesOnOriginalSide(rh);
// result must be 0 and 1 or 1 and 0 to be valid
if ((lrOOrder ^ trOOrder) == 1) {
return trOOrder;
Expand All @@ -212,18 +212,13 @@ bool SkOpAngle::after(SkOpAngle* test) {
return COMPARE_RESULT(13, !lrOrder);
}

// given a line, see if the opposite curve's convex hull is all on one side
// returns -1=not on one side 0=this CW of test 1=this CCW of test
int SkOpAngle::allOnOneSide(const SkOpAngle* test) {
SkASSERT(!fPart.isCurve());
SkASSERT(test->fPart.isCurve());
SkDPoint origin = fPart.fCurve[0];
SkDVector line = fPart.fCurve[1] - origin;
int SkOpAngle::lineOnOneSide(const SkDPoint& origin, const SkDVector& line, const SkOpAngle* test,
bool useOriginal) const {
double crosses[3];
SkPath::Verb testVerb = test->segment()->verb();
int iMax = SkPathOpsVerbToPoints(testVerb);
// SkASSERT(origin == test.fCurveHalf[0]);
const SkDCurve& testCurve = test->fPart.fCurve;
const SkDCurve& testCurve = useOriginal ? test->fOriginalCurvePart : test->fPart.fCurve;
for (int index = 1; index <= iMax; ++index) {
double xy1 = line.fX * (testCurve[index].fY - origin.fY);
double xy2 = line.fY * (testCurve[index].fX - origin.fX);
Expand All @@ -246,12 +241,26 @@ int SkOpAngle::allOnOneSide(const SkOpAngle* test) {
if (SkPath::kCubic_Verb == testVerb && crosses[2]) {
return crosses[2] < 0;
}
fUnorderable = true;
return -1;
return -2;
}

// given a line, see if the opposite curve's convex hull is all on one side
// returns -1=not on one side 0=this CW of test 1=this CCW of test
int SkOpAngle::lineOnOneSide(const SkOpAngle* test, bool useOriginal) {
SkASSERT(!fPart.isCurve());
SkASSERT(test->fPart.isCurve());
SkDPoint origin = fPart.fCurve[0];
SkDVector line = fPart.fCurve[1] - origin;
int result = this->lineOnOneSide(origin, line, test, useOriginal);
if (-2 == result) {
fUnorderable = true;
result = -1;
}
return result;
}

// experiment works only with lines for now
int SkOpAngle::allOnOriginalSide(const SkOpAngle* test) {
int SkOpAngle::linesOnOriginalSide(const SkOpAngle* test) {
SkASSERT(!fPart.isCurve());
SkASSERT(!test->fPart.isCurve());
SkDPoint origin = fOriginalCurvePart[0];
Expand Down Expand Up @@ -578,7 +587,32 @@ bool SkOpAngle::endsIntersect(SkOpAngle* rh) {
}
double maxWidth = SkTMax(maxX - minX, maxY - minY);
delta = sk_ieee_double_divide(delta, maxWidth);
if (delta > 1e-3 && (useIntersect ^= true)) { // FIXME: move this magic number
// FIXME: move these magic numbers
// This fixes skbug.com/8380
// Larger changes (like changing the constant in the next block) cause other
// tests to fail as documented in the bug.
// This could probably become a more general test: e.g., if translating the
// curve causes the cross product of any control point or end point to change
// sign with regard to the opposite curve's hull, treat the curves as parallel.

// Moreso, this points to the general fragility of this approach of assigning
// winding by sorting the angles of curves sharing a common point, as mentioned
// in the bug.
if (delta < 4e-3 && delta > 1e-3 && !useIntersect && fPart.isCurve()
&& rh->fPart.isCurve() && fOriginalCurvePart[0] != fPart.fCurve.fLine[0]) {
// see if original curve is on one side of hull; translated is on the other
const SkDPoint& origin = rh->fOriginalCurvePart[0];
int count = SkPathOpsVerbToPoints(rh->segment()->verb());
const SkDVector line = rh->fOriginalCurvePart[count] - origin;
int originalSide = rh->lineOnOneSide(origin, line, this, true);
if (originalSide >= 0) {
int translatedSide = rh->lineOnOneSide(origin, line, this, false);
if (originalSide != translatedSide) {
continue;
}
}
}
if (delta > 1e-3 && (useIntersect ^= true)) {
sRayLonger = rayLonger;
sCept = cept;
sCeptT = smallTs[index];
Expand Down Expand Up @@ -881,14 +915,14 @@ int SkOpAngle::orderable(SkOpAngle* rh) {
SkASSERT(x_ry != rx_y); // indicates an undetected coincidence -- worth finding earlier
return x_ry < rx_y ? 1 : 0;
}
if ((result = this->allOnOneSide(rh)) >= 0) {
if ((result = this->lineOnOneSide(rh, false)) >= 0) {
return result;
}
if (fUnorderable || approximately_zero(rh->fSide)) {
goto unorderable;
}
} else if (!rh->fPart.isCurve()) {
if ((result = rh->allOnOneSide(this)) >= 0) {
if ((result = rh->lineOnOneSide(this, false)) >= 0) {
return result ? 0 : 1;
}
if (rh->fUnorderable || approximately_zero(fSide)) {
Expand Down
6 changes: 4 additions & 2 deletions src/pathops/SkOpAngle.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,6 @@ class SkOpAngle {
private:
bool after(SkOpAngle* test);
void alignmentSameSide(const SkOpAngle* test, int* order) const;
int allOnOneSide(const SkOpAngle* test);
int allOnOriginalSide(const SkOpAngle* test);
bool checkCrossesZero() const;
bool checkParallel(SkOpAngle* );
bool computeSector();
Expand All @@ -108,6 +106,10 @@ class SkOpAngle {
bool endsIntersect(SkOpAngle* );
int findSector(SkPath::Verb verb, double x, double y) const;
SkOpGlobalState* globalState() const;
int lineOnOneSide(const SkDPoint& origin, const SkDVector& line, const SkOpAngle* test,
bool useOriginal) const;
int lineOnOneSide(const SkOpAngle* test, bool useOriginal);
int linesOnOriginalSide(const SkOpAngle* test);
bool merge(SkOpAngle* );
double midT() const;
bool midToSide(const SkOpAngle* rh, bool* inside) const;
Expand Down
2 changes: 1 addition & 1 deletion tests/PathOpsAngleTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ class PathOpsAngleTester {
}

static int AllOnOneSide(SkOpAngle& lh, SkOpAngle& rh) {
return lh.allOnOneSide(&rh);
return lh.lineOnOneSide(&rh, false);
}

static int ConvexHullOverlaps(SkOpAngle& lh, SkOpAngle& rh) {
Expand Down
25 changes: 25 additions & 0 deletions tests/PathOpsOpTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9079,13 +9079,38 @@ static void bug8228(skiatest::Reporter* reporter, const char* filename) {
testPathOp(reporter, path1, path2, kIntersect_SkPathOp, filename);
}

static void bug8380(skiatest::Reporter* reporter, const char* filename) {
SkPath path, path2;
path.setFillType(SkPath::kEvenOdd_FillType);
path.moveTo(SkBits2Float(0xa6800000), SkBits2Float(0x43b0f22d)); // -8.88178e-16f, 353.892f
path.lineTo(SkBits2Float(0x42fc0000), SkBits2Float(0x4116566d)); // 126, 9.3961f
path.cubicTo(SkBits2Float(0x42fb439d), SkBits2Float(0x4114bbc7), SkBits2Float(0x42fa3ed7), SkBits2Float(0x411565bd), SkBits2Float(0x42f934d2), SkBits2Float(0x4116131e)); // 125.632f, 9.29584f, 125.123f, 9.33734f, 124.603f, 9.37967f
path.cubicTo(SkBits2Float(0x42f84915), SkBits2Float(0x4116acc3), SkBits2Float(0x42f75939), SkBits2Float(0x41174918), SkBits2Float(0x42f693f8), SkBits2Float(0x4116566d)); // 124.143f, 9.41718f, 123.674f, 9.45535f, 123.289f, 9.3961f
path.lineTo(SkBits2Float(0x42ec3cee), SkBits2Float(0x410127bb)); // 118.119f, 8.0722f
path.lineTo(SkBits2Float(0x4102c0ec), SkBits2Float(0x42d06d0e)); // 8.1721f, 104.213f
path.lineTo(SkBits2Float(0xa6000000), SkBits2Float(0x4381a63d)); // -4.44089e-16f, 259.299f
path.lineTo(SkBits2Float(0x00000000), SkBits2Float(0x43b0f22d)); // 0, 353.892f
path.lineTo(SkBits2Float(0xa6800000), SkBits2Float(0x43b0f22d)); // -8.88178e-16f, 353.892f
path.close();
path2.setFillType(SkPath::kEvenOdd_FillType);
path2.moveTo(SkBits2Float(0x4102c0ec), SkBits2Float(0x42d06d0e)); // 8.1721f, 104.213f
path2.lineTo(SkBits2Float(0xc0ba5a1d), SkBits2Float(0x43b8e831)); // -5.8235f, 369.814f
path2.lineTo(SkBits2Float(0x42fc0000), SkBits2Float(0x411656d6)); // 126, 9.3962f
path2.cubicTo(SkBits2Float(0x42fa9cac), SkBits2Float(0x41134fdf), SkBits2Float(0x42f837cf), SkBits2Float(0x41185aee), SkBits2Float(0x42f693f8), SkBits2Float(0x411656d6)); // 125.306f, 9.207f, 124.109f, 9.5222f, 123.289f, 9.3962f
path2.lineTo(SkBits2Float(0x42ec3cee), SkBits2Float(0x410127bb)); // 118.119f, 8.0722f
path2.lineTo(SkBits2Float(0x4102c0ec), SkBits2Float(0x42d06d0e)); // 8.1721f, 104.213f
path2.close();
testPathOp(reporter, path, path2, kIntersect_SkPathOp, filename);
}

static void (*skipTest)(skiatest::Reporter* , const char* filename) = 0;
static void (*firstTest)(skiatest::Reporter* , const char* filename) = 0;
static void (*stopTest)(skiatest::Reporter* , const char* filename) = 0;

#define TEST(name) { name, #name }

static struct TestDesc tests[] = {
TEST(bug8380),
TEST(crbug_526025),
TEST(bug8228),
TEST(op_4),
Expand Down
5 changes: 3 additions & 2 deletions tools/pathops_sorter.htm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@
<div style="height:0">

<div id="cubics">
{{{0.00000000000000000, 0.00000000000000000 }, {0.00022939755581319332, 0.00022927834652364254 },{0.00022930106206331402, 0.00022929999977350235 }, {0.00022930069826543331, 0.00022913678549230099}}},
{{{0.00022930069826543331, 0.00022930069826543331 }, {0.00011465034913271666, 0.00011465034913271666 },{0.00011465061106719077, 0.00011460937093943357 }, {0.00014331332931760699, 0.00014325146912597120}}},
{{{fX=124.70011901855469 fY=9.3718261718750000 } {fX=124.66775026544929 fY=9.3744316215161234 } {fX=124.63530969619751 fY=9.3770743012428284 }{fX=124.60282897949219 fY=9.3797206878662109 } id=10
{{{fX=124.70011901855469 fY=9.3718004226684570 } {fX=124.66775026544929 fY=9.3744058723095804 } {fX=124.63530969619751 fY=9.3770485520362854 } {fX=124.60282897949219 fY=9.3796949386596680 } id=1
{{{fX=124.70011901855469 fY=9.3718004226684570 } {fX=124.66786243087600 fY=9.3743968522034287 } {fX=124.63553249625420 fY=9.3770303056986286 } {fX=124.60316467285156 fY=9.3796672821044922 } id=2
</div>

</div>
Expand Down
Loading

0 comments on commit 9d6049a

Please sign in to comment.