Bug 106188 - Proposal: Add support for even-odd fill and clip to Canvas
: Proposal: Add support for even-odd fill and clip to Canvas
Status: RESOLVED FIXED
: WebKit
Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 106871 106872 106873 107065
:
  Show dependency treegraph
 
Reported: 2013-01-05 16:24 PST by
Modified: 2013-01-21 13:06 PST (History)


Attachments
Patch (10.83 KB, patch)
2013-01-05 16:36 PST, Rik Cabanier
no flags Review Patch | Details | Formatted Diff | Diff
Added support for optional winding rule in isPointInPath (15.04 KB, patch)
2013-01-05 17:08 PST, Rik Cabanier
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.63 KB, patch)
2013-01-06 21:06 PST, Rik Cabanier
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.03 KB, text/plain)
2013-01-10 11:50 PST, Rik Cabanier
no flags Details
Patch (18.03 KB, patch)
2013-01-10 11:59 PST, Rik Cabanier
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.08 KB, patch)
2013-01-17 23:29 PST, Rik Cabanier
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.18 KB, text/plain)
2013-01-18 10:13 PST, Rik Cabanier
no flags Details
Patch (20.21 KB, patch)
2013-01-18 10:20 PST, Rik Cabanier
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.97 KB, patch)
2013-01-19 19:27 PST, Rik Cabanier
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.99 KB, patch)
2013-01-19 19:54 PST, Rik Cabanier
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2013-01-05 16:24:50 PST
This is a counterproposal for 105508.
This patch adds 2 new methods to the Canvas API that allow you to set the winding rule that needs to be applied when filling or clipping.

I believe the underlying code can be cleaned up since there is no need to store the fillrule separately. That can be addressed in a follow-up patch.

I also added a patch for mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=827053
------- Comment #1 From 2013-01-05 16:36:35 PST -------
Created an attachment (id=181447) [details]
Patch
------- Comment #2 From 2013-01-05 17:08:28 PST -------
Created an attachment (id=181448) [details]
Patch
------- Comment #3 From 2013-01-05 18:12:15 PST -------
(From update of attachment 181448 [details])
Attachment 181448 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15738268

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html
------- Comment #4 From 2013-01-05 18:22:15 PST -------
(From update of attachment 181448 [details])
Attachment 181448 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15734297

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html
------- Comment #5 From 2013-01-05 18:33:47 PST -------
(From update of attachment 181448 [details])
Attachment 181448 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15737280

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html
------- Comment #6 From 2013-01-06 21:06:02 PST -------
Created an attachment (id=181471) [details]
Patch
------- Comment #7 From 2013-01-08 14:19:32 PST -------
(From update of attachment 181471 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=181471&action=review

You can not ask to stop the review process for discussion and put an own patch online that does what you want (without keeping the thread on webkit-dev in the loop). It is everything else then obvious what eoFill() and eoClip() mean. I strongly suggest another name, better another way to do it. Do not increase the number of functions on Canvas unnecessarily. I am unsetting the review flag for now. This should be discussed on the mailing list further.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:154
> +    bool isPointInPath(const float x, const float y, const bool isEvenOdd);

Use enumeration here. We have an enumeration for winding rules in WebCore already. It should be exposed to the API as well.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:134
>      boolean isPointInPath(in [Optional=DefaultIsUndefined] float x,
> -                          in [Optional=DefaultIsUndefined] float y);
> +                          in [Optional=DefaultIsUndefined] float y,
> +                          in [Optional=DefaultIsUndefined] boolean isEvenOdd);

don't use an boolean for something that isn't so obvious for people who are not familiar with winding rules.

> LayoutTests/ChangeLog:8
> +        Add tests to verify that the winding rule work as expected

s/Add tests to verify that the winding rule work as expected/Add tests to verify that the winding rule work as expected on HTML Canvas./

The tests look fine for me.

> LayoutTests/fast/canvas/script-tests/canvas-clip-rule.js:54
> +prepareTestScenario(5);

Didn't know this function. What is the test scenario 5?
------- Comment #8 From 2013-01-08 14:41:15 PST -------
(In reply to comment #7)
> (From update of attachment 181471 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181471&action=review
> 
> You can not ask to stop the review process for discussion and put an own patch online that does what you want (without keeping the thread on webkit-dev in the loop). It is everything else then obvious what eoFill() and eoClip() mean. I strongly suggest another name, better another way to do it. Do not increase the number of functions on Canvas unnecessarily. 

So far, canvas has been pretty close to the Core Graphics API set. Since CG has these same methods, it seems more logical to do the same in Canvas


> I am unsetting the review flag for now. This should be discussed on the mailing list further.

That's fine. I'm just looking for feedback. Note that I didn't ask to commit the patch.

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:154
> > +    bool isPointInPath(const float x, const float y, const bool isEvenOdd);
> 
> Use enumeration here. We have an enumeration for winding rules in WebCore already. It should be exposed to the API as well.

Canvas has other instances where it uses boolean (ie clockwise).
Why not do the same here?

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:134
> >      boolean isPointInPath(in [Optional=DefaultIsUndefined] float x,
> > -                          in [Optional=DefaultIsUndefined] float y);
> > +                          in [Optional=DefaultIsUndefined] float y,
> > +                          in [Optional=DefaultIsUndefined] boolean isEvenOdd);
> 
> don't use an boolean for something that isn't so obvious for people who are not familiar with winding rules.

How would an enum help people that don't know about winding rules?

> 
> > LayoutTests/ChangeLog:8
> > +        Add tests to verify that the winding rule work as expected
> 
> s/Add tests to verify that the winding rule work as expected/Add tests to verify that the winding rule work as expected on HTML Canvas./
> 
> The tests look fine for me.
> 
> > LayoutTests/fast/canvas/script-tests/canvas-clip-rule.js:54
> > +prepareTestScenario(5);
> 
> Didn't know this function. What is the test scenario 5?

that was a remnant of an old test. I will remove this.
------- Comment #9 From 2013-01-08 21:27:24 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 181471 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181471&action=review
> > 
> > You can not ask to stop the review process for discussion and put an own patch online that does what you want (without keeping the thread on webkit-dev in the loop). It is everything else then obvious what eoFill() and eoClip() mean. I strongly suggest another name, better another way to do it. Do not increase the number of functions on Canvas unnecessarily. 
> 
> So far, canvas has been pretty close to the Core Graphics API set. Since CG has these same methods, it seems more logical to do the same in Canvas

That is not a good reason at all :)

> 
> 
> > I am unsetting the review flag for now. This should be discussed on the mailing list further.
> 
> That's fine. I'm just looking for feedback. Note that I didn't ask to commit the patch.
> 
> > 
> > > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:154
> > > +    bool isPointInPath(const float x, const float y, const bool isEvenOdd);
> > 
> > Use enumeration here. We have an enumeration for winding rules in WebCore already. It should be exposed to the API as well.
> 
> Canvas has other instances where it uses boolean (ie clockwise).
> Why not do the same here?

Mistakes of the past maybe. Doesn't mean that it can't be done the right way here.

> 
> > 
> > > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:134
> > >      boolean isPointInPath(in [Optional=DefaultIsUndefined] float x,
> > > -                          in [Optional=DefaultIsUndefined] float y);
> > > +                          in [Optional=DefaultIsUndefined] float y,
> > > +                          in [Optional=DefaultIsUndefined] boolean isEvenOdd);
> > 
> > don't use an boolean for something that isn't so obvious for people who are not familiar with winding rules.
> 
> How would an enum help people that don't know about winding rules?

How would a boolean help anymore then? The question is, how do people (who are familiar with winding rules) know that 'false' is 'nonzero' and 'true' is 'evenodd'?

> 
> > 
> > > LayoutTests/ChangeLog:8
> > > +        Add tests to verify that the winding rule work as expected
> > 
> > s/Add tests to verify that the winding rule work as expected/Add tests to verify that the winding rule work as expected on HTML Canvas./
> > 
> > The tests look fine for me.
> > 
> > > LayoutTests/fast/canvas/script-tests/canvas-clip-rule.js:54
> > > +prepareTestScenario(5);
> > 
> > Didn't know this function. What is the test scenario 5?
> 
> that was a remnant of an old test. I will remove this.

Ok.
------- Comment #10 From 2013-01-09 10:57:23 PST -------
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 181471 [details] [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=181471&action=review
> > > 
> > > You can not ask to stop the review process for discussion and put an own patch online that does what you want (without keeping the thread on webkit-dev in the loop). It is everything else then obvious what eoFill() and eoClip() mean. I strongly suggest another name, better another way to do it. Do not increase the number of functions on Canvas unnecessarily. 
> > 
> > So far, canvas has been pretty close to the Core Graphics API set. Since CG has these same methods, it seems more logical to do the same in Canvas
> 
> That is not a good reason at all :)

There seems to be a consensus to go with parameters instead of an optional call. I will update the patch accordingly.

> 
> > 
> > 
> > > I am unsetting the review flag for now. This should be discussed on the mailing list further.
> > 
> > That's fine. I'm just looking for feedback. Note that I didn't ask to commit the patch.
> > 
> > > 
> > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:154
> > > > +    bool isPointInPath(const float x, const float y, const bool isEvenOdd);
> > > 
> > > Use enumeration here. We have an enumeration for winding rules in WebCore already. It should be exposed to the API as well.
> > 
> > Canvas has other instances where it uses boolean (ie clockwise).
> > Why not do the same here?
> 
> Mistakes of the past maybe. Doesn't mean that it can't be done the right way here.
> 
> > 
> > > 
> > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:134
> > > >      boolean isPointInPath(in [Optional=DefaultIsUndefined] float x,
> > > > -                          in [Optional=DefaultIsUndefined] float y);
> > > > +                          in [Optional=DefaultIsUndefined] float y,
> > > > +                          in [Optional=DefaultIsUndefined] boolean isEvenOdd);
> > > 
> > > don't use an boolean for something that isn't so obvious for people who are not familiar with winding rules.
> > 
> > How would an enum help people that don't know about winding rules?
> 
> How would a boolean help anymore then? The question is, how do people (who are familiar with winding rules) know that 'false' is 'nonzero' and 'true' is 'evenodd'?
> 
> > 
> > > 
> > > > LayoutTests/ChangeLog:8
> > > > +        Add tests to verify that the winding rule work as expected
> > > 
> > > s/Add tests to verify that the winding rule work as expected/Add tests to verify that the winding rule work as expected on HTML Canvas./
> > > 
> > > The tests look fine for me.
> > > 
> > > > LayoutTests/fast/canvas/script-tests/canvas-clip-rule.js:54
> > > > +prepareTestScenario(5);
> > > 
> > > Didn't know this function. What is the test scenario 5?
> > 
> > that was a remnant of an old test. I will remove this.
> 
> Ok.
------- Comment #11 From 2013-01-09 11:37:44 PST -------
Isn't there still the open question on whatwg list if the argument should be Boolean or enum? You may want to wait for the outcome.
------- Comment #12 From 2013-01-10 11:50:56 PST -------
Created an attachment (id=182181) [details]
Patch
------- Comment #13 From 2013-01-10 11:59:13 PST -------
Created an attachment (id=182185) [details]
Patch
------- Comment #14 From 2013-01-10 13:25:26 PST -------
(From update of attachment 182185 [details])
Attachment 182185 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15803089

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-clip-rule.html
fast/canvas/canvas-clip-rule.html
------- Comment #15 From 2013-01-10 13:57:24 PST -------
(From update of attachment 182185 [details])
Attachment 182185 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15811015

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-clip-rule.html
fast/canvas/canvas-clip-rule.html
------- Comment #16 From 2013-01-17 23:29:55 PST -------
Created an attachment (id=183387) [details]
Patch
------- Comment #17 From 2013-01-18 00:40:22 PST -------
(From update of attachment 183387 [details])
Attachment 183387 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15943416

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
------- Comment #18 From 2013-01-18 07:33:37 PST -------
(In reply to comment #17)
> (From update of attachment 183387 [details] [details])
> Attachment 183387 [details] [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15943416
> 
> New failing tests:
> inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

Doesn't sound like this is because of this patch. Can you investigate please?
------- Comment #19 From 2013-01-18 07:44:35 PST -------
(From update of attachment 183387 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=183387&action=review

Some snippets and questions.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1027
> +void CanvasRenderingContext2D::fill(const String& winding)

can you rename this please? To windingRuleString or windingRuleEnumeration? (at this time it is the first, so maybe choose this one).

Since you pass a value, doesn't it make sense to raise exceptions now? I would pass an EC object here and say that the passed string was invalid.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1083
> +void CanvasRenderingContext2D::clip(const String& winding)

Ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1103
> +bool CanvasRenderingContext2D::isPointInPath(const float x, const float y, const String& winding)

Ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1161
> +bool CanvasRenderingContext2D::parseWinding(const String& winding, WindRule& windRule)

Ditto. Can you make this an inline function? Doesn't look like it needs to be in the class.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:26
> +enum CanvasWindingRule { "nonzero", "evenodd" };

This enumeration doesn't look like it is used. I guess you added it since it should be used as WindingRule according to WebIDL and the Firefox implementation. Can you add a comment or fixme that this should be used once we support WebIDL completely?

> LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:6
> +Testing NZO isPointInPath

please use the enumeration value. Don't be lazy :)

> LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:9
> +Testing NZO isPointInPath

Ditto.

> LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:10
> +PASS ctx.isPointInPath(50, 50, 'evenodd') is false

Is the above text correct for this test?

I would also like to see a bunch of negative tests in a separate file. At least one test seems to be missing in this file: isPointInPath(50, 50, 'nonzero')
------- Comment #20 From 2013-01-18 09:55:44 PST -------
(In reply to comment #19)
> (From update of attachment 183387 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183387&action=review
> 
> Some snippets and questions.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1027
> > +void CanvasRenderingContext2D::fill(const String& winding)
> 
> can you rename this please? To windingRuleString or windingRuleEnumeration? (at this time it is the first, so maybe choose this one).
> 
> Since you pass a value, doesn't it make sense to raise exceptions now? I would pass an EC object here and say that the passed string was invalid.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1083
> > +void CanvasRenderingContext2D::clip(const String& winding)
> 
> Ditto.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1103
> > +bool CanvasRenderingContext2D::isPointInPath(const float x, const float y, const String& winding)
> 
> Ditto.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1161
> > +bool CanvasRenderingContext2D::parseWinding(const String& winding, WindRule& windRule)
> 
> Ditto. Can you make this an inline function? Doesn't look like it needs to be in the class.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:26
> > +enum CanvasWindingRule { "nonzero", "evenodd" };
> 
> This enumeration doesn't look like it is used. I guess you added it since it should be used as WindingRule according to WebIDL and the Firefox implementation. Can you add a comment or fixme that this should be used once we support WebIDL completely?

Will do all of the above.

> 
> > LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:6
> > +Testing NZO isPointInPath
> 
> please use the enumeration value. Don't be lazy :)

I will add a case for 'nonzero' to all 3 tests.
No parameter needs to be tested too since that will be the most common case.

> 
> > LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:9
> > +Testing NZO isPointInPath
> 
> Ditto.
> 
> > LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:10
> > +PASS ctx.isPointInPath(50, 50, 'evenodd') is false
> 
> Is the above text correct for this test?

Yes. With even-odd, there's a hole in the path so isPointInPath should return false. 

> 
> I would also like to see a bunch of negative tests in a separate file. At least one test seems to be missing in this file: isPointInPath(50, 50, 'nonzero')

what's a negative test?
------- Comment #21 From 2013-01-18 10:13:54 PST -------
Created an attachment (id=183494) [details]
Patch
------- Comment #22 From 2013-01-18 10:14:42 PST -------
(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 183387 [details] [details] [details])
> > Attachment 183387 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/15943416
> > 
> > New failing tests:
> > inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
> 
> Doesn't sound like this is because of this patch. Can you investigate please?

I ran the test locally and it didn't crash. Flakiness?
------- Comment #23 From 2013-01-18 10:20:51 PST -------
Created an attachment (id=183496) [details]
Patch
------- Comment #24 From 2013-01-19 11:21:57 PST -------
(From update of attachment 183496 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=183496&action=review

Looks good. Just some snippets. Code and tests look great! r- since you are not committer yet. I would also like to wait for possible responses on webkit-dev before giving my final OK.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:287
> +    

remove white spaces here please.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:26
> +// FIXME: use CanvasWindingRule once support for WebIDL is completed

A real sentence please. We have a bug report for that. Can you link to this (need to search it)?

> LayoutTests/ChangeLog:36
> +2013-01-17  Rik Cabanier  <cabanier@adobe.com>
> +
> +        Proposal: Add support for even-odd fill and clip to Canvas
> +        https://bugs.webkit.org/show_bug.cgi?id=106188
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add tests to verify that the winding rule work as expected with clip, fill and isPointInPath
> +
> +        * fast/canvas/canvas-clip-rule-expected.txt: Added.
> +        * fast/canvas/canvas-clip-rule.html: Added.
> +        * fast/canvas/canvas-fill-rule-expected.txt: Added.

This is a duplicate. Please remove it.
------- Comment #25 From 2013-01-19 19:27:00 PST -------
Created an attachment (id=183652) [details]
Patch
------- Comment #26 From 2013-01-19 19:54:09 PST -------
Created an attachment (id=183654) [details]
Patch
------- Comment #27 From 2013-01-19 19:55:04 PST -------
(In reply to comment #24)
> (From update of attachment 183496 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183496&action=review
> 
> Looks good. Just some snippets. Code and tests look great! r- since you are not committer yet. I would also like to wait for possible responses on webkit-dev before giving my final OK.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:287
> > +    
> 
> remove white spaces here please.

Done

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:26
> > +// FIXME: use CanvasWindingRule once support for WebIDL is completed
> 
> A real sentence please. We have a bug report for that. Can you link to this (need to search it)?

Done. Found bug and added it to fixme

> 
> > LayoutTests/ChangeLog:36
> > +2013-01-17  Rik Cabanier  <cabanier@adobe.com>
> > +
> > +        Proposal: Add support for even-odd fill and clip to Canvas
> > +        https://bugs.webkit.org/show_bug.cgi?id=106188
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Add tests to verify that the winding rule work as expected with clip, fill and isPointInPath
> > +
> > +        * fast/canvas/canvas-clip-rule-expected.txt: Added.
> > +        * fast/canvas/canvas-clip-rule.html: Added.
> > +        * fast/canvas/canvas-fill-rule-expected.txt: Added.
> 
> This is a duplicate. Please remove it.

Strange! removed
------- Comment #28 From 2013-01-21 13:00:06 PST -------
(From update of attachment 183654 [details])
LGTM. r=me
------- Comment #29 From 2013-01-21 13:06:28 PST -------
(From update of attachment 183654 [details])
Clearing flags on attachment: 183654

Committed r140352: <http://trac.webkit.org/changeset/140352>
------- Comment #30 From 2013-01-21 13:06:37 PST -------
All reviewed patches have been landed.  Closing bug.