827053 - Add support for EOFill and EOClip to canvas + support for winding in isPointInPath

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Rik Cabanier]

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]

Attachment: Add support for eofill and eoclip + tests the feature

Comment 2

[Rik Cabanier]

Corresponding webkit bug: https://bugs.webkit.org/show_bug.cgi?id=106188

Comment 3

[Rik Cabanier]

Attachment: Add support for eofill and eoclip + isPointInPath + tests the feature

Comment 4

[Yury Delendik (:yury)]

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]

(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 [:bzbarsky]]

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]

(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]

Attachment: Add support for eofill and eoclip + isPointInPath + tests the feature

Comment 9

[Bas Schouten (:bas.schouten)]

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]

Attachment: Add support for winding in fill + clip + isPointInPath + tests the feature

Comment 11

[Rik Cabanier]

Attachment: Add support for winding in fill + clip + isPointInPath + tests the feature

Comment 12

[Rik Cabanier]

Attachment: 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 [:bzbarsky]]

>+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]

(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 [:bzbarsky]]

That, I don't know.  My graphics API fu is weak; I just do DOM bindings. ;)

Comment 16

[Masatoshi Kimura [:emk]]

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]

Attachment: 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]

(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]

(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]

(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]]

(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]

(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 [:bzbarsky]]

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]

(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]]

I didn't expect fill() and isPointInPath() would ever have an additional argument :)

Comment 26

[Tobias Schneider [:tobytailor]]

(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]

(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]]

Don't do what?

Comment 29

[Rik Cabanier]

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]]

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]

I agree. People don't seem engaged so it would be great if someone from Mozilla participated.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64450d6fee96

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/64450d6fee96

Comment 35

[Tobias Schneider [:tobytailor]]

(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)]

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]

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
  }"