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

Resurrect pico_svg and use it to replace vello_svg in the examples. #518

Merged
merged 16 commits into from
Apr 22, 2024

Conversation

xorgy
Copy link
Member

@xorgy xorgy commented Mar 10, 2024

No description provided.

@xorgy xorgy force-pushed the bad-pico-svg-resurrection branch 7 times, most recently from 65bd3b9 to 3ce81b2 Compare March 11, 2024 19:55
@xorgy xorgy marked this pull request as ready for review March 11, 2024 20:04
@xorgy xorgy force-pushed the bad-pico-svg-resurrection branch from b4060f9 to a656e57 Compare March 11, 2024 20:53
README.md Show resolved Hide resolved
examples/scenes/src/svg.rs Outdated Show resolved Hide resolved
examples/scenes/src/svg.rs Outdated Show resolved Hide resolved
examples/scenes/src/svg.rs Outdated Show resolved Hide resolved
examples/scenes/src/svg.rs Outdated Show resolved Hide resolved
examples/scenes/src/pico_svg.rs Show resolved Hide resolved
examples/scenes/src/pico_svg.rs Outdated Show resolved Hide resolved
examples/scenes/src/svg.rs Outdated Show resolved Hide resolved
@xorgy xorgy requested review from DJMcNab, raphlinus and dfrg March 12, 2024 15:56
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I've only skimmed the code, but it looks reasonable.

I don't think we can land this in a form which doesn't support all of the default downloads.

  1. Coat of arms of Poland gives "Unable to parse a number"
  2. More concerning, the SVG stroke test examples don't output any elements at all

The solution for 1 could be removing Coat of arms of Poland. But I don't think that's viable for issue 2.

examples/scenes/src/svg.rs Outdated Show resolved Hide resolved
examples/scenes/src/pico_svg.rs Outdated Show resolved Hide resolved
@xorgy
Copy link
Member Author

xorgy commented Mar 22, 2024

@DJMcNab processing the examples with usvg CLI makes them all render, most of them correctly (except the ones that rely on stroke-linejoin and other things that are not decoded in pico_svg). With many of them, the issue is the use of CSS instead of attributes where it's not necessary.

@xorgy xorgy force-pushed the bad-pico-svg-resurrection branch from 620820f to 3ab8d34 Compare March 22, 2024 14:06
@xorgy xorgy force-pushed the bad-pico-svg-resurrection branch from 3ab8d34 to bb8c29d Compare March 22, 2024 14:14
@xorgy xorgy requested a review from DJMcNab March 22, 2024 14:16
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

@DJMcNab processing the examples with usvg CLI makes them all render, most of them correctly (except the ones that rely on stroke-linejoin and other things that are not decoded in pico_svg). With many of them, the issue is the use of CSS instead of attributes where it's not necessary.

I hope you can understand my thinking that's not a great end state. If the stroke features are out of scope, then I'd be happy for us to remove those examples.
I really don't think we should be downloading SVGs which don't work in the default state.
An alternative would be to host the converted SVGs ourselves (but not in this repo)

I ran into https://github.com/anishathalye/assets, which seems like a reasonable solution (of course, being careful about licenses). I think we should do something similar for the readme image, for example

So, approving conditional on removing the "broken" assets from the default_downloads, but with suggestions of alternative solutions which we could use.

@dfrg
Copy link
Collaborator

dfrg commented Mar 22, 2024

My understanding was that we’d punt fancy SVG functionality to vello_svg and this repo would focus purely on tests that are directly relevant to vello dev, specifically tiger and Paris (and maybe Poland?). I don’t see why we need the download bits at all.

@xorgy xorgy added this pull request to the merge queue Apr 22, 2024
Merged via the queue into linebender:main with commit 17585c3 Apr 22, 2024
15 checks passed
@xorgy xorgy deleted the bad-pico-svg-resurrection branch April 22, 2024 17:51
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.

3 participants