Last Comment Bug 827053 - Add support for EOFill and EOClip to canvas + support for winding in isPointInPath
: Add support for EOFill and EOClip to canvas + support for winding in isPointI...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Rik Cabanier
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-05 14:25 PST by Rik Cabanier
Modified: 2013-01-17 09:56 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add support for eofill and eoclip + tests the feature (6.44 KB, patch)
2013-01-05 15:10 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support for eofill and eoclip + isPointInPath + tests the feature (8.75 KB, patch)
2013-01-05 21:51 PST, Rik Cabanier
bzbarsky: feedback+
Details | Diff | Splinter Review
Add support for eofill and eoclip + isPointInPath + tests the feature (8.74 KB, patch)
2013-01-08 15:30 PST, Rik Cabanier
bas: review+
Details | Diff | Splinter Review
Add support for winding in fill + clip + isPointInPath + tests the feature (10.09 KB, patch)
2013-01-09 21:03 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support for winding in fill + clip + isPointInPath + tests the feature (9.87 KB, patch)
2013-01-09 21:16 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support for winding in fill + clip + isPointInPath + tests the feature (9.94 KB, patch)
2013-01-09 21:23 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support for winding in fill + clip + isPointInPath + tests the feature (9.70 KB, patch)
2013-01-10 09:23 PST, Rik Cabanier
bas: review+
Details | Diff | Splinter Review

Description Rik Cabanier 2013-01-05 14:25:02 PST
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.101 Safari/537.11

Steps to reproduce:

Mozilla current has support for winding through the mozFillRule.
I propose to add 2 calls that set the winding rule internally since that is how most graphic libraries are implemented.

This will make things easier later since paths, hit testing and regions will need to be aware of winding too.
In addition, making the winding part of the clip and fill negates the needs for extra calls to set/unset the winding rule
Comment 1 Rik Cabanier 2013-01-05 15:10:15 PST
Created attachment 698346 [details] [diff] [review]
Add support for eofill and eoclip + tests the feature
Comment 2 Rik Cabanier 2013-01-05 16:22:59 PST
Corresponding webkit bug: https://bugs.webkit.org/show_bug.cgi?id=106188
Comment 3 Rik Cabanier 2013-01-05 21:51:28 PST
Created attachment 698389 [details] [diff] [review]
Add support for eofill and eoclip + isPointInPath + tests the feature
Comment 4 Yury Delendik (:yury) 2013-01-07 15:39:07 PST
FYI, fillRule was introduced to the HTML5 spec, see https://www.w3.org/Bugs/Public/show_bug.cgi?id=19932 for details
Comment 5 Rik Cabanier 2013-01-07 16:56:43 PST
(In reply to Yury (:yury) from comment #4)
> FYI, fillRule was introduced to the HTML5 spec, see
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=19932 for details

Adding fillrule to the graphics state is not the correct place. Not only does it also apply to clipping, it will create issues when we add proper support for paths later.
Comment 6 Boris Zbarsky [:bz] 2013-01-08 11:23:09 PST
Comment on attachment 698389 [details] [diff] [review]
Add support for eofill and eoclip + isPointInPath + tests the feature

Trying for an actual reviewer.

On the binding bit, I think it would make more sense to make the WebIDL look like so:

  boolean isPointInPath(unrestricted double x, unrestricted double y,
                        optional boolean isEvenOdd = false);

Then you won't need Optional<> in the C++; you'll just get a boolean you can work with.
Comment 7 Rik Cabanier 2013-01-08 13:36:09 PST
(In reply to Boris Zbarsky (:bz) from comment #6)
> Comment on attachment 698389 [details] [diff] [review]
> Add support for eofill and eoclip + isPointInPath + tests the feature
> 
> Trying for an actual reviewer.
> 
> On the binding bit, I think it would make more sense to make the WebIDL look
> like so:
> 
>   boolean isPointInPath(unrestricted double x, unrestricted double y,
>                         optional boolean isEvenOdd = false);
> 
> Then you won't need Optional<> in the C++; you'll just get a boolean you can
> work with.

Good point!
will do
Comment 8 Rik Cabanier 2013-01-08 15:30:49 PST
Created attachment 699474 [details] [diff] [review]
Add support for eofill and eoclip + isPointInPath + tests the feature
Comment 9 Bas Schouten (:bas.schouten) 2013-01-09 07:40:00 PST
Comment on attachment 699474 [details] [diff] [review]
Add support for eofill and eoclip + isPointInPath + tests the feature

Review of attachment 699474 [details] [diff] [review]:
-----------------------------------------------------------------

Not too pretty :) But looks like it does the job to me!

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2858,5 @@
>  {
>    if (!FloatValidate(x,y)) {
>      return false;
>    }
> +  

nit: whitespace

@@ +2876,2 @@
>    }
> +  

idem

@@ +2876,4 @@
>    }
> +  
> +  CurrentState().fillRule = fillRule;
> +  

idem

::: content/canvas/test/test_2d.isPointInPath.winding.html
@@ +15,5 @@
> +ctx.beginPath();
> +ctx.rect(0, 0, 100, 100);
> +ctx.rect(25, 25, 50, 50);
> +ok(ctx.isPointInPath(50, 50));             
> +

nit: whitespace

@@ +19,5 @@
> +
> +ctx.beginPath();
> +ctx.rect(0, 0, 100, 100);
> +ctx.rect(25, 25, 50, 50);
> +ok(ctx.isPointInPath(50, 50, true) == false);  

idem
Comment 10 Rik Cabanier 2013-01-09 21:03:32 PST
Created attachment 700197 [details] [diff] [review]
Add support for winding in fill + clip + isPointInPath + tests the feature
Comment 11 Rik Cabanier 2013-01-09 21:16:35 PST
Created attachment 700199 [details] [diff] [review]
Add support for winding in fill + clip + isPointInPath + tests the feature
Comment 12 Rik Cabanier 2013-01-09 21:23:27 PST
Created attachment 700202 [details] [diff] [review]
Add support for winding in fill + clip + isPointInPath + tests the feature

not quite ready for review.
I want to make sure that the enums don't need to be uppercased.
Comment 13 Boris Zbarsky [:bz] 2013-01-09 21:44:35 PST
>+CanvasRenderingContext2D::Clip(const mozilla::dom::CanvasWindingRuleValues::valuelist& 

How about:

  CanvasRenderingContext2D::Clip(CanvasWindingRule winding)

instead?  All this stuff is in the mozilla::dom namespace, and the name of the IDL enum is a typedef for the actual C++ value enum, precisely so the code can end up looking sane.  Or at least that's the hope.  I updated https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Enumeration_types to describe this clearly, since it was actually out of date before...
Comment 14 Rik Cabanier 2013-01-09 23:06:36 PST
(In reply to Boris Zbarsky (:bz) from comment #13)
> >+CanvasRenderingContext2D::Clip(const mozilla::dom::CanvasWindingRuleValues::valuelist& 
> 
> How about:
> 
>   CanvasRenderingContext2D::Clip(CanvasWindingRule winding)
> 
> instead?  All this stuff is in the mozilla::dom namespace, and the name of
> the IDL enum is a typedef for the actual C++ value enum, precisely so the
> code can end up looking sane.  Or at least that's the hope.  I updated
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Enumeration_types to describe this clearly, since it was
> actually out of date before...

Will do! Thanks for the feedback.
Do you think it should be evenodd/nonzero or evenOdd/nonZero?
It seems the idl interfaces are inconsistent and Ian had them as lower case.
Comment 15 Boris Zbarsky [:bz] 2013-01-09 23:36:25 PST
That, I don't know.  My graphics API fu is weak; I just do DOM bindings. ;)
Comment 16 Masatoshi Kimura [:emk] 2013-01-10 01:05:26 PST
I think WebIDL should support case-insensitive enum type.
>+  void fill(optional CanvasWindingRule winding = "nonzero");
New options would never be added? What about using a dictionary such as:
  dictionary CanvasFillOptions {
    CanvasWindingRule winding;
  };
Usage example:
  ctx.fill({winding: "evenodd"});
Comment 17 Rik Cabanier 2013-01-10 09:23:06 PST
Created attachment 700491 [details] [diff] [review]
Add support for winding in fill + clip + isPointInPath + tests the feature

After more discussion, the API was changed slightly.
With the new API, the patch applies much cleaner.
Comment 18 Rik Cabanier 2013-01-10 09:24:09 PST
(In reply to Boris Zbarsky (:bz) from comment #15)
> That, I don't know.  My graphics API fu is weak; I just do DOM bindings. ;)

Mozilla's WebIDL framework is very nice! I changed the code to use the typedef.
Comment 19 Rik Cabanier 2013-01-10 09:25:11 PST
(In reply to Masatoshi Kimura [:emk] from comment #16)
> I think WebIDL should support case-insensitive enum type.
> >+  void fill(optional CanvasWindingRule winding = "nonzero");
> New options would never be added? What about using a dictionary such as:
>   dictionary CanvasFillOptions {
>     CanvasWindingRule winding;
>   };
> Usage example:
>   ctx.fill({winding: "evenodd"});

I don't know of any other rules for filling so a dictionary might be a bit over the top. Passing a composite object will surely make things slower.
Comment 20 Rik Cabanier 2013-01-10 12:50:29 PST
(In reply to Bas Schouten (:bas.schouten) from comment #9)
> Comment on attachment 699474 [details] [diff] [review]
> Add support for eofill and eoclip + isPointInPath + tests the feature
> 
> Review of attachment 699474 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not too pretty :) But looks like it does the job to me!
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +2858,5 @@
> >  {
> >    if (!FloatValidate(x,y)) {
> >      return false;
> >    }
> > +  
> 
> nit: whitespace
> 
> @@ +2876,2 @@
> >    }
> > +  
> 
> idem
> 
> @@ +2876,4 @@
> >    }
> > +  
> > +  CurrentState().fillRule = fillRule;
> > +  
> 
> idem
> 
> ::: content/canvas/test/test_2d.isPointInPath.winding.html
> @@ +15,5 @@
> > +ctx.beginPath();
> > +ctx.rect(0, 0, 100, 100);
> > +ctx.rect(25, 25, 50, 50);
> > +ok(ctx.isPointInPath(50, 50));             
> > +
> 
> nit: whitespace
> 
> @@ +19,5 @@
> > +
> > +ctx.beginPath();
> > +ctx.rect(0, 0, 100, 100);
> > +ctx.rect(25, 25, 50, 50);
> > +ok(ctx.isPointInPath(50, 50, true) == false);  
> 
> idem

Thanks!
I fixed the whitespace issues.

With the new spec that put the winding rule in the fill or clip, the code looks much better,
Can you review again?
Comment 21 Masatoshi Kimura [:emk] 2013-01-10 14:55:35 PST
(In reply to Rik Cabanier from comment #19)
> I don't know of any other rules for filling so a dictionary might be a bit
> over the top.

See the discussion about the second parameter of createObjectURL, for example.
http://lists.w3.org/Archives/Public/public-webapps/2012JanMar/0349.html

> Passing a composite object will surely make things slower.

Have you measured?
Comment 22 Rik Cabanier 2013-01-10 14:58:59 PST
(In reply to Masatoshi Kimura [:emk] from comment #21)
> (In reply to Rik Cabanier from comment #19)
> > I don't know of any other rules for filling so a dictionary might be a bit
> > over the top.
> 
> See the discussion about the second parameter of createObjectURL, for
> example.
> http://lists.w3.org/Archives/Public/public-webapps/2012JanMar/0349.html

Yes, that is why it's no longer a boolean but an enum.
(Tab Atkins agreed to the notation)

> 
> > Passing a composite object will surely make things slower.
> 
> Have you measured?

I did not.
Boris did a quick test and found a trivial amount of slowdown with a simple string. A dictionary seems much slower though.
Comment 23 Boris Zbarsky [:bz] 2013-01-10 17:36:52 PST
I could measure that in Gecko pretty easily (just make up an API that has a dictionary argument).  But I do expect it to be a good bit slower just because of how much more work it has to do...  Property gets on JS objects are not cheap.
Comment 24 Rik Cabanier 2013-01-10 19:31:04 PST
(In reply to Boris Zbarsky (:bz) from comment #23)
> I could measure that in Gecko pretty easily (just make up an API that has a
> dictionary argument).  But I do expect it to be a good bit slower just
> because of how much more work it has to do...  Property gets on JS objects
> are not cheap.

Yes, unless there is a good reason, I would not recommend a dictionary.
Masatoshi do you know any reason why a fill would have more then 1 parameter?
Comment 25 Masatoshi Kimura [:emk] 2013-01-10 19:45:01 PST
I didn't expect fill() and isPointInPath() would ever have an additional argument :)
Comment 26 Tobias Schneider [:tobytailor] 2013-01-16 02:28:53 PST
(In reply to Masatoshi Kimura [:emk] from comment #25)
> I didn't expect fill() and isPointInPath() would ever have an additional
> argument :)

Well, once we consider adding support for Path primitives (which I'm already working on in https://bugzilla.mozilla.org/show_bug.cgi?id=830734), there will be an optional path argument for fill, stroke, clip and isPointInPath/Stroke.
Comment 27 Rik Cabanier 2013-01-16 09:53:15 PST
(In reply to Tobias Schneider from comment #26)
> (In reply to Masatoshi Kimura [:emk] from comment #25)
> > I didn't expect fill() and isPointInPath() would ever have an additional
> > argument :)
> 
> Well, once we consider adding support for Path primitives (which I'm already
> working on in https://bugzilla.mozilla.org/show_bug.cgi?id=830734), there
> will be an optional path argument for fill, stroke, clip and
> isPointInPath/Stroke.

Please don't do that. 
The Path object should contain the winding rule (just like it contains all stroking and text parameters)
Comment 28 Tobias Schneider [:tobytailor] 2013-01-16 09:58:44 PST
Don't do what?
Comment 29 Rik Cabanier 2013-01-16 10:02:55 PST
Don't add an optional path argument.
There should be a new fill that takes just a path.
The new isPointInPath should be on the path object itself, not the canvas.
Comment 30 Tobias Schneider [:tobytailor] 2013-01-16 10:09:33 PST
Well, that's what the current spec says. Would like to see more discussion on the mailing list first before changing anything.
Comment 31 Rik Cabanier 2013-01-16 10:48:07 PST
I agree. People don't seem engaged so it would be great if someone from Mozilla participated.
Comment 32 Ryan VanderMeulen [:RyanVM] 2013-01-16 18:51:39 PST
Comment on attachment 700491 [details] [diff] [review]
Add support for winding in fill + clip + isPointInPath + tests the feature

Please use checkin-needed in the future.
Comment 33 Ryan VanderMeulen [:RyanVM] 2013-01-16 18:56:49 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/64450d6fee96
Comment 34 Ed Morley [:emorley] 2013-01-17 02:50:32 PST
https://hg.mozilla.org/mozilla-central/rev/64450d6fee96
Comment 35 Tobias Schneider [:tobytailor] 2013-01-17 04:16:19 PST
(In reply to Rik Cabanier from comment #29)
> Don't add an optional path argument.
> There should be a new fill that takes just a path.

Yeah, you're right. That's actually how I've implemented it. :)
Comment 36 Yury Delendik (:yury) 2013-01-17 06:09:46 PST
We would need some docs for that since the spec does not define it this way: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#drawing-paths-to-the-canvas

Rik, how would I detect if optional winding parameter is supported by browser?
Comment 37 Rik Cabanier 2013-01-17 09:56:09 PST
You can use 'isPointInPath' with the 'evenOdd' parameter to see if it's being honored. (draw a big rectangle and a small rectangle that's inside the first one. Then see if isPointInPath returns false if you specific 'eofill' and a point in the small rectangle)

Also, Boris Zbarsky had the following remark:
  "That said, you can detect this via hacks like this, I expect:

  var ruleArgSupported = false;
  try {
    ctx.fill({ defaultValue: function() { ruleArgSupported = true; return false; } });
  } catch (e) {
    // Really shouldn't happen, but who knows
  }"

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