-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
[SVG] Adds clip operator, fixes setGState, fill, stroke operators #5034
Conversation
@pramodhkp I have added some review comments. Could you link one or two specific PDFs that are affected (i.e. render better) with this change? |
Ah, yeah. I meant to do that. I will add it after I make the changes. Thanks for the review. |
@timvandermeij I have made the changes. I also separated the changes in fillStroke method into a separate commit. I have updated the reference pdfs in the PR comment. |
@pramodhkp I can confirm that I have also reviewed the code again and added two small comments. Overall, this is starting to look really good. Below a screenshot of how |
@timvandermeij I am not sure what's wrong with the rendering of As for the clippping, by default PDF, I meant the 'default' in SVG. (liveprogramming.pdf). Anyway, I think it's not a good example. I tried it on the PDF 32000 document, and I think the clip is still not working properly. For reference, it's Page No. 240 in that document. |
This is how Adobe Acrobat (and Adobe Reader for that matter) renders it: So we have two different renderings for this file now: Adobe's rendering and PDF.js's canvas/your current PR rendering. Since your SVG rendering is now equal to PDF.js's rendering, I wouldn't worry about this file too much. PDF.js currently also renders it wrong compared to Adobe. |
@@ -307,6 +313,12 @@ var SVGGraphics = (function SVGGraphicsClosure(ctx) { | |||
case OPS.fillStroke: | |||
this.fillStroke(); | |||
break; | |||
case OPS.clip: | |||
this.clip(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: try to not use method call with different amount of parameters, e.g. make it this.clip('nonzero')
and if needed use e.g. this.applyClip('nonzero')
Really good, thank you. |
[SVG] Adds clip operator, fixes setGState, fill, stroke operators
This PR: