avoid generating degenerate conic from arc

avoid generating degenerate conic from arc

R=bsalomon@google.com
BUG=skia:5672
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2281953002

Committed: https://skia.googlesource.com/skia/+/f71ab8f58b0a7abb7818cf665dfb116ef370d572

[caryclark, 2016-08-26 14:41:29]

Description was changed from

==========
avoid generating degenerate conic from arc

R=bsalomon@google.com
BUG=skia:5672
==========

to

==========
avoid generating degenerate conic from arc

R=bsalomon@google.com
BUG=skia:5672
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2281953002
==========

[caryclark, 2016-08-26 14:43:16]

The CQ bit was checked by caryclark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 14:43:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 15:02:29]

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

[commit-bot: I haz the power, 2016-08-26 15:02:30]

Dry run: This issue passed the CQ dry run.

[caryclark, 2016-08-26 16:12:16]

The CQ bit was checked by caryclark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 16:12:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bsalomon, 2016-08-26 16:15:02]

lgtm

[commit-bot: I haz the power, 2016-08-26 16:42:31]

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

[commit-bot: I haz the power, 2016-08-26 16:42:32]

Dry run: This issue passed the CQ dry run.

[caryclark, 2016-08-26 16:53:28]

The CQ bit was checked by caryclark@google.com

[commit-bot: I haz the power, 2016-08-26 16:53:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 16:54:27]

Description was changed from

==========
avoid generating degenerate conic from arc

R=bsalomon@google.com
BUG=skia:5672
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2281953002
==========

to

==========
avoid generating degenerate conic from arc

R=bsalomon@google.com
BUG=skia:5672
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2281953002

Committed:
https://skia.googlesource.com/skia/+/f71ab8f58b0a7abb7818cf665dfb116ef370d572
==========

[commit-bot: I haz the power, 2016-08-26 16:54:28]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/f71ab8f58b0a7abb7818cf665dfb116ef370d572

[Mark Kilgard, 2016-08-26 17:13:07]

markkilgard@gmail.com changed reviewers:
+ markkilgard@gmail.com

[Mark Kilgard, 2016-08-26 17:13:07]

You should avoid epsilon calculations.

I don't think this is a good approach.

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

https://codereview.chromium.org/2281953002/diff/20001/src/core/SkGeometry.cpp...
src/core/SkGeometry.cpp:1357: }
I don't like this epsilon handling here.  It's just going to shift the issue.

The method looks much as I expected it to look.  It divides large arcs into 90
degree quadrants.

Can you instead figure out how many even segments to divide the arc into?

This approach has two advantages:
1) more stable, no epsilon change
2) each conic's w value will be identical

Example:  If the angle is 200 degrees, notice that ceil(200/90) is 3 (2.222
rounds up to 3) then 200/3 = 66.66.  Now you can do the sqrt((1+dot)/2) math to
determine a single w where dot is cos of 66.66 degrees.

Make sense?  I think this can be a simpler overall algorithm than what you have
here.

Keep in mind if somebody had a abnormally large miter limit and used a miter
join, they might still be able to get zinged join geometry.

[xidachen, 2016-09-29 20:11:15]

On 2016/08/26 17:13:07, Mark Kilgard wrote:
> You should avoid epsilon calculations.
> 
> I don't think this is a good approach.
> 
> https://codereview.chromium.org/2281953002/diff/20001/src/core/SkGeometry.cpp
> File src/core/SkGeometry.cpp (right):
> 
>
https://codereview.chromium.org/2281953002/diff/20001/src/core/SkGeometry.cpp...
> src/core/SkGeometry.cpp:1357: }
> I don't like this epsilon handling here.  It's just going to shift the issue.
> 
> The method looks much as I expected it to look.  It divides large arcs into 90
> degree quadrants.
> 
> Can you instead figure out how many even segments to divide the arc into?
> 
> This approach has two advantages:
> 1) more stable, no epsilon change
> 2) each conic's w value will be identical
> 
> Example:  If the angle is 200 degrees, notice that ceil(200/90) is 3 (2.222
> rounds up to 3) then 200/3 = 66.66.  Now you can do the sqrt((1+dot)/2) math
to
> determine a single w where dot is cos of 66.66 degrees.
> 
> Make sense?  I think this can be a simpler overall algorithm than what you
have
> here.
> 
> Keep in mind if somebody had a abnormally large miter limit and used a miter
> join, they might still be able to get zinged join geometry.

I did some investigation, and this change is the cause of this bug (due to the
epsilon):
https://bugs.chromium.org/p/skia/issues/detail?id=5807