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

Add page.drawSVG(path) method #202

Merged
merged 3 commits into from
Oct 4, 2019
Merged

Conversation

jlmessenger
Copy link
Contributor

Brought over the SVG path methods from pdfkit, with attribution and updated to TypeScript.

Addresses issue: #114

Here's a sample test page with the output with variations on the options.
https://gist.github.com/jlmessenger/c8323fcb7891f30843e59a391cae925a

@jlmessenger
Copy link
Contributor Author

@Hopding let me know what you think.

@Hopding
Copy link
Owner

Hopding commented Oct 1, 2019

Hello @jlmessenger! This is certainly a feature that I'd like to get into pdf-lib. So thank you for working on this. I'm planning to do a thorough review within the next week. However, I've looked over this briefly and a few things pop out at me:

  • This new code will need to have some unit tests added before it can be merged. Primarily src/api/svgPath.ts.
  • I also try to ensure that all new API changes are tested in the three tester apps (in the apps/ dir) to prevent regressions.
  • I do not think these changes fully address Missing support for embedding SVG #114, because these changes support drawing SVG paths. They do not allow SVG files to be embedded. It certainly does not have to be done as part of this PR, but I'm curious how much effort you think it would take to support SVG embedding?
  • Along the lines of my previous note, I think it would be better to call the new method PDFPage.drawSvgPath(...).

As I mentioned, I'll try to do a more thorough review of the code itself soon. Please let me know if you have any questions/concerns!

@jlmessenger
Copy link
Contributor Author

@Hopding - I went ahead and renamed the method to drawSvgPath per your suggestion.

My main goal with this PR is getting feature parity with pdfkit's SVG path support, allowing projects to migrate from pdfkit to pdf-lib.

Full SVG support would require XML parsing, and support for a variety of additional svg tags. In my experience manually converting SVG text and shape elements to pdf-lib's built-in methods is trivial, except for <path> tags as they represent very complex shapes. A fully automated SVG to PDF lib might be best as it's own project, similar to how you split out @pdf-lib/fontkit functionality from the pdf-lib core.

There are no unit tests for the SVG path code in pdfkit, and I'm not very knowledgeable about the arc solving math. I was able to find some integration tests, so perhaps those can be ported to the tester apps in some way.

I kept the original code and modifications as separate commits in the PR, so you can see how little I modified the code to make it pdf-lib & typescript compatible.

I'll see what types of tests I can come-up with, perhaps just ensuring the correct operation arguments are emitted for some sample paths? Suggestions, ideas or support on that is appreciated.

Thanks!

@Hopding
Copy link
Owner

Hopding commented Oct 1, 2019

I think the best way to test the src/api/svgPath.ts module would be to have a series of tests cases that call svgPathOperators with various path strings and then verify that the resulting array contains the correct operators and arguments in the correct sequence. The key thing would be to exercise all the different paths/functions through the code in src/api/svgPath.ts. I would like to have nearly 100% coverage for code like this (similar to how the PDF parser code has almost 100% test coverage). You can generate a coverage report with yarn testc.

I hope this helps. Please let me know if you have any additional questions!

@jlmessenger jlmessenger force-pushed the draw-svg branch 2 times, most recently from c704fc2 to cfda79d Compare October 2, 2019 20:46
@jlmessenger
Copy link
Contributor Author

jlmessenger commented Oct 2, 2019

@Hopding
I used the SVG path specification as a source for path samples, created absolute and relative variations and added unit tests using those, which provided good coverage.

I've also added the curves and shapes from that SVG path spec into the test applications. Please verify the android output, but I was able to inspect the others.

Hopefully that checks the last few boxes for you to feel comfortable moving forward.
Thanks for a great lib!

Copy link
Owner

@Hopding Hopding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlmessenger This looks great! The tests are very thorough. Thank you for adding them.

These changes will go out in the next pdf-lib release. Probably within the next few days.

@Hopding Hopding merged commit f7707f1 into Hopding:master Oct 4, 2019
@Hopding
Copy link
Owner

Hopding commented Oct 9, 2019

Version 1.2.0 is now published. It contains the PDFPage.drawSvgPath(...) method. The full release notes are available here.

You can install this new version with npm:

npm install pdf-lib@1.2.0

It's also available on unpkg:

This was referenced Feb 22, 2020
Hopding pushed a commit that referenced this pull request Aug 30, 2021
* Import original SVG Path code

* Add page.drawSvgPath() method

* Include svg path tester apps
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.

None yet

2 participants