search

typst / svg2pdf

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

Preliminary fix for #17 #20

Closed LaurenzV closed 1 year ago

LaurenzV commented 1 year ago

I believe I found the culprits for why the rust logo in #17 doesn't render correctly.

So as far as I can see, there are two errors:

https://github.com/typst/svg2pdf/blob/b24680b27ee6047bcda512febdc6874d75b1e0b6/src/render.rs#L539-L548

In this part, the context is resetted before the apply_mask method is called, meaning that the context won't be considered in the method. I believe this is shouldn't be the case (and kind of my fault because I put it in the wrong place in my last PR)?

And secondly,

https://github.com/typst/svg2pdf/blob/b24680b27ee6047bcda512febdc6874d75b1e0b6/src/lib.rs#L279-L292

when calling the write_masksmethod, we pass the "original" context to it (which subsequentially is passed to the content_stream method) instead of the context of where the mask of the pending group was actually defined. Now I think this is a bit tricky to solve because currently, we are not storing the corresponding transformation anywhere, so we have no way of accessing it. What I did now as a "hotfix" is to store the transformation in the pending group when pushing it, but I'm not sure how good of an idea that is, so please just consider this PR as a draft for now. Also, I'm not sure if we have a similar issue with other pending objects (e.g. gradients) and I'm afraid I don't know enough about how gradients work in PDF, so I can't really judge it.

What do you think? @reknih

reknih commented 1 year ago

The first fix looks great, my bad for not having spotted it in the previous PR. The second bug is unfortunately based on the instant-write nature of pdf-writer: We cannot write the masks in the content stream but must create separate XObjects with their content streams for each of the masks. This contradicts the stack-like design of the Context struct. Hence, you rightly stored the transformation state in the PendingGroup struct to recover it later. This should work because pending groups are only ever created for masks.

The context struct not only stores transforms, but also which resources a content stream used. This mixing of global (next_id, next_..., compress, tree, box, function_map), content stream specific (the pending_..., properties and checkpoints), and rendering stack specific (c and initial_mask) context should be undone, so that the rendering part of the context can be restored even for other content streams. But don't worry: I'll also accept this PR as-is, without it and do it myself.

With regards to gradients: I've checked and created #22. We currently ignore gradient transforms (but apply them for patterns, paths, and groups).

LaurenzV commented 1 year ago

Okay great, thanks a lot!

LaurenzV commented 1 year ago

Regarding the untangling of the Context struct, how urgently do you want to implement that and do you think it's very difficult to do? I was actually thinking of trying it myself (would be good opportunity of practice for me :D), but I'm currently working full-time, so it might take me a few days or even a few weeks (if I manage to do it at all, since I don't know how difficult that is) to implement it.

So if you want to merge this PR quickly so that you can deploy the fix to Typst, it would probably make more sense if you just do it yourself, but if you currently have other things to work on anyway and don't consider this to be urgent, I'd love to give it a shot!

reknih commented 1 year ago

I think I'll just merge this PR and eventually bump Typst, best not to block on the refactoring. Refactoring Context is still on the agenda, I'm of course happy if you will have a go (thanks!!!), otherwise I'll do it. If I get to it, I'll notify you so you don't have to waste time on it.

LaurenzV commented 1 year ago

Okay! I'll give it a shot then!