Fix bug where SkPath will convert an arc to an oval and change the starting point.

Fix bug where SkPath will convert an arc to an oval and change the starting point.

Adds unit tests to test the behavior of addArc to oval conversions.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2017743002

Committed: https://skia.googlesource.com/skia/+/1978ce22eb488e3f05670189ea35d9dfae6a6784

[bsalomon, 2016-05-26 21:03:48]

Description was changed from

==========
Fix bug where SkPath will convert an arc to an oval and change the starting
point.

Adds unit tests to test the behavior of addArc to oval conversions.
==========

to

==========
Fix bug where SkPath will convert an arc to an oval and change the starting
point.

Adds unit tests to test the behavior of addArc to oval conversions.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2017743002
==========

[bsalomon, 2016-05-26 21:06:17]

bsalomon@google.com changed reviewers:
+ robertphillips@google.com

[bsalomon, 2016-05-26 21:06:18]

I noticed this bug while working on the rrect/oval dir and start index change.

[robertphillips, 2016-05-27 12:12:30]

lgtm

https://codereview.chromium.org/2017743002/diff/20001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/2017743002/diff/20001/src/core/SkPath.cpp#new...
src/core/SkPath.cpp:1423: 
It is unfortunate we have to always do this since the start position will only
matter for some paints.

Is there no way to push the pain of the start point into the dash effect?

https://codereview.chromium.org/2017743002/diff/20001/src/core/SkPath.cpp#new...
src/core/SkPath.cpp:1427: float n = startAngle / 90.f;
i -> intPortion, r -> remainder ?

https://codereview.chromium.org/2017743002/diff/20001/src/core/SkPath.cpp#new...
src/core/SkPath.cpp:1435: i += 1;  // index 1 is at angle 0.
Maybe introduce a new variable (int startIndex) here, rather than reuse 'r' ?

https://codereview.chromium.org/2017743002/diff/20001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/2017743002/diff/20001/tests/PathTest.cpp#newc...
tests/PathTest.cpp:3341: REPORTER_ASSERT(reporter, path == recreatedPath);
Do we still need these next 3 lines? They look like they're left over from
debugging.

[bsalomon, 2016-05-27 14:29:33]

The implementation wasn't correct and the test didn't catch it. I fixed the
implementation and the test.

https://codereview.chromium.org/2017743002/diff/20001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/2017743002/diff/20001/src/core/SkPath.cpp#new...
src/core/SkPath.cpp:1423: 
On 2016/05/27 12:12:29, robertphillips wrote:
> It is unfortunate we have to always do this since the start position will only
> matter for some paints.
> 
> Is there no way to push the pain of the start point into the dash effect?

I agree that this is painful. The problem is that the geometry and the paint are
constructed completely independently and by the time we have both the start
angle is lost and so there is no way to communicate it to the dash. I think the
way to preserve the oval status of arbitrary addArc start angles is to record
the actual start angle instead of an index to a set of 4 possible start angles.
That can be done in a future change.

https://codereview.chromium.org/2017743002/diff/20001/src/core/SkPath.cpp#new...
src/core/SkPath.cpp:1427: float n = startAngle / 90.f;
On 2016/05/27 12:12:29, robertphillips wrote:
> i -> intPortion, r -> remainder ?

This code wasn't quite correct. The new version is fewer lines and uses more
explicit var names.

https://codereview.chromium.org/2017743002/diff/20001/src/core/SkPath.cpp#new...
src/core/SkPath.cpp:1435: i += 1;  // index 1 is at angle 0.
On 2016/05/27 12:12:30, robertphillips wrote:
> Maybe introduce a new variable (int startIndex) here, rather than reuse 'r' ?

Done.

https://codereview.chromium.org/2017743002/diff/20001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/2017743002/diff/20001/tests/PathTest.cpp#newc...
tests/PathTest.cpp:3341: REPORTER_ASSERT(reporter, path == recreatedPath);
On 2016/05/27 12:12:30, robertphillips wrote:
> Do we still need these next 3 lines? They look like they're left over from
> debugging.

Done.

[robertphillips, 2016-05-27 15:24:59]

lgtm

https://codereview.chromium.org/2017743002/diff/40001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/2017743002/diff/40001/tests/PathTest.cpp#newc...
tests/PathTest.cpp:3377: for (float delta : {-SK_ScalarNearlyZero,
SK_ScalarNearlyZero}) {
Can we have a shared check_oval helper that takes and betterBeAnOval boolean?

[bsalomon, 2016-05-27 21:14:21]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-27 21:14:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/2017743002/#ps80001
(title: "formatting")

[commit-bot: I haz the power, 2016-05-27 21:14:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017743002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017743002/80001

[commit-bot: I haz the power, 2016-05-27 21:19:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 21:19:59]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[bsalomon, 2016-05-31 17:25:49]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-31 17:25:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/2017743002/#ps100001
(title: "Use SkScalarRoundToScalar to avoid android std::round issue")

[commit-bot: I haz the power, 2016-05-31 17:25:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017743002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017743002/100001

[commit-bot: I haz the power, 2016-05-31 17:42:18]

Description was changed from

==========
Fix bug where SkPath will convert an arc to an oval and change the starting
point.

Adds unit tests to test the behavior of addArc to oval conversions.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2017743002
==========

to

==========
Fix bug where SkPath will convert an arc to an oval and change the starting
point.

Adds unit tests to test the behavior of addArc to oval conversions.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2017743002

Committed:
https://skia.googlesource.com/skia/+/1978ce22eb488e3f05670189ea35d9dfae6a6784
==========

[commit-bot: I haz the power, 2016-05-31 17:42:19]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/1978ce22eb488e3f05670189ea35d9dfae6a6784

[bsalomon, 2016-06-10 15:04:34]

https://codereview.chromium.org/2017743002/diff/40001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/2017743002/diff/40001/tests/PathTest.cpp#newc...
tests/PathTest.cpp:3377: for (float delta : {-SK_ScalarNearlyZero,
SK_ScalarNearlyZero}) {
On 2016/05/27 15:24:59, robertphillips wrote:
> Can we have a shared check_oval helper that takes and betterBeAnOval boolean?

Done.