Adding new 'extend' mode to SkPath::addPath

Adding new 'extend' mode to SkPath::addPath

BUG=261727
Committed: http://code.google.com/p/skia/source/detail?r=13415

[Justin Novosad, 2014-02-07 16:13:20]

PTAL

[reed1, 2014-02-07 16:22:19]

1. Seems like we could use more explicit dox on addPath()
2. Is the difference between addPath and joinPath ....?
  - addPath always creates new contours for the additional data
  - joinPath wants to extend the last contour of the dst path with the first
contour of the src path
3. If #2 is true, perhaps we can just add a public enum param to addPath()?

[Justin Novosad, 2014-02-07 16:30:04]

On 2014/02/07 16:22:19, reed1 wrote:
> 1. Seems like we could use more explicit dox on addPath()
> 2. Is the difference between addPath and joinPath ....?
>   - addPath always creates new contours for the additional data
>   - joinPath wants to extend the last contour of the dst path with the first
> contour of the src path
> 3. If #2 is true, perhaps we can just add a public enum param to addPath()?

1. Agreed I'll do that
2. Yes, except that add path does not always start a new contour. addPath just
adds src as is. If the added path starts with a moveTo, then we get a new
contour.
3. OK makes sense to me.

[Justin Novosad, 2014-02-07 16:49:36]

On 2014/02/07 16:30:04, junov wrote:
> On 2014/02/07 16:22:19, reed1 wrote:
> > 1. Seems like we could use more explicit dox on addPath()
> > 2. Is the difference between addPath and joinPath ....?
> >   - addPath always creates new contours for the additional data
> >   - joinPath wants to extend the last contour of the dst path with the first
> > contour of the src path
> > 3. If #2 is true, perhaps we can just add a public enum param to addPath()?
> 
> 1. Agreed I'll do that
> 2. Yes, except that add path does not always start a new contour. addPath just
> adds src as is. If the added path starts with a moveTo, then we get a new
> contour.
> 3. OK makes sense to me.

Done.

[reed1, 2014-02-07 16:55:48]

On 2014/02/07 16:30:04, junov wrote:
> On 2014/02/07 16:22:19, reed1 wrote:
> > 1. Seems like we could use more explicit dox on addPath()
> > 2. Is the difference between addPath and joinPath ....?
> >   - addPath always creates new contours for the additional data
> >   - joinPath wants to extend the last contour of the dst path with the first
> > contour of the src path
> > 3. If #2 is true, perhaps we can just add a public enum param to addPath()?
> 
> 1. Agreed I'll do that
> 2. Yes, except that add path does not always start a new contour. addPath just
> adds src as is. If the added path starts with a moveTo, then we get a new
> contour.

Does the added path HAVE to start with a moveTo? What else can it start with?

> 3. OK makes sense to me.

[reed1, 2014-02-07 17:13:06]

https://codereview.chromium.org/151353006/diff/50001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.chromium.org/151353006/diff/50001/include/core/SkPath.h#ne...
include/core/SkPath.h:707: kAppend_AddPathMode,
/**
 * source path contours are added as new contours (or something like that)
 */

I think we should somehow mention contours, to be more parallel to the
explanation for kJoin...

https://codereview.chromium.org/151353006/diff/50001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/151353006/diff/50001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:3069: p.addPath(q, SkPath::kJoin_AddPathMode);
Seems like we should have parallel tests for both enum values here.

[Justin Novosad, 2014-02-07 17:54:31]

On 2014/02/07 17:13:06, reed1 wrote:

>
https://codereview.chromium.org/151353006/diff/50001/include/core/SkPath.h#ne...
> include/core/SkPath.h:707: kAppend_AddPathMode,
> /**
>  * source path contours are added as new contours (or something like that)
>  */

You are right.  I didn't realize this at first, but contours all start with an
implicit moveTo verb.
Therfore, if we concatenate paths, we always get separate contours.

[Justin Novosad, 2014-02-07 17:59:39]

new patch

[reed1, 2014-02-07 18:05:50]

lgtm w/ name-change

https://codereview.chromium.org/151353006/diff/130001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.chromium.org/151353006/diff/130001/include/core/SkPath.h#n...
include/core/SkPath.h:712: kJoin_AddPathMode
How about kExtend_, esp. since that is the word you chose for your dox (and it
was the first word that came to mind by some others in Skia I surveyed)

https://codereview.chromium.org/151353006/diff/130001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/151353006/diff/130001/tests/PathTest.cpp#newc...
tests/PathTest.cpp:3063: static void test_addPathJoin(skiatest::Reporter*
reporter, bool explicitMoveTo) {
addPathExtend?

https://codereview.chromium.org/151353006/diff/130001/tests/PathTest.cpp#newc...
tests/PathTest.cpp:3099: REPORTER_ASSERT(reporter, verbs[2] ==
SkPath::kMove_Verb);
I wonder if we can make these new test more compact. It appears that these two
differ only in the enum and 1 assert. Can we refactor that to share more code?
If that is a bother right now, it can be done later (or I can do it).

In general I think Skia as a whole needs to put more time into making our tests
as readable/maintainable as possible (not picking on this CL per-se).

[reed1, 2014-02-07 18:08:01]

undo-lgtm for now

What is the behavior if the dst-path's last contour is open or closed? Do we
extend closed paths?

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

https://codereview.chromium.org/151353006/diff/130001/src/core/SkPath.cpp#new...
src/core/SkPath.cpp:1461: bool firstVerb = true;
can we change this to:

bool firstVerb = (kExtend == mode);

and then the lower test can become just

if (firstVerb) ... lineTo

[Justin Novosad, 2014-02-07 18:34:32]

On 2014/02/07 18:08:01, reed1 wrote:

> What is the behavior if the dst-path's last contour is open or closed? Do we
> extend closed paths?

Yes. I need to be able to do that for the purpose of making Canvas2D paths spec
compliant.
If we ever have a use case where we need conditional behavior, then the call
site can switch modes based on whether the last countour is closed, or we can
add an other mode to the enum.

[reed1, 2014-02-07 18:36:46]

On 2014/02/07 18:34:32, junov wrote:
> On 2014/02/07 18:08:01, reed1 wrote:
> 
> > What is the behavior if the dst-path's last contour is open or closed? Do we
> > extend closed paths?
> 
> Yes. I need to be able to do that for the purpose of making Canvas2D paths
spec
> compliant.
> If we ever have a use case where we need conditional behavior, then the call
> site can switch modes based on whether the last countour is closed, or we can
> add an other mode to the enum.

Please update the dox/tests to reflect this....

?
a) we extend closed paths and respect the new path's ending (open or closed)
b) we extend closed paths but force the extended path to stay closed
c) we don't extend closed paths, only open ones

[robertphillips, 2014-02-07 19:03:45]

Just FYI there is this pre-existing issue
(https://code.google.com/p/skia/issues/detail?id=1267)

[reed1, 2014-02-07 20:13:10]

On 2014/02/07 19:03:45, robertphillips wrote:
> Just FYI there is this pre-existing issue
> (https://code.google.com/p/skia/issues/detail?id=1267)

Interesting. What is the relationship between addPath and pathTo, and could they
be combined?

[Justin Novosad, 2014-02-07 20:18:19]

On 2014/02/07 20:13:10, reed1 wrote:
> On 2014/02/07 19:03:45, robertphillips wrote:
> > Just FYI there is this pre-existing issue
> > (https://code.google.com/p/skia/issues/detail?id=1267)
> 
> Interesting. What is the relationship between addPath and pathTo, and could
they
> be combined?

I do not see pathTo, only reversePathTo.

[Justin Novosad, 2014-02-07 20:19:13]

On 2014/02/07 20:18:19, junov wrote:
> On 2014/02/07 20:13:10, reed1 wrote:
> > On 2014/02/07 19:03:45, robertphillips wrote:
> > > Just FYI there is this pre-existing issue
> > > (https://code.google.com/p/skia/issues/detail?id=1267)
> > 
> > Interesting. What is the relationship between addPath and pathTo, and could
> they
> > be combined?
> 
> I do not see pathTo, only reversePathTo.

New patch. Handles closed paths as desired, and documented the behavior

[Justin Novosad, 2014-02-10 16:09:11]

Ping.

[reed1, 2014-02-10 21:03:14]

https://codereview.chromium.org/151353006/diff/250001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.chromium.org/151353006/diff/250001/include/core/SkPath.h#n...
include/core/SkPath.h:711: the destination path is closed, it will be
pseudo-extended by
lets assume dst and src each have only 1 contour

1. if dst is closed, then
    remove the close verb
2. replace the src moveTo with lineTo(?)
3. append the rest of src

Is that correct? I couldn't really tell from your dox.

[Justin Novosad, 2014-02-11 16:48:32]

On 2014/02/10 21:03:14, reed1 wrote:
> https://codereview.chromium.org/151353006/diff/250001/include/core/SkPath.h
> File include/core/SkPath.h (right):
> 
>
https://codereview.chromium.org/151353006/diff/250001/include/core/SkPath.h#n...
> include/core/SkPath.h:711: the destination path is closed, it will be
> pseudo-extended by
> lets assume dst and src each have only 1 contour
> 
> 1. if dst is closed, then
>     remove the close verb
> 2. replace the src moveTo with lineTo(?)
> 3. append the rest of src
> 
> Is that correct? I couldn't really tell from your dox.

Not quit correct. The close verb stays intact.
1. add to dst a moveTo to the staring point of the last contour in dst
2. replace the src moveTo with lineTo
3. append the rest of src

This behavior is exactly consistent with what is required by the canvas 2D spec.
The text in the spec says: 

The closePath() method must do nothing if the object's path has no subpaths.
Otherwise, it must mark the last subpath as closed, create a new subpath whose
first point is the same as the previous subpath's first point, and finally add
this new subpath to the path.

Elsewhere in in the spec, the description of arc and ellipse states:

When the ellipse() method is invoked, it must proceed as follows. First, if the
object's path has any subpaths, then the method must add a straight line from
the last point in the subpath to the start point of the arc.

[Justin Novosad, 2014-02-11 18:16:11]

Ok, I updated the comment to make it clearer but without making it too lengthy
either.
Is this better?

[reed1, 2014-02-11 18:35:42]

Just to be sure, are you agreeing with my last 1,2,3 itemization?

If so, I think your dox are better, and can be improved perhaps over time.

I can't tell for sure, but if you unittest tests all the variation that my 1,2,3
recipe mentioned, then lgtm.

Mostly, I'm interested in ensuring that

1. The old behavior is unchanged (and well/better documented)
2. The new behavior is very well tested and documented (and understandable)
...
3. We actually need the new behavior (I am deferring to your read of the spec on
this one)

[Justin Novosad, 2014-02-11 18:49:18]

On 2014/02/11 18:35:42, reed1 wrote:
> Just to be sure, are you agreeing with my last 1,2,3 itemization?
> 
> If so, I think your dox are better, and can be improved perhaps over time.
> 
> I can't tell for sure, but if you unittest tests all the variation that my
1,2,3
> recipe mentioned, then lgtm.
> 
> Mostly, I'm interested in ensuring that
> 
> 1. The old behavior is unchanged (and well/better documented)
> 2. The new behavior is very well tested and documented (and understandable)
> ...
> 3. We actually need the new behavior (I am deferring to your read of the spec
on
> this one)

IMHO all those criteria are met, except that I just thought of edge cases that
are not covered by the current unit tests : empty dst and/or src paths.
I'll go and add those now.

[caryclark, 2014-02-11 19:05:59]

Is there a reference canvas test that shows whether other browsers also
implement the proper behavior for the spec and for these edge cases?

[Justin Novosad, 2014-02-11 19:54:52]

On 2014/02/11 19:05:59, caryclark wrote:
> Is there a reference canvas test that shows whether other browsers also
> implement the proper behavior for the spec and for these edge cases?

To give more context, the bug I am fixing has to do with the fact that paths
misbehave when a 2D canvas has a non-invertible CTM. Right now, not only does
Chrome not early-out when the CTM is non-invertibele, but it scraps all the path
primitive that were recorded before the matrix became singular, which is a
significant bug.  This solution is to avoid bring previously recorded primitives
into the local coordinate space, and to instead retain the canvas context's
current path in global space (as describe in the spec).  This way we never
actually need to invert the CTM for the purpose of building the current path.

In many cases Path primitives can be added added directly to global space by
transforming the point parameters of the primitive (works for lines, quadratic
curves and beziers). The difficulty I encountered while implementing the
solution is that some primitives need to be constructed in local space, then
transformed to global space (arcs, ellipses). Therefore, the primitives need to
be constructed into a separate SkPath, which is subsequently transformed and
joined to the current path.  This joining of paths proved problematic because
the current addPath implementation does not extend contours.

I have a new layout test in my WIP Blink change. I verified that FireFox is
already doing the right thing. According a thread on whatwg, only webkit (and
now blink) based browsers have this bug, but I have not personally verified IE
yet.

[Justin Novosad, 2014-02-11 19:56:20]

On 2014/02/11 19:54:52, junov wrote:
> On 2014/02/11 19:05:59, caryclark wrote:
> > Is there a reference canvas test that shows whether other browsers also
> > implement the proper behavior for the spec and for these edge cases?
> 
> To give more context, the bug I am fixing has to do with the fact that paths
> misbehave when a 2D canvas has a non-invertible CTM. Right now, not only does
> Chrome not early-out when the CTM is non-invertibele, but it scraps all the
path
> primitive that were recorded before the matrix became singular, which is a
> significant bug.  This solution is to avoid bring previously recorded
primitives
> into the local coordinate space, and to instead retain the canvas context's
> current path in global space (as describe in the spec).  This way we never
> actually need to invert the CTM for the purpose of building the current path.
> 
> In many cases Path primitives can be added added directly to global space by
> transforming the point parameters of the primitive (works for lines, quadratic
> curves and beziers). The difficulty I encountered while implementing the
> solution is that some primitives need to be constructed in local space, then
> transformed to global space (arcs, ellipses). Therefore, the primitives need
to
> be constructed into a separate SkPath, which is subsequently transformed and
> joined to the current path.  This joining of paths proved problematic because
> the current addPath implementation does not extend contours.
> 
> I have a new layout test in my WIP Blink change. I verified that FireFox is
> already doing the right thing. According a thread on whatwg, only webkit (and
> now blink) based browsers have this bug, but I have not personally verified IE
> yet.

Typo: "not only does Chrome not early-out when the CTM is non-invertible"
I meant: "not only does Chrome early-out"

[Justin Novosad, 2014-02-11 20:57:05]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-02-11 20:57:14]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/junov@chromium.org/151353006/390001

[commit-bot: I haz the power, 2014-02-11 21:16:32]

Change committed as 13415