Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to eoFIll and eoClip #2534

Closed
wants to merge 3 commits into from
Closed

Conversation

cabanier
Copy link

@cabanier cabanier commented Jan 7, 2013

Changed implementation so it now uses eoFill and eoClip instead of the fill rule

Changed implementation so it now uses eoFill and eoClip instead of the fill rule
@yurydelendik
Copy link
Contributor

@cabanier , I cannot find eoFill and eoClip in the spec at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#2dcontext , however I see fillRule there. Not sure what you are trying to do.

@cabanier
Copy link
Author

cabanier commented Jan 8, 2013

Hi yury,

Ian and I have a disagreement on how this should be implemented.
Every graphic library except cairo sets the winding on the fill or clip operator. (I’m trying to find out why cairo is different)
It also makes no sense to add it to the state as the winding rule is generally not reused between fills and clips. In addition, it’s very verbose and results in constant setting/resetting of the fill rule.

Rik

From: Yury Delendik [mailto:[email protected]]
Sent: Monday, January 07, 2013 3:28 PM
To: mozilla/pdf.js
Cc: Rik Cabanier
Subject: Re: [pdf.js] Switch to eoFIll and eoClip (#2534)

@cabanierhttps://github.com/cabanier , I cannot find eoFill and eoClip in the spec at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#2dcontext , however I see fillRule there. Not sure what you are trying to do.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2534#issuecomment-11977420.

@cabanier
Copy link
Author

cabanier commented Jan 8, 2013

For a longer reply, look at the recent discussion on webkit and whatwg mailing lists

From: Yury Delendik [mailto:[email protected]]
Sent: Monday, January 07, 2013 3:28 PM
To: mozilla/pdf.js
Cc: Rik Cabanier
Subject: Re: [pdf.js] Switch to eoFIll and eoClip (#2534)

@cabanierhttps://github.com/cabanier , I cannot find eoFill and eoClip in the spec at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#2dcontext , however I see fillRule there. Not sure what you are trying to do.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2534#issuecomment-11977420.

@yurydelendik
Copy link
Contributor

Okay, I see the https://bugs.webkit.org/show_bug.cgi?id=105508 conversation. Proposed fillRule attribute was based on SVG's fill-rule, which is "stateful" for children and it's useful in some cases.

@cabanier
Copy link
Author

cabanier commented Jan 8, 2013

I have patches for eofill and eoclip for webkit:
https://bugs.webkit.org/show_bug.cgi?id=106188
and mozilla:
https://bugzilla.mozilla.org/show_bug.cgi?id=827053

In response to your remark about SVG, SVG's fill rule is also not stateful as SVG is completely stateless. (SVG is based on the Adobe imaging model which also have eofill and eoclip)

I'm not proposing that this patch should be merged immediatly. We should first wait until the mozilla bug is accepted.
Notice how much simpler the code is with this change since the new API matches PDF. (I can simplify the fill/eofill case if needed)

@yurydelendik
Copy link
Contributor

Really good, in this case this pull request blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=827053 . We will need to keep/include the fallback to the mozFillRule for older Firefox versions.

@cabanier
Copy link
Author

cabanier commented Jan 8, 2013

That is unfortunately true.
How is pdf.js released? I assumed it was part of FF deliverable.


From: Yury Delendik [[email protected]]
Sent: Tuesday, January 08, 2013 7:40 AM
To: mozilla/pdf.js
Cc: Rik Cabanier
Subject: Re: [pdf.js] Switch to eoFIll and eoClip (#2534)

Really good, in this case this pull request blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=827053 . We will need to keep/include the fallback to the mozFillRule for older Firefox versions.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2534#issuecomment-12002067.

@yurydelendik
Copy link
Contributor

How is pdf.js released? I assumed it was part of FF deliverable.

It is being released to several channels:

  • mozilla-centeral (controlled by firefox product team)
  • b2g/gaia (controlled by b2g team)
  • pdf.js addon (periodically submitted to the amo; supported for ff10+)
  • generic web viewer for html5 compatible browsers (automatically)

Last two require presence of the fallback for older browsers.

@cabanier
Copy link
Author

cabanier commented Jan 8, 2013

Thanks!
I will update my pull request to keep the old stuff in.


From: Yury Delendik [[email protected]]
Sent: Tuesday, January 08, 2013 9:16 AM
To: mozilla/pdf.js
Cc: Rik Cabanier
Subject: Re: [pdf.js] Switch to eoFIll and eoClip (#2534)

How is pdf.js released? I assumed it was part of FF deliverable.

It is being released to several channels:

  • mozilla-centeral (controlled by firefox product team)
  • b2g/gaia (controlled by b2g team)
  • pdf.js addon (periodically submitted to the amo; supported for ff10+)
  • general web viewer for html5 compatible browsers (automatically)

Last two require presence of the fallback for older browsers.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2534#issuecomment-12006733.

@cabanier cabanier closed this Jan 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants