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

RESOLVED FIXED in mozilla21

Status

()

Core
Canvas: 2D
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Rik Cabanier, Assigned: Rik Cabanier)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
(Assignee)

Comment 1

5 years ago
Created attachment 698346 [details] [diff] [review]
Add support for eofill and eoclip + tests the feature
(Assignee)

Comment 2

5 years ago
Corresponding webkit bug: https://bugs.webkit.org/show_bug.cgi?id=106188
(Assignee)

Comment 3

5 years ago
Created attachment 698389 [details] [diff] [review]
Add support for eofill and eoclip + isPointInPath + tests the feature
Attachment #698346 - Attachment is obsolete: true
Attachment #698389 - Flags: review?
Attachment #698389 - Flags: feedback?
(Assignee)

Updated

5 years ago
Summary: Add support for EOFill and EOClip to canvas → Add support for EOFill and EOClip to canvas + support for winding in isPointInPath
FYI, fillRule was introduced to the HTML5 spec, see https://www.w3.org/Bugs/Public/show_bug.cgi?id=19932 for details
(Assignee)

Comment 5

5 years ago
(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 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.
Attachment #698389 - Flags: review?(bas)
Attachment #698389 - Flags: review?
Attachment #698389 - Flags: feedback?
Attachment #698389 - Flags: feedback+
Attachment #698389 - Flags: feedback?(jmuizelaar)
(Assignee)

Comment 7

5 years ago
(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
(Assignee)

Comment 8

5 years ago
Created attachment 699474 [details] [diff] [review]
Add support for eofill and eoclip + isPointInPath + tests the feature
Attachment #698389 - Attachment is obsolete: true
Attachment #698389 - Flags: review?(bas)
Attachment #698389 - Flags: feedback?(jmuizelaar)
Attachment #699474 - Flags: review?
Attachment #699474 - Flags: review?(bas)
Attachment #699474 - Flags: review?
Attachment #699474 - Flags: feedback?(jmuizelaar)
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
Attachment #699474 - Flags: review?(bas) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 700197 [details] [diff] [review]
Add support for winding in fill + clip + isPointInPath + tests the feature
Attachment #699474 - Attachment is obsolete: true
Attachment #699474 - Flags: feedback?(jmuizelaar)
(Assignee)

Updated

5 years ago
Attachment #700197 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 700199 [details] [diff] [review]
Add support for winding in fill + clip + isPointInPath + tests the feature
(Assignee)

Comment 12

5 years ago
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.
Attachment #700199 - Attachment is obsolete: true
>+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...
(Assignee)

Comment 14

5 years ago
(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.
That, I don't know.  My graphics API fu is weak; I just do DOM bindings. ;)
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"});
Assignee: nobody → cabanier
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 17

5 years ago
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.
Attachment #700202 - Attachment is obsolete: true
Attachment #700491 - Flags: review?(bas)
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Comment 20

5 years ago
(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?
(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?
(Assignee)

Comment 22

5 years ago
(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.
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.
(Assignee)

Comment 24

5 years ago
(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?
I didn't expect fill() and isPointInPath() would ever have an additional argument :)
Attachment #700491 - Flags: review?(bas) → review+
(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.
(Assignee)

Comment 27

5 years ago
(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)
Don't do what?
(Assignee)

Comment 29

5 years ago
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.
Well, that's what the current spec says. Would like to see more discussion on the mailing list first before changing anything.
(Assignee)

Comment 31

5 years ago
I agree. People don't seem engaged so it would be great if someone from Mozilla participated.
(Assignee)

Updated

5 years ago
Attachment #700491 - Flags: checkin?
(Assignee)

Updated

5 years ago
Attachment #700491 - Flags: checkin? → checkin?(ryanvm)
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #700491 - Flags: checkin?(ryanvm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/64450d6fee96
https://hg.mozilla.org/mozilla-central/rev/64450d6fee96
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(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. :)
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?
(Assignee)

Comment 37

5 years ago
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
  }"
You need to log in before you can comment on or make changes to this bug.