-
Notifications
You must be signed in to change notification settings - Fork 770
feature/fontique: switch femtovg to parley #9466
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
Conversation
|
One thing I've noticed is that there are significant performance issues with the glyph rendering, because we're parsing the font and creating the rustybuzz structures for every single glyph when we call face_ref. Rustybuzz borrows all data internally though, making this a tricky problem to fix. Wish we had self-referential types but oh well. |
|
Good catch. That should be fixable in femtovg, I guess. Maybe by changing the API to draw runs with same font. |
|
This is starting to look more ready. Font metrics are the main missing thing and then we can start thinking about building up parity with the current text layout. |
| let _overflow = text.overflow(); | ||
| // TODO | ||
| let _letter_spacing = text.letter_spacing(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
letter spacing I believe is handled as part of the font request stuff, but overflows need to be done.
|
Okay, apart from the ellipses stuff I think this is feature complete |
HarfRust (google backed fork of RustyBuzz) has an API that avoids the need for self-referential types (and is also ~3-4x faster than RustyBuzz). Alternatively, cosmic-text uses the yoke crate to store a self-referential RustyBuzz face. |
tronical
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent progress :). A few comments inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if you rebase your branch, this should go away :)
d305adc to
a6e0cd1
Compare
Thanks @nicoburns, that'll be very helpful! |
|
I checked the sizes of parley stuff and boxing them does make a lot of sense: |
3547cc1 to
a4278e6
Compare
|
Just rebased the feature/fontique branch again. |
| mut paint: femtovg::Paint, | ||
| ) -> femtovg::Paint { | ||
| paint.set_font(&self.fonts); | ||
| paint.set_font_size(self.pixel_size.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried out the branch and was wondering why all the text looked so small. This call is missing, so all the glyphs have the same size. I think this should probably be done in draw_glyphs by getting the font size out of the glyph run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kerning looks wrong both before and after to me! (look at the kerning between H and y in Hyperlink in the sidebar for example). I'd be interested to see what text in the same font looks like in a web browser.
|
This PR also seems to resolve our font rendering woes (#9310). Before and after:
|
|
Y'all should definitely look into using skrifa rather than |
The `sharedparley` module declares a public `Layout` struct, which clashes with the `Layout` struct we use in the C++ API. The former however is entirely internal to Rust, so we can instruct cbindgen to ignore the module.







Needs a lot of work before merging. @tronical is this the right approach at all?