Skip to content

Conversation

@expenses
Copy link
Contributor

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

@expenses expenses requested a review from tronical September 19, 2025 12:33
@expenses
Copy link
Contributor Author

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.
20250922_10h54m32s_grim

Rustybuzz borrows all data internally though, making this a tricky problem to fix. Wish we had self-referential types but oh well.

@tronical
Copy link
Member

Good catch. That should be fixable in femtovg, I guess. Maybe by changing the API to draw runs with same font.

@expenses expenses marked this pull request as ready for review September 23, 2025 12:06
@expenses
Copy link
Contributor Author

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.

Comment on lines 418 to 420
let _overflow = text.overflow();
// TODO
let _letter_spacing = text.letter_spacing();
Copy link
Contributor Author

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.

@expenses
Copy link
Contributor Author

Okay, apart from the ellipses stuff I think this is feature complete

@expenses expenses requested a review from tronical September 23, 2025 14:39
@expenses
Copy link
Contributor Author

See linebender/parley#304.

@nicoburns
Copy link

Rustybuzz borrows all data internally though, making this a tricky problem to fix. Wish we had self-referential types but oh well.

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.

Copy link
Member

@tronical tronical left a 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.

Copy link
Member

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 :)

@expenses expenses force-pushed the parley branch 2 times, most recently from d305adc to a6e0cd1 Compare September 24, 2025 08:03
@expenses
Copy link
Contributor Author

Rustybuzz borrows all data internally though, making this a tricky problem to fix. Wish we had self-referential types but oh well.

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.

Thanks @nicoburns, that'll be very helpful!

@expenses expenses requested a review from tronical September 24, 2025 10:59
@expenses
Copy link
Contributor Author

I checked the sizes of parley stuff and boxing them does make a lot of sense:

[internal/core/textlayout/sharedparley.rs:72:5] std::mem::size_of::< parley::LayoutContext<Brush> >() = 1240
[internal/core/textlayout/sharedparley.rs:73:5] std::mem::size_of::< Contexts >() = 1904
[internal/core/textlayout/sharedparley.rs:74:5] std::mem::size_of::< RefCell<Contexts> >() = 1912
[internal/core/textlayout/sharedparley.rs:75:5] std::mem::size_of::< RefCell<Box<Contexts>> >() = 16

@expenses
Copy link
Contributor Author

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());
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks better now, but some of the kerning after the 'W' looks wrong. Before & after comparison:
20250925_12h34m49s_grim
20250925_12h35m16s_grim

Copy link

@nicoburns nicoburns Sep 25, 2025

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.

@expenses
Copy link
Contributor Author

This PR also seems to resolve our font rendering woes (#9310). Before and after:

20250925_12h43m59s_grim 20250925_12h44m24s_grim

@expenses
Copy link
Contributor Author

Here's what it looks like in the software renderer:
software
vs femtovg:
femtovg

All in all I'm pretty happy with things.

@nicoburns
Copy link

Y'all should definitely look into using skrifa rather than ttf_parser for outlining glyphs. It does hinting (matching freetype), so the glyphs should look much better.

@expenses expenses changed the title feature/fontique: start switcing femtovg to parley feature/fontique: switch femtovg to parley Sep 25, 2025
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.
@expenses expenses merged commit 13623bc into slint-ui:feature/fontique Sep 25, 2025
40 of 41 checks passed
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