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

RFC: svg cleanup. #2106

Merged
merged 2 commits into from
Jan 16, 2023
Merged

RFC: svg cleanup. #2106

merged 2 commits into from
Jan 16, 2023

Conversation

james-lawrence
Copy link
Contributor

@james-lawrence james-lawrence commented Jan 3, 2022

removes most of the SVG code from the codebase. relying instead of resvg's implementation. opened as a PR as a proof of concept. it renders more SVGs correctly, downside of course is the additional optional dependencies.

opened as is. I don't intend to do any additional work on the PR (unless its really small). just opened it to demonstrate the idea, and start the conversation.

druid/src/widget/svg.rs Outdated Show resolved Hide resolved
@xStrom
Copy link
Member

xStrom commented Jan 12, 2023

Okay, let's try to figure out how to move forward here.

There hasn't been any interest in reviewing this, which is unfortunate. I'm not very familiar with SVG stuff but could do a surface level review.

Regarding the heavier dependencies, given that this whole SVG stuff is behind a feature flag anyway, I think the increase in dependencies can be survived.

On a very fundamental level, if you're saying that this PR would greatly increase the quality of the SVG feature then I think at this point that's a good enough reason to merge.

Which gets us to the point of additional work on this PR, which you already claimed to probably not be interested in. In order for me to merge this, this PR needs to be in a mergeable state and pass CI.

Thus I see two paths forward:

a) You rebase on master and fix the merge conflicts. We pass the CI, I verify that the examples don't crash or otherwise obviously do something very wrong. We merge.
b) We close this PR and continue living with the hodge podge.

What's your take @james-lawrence, how do we proceed?

@xStrom xStrom added the S-waiting-on-author waits for changes from the submitter label Jan 12, 2023
@james-lawrence
Copy link
Contributor Author

james-lawrence commented Jan 12, 2023

@xStrom this implementation is standalone it completely replaced the current implementation. I'll follow up later with some misc details regarding the differences between the two.

@james-lawrence james-lawrence force-pushed the svg-refactor branch 3 times, most recently from 9038cbe to 0c7a7ed Compare January 16, 2023 11:52
@james-lawrence
Copy link
Contributor Author

james-lawrence commented Jan 16, 2023

@xStrom - main differences between the implementations:

  • removed upfront font loading. massive performance hit having it vs allowing people to opt into font loading. the performance hit was primarily due to loading it for every image vs having a cached value somewhere.
  • overall support for rendering svgs properly is much better in this branch. original version was reimplementing a very sm all subset of the work usvg does.

outside of the font issues IIRC this was api backwards compatible.

fixing up the linting errors and we should be gtg.

@james-lawrence
Copy link
Contributor Author

we should be gtg. only have the ability to test linux backends personally they all opened and rendered the svg example.

cargo run --features="svg" --example svg # gtk
cargo run --features="svg wayland" --example svg
cargo run --features="svg x11" --example svg

@xStrom xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jan 16, 2023
@xStrom
Copy link
Member

xStrom commented Jan 16, 2023

I tried the example on Windows and the tiger looks low-res. I'm guessing that there's some rasterization happening at the resolution specified in dp (which the Druid widgets operate with), while thinking that is the final physical pixel value.

I wonder if this is a straightforward fix. Maybe we can just ask for a higher res rasterization.

Any thoughts? I'll peek at the code a bit myself too.

@xStrom
Copy link
Member

xStrom commented Jan 16, 2023

I sort of fixed the scale problem in my local branch. I'll see how easily I could make it mergeable.

@xStrom
Copy link
Member

xStrom commented Jan 16, 2023

Unfortunately looks like Scale (the info) isn't piped from internals to the widgets yet. The fix is rather easy, and I have it confirmed with a hardcoded 1.75 scale factor as that matches my setup.

I think the move here is to just accept the regression for now and merge. I'll fix up the scale later myself.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

As mentioned earlier, my review here is rather surface level.

  • The example still runs fine.
  • There is a scale issue, which needs further Druid internals work and I'll document separately.
  • I defer judgement of superiority here and just accept the claim.

As such, I think we can merge.

@xStrom xStrom removed the S-needs-review waits for review label Jan 16, 2023
@xStrom xStrom merged commit 47780f8 into linebender:master Jan 16, 2023
@james-lawrence james-lawrence deleted the svg-refactor branch January 16, 2023 19:07
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