typst / svg2pdf

Converts SVG files to PDF.
Apache License 2.0
273 stars 32 forks source link

Update usvg requirement from 0.32 to 0.34 #32

Closed migmedia closed 1 year ago

migmedia commented 1 year ago

Changes needed to update dependencies usvg-0.34 and fontdb-0.14.

LaurenzV commented 1 year ago

I think we should wait a bit before merging this. I ran this commit over the test suite I created (see #33) and there seem to be two new issues:

Firstly, some test cases crash which didn't crash before, for example this one:

<svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
    <title>`patternContentUnits` with `viewBox`</title>
    <desc>`patternContentUnits` should be ignored</desc>

    <pattern id="patt1" patternContentUnits="objectBoundingBox"
             x="0.075" y="0.05" width="0.2" height="0.2" viewBox="0 0 10 20">
        <rect id="rect1" x="0" y="0" width="10" height="10" fill="grey"/>
        <rect id="rect2" x="10" y="10" width="10" height="10" fill="green"/>
    </pattern>

    <rect id="rect3" x="20" y="20" width="160" height="70" fill="url(#patt1)" stroke="darkblue"/>
    <rect id="rect4" x="20" y="110" width="70" height="70" fill="url(#patt1)" stroke="darkblue"/>

    <!-- image frame -->
    <rect id="frame" x="1" y="1" width="198" height="198" fill="none" stroke="black"/>
</svg>

which panics with the following message:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/render.rs:441:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Secondly, curves seem to be rendered a bit differently now.

For example, the following SVG

<svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
    <title>M Q</title>

    <path id="path1" d="M 30 40 Q 171 45 180 155"
          fill="none" stroke="green" stroke-width="5"/>

    <!-- image frame -->
    <rect id="frame" x="1" y="1" width="198" height="198" fill="none" stroke="black"/>
</svg>

is now rendered as: M-Q-actual

while before it was rendered as: M-Q-reference

The difference is small, but it's definitely there, as can be seen in the diff image below: M-Q-diff

I compared it with other svg viewers online and it seems like the way it was before is the correct one. So we should investigate the cause of this before updating the dependency.

migmedia commented 1 year ago

The first tissue I found a fix for. But I have still no clue regarding the wrong bezier calculation.

LaurenzV commented 1 year ago

Hey, just to check up, are you currently still actively attempting to port svg2pdf to resvg 0.34? The thing is that I'm currently working on a complete rewrite of svg2pdf from the ground up (which will also include a comprehensive automatic test suite) to address a number of currently existing issues, and while I do not know whether I will be able to finish it any time soon and whether the maintainers will actually end up wanting to merge it, I just wanted to let you know so that you don't do any unnecessary work. :)

This rewrite is based on 0.32, because while attempting to port my own rewrite to 0.34 I found out that there are even more breaking changes in this new resvg update which we need to account for (skews in Transform seem to be different now and the stroke width isn't considered in the calculation of the stroke box), so I would defintely only make the switch to the new version if we can confirm via regression tests that the output doesn't change in unintended ways. So my idea was to first get the rewrite working on 0.32, then maybe merge this into svg2pdf and only then start working on updating to 0.34.

Just wanted to let you know!