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

SVG parsing issue, tries to convert "c" to float #450

Closed
preshtildeath opened this issue Jun 6, 2022 · 11 comments
Closed

SVG parsing issue, tries to convert "c" to float #450

preshtildeath opened this issue Jun 6, 2022 · 11 comments

Comments

@preshtildeath
Copy link

Trying to add an SVG to an empty PDF using (essentially) the code from the example at: https://pyfpdf.github.io/fpdf2/SVG.html
Could be a strange SVG, main reason I'm making it into a PDF is because Photoshop doesn't open it correctly, despite it rendering fine in Chrome and Edge.

Environment
Windows 10 Pro version 21H1
Python 3.10.4
fpdf2 version 2.5.4, also checked the latest master branch

Error details
Full traceback is as follows:

Traceback (most recent call last):
  File ".\main.py", line 16, in <module>
    convert("AFR.svg")
  File ".\main.py", line 8, in convert
    svg = fpdf.svg.SVGObject.from_file(svg_path)
  File ".\venv\lib\site-packages\fpdf\svg.py", line 788, in from_file
    return cls(svgfile.read(), *args, **kwargs)
  File ".\venv\lib\site-packages\fpdf\svg.py", line 799, in __init__
    self.convert_graphics(svg_tree)
  File ".\venv\lib\site-packages\fpdf\svg.py", line 849, in convert_graphics
    self.build_group(root_tag, base_group)
  File ".\venv\lib\site-packages\fpdf\svg.py", line 1035, in build_group
    pdf_group.add_item(self.build_path(child))
  File ".\venv\lib\site-packages\fpdf\svg.py", line 1057, in build_path
    svg_path_converter(pdf_path, svg_path)
  File ".\venv\lib\site-packages\fpdf\svg.py", line 761, in svg_path_converter
    numbers, svg_path = _read_n_numbers(svg_path[read_idx:], read_count)
  File ".\venv\lib\site-packages\fpdf\svg.py", line 710, in _read_n_numbers
    return tuple(float(num) for num in numbers), leftover.lstrip()
  File ".\venv\lib\site-packages\fpdf\svg.py", line 710, in <genexpr>
    return tuple(float(num) for num in numbers), leftover.lstrip()
ValueError: could not convert string to float: 'c'

The offending SVG:

<svg viewBox="0 0 17 19" xmlns="http://www.w3.org/2000/svg">
<path d="M8.245.182l8 4.619v9.237l-8 4.619-8-4.619V4.801l8-4.62zm-.853 1.844c-.589 1-.733 2.116-.43 3.35.922.664 1.489 1.392 1.701 2.183-.565-.8-1.324-1.394-2.276-1.783C4.96 5.193 3.834 3.59 3.834 3.59c-.16.763-.038 1.498.37 2.204-.595-.178-1.271-.205-2.03-.083.7.301 1.081.703 1.143 1.205.061.502-.106.815-.501.94-.822.265-1.557.757-2.204 1.477 1.772-.126 2.443.738 2.013 2.59a5.168 5.168 0 013.06-1.837c-.242-.189-.643-.257-1.203-.206 1.075-1.28 2.54-1.72 4.395-1.325-1.18.053-2.055.331-2.627.833.647-.004 1.131.173 1.451.53-1.296.325-2.224.987-2.784 1.986 1.69-.194 2.823.152 3.401 1.04.85 1.303.034 2.136.001 2.169.332.044.604-.023.817-.202.004.754-.57 1.282-1.722 1.583 2.37.225 4.034-.462 4.989-2.062-.377.285-.785.363-1.222.232-.657-.196-.526-.825-.282-.948-.48.102-.779.049-.897-.159-.177-.311-.1-.695.233-.889-.525.095-.818-.033-.88-.382-.091-.524.08-.89.481-1.132-.487-.279-1.014-.32-1.58-.123.793-.62 1.815-.734 3.066-.343.623.299.777.703.465 1.212 1.681-.19 2.44.355 2.276 1.635 1.222-.657 1.735-1.834 1.538-3.533-.174.583-.507.958-1 1.126.059-.526-.167-.8-.678-.82.218-1.16-.087-2.136-.913-2.93a52.492 52.492 0 00-2.657-2.378c-.834-.842-1.23-1.657-1.19-2.443-.44.971-.362 1.749.235 2.333.897.876 2.931 2.44 2.97 3.81-.792-1.112-1.56-2.005-2.301-2.68-1.112-1.01-2.716-1.814-2.675-3.993zm3.238 6.607c-.12-.325-.083-.48.11-.466.832.416 1.337.992 1.515 1.73-.156.085-.367.085-.633 0-.553-.332-.811-.778-.992-1.264z" fill="#000" fill-rule="nonzero"/>
</svg>

If it's something I could do to "fix" the SVG before throwing it at fpdf2, I'd love to hear some ideas. Thanks for all the work you all do!

@Lucas-C Lucas-C added the svg label Jun 6, 2022
@Lucas-C
Copy link
Member

Lucas-C commented Jun 6, 2022

Thanks for reporting this.

After having a quick look, I think the issue comes from this part of the SVG path: a5.168 5.168 0 013.06-1.837c

If you remove the c character and all that comes after, you'll get this error:

TypeError: arc_relative() missing 3 required positional arguments: 'positive_sweep', 'dx', and 'dy'

With added whitespaces, the following is a valid SVG path string: a5.168 5.168 0 0 13 .06 -1.837

But with the original string, our parser doesn't know how to parse a5.168 5.168 0 013.06-1.837 and extract the required parameters to draw an arc, hence the original error when it tries to parse c as a number.

I don't what the fix should be exactly...

I checked how Firefox renders this SVG path, and it draws the arc like this:
issue-450

We should find a way to adapt our parser in order to have fpdf2 behave has other SVG reader softwares.

@Lucas-C
Copy link
Member

Lucas-C commented Jun 6, 2022

The SVG 1.1 spec does not seem to mention how the parser should behave if there are not enough parameters provided to the a command...

I think we could adapt our parser so that 013.06 gets parsed into 0 13.06,
but we will still be missing 2 parameters in that string, and I don't see where Firefox find them,
or what default values to use for some parameters...

@gmischler
Copy link
Collaborator

The specs say:

Superfluous white space and separators such as commas can be eliminated

but are unfortunately not very specific about which ones are to be considered actually superfluous.

I think we could adapt our parser so that 013.06 gets parsed into 0 13.06

Possibly, but I think there's another mechanism at play here.

I guess for testing purposes you just did a random split to arrive at this:

  • a5.168 5.168 0 0 13 .06 -1.837

Turns out the correct solution is slightly different:

  • a 5.168 5.168 0 0 1 3.06 -1.837

The arguments 4 (large-arc-flag) and 5 (sweep-flag) for an arc are flags, which means they can each only be one of 0|1. And if we know that an argument can only be a single character long, then we don't need a space to seperate it from whatever comes after it.
This is evidently how all the browsers process this file, while the Photoshop developers apparently didn't get the memo.
If we manage to tweak our own parser to split the arguments this way, then the output will be what the producing software intended.

@Lucas-C
Copy link
Member

Lucas-C commented Jun 7, 2022

The arguments 4 (large-arc-flag) and 5 (sweep-flag) for an arc are flags, which means they can each only be one of 0|1. And if we know that an argument can only be a single character long, then we don't need a space to seperate it from whatever comes after it.
This is evidently how all the browsers process this file, while the Photoshop developers apparently didn't get the memo.
If we manage to tweak our own parser to split the arguments this way, then the output will be what the producing software intended.

Nice! Well spotted @gmischler :)

@preshtildeath: are you interested on working on a PR to fix this, or not necessarily?

@preshtildeath
Copy link
Author

Not necessarily! That sounds like regex, which makes me nervous haha. This conversation has been very enlightening, I'll definitely be fiddling around some more with my project. Thank you both!

@Lucas-C
Copy link
Member

Lucas-C commented Jun 7, 2022

Alright, thank you for your answer @preshtildeath 😊

I may give @gmischler suggestion a try when I have some time, but anyone else is free to grab it and submit a PR!

@gmischler
Copy link
Collaborator

gmischler commented Jun 7, 2022

I may give gmischler suggestion a try when I have some time,

You might find some inspiration in svg.path. Their approach of type checking and parsing path arguments looks quite helpful.

@Lucas-C
Copy link
Member

Lucas-C commented Jun 8, 2022

In #453 I tried to use svg.path in fpdf2.
There are only a couple of issues to resolve on their side and we should be able to get rid of several regular expressions in our code, and fix this bug.

@gmischler
Copy link
Collaborator

I didn't actually want to suggest adding another dependency, I just liked their way of parametrizing the parameter parsing.
But if it also works that way... Have you done any performance comparisons yet?

@Lucas-C
Copy link
Member

Lucas-C commented Jun 8, 2022

Have you done any performance comparisons yet?

No, but that would be a wise idea.
I'll perform some if the minor issues in svg.path get resolved and we go on with using this lib.

@Lucas-C
Copy link
Member

Lucas-C commented Jun 18, 2022

I have merged #453 that fixes this bug.

That PR didn't make it in v2.5.5, but it will be included in the next fpdf2 release.

@Lucas-C Lucas-C closed this as completed Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants