Skip to content

Commit

Permalink
Reland: Ensure arcTo (SVG) ends at the targeted point
Browse files Browse the repository at this point in the history
Floating-point errors in the calculations in arcTo can cause the final
point to differ significantly enough from the supplied target point that
a subsequent 'close' operation inserts a lineTo to close the curve. This
can result in bad miters on curves with thick outlines.

The fix is to ensure that the final point used is *exactly* the point
that was supplied.

The fix is now staged behind a flag so that we can fix Chromium test
failures before turning it on.

Bug: chromium:1001768
Change-Id: I82d872f2ae39b5486f7d810a61ba00eeda1b3a62
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/270503
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Mike Reed <reed@google.com>
  • Loading branch information
ijprest authored and Skia Commit-Bot committed Feb 14, 2020
1 parent 4b40cf2 commit 05676f7
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/core/SkPath.cpp
Expand Up @@ -1262,6 +1262,12 @@ SkPath& SkPath::arcTo(SkScalar rx, SkScalar ry, SkScalar angle, SkPath::ArcSize
this->conicTo(mapped[0], mapped[1], w);
startTheta = endTheta;
}

#ifndef SK_LEGACY_PATH_ARCTO_ENDPOINT
// The final point should match the input point (by definition); replace it to
// ensure that rounding errors in the above math don't cause any problems.
this->setLastPt(x, y);
#endif
return *this;
}

Expand Down
14 changes: 14 additions & 0 deletions tests/PathTest.cpp
Expand Up @@ -3908,6 +3908,20 @@ static void test_arcTo(skiatest::Reporter* reporter) {
p.reset();
p.arcTo(noOvalHeight, 0, 360, false);
REPORTER_ASSERT(reporter, p.isEmpty());

#ifndef SK_LEGACY_PATH_ARCTO_ENDPOINT
// Inspired by http://code.google.com/p/chromium/issues/detail?id=1001768
{
p.reset();
p.moveTo(216, 216);
p.arcTo(216, 108, 0, SkPath::ArcSize::kLarge_ArcSize, SkPathDirection::kCW, 216, 0);
p.arcTo(270, 135, 0, SkPath::ArcSize::kLarge_ArcSize, SkPathDirection::kCCW, 216, 216);

// The 'arcTo' call should end up exactly at the starting location.
int n = p.countPoints();
REPORTER_ASSERT(reporter, p.getPoint(0) == p.getPoint(n - 1));
}
#endif
}

static void test_addPath(skiatest::Reporter* reporter) {
Expand Down

0 comments on commit 05676f7

Please sign in to comment.