Use underline position and thickness value in font file #31086
Conversation
|
Today I learned fonts bundle their own underline position and thickness information 🙂 |
|
Seems good to me, though it breaks compat. Is it configurable though? In the case of the builtin help, I think the thin underlines looked better than the thicker underlines from the font data. |
|
might be configurable in the font data but not sure if it's good to have it. |
|
i like the top better, the lines are too thick |
|
We could base the underline thickness on the font size, so that underlines automatically become thicker as the rendered font size grows. Last time I checked, this is what Chromium does when underlining text. |
| @@ -497,7 +497,7 @@ int RichTextLabel::_process_line(ItemFrame *p_frame, const Vector2 &p_ofs, int & | |||
| Color uc = color; | |||
| uc.a *= 0.5; | |||
| int uy = y + lh / 2 - line_descent + 2; | |||
| float strikethrough_width = 1.0; | |||
| float strikethrough_width = font->get_underline_thickness(); | |||
| #ifdef TOOLS_ENABLED | |||
| strikethrough_width *= EDSCALE; | |||
There was a problem hiding this comment.
Is the EDSCALE factor still needed now that we retrieve this information from the font? Or would it scale automatically together with the font? CC @Calinou
There was a problem hiding this comment.
I left a question about whether EDSCALE scaling is still necessary for HiDPI, but otherwise it should be good to merge.
The docs display can be improved later on by replacing the link underline by a more subtle highlight (e.g. different color, and underline on hover).
|
HiDPI support seems fine to me, but it seems there are some issues with relying on the underline width defined in the font data. If I use the copy of Noto Sans Regular from the official website, I get a 2-pixel underline at 200% editor scale:
If I use the copy of Noto Sans UI Regular bundled in the Godot Git repository, I get a thicker underline (4 pixels at 200% editor scale):
All in all, this could cause underline widths to vary in unexpected ways without a way for users to control it easily (unless they alter the font data, which is non-trivial). Therefore, I'm wary about relying on the font data for the underline width. The underline can also get really close from the text, which makes it more difficult to read. We should probably look at calculating the underline width automatically based on the font size (or by using custom constants). As for the underline position, we could add custom constants to RichTextLabel and LinkButton. |
if so, underline position & thinkness should be a some kind of floating ratio value to show properly based on font size. otherwise, we need to change position and thinkness every time when changing font size. |
|
@volzhs Unfortunately, custom constants can currently only contain integers 🙁 This may change with Godot 4.0 — last time he told me, reduz was OK with the idea of changing custom constants to be floats. |
|
then I would let @akien-mga decide to merge this for 3.2, or wait for 4.0 to cover all feature. 😄 |
|
I think it's better to wait for 4.0 and make sure that we handle all aspects of this fully. |
|
Hope this one gets implemented soon. I just experienced the underlining issue. I can barely tell there is any underlining. |
|
Needs a rebase. IIUC, what we need to make this work fully is to support float constants in the theme? Maybe we should have a GIP about it and proceed with that? |
I think it just involves replacing a lot of integers with float types in the codebase (unless we want to keep integer constants for some reason, but I can't see any for UI purposes). |
|
@akien-mga rebased |
|
Let's merge and see what users think of it in the |
|
Thanks! |
not just using constant
2for position and1for thickness.old on left, new on right.
old on top, new on bottom.
