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

Center-align all text vertically #5117

Merged
merged 19 commits into from
Sep 19, 2024
Merged

Center-align all text vertically #5117

merged 19 commits into from
Sep 19, 2024

Conversation

emilk
Copy link
Owner

@emilk emilk commented Sep 17, 2024

The end result is that text centers better in buttons and other places, especially when mixing in emojis.
Before, mixing text of different heights (e.g. emojis and latin text) in a label or button would cause the text to jump vertically.

Before

This is master, with custom FontTweak to move fonts up and down:
image
image

After

This PR, with the default (zero) FontTweak

image image

@emilk emilk added text Problems related to text egui epaint style visuals and theming labels Sep 17, 2024
@lictex
Copy link
Contributor

lictex commented Sep 17, 2024

ah mixing fonts is always that hard.. :(

but this looks like still break things in some rare cases

image master (baseline aligned)
sourcehansans is higher than cantarell and it pushes the baseline down,
while characters are still aligned properly
image this pr (center aligned)
...misaligned,
seems that glyph center of some fonts is just not the visual center

Before, mixing text of different heights (e.g. emojis and latin text) in a label or button would cause the text to jump vertically.

wondering if sth like below could make things better

diff --git a/crates/epaint/src/text/font.rs b/crates/epaint/src/text/font.rs
index 5a119cdc..3135d870 100644
--- a/crates/epaint/src/text/font.rs
+++ b/crates/epaint/src/text/font.rs
@@ -473,6 +473,10 @@ impl Font {
         }
         None
     }
+
+    pub fn ascent(&self) -> f32 {
+        self.fonts.first().map_or(0., |f| f.ascent())
+    }
 }
 
 /// Code points that will always be invisible (zero width).
diff --git a/crates/epaint/src/text/text_layout.rs b/crates/epaint/src/text/text_layout.rs
index 4fa3ba49..4cd22dcb 100644
--- a/crates/epaint/src/text/text_layout.rs
+++ b/crates/epaint/src/text/text_layout.rs
@@ -172,7 +172,7 @@ fn layout_section(
                 chr,
                 pos: pos2(paragraph.cursor_x, f32::NAN),
                 size: vec2(glyph_info.advance_width, line_height),
-                ascent: font_impl.map_or(0.0, |font| font.ascent()), // Failure to find the font here would be weird
+                ascent: font.ascent(),
                 uv_rect: glyph_info.uv_rect,
                 section_index,
             });

this way only metrics from the primary (first) font are used during layout

@emilk emilk added this to the Next Major Release milestone Sep 18, 2024
@emilk
Copy link
Owner Author

emilk commented Sep 18, 2024

It doesn't really help. I think the problem is mixing fonts that don't properly center the text in them.

We could center on the visual bounds, but the problem with that would be that text would move vertically when you add new letters to a TextEdit.

I think the code in this PR is still an improvement. You can tweak the problematic fonts on load with FontTweak::y_offset_factor

@lictex
Copy link
Contributor

lictex commented Sep 18, 2024

but the problem with that would be that text would move vertically when you add new letters to a TextEdit.

with a fixed ascent value i don t really get how this move could happen..

did a quick test and the results are:

master master with diff above
image image
15.mp4
16.mp4

@emilk
Copy link
Owner Author

emilk commented Sep 18, 2024

You misunderstand me – your diff does not move the text, but it also does not solve all centering issues. Here is egui with this PR plus you diff, using Cantarell:

image

Not great.

@lictex
Copy link
Contributor

lictex commented Sep 18, 2024

EDIT: oh wait i meant to apply that diff to master, not this pr, sry


😮 i really got some different results..

+        font_data.insert(
+            "Cantarell".to_owned(),
+            FontData::from_static(include_bytes!("/usr/share/fonts/truetype/Cantarell-VF.otf")),
+        );
         families.insert(
             FontFamily::Proportional,
             vec![
-                "Ubuntu-Light".to_owned(),
+                "Cantarell".to_owned(),
                 "NotoEmoji-Regular".to_owned(),
                 "emoji-icon-font".to_owned(),

image


and if i remove all the FontTweak on default fonts..
image
while it doesn t look that good, it renders just like in my web browser (msedge):
image
which should be somewhat correct

@emilk
Copy link
Owner Author

emilk commented Sep 18, 2024

Good point… I'll try out your ascent idea in a separate PR then and hold off on this one for a little bit

@emilk emilk marked this pull request as draft September 18, 2024 11:43
@emilk emilk changed the title Better vertical alignment of text Center-align all text vertically Sep 18, 2024
@emilk
Copy link
Owner Author

emilk commented Sep 19, 2024

This is starting to look good now! This is using default (zero) FontTweak for everything (including emoji fonts):

With default egui fonts:
Screenshot 2024-09-19 at 08 56 33

With Cantarell and NotoSansCJK:
image

Looks pretty good imho, and without any tweaking!

@emilk emilk marked this pull request as ready for review September 19, 2024 07:06
@emilk
Copy link
Owner Author

emilk commented Sep 19, 2024

There is a big downside to using valign = Center though: mixing fonts of different heights (e.g. Heading and Body) looks bad:

Screenshot 2024-09-19 at 09 33 22

I think there should be a solution to this though, which looks something like this:

  • Revert to using valign = Bottom
  • Always center glyphs within the same Font (and size), regardless of valign
  • Only apply valign to the difference in height between the largest Font height the the height of the current glyph

I guess I need to do some more experiments…

EDIT: actually, the old code didn't do a good job of this either, so this is not necessarily a blocker. It would just be nice to be able to fix it all in one go.

@emilk
Copy link
Owner Author

emilk commented Sep 19, 2024

Ok, I think this is good now!

@emilk emilk merged commit 2a40d16 into master Sep 19, 2024
39 checks passed
@emilk emilk deleted the emilk/tweak-fonts branch September 19, 2024 09:44
@emilk emilk restored the emilk/tweak-fonts branch September 19, 2024 09:48
hacknus pushed a commit to hacknus/egui that referenced this pull request Oct 30, 2024
* Closes emilk#4929
* Builds on top of emilk#2724 by @lictex
(ptal!)
* Implement `Center` and `Max` vertical text alignment properly
* Change default vertical alignment of text to centering

The end result is that text centers better in buttons and other places,
especially when mixing in emojis.
Before, mixing text of different heights (e.g. emojis and latin text) in
a label or button would cause the text to jump vertically.

## Before
This is `master`, with custom `FontTweak` to move fonts up and down:
<img width="1714" alt="image"
src="https://github.com/user-attachments/assets/a10e2927-e824-4580-baea-124c0b38a527">
<img width="102" alt="image"
src="https://github.com/user-attachments/assets/cd41f415-197b-42cd-9558-d46d63c21dcb">


## After
This PR, with the default (zero) `FontTweak`

<img width="102" alt="image"
src="https://github.com/user-attachments/assets/15e7d896-66b1-4996-ab58-dd1850b19a63">

<img width="1714" alt="image"
src="https://github.com/user-attachments/assets/54ec708c-7698-4754-b1fc-fea0fd240ec9">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui epaint style visuals and theming text Problems related to text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve vertical centering of text (e.g. in buttons)
2 participants