Skip to content

Conversation

valadaptive
Copy link
Contributor

Contains several cleanups in the font loading and rendering code. Some of them were ported from the Parley PR, some are new. A lot of them center around passing in state (like font size and DPI) instead of storing it.

  • Stop storing FontsImpl in an Arc<Mutex<..>>. We never need to clone it, and its only purpose seems to be allowing the Fonts type to make use of caching while still using a non-mutable &self as a receiver. That doesn't seem to pull its own weight. We do now have to add a ctx.fonts_mut method though.
  • Remove the FontTweak::baseline_offset_factor property. I cannot construct any scenario where it behaves differently than y_offset_factor.
  • Make the Font type non-scaled (independent of font size and pixels_per_point). Most of its methods don't depend on its scale, and for those that do, we can just pass the scale in. This reduces the number of fonts we need to cache at once.
  • Stop storing FontImpl and TextureAtlas in an Arc<Mutex<..>> as well. We do this by mapping each font face to an ID, and storing each FontImpl by ID. This means that types which reference a font face can just store the ID instead of an Arc<FontImpl>. We also now just pass in the TextureAtlas to all methods that render into it. To facilitate the same API as we had previously, Font is now a view type which stores references to the "fonts by ID" map, font family, and texture atlas. This means we can only materialize one Font at a time, but that should be okay.
  • Stop storing pixels_per_point in Fonts and FontsImpl. Instead, pass it into all methods that use it. ctx.fonts (and the new ctx.fonts_mut) return a new view type (FontsView) which does store the context's current pixels_per_point. This lets us get rid of the hack where we have to recreate the font atlas every frame if we have mixed-DPI viewports.

Copy link

github-actions bot commented Aug 7, 2025

Preview available at https://egui-pr-preview.github.io/pr/7431-fonts-cleanup
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@valadaptive
Copy link
Contributor Author

Looks like the "42 years" button in the Code Example window is a pixel shorter, which is causing tests to fail. Not sure if that's a big deal or not.

@lucasmerlin lucasmerlin self-requested a review August 8, 2025 08:04
@emilk
Copy link
Owner

emilk commented Aug 8, 2025

Let's make sure we test this with two different viewports on screens with different pixels_per_point

Copy link
Collaborator

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

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

Awesome!

I was curious if this would have a noticable effect on performance, and while cached performance indeed improves by 6% (nice!) uncached performance is now 3x slower 😅:

Screenshot 2025-08-08 at 12 42 37

Profiling the benchmark shows that now way more time is spent calculating the ascent, so that seems to be a expensive operation:

On main:
Screenshot 2025-08-08 at 12 43 57

On your branch:
Screenshot 2025-08-08 at 12 43 45

So it probably makes sense to precalculate / cache the ascent values (like it was done previously).

@lucasmerlin
Copy link
Collaborator

Let's make sure we test this with two different viewports on screens with different pixels_per_point

Works! Also tested having the windows on different screens with different pixels per point.

Screenshot 2025-08-08 at 12 56 51

@lucasmerlin
Copy link
Collaborator

lucasmerlin commented Aug 8, 2025

Also gave this a try in rerun, didn't notice anything major but interestingly the help view text shifted down a row of pixels:

Screen.Recording.2025-08-08.at.13.21.56.mov

I guess it's the same as the "42 years" in the egui demos, a rounding error. Also to me it looks more aligned now, so yay?

@emilk
Copy link
Owner

emilk commented Sep 5, 2025

I was curious if this would have a noticable effect on performance, and while cached performance indeed improves by 6% (nice!) uncached performance is now 3x slower 😅:

So it probably makes sense to precalculate / cache the ascent values (like it was done previously).

The new code in this PR uses a lot of .pt_scaled to scale the font on-demand, and then call e.g. .ascent on the result.

What we would like to do it is to only run pt_scaled once for each font_size, and then cache the resulting ascent/descent/line_gap etc.

But that brings us back to having a scale baked into FontImpl, or splitting out a new ScaledFontImpl that wraps FontIml and caches the ascent/descent/line_gap etc.

The best place to fix this is probably deep down in ab_glyph 😭

valadaptive and others added 16 commits September 7, 2025 21:34
We never need to clone it, and none of its methods took `self` as
mutable, meaning it wasn't making use of the semantic difference between
the two.

This API is about to get reworked, and simplifying it is a first step.
Even testing this out *with* font fallback, which seems to be its
intended purpose, this does exactly the same thing as y_offset_factor.
It's likely that some subsequent change (perhaps
emilk#2724) removed the code path that
made it function differently.
By making `Font` a view type and indexing by font ID, we can avoid
wrapping `FontImpl` and `TextureAtlas` in an `Arc<Mutex<...>>`.
Not sure *what* I replaced this with, but it's unnecessary now
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Nice! Performance looks good now, and as far as I can tell we haven't regressed on #382

…but the text sizes (or at least letter spacing) has changed a lot!

Is this expected?
How sure are we that the new sizes/spacing is more correct (e.g. more in line with what you would see in a browser)?

Before

Image

After

Image

@emilk
Copy link
Owner

emilk commented Sep 8, 2025

This is starting to look real good - the kerning is much nicer now with the subpixel binning.

One bug I found though is that the the Tab (\t) character shrunk a lot:

Code Editor old Code Editor

BTW, you can generate these before/after pictures by running UPDATE_SNAPSHOTS=true cargo test --all-features --all-targets


EDIT: similarly, the "thin space" character is broken

Before/after
image image

Test text:

1 234 567 890
1 234 567 890


EDIT: fixed

@emilk emilk changed the title Refactor fonts-related code More even text kerning Sep 8, 2025
@emilk
Copy link
Owner

emilk commented Sep 8, 2025

The kerning is indeed a lot better, at the cost of slightly more blurry characters in some situations. Well worth the tradeoff imho, especially since it also simplifies some of the code.

Before / After

widget_gallery_dark_x1 old widget_gallery_dark_x1

@emilk
Copy link
Owner

emilk commented Sep 8, 2025

Even on high-DPI (retina) screens, the kerning is noticeably better:

Before:
retina-before

After:
retina-after

@emilk emilk added text Problems related to text visuals Renderings / graphics releated labels Sep 8, 2025
@emilk emilk merged commit d5b0a6f into emilk:main Sep 8, 2025
49 checks passed
@vkahl
Copy link

vkahl commented Sep 10, 2025

The kerning is indeed a lot better, at the cost of slightly more blurry characters in some situations. Well worth the tradeoff imho, especially since it also simplifies some of the code.

I agree about the kerning, but honestly, to me the blur looks terrible, at least in these particular screenshots! I wouldn't easily say it's worth it (not using a HiDPI display). Especially those "Click me!" buttons look really bad. For me this seems to thwart the recent changes that improved text rendering so much.

This is probably very subjective though, so if nobody agrees with me, it's probably fine :-P

@emilk
Copy link
Owner

emilk commented Sep 11, 2025

@vkahl you are right about the blur - fortunately this was fixed by bumping the text size from 12.5 to an even 13:

See e.g.
https://github.com/emilk/egui/pull/7521/files#diff-471aeabf4530b33f5aabc5b027335feb3a969471cfc238dd7f4c8d7dcdba7c31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

egui epaint text Problems related to text visuals Renderings / graphics releated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants