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

Switch to Vello (new PR) #635

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Switch to Vello (new PR) #635

wants to merge 8 commits into from

Conversation

jrmoulton
Copy link
Contributor

@jrmoulton jrmoulton commented Oct 21, 2024

Vello uses a lot more memory than vger does.

In debug mode, the widget gallery with vger uses about 90MB of memory, vello uses over 400. (specific numbers depend greatly on window size).

similar in release mode.

Each new window will use another another renderer which will use another 400-600 MB of memory which gets excessive quickly.

Need to fix:

  • I have a bug where text that is italic / bold is just rendered as normal. I think this is something I am doing wrong.
  • Not all svgs that resvg rendered are getting rendered by vello_svg. I'm not sure why this is. If it is in vello_svg it is failing without warning.
  • svg rendering in general has issues (sizes, colors, unwanted background color)

Marking as draft because:

  • vello doesn't have the font_embolden. (Personally I don't like the look on macOS with font embolden so I am fine with that.)

  • high memory usage

  • Need to verify that vello actually performs comparable to vger

I do really want the features (and correctness) vello offers. Even with the tradeoffs I'd much prefer to be working on floem with vello as the renderer.
@DJMcNab

@jrmoulton jrmoulton marked this pull request as draft October 21, 2024 04:38
@DJMcNab
Copy link
Contributor

DJMcNab commented Oct 21, 2024

It's really great to see this.

Why would a second window require a second Renderer?

We have not put much effort into reducing memory usage yet - some work has started in linebender/vello#606 but has stalled due to a lack of review bandwidth.

Italic/bold text should be handled by your text rendering library providing you with the bold/italic font for that run, or else providing the correct emulated variable font parameters.
vello_svg has not been carefully tested for correctness/completeness. Contributions in that direction would be welcome.

@jrmoulton
Copy link
Contributor Author

Why would a second window require a second Renderer?

Sounds like it doesn't need to. I'll see about fixing that.

We have not put much effort into reducing memory usage yet - some work has started in linebender/vello#606 but has stalled due to a lack of review bandwidth.

👍

Italic/bold text should be handled by your text rendering library providing you with the bold/italic font for that run, or else providing the correct emulated variable font parameters.

Yeah I've probably just messed something up.

vello_svg has not been carefully tested for correctness/completeness. Contributions in that direction would be welcome.

I'll see what I can do there.

@jrmoulton
Copy link
Contributor Author

@dzhou121 What do you think/what are priorities?

@dzhou121
Copy link
Contributor

I think maybe we can still keep vger, and control via feature flags?

@jrmoulton
Copy link
Contributor Author

Yeah that would probably be the way to go for now. That would allow us to keep improving the vello integration without affecting lapce.

I'll add back vger and then mark this as ready for review and then open issues for the current issues that I've mentioned

@jrmoulton jrmoulton marked this pull request as ready for review October 21, 2024 16:29
@jrmoulton
Copy link
Contributor Author

I think this is ready for review/to be merged now that it has vger as the defualt and vello behind a flag

@@ -37,7 +38,7 @@ fn app_view() -> impl IntoView {
.animation(move |_| animation.get())
.animation(move |a| {
a.keyframe(100, |kf| {
kf.style(|s| s.border(5).border_color(Color::PURPLE))
kf.style(|s| s.border(Stroke::new(5.)).border_color(Color::PURPLE))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could make a trait to convert numbers to StrokeWrap so that we can still write it the old way.

Copy link
Contributor Author

@jrmoulton jrmoulton Oct 22, 2024

Choose a reason for hiding this comment

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

Ah we can. I first started converting them to Stroke::new and after it was too much of a pain I added impls for From<num> for StrokeWrap. I just haven't changed these examples back to just numbers but it would work if I did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dzhou121 do you want these changed back?

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