cumulative pathops patch

cumulative pathops patch

Replace the implicit curve intersection with a geometric curve intersection. The implicit intersection proved mathematically unstable and took a long time to zero in on an answer.

Use pointers instead of indices to refer to parts of curves. Indices required awkward renumbering.

Unify t and point values so that small intervals can be eliminated in one pass.

Break cubics up front to eliminate loops and cusps. 

Make the Simplify and Op code more regular and eliminate arbitrary differences.

Add a builder that takes an array of paths and operators.

Delete unused code.



BUG=skia:3588
R=reed@google.com

Committed: https://skia.googlesource.com/skia/+/54359294a7c9dc54802d512a5d891a35c1663392

[caryclark, 2015-03-25 15:54:24]

looking for an lgtm if the spirit moves you

[reed1, 2015-03-25 20:29:24]

lgtm w/ naming convention nits.

https://codereview.chromium.org/1037573004/diff/1/include/pathops/SkPathOps.h
File include/pathops/SkPathOps.h (right):

https://codereview.chromium.org/1037573004/diff/1/include/pathops/SkPathOps.h...
include/pathops/SkPathOps.h:22: kDifference_PathOp,         //!< subtract the op
path from the first path
I know its not part of this CL, but these enums should have SkPathOp in their
suffice, not just PathOp (see SkAlphaType, SkColorType, etc. for our global enum
convention).

https://codereview.chromium.org/1037573004/diff/1/include/pathops/SkPathOps.h...
include/pathops/SkPathOps.h:71: class SK_API SkOpBuilder {
Op seems a little too generic for a name. SkPathOpBuilder seems more specific.

[caryclark, 2015-03-26 12:30:55]

The CQ bit was checked by caryclark@google.com

[caryclark, 2015-03-26 12:30:56]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1037573004/#ps10001
(title: "fix naming issues, erroneous call to sk array size")

[commit-bot: I haz the power, 2015-03-26 12:31:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1037573004/10001

[commit-bot: I haz the power, 2015-03-26 12:35:54]

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

[commit-bot: I haz the power, 2015-03-26 12:35:55]

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...)

[caryclark, 2015-03-26 14:45:33]

The CQ bit was checked by caryclark@google.com

[caryclark, 2015-03-26 14:45:34]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1037573004/#ps20001
(title: "fix pathopsinverse gm")

[commit-bot: I haz the power, 2015-03-26 14:45:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1037573004/20001

[caryclark, 2015-03-26 14:46:52]

https://codereview.chromium.org/1037573004/diff/1/include/pathops/SkPathOps.h
File include/pathops/SkPathOps.h (right):

https://codereview.chromium.org/1037573004/diff/1/include/pathops/SkPathOps.h...
include/pathops/SkPathOps.h:22: kDifference_PathOp,         //!< subtract the op
path from the first path
On 2015/03/25 20:29:24, reed1 wrote:
> I know its not part of this CL, but these enums should have SkPathOp in their
> suffice, not just PathOp (see SkAlphaType, SkColorType, etc. for our global
enum
> convention).

Done.

https://codereview.chromium.org/1037573004/diff/1/include/pathops/SkPathOps.h...
include/pathops/SkPathOps.h:71: class SK_API SkOpBuilder {
On 2015/03/25 20:29:24, reed1 wrote:
> Op seems a little too generic for a name. SkPathOpBuilder seems more specific.

Done.

[commit-bot: I haz the power, 2015-03-26 14:52:50]

Committed patchset #3 (id:20001) as
https://skia.googlesource.com/skia/+/54359294a7c9dc54802d512a5d891a35c1663392

[Stephen Chennney, 2015-03-26 15:24:15]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2015-03-26 15:24:16]

This breaks the chrome build, specifically:

../../third_party/WebKit/Source/platform/graphics/Path.cpp:494:37: error: use of
undeclared identifier 'kDifference_PathOp'; did you mean 'kDifference_SkPathOp'?
    return Op(m_path, other.m_path, kDifference_PathOp, &m_path);
                                    ^~~~~~~~~~~~~~~~~~
                                    kDifference_SkPathOp
../../third_party/skia/include/pathops/SkPathOps.h:22:5: note:
'kDifference_SkPathOp' declared here
    kDifference_SkPathOp,         //!< subtract the op path from the first path
    ^
../../third_party/WebKit/Source/platform/graphics/Path.cpp:499:37: error: use of
undeclared identifier 'kIntersect_PathOp'; did you mean 'kIntersect_SkPathOp'?
    return Op(m_path, other.m_path, kIntersect_PathOp, &m_path);
                                    ^~~~~~~~~~~~~~~~~
                                    kIntersect_SkPathOp
../../third_party/skia/include/pathops/SkPathOps.h:23:5: note:
'kIntersect_SkPathOp' declared here
    kIntersect_SkPathOp,          //!< intersect the two paths
    ^
../../third_party/WebKit/Source/platform/graphics/Path.cpp:504:37: error: use of
undeclared identifier 'kUnion_PathOp'; did you mean 'kUnion_SkPathOp'?
    return Op(m_path, other.m_path, kUnion_PathOp, &m_path);
                                    ^~~~~~~~~~~~~
                                    kUnion_SkPathOp
../../third_party/skia/include/pathops/SkPathOps.h:24:5: note: 'kUnion_SkPathOp'
declared here
    kUnion_SkPathOp,              //!< union (inclusive-or) the two paths
    ^
You'll either need to revert the renaming in SkPathOps.h or you'll need some way
to control which names are used in chrome.