Set font size to be the em square's size and decouple from the line height #8857
Conversation
cfillion
changed the title
|
Taking note that decoupling LineHeight from FontSize is also required for #4742. |
cfillion
force-pushed
the
size=em-square
branch
2 times, most recently
from
1742dd1 to
4ec08d9
Compare
|
Rebased against the master branch. Separate patch for the docking branch including conflict resolutions: Expanddiff --git a/imgui.cpp b/imgui.cpp
index 277df8d3..345f9cca 100644
--- a/imgui.cpp
+++ b/imgui.cpp
@@ -19246,7 +19246,7 @@ static void ImGui::DockNodeCalcTabBarLayout(const ImGuiDockNode* node, ImRect* o
ImGuiContext& g = *GImGui;
ImGuiStyle& style = g.Style;
- ImRect r = ImRect(node->Pos.x, node->Pos.y, node->Pos.x + node->Size.x, node->Pos.y + g.FontSize + g.Style.FramePadding.y * 2.0f);
+ ImRect r = ImRect(node->Pos.x, node->Pos.y, node->Pos.x + node->Size.x, node->Pos.y + g.LineHeight + g.Style.FramePadding.y * 2.0f);
if (out_title_rect) { *out_title_rect = r; }
r.Min.x += style.WindowBorderSize;
diff --git a/imgui_internal.h b/imgui_internal.h
remerge CONFLICT (content): Merge conflict in imgui_internal.h
index 91a4fe7e..61a8c18a 100644
--- a/imgui_internal.h
+++ b/imgui_internal.h
@@ -3848,13 +3848,8 @@ namespace ImGui
IMGUI_API bool CheckboxFlags(const char* label, ImU64* flags, ImU64 flags_value);
// Widgets: Window Decorations
-<<<<<<< eda70b4e (Tabs: docking nodes use ImGuiTabBarFlags_FittingPolicyMixed. (explicit default, solely for discoverability). (#3421, #8800))
- IMGUI_API bool CloseButton(ImGuiID id, const ImVec2& pos);
- IMGUI_API bool CollapseButton(ImGuiID id, const ImVec2& pos, ImGuiDockNode* dock_node);
-=======
IMGUI_API bool CloseButton(ImGuiID id, ImVec2 pos);
- IMGUI_API bool CollapseButton(ImGuiID id, ImVec2 pos);
->>>>>>> 9c83827d (make font size be the em square's size when ImFontCfg::SizePixels is <= 0.0f)
+ IMGUI_API bool CollapseButton(ImGuiID id, ImVec2 pos, ImGuiDockNode* dock_node);
IMGUI_API void Scrollbar(ImGuiAxis axis);
IMGUI_API bool ScrollbarEx(const ImRect& bb, ImGuiID id, ImGuiAxis axis, ImS64* p_scroll_v, ImS64 avail_v, ImS64 contents_v, ImDrawFlags draw_rounding_flags = 0);
IMGUI_API ImRect GetWindowScrollbarRect(ImGuiWindow* window, ImGuiAxis axis);
diff --git a/imgui_widgets.cpp b/imgui_widgets.cpp
remerge CONFLICT (content): Merge conflict in imgui_widgets.cpp
index 4e655c5c..04089171 100644
--- a/imgui_widgets.cpp
+++ b/imgui_widgets.cpp
@@ -923,12 +923,8 @@ bool ImGui::CloseButton(ImGuiID id, ImVec2 pos)
return pressed;
}
-<<<<<<< eda70b4e (Tabs: docking nodes use ImGuiTabBarFlags_FittingPolicyMixed. (explicit default, solely for discoverability). (#3421, #8800))
// The Collapse button also functions as a Dock Menu button.
-bool ImGui::CollapseButton(ImGuiID id, const ImVec2& pos, ImGuiDockNode* dock_node)
-=======
-bool ImGui::CollapseButton(ImGuiID id, ImVec2 pos)
->>>>>>> 9c83827d (make font size be the em square's size when ImFontCfg::SizePixels is <= 0.0f)
+bool ImGui::CollapseButton(ImGuiID id, ImVec2 pos, ImGuiDockNode* dock_node)
{
ImGuiContext& g = *GImGui;
ImGuiWindow* window = g.CurrentWindow;Replaced (Also took the opportunity to replace EDIT: Some fonts (eg. Segoe UI @ 12px) still had 1px discrepancy due to I've also reverted the close/collapse button size change. On second thought, and especially with lower vertical frame padding settings (eg. 1), I think having them be line-height-tall looked a bit iffy. EDIT2: Removed line gap from the line height after testing with a font that has a non-zero one (Nimbus Sans). |
cfillion
force-pushed
the
size=em-square
branch
2 times, most recently
from
9c83827 to
8d4d36e
Compare
|
I'll be trying to tackle this soon. (1) Any reason why this is still a draft?
(2) It would be tempted to promote new behavior and introduce a
(3) Can you tell of a free font that exhibit same property? (4) If you have time to rebase this, you can apply two minor changes
|
cfillion
force-pushed
the
size=em-square
branch
from
8d4d36e to
515b486
Compare
cfillion
force-pushed
the
size=em-square
branch
from
515b486 to
e8ce4fa
Compare
Figured I'd give it time for additional testing, but I think it's good now: so far since releasing it, the only related reports I got from users have been from surprise at the increased size of the fonts which were previously rendered too small.
Nimbus Sans is free (part of the Ghostscript font set shipped in many Linux distros): https://github.com/ArtifexSoftware/urw-base35-fonts/blob/master/fonts/NimbusSans-Regular.otf FreeType: Speaking of which (unrelated to the PR): that specific font renders a bit misaligned (higher than others). The following solves that but I don't know if it's the correct way to handle every font with a non-zero line gap value: baked->Ascent += ImMax(0.0f, ((float)FT_CEIL(metrics.height) * scale) - (baked->Ascent - baked->Descent));
Done! |
I'm not sure I understand what this refers to. There's no use of
Ditto, what code is this referring to? I don't see a max() in your code. |
|
It's just The previous iterations of the PR were:
|
|
Notes:
It seems like the correct recipe is to honor line_gap, which would get it closer to: stb_truetype: baked->Ascent = ImFloor((unscaled_ascent + unscaled_line_gap) * scale_for_layout);
baked->Descent = ImFloor(unscaled_descent * scale_for_layout);
baked->LineHeight = ImFloor((unscaled_ascent - unscaled_descent + unscaled_line_gap) * scale_for_layout);freetype: baked->Ascent = (float)FT_CEIL(metrics.height + metrics.descender) * scale;
baked->Descent = (float)(metrics.descender >> 6) * scale;
baked->LineHeight = (float)FT_CEIL(metrics.height) * scale;Which contradicts:
You can test on NimbusSans-Regular.otf and ProggyVector.ttf which both have non-zero line gap but use it differently. We could perfectly decide to expose an optional to "compact" lines but I think we first need to get to the bottom of using/understanding those values better. |
|
I found that the best way to reason about this is to enable both backends, and printout a maximum numbers of infos, e.g. FT_Pos line_gap = metrics.height - metrics.ascender + metrics.descender;
IMGUI_DEBUG_LOG("[FreeType] '%s' at %.3f\n", src->Name, baked->Size);
IMGUI_DEBUG_LOG("[FreeType] -- in ascender %.3f, descender %.3f, height %.3f, line_gap %.3f\n", metrics.ascender / 64.0f, metrics.descender / 64.0f, metrics.height / 64.0f, line_gap / 64.0f);
IMGUI_DEBUG_LOG("[FreeType] -- out Ascent %.2f, Descent %.2f, LineHeight %.2f\n", baked->Ascent, baked->Descent, baked->LineHeight);IMGUI_DEBUG_LOG("[stb_true] '%s' at %.3f\n", src->Name, baked->Size);
IMGUI_DEBUG_LOG("[stb_true] -- in ascent %.3f, descent %.3f, line_gap %.3f .. sum %.3f\n", unscaled_ascent * scale_for_layout, unscaled_descent * scale_for_layout, unscaled_line_gap * scale_for_layout, (unscaled_ascent - unscaled_descent + unscaled_line_gap) * scale_for_layout);
IMGUI_DEBUG_LOG("[stb_true] -- out Ascent %.2f, Descent %.2f, LineHeight %.2f\n", baked->Ascent, baked->Descent, baked->LineHeight);(don't mind exact number here, it's just an example of a way to look at the calc next to the visuals) |
|
In #8857 (comment) I edited the stb_truetype's |
Yes, as they refer to the X axis rotated (plus
Ah, nice! However it generally renders 1-2px higher than the full ascent because of that rounding FreeType does (eg. Segoe UI has
...that one pixel is very noticeable with Helvetica: 
EDIT: Rounding differences depending on the font size: Ascent: ascender -> height+descent, LineHeight: ascent-descent -> height
SegoeUI: [12px] Ascent: 13->12 LineHeight: 17->16
SegoeUI: [13px] Ascent: 15->13 LineHeight: 19->17 !!!
SegoeUI: [14px] Ascent: 16->15 LineHeight: 20->19
SegoeUI: [15px] Ascent: 17->16 LineHeight: 21->20
SegoeUI: [16px] Ascent: 18->16 LineHeight: 23->21 !!!
SegoeUI: [17px] Ascent: 19->18 LineHeight: 24->23
...
SegoeUI: [22px] Ascent: 24->23 LineHeight: 30->29
SegoeUI: [23px] Ascent: 25->25 LineHeight: 31->31 !
SegoeUI: [24px] Ascent: 26->25 LineHeight: 33->32
...
SegoeUI: [27px] Ascent: 30->29 LineHeight: 37->36
SegoeUI: [28px] Ascent: 31->29 LineHeight: 39->37 !!!
SegoeUI: [29px] Ascent: 32->31 LineHeight: 40->39
...
SegoeUI: [34px] Ascent: 37->36 LineHeight: 46->45
SegoeUI: [35px] Ascent: 38->38 LineHeight: 47->47 !
SegoeUI: [36px] Ascent: 39->38 LineHeight: 49->48
SegoeUI: [37px] Ascent: 40->39 LineHeight: 50->49
SegoeUI: [38px] Ascent: 41->41 LineHeight: 51->51 !
SegoeUI: [39px] Ascent: 43->42 LineHeight: 53->52
SegoeUI: [40px] Ascent: 44->42 LineHeight: 55->53 !!!
SegoeUI: [41px] Ascent: 45->44 LineHeight: 56->55 |
|
Perhaps it could dynamically switch depending on whether the font has a line gap? const FT_Pos line_gap = metrics.height - metrics.ascender + metrics.descender;
baked->Descent = (float)(metrics.descender >> 6) * scale;
if (line_gap > 0)
{
baked->Ascent = (float)FT_CEIL(metrics.height + metrics.descender) * scale;
baked->LineHeight = (float)FT_CEIL(metrics.height) * scale;
}
else
{
baked->Ascent = (float)FT_CEIL(metrics.ascender) * scale;
baked->LineHeight = baked->Ascent - baked->Descent;
}EDIT: Some level of alignment difference appears normal (happens in browsers too): https://jsfiddle.net/p4hq2atj/. 
EDIT3: To prevent FreeType's rounding from causing any false detections: const FT_Pos line_gap = bd_font_data->FtFace->height - bd_font_data->FtFace->ascender + bd_font_data->FtFace->descender; |
|
Thanks for the details, though the sum of details gets easily confusing. |
What's the reasoning for that again? the (line_gap>0) path seems ok to me, where does it breaks? const int LOGIC = 'A';
if (LOGIC == 'A')
{
baked->Ascent = (float)FT_CEIL(metrics.height + metrics.descender) * scale;
baked->LineHeight = (float)FT_CEIL(metrics.height) * scale;
}
else if (LOGIC == 'B')
{
baked->Ascent = (float)FT_CEIL(metrics.height + metrics.descender) * scale;
baked->LineHeight = baked->Ascent - baked->Descent;
}
else if (LOGIC == 'C')
{
baked->Ascent = (float)FT_CEIL(metrics.ascender) * scale;
baked->LineHeight = (float)FT_CEIL(metrics.height) * scale;
}
else if (LOGIC == 'D')
{
baked->Ascent = (float)FT_CEIL(metrics.ascender) * scale;
baked->LineHeight = baked->Ascent - baked->Descent;
}Which seems slightly improved with: else if (LOGIC == 'E')
{
float ascent_a = (float)FT_CEIL(metrics.height + metrics.descender) * scale;
float ascent_c = (float)FT_CEIL(metrics.ascender)* scale;
baked->Ascent = ImMax(ascent_a, ascent_c);
baked->LineHeight = (float)FT_CEIL(metrics.height) * scale;
}EDIT Pushed 045645e which may be helpful to compare fonts with less clicks. |
Helvetica and Cousine don't have a line gap so they do use metrics.ascender. Helvetica (1px too high/shorter in gap path, OK in non-gap path) EDIT2: Noto Sans (same, looks visually too high in gap path) Helvetica, Cousine and Nimbus Sans all look a bit high (same as non-gap path) in Firefox w/ FreeType so I assume it's just how the fonts are? At least Nimbus Sans w/ line_gap path is better than when I ignored the gap. Segoe UI (1px too high/2px shorter in gap path) The 'E' logic renders at the correct position but the line is still 2px too short (@ 13px): EDIT: 
|
|
What settings or piece of code do you use to have a one to one mapping between Firebox and Dear ImGui, in terms of spacing and DPI handling? for (int n = 0; n < io.Fonts->Fonts.Size; n++)
{
ImFont* font = io.Fonts->Fonts[n];
ImGui::PushFont(font, 0.0f);
ImGui::Text("Test %s", font->GetDebugName());
ImGui::DebugDrawItemRect();
ImGui::PopFont();
}
This is with `int LOGIC = (line_gap1 > 0) ? 'A' : 'D';` and I can't get it match as neatly.
|
|
I was using I've compared with Firefox 115 on macOS (CoreText) and 142 on Windows (DirectWrite): it renders differently there so it's not consistent with itself across platforms... I've traced where and how Firefox on Linux (FreeType) computes its metrics: it does essentially Values observed at the end of Ported to ImGui: diff --git i/misc/freetype/imgui_freetype.cpp w/misc/freetype/imgui_freetype.cpp
index 0eff6be0..78c6378a 100644
--- i/misc/freetype/imgui_freetype.cpp
+++ w/misc/freetype/imgui_freetype.cpp
@@ -48,6 +48,7 @@
#include FT_GLYPH_H // <freetype/ftglyph.h>
#include FT_SIZES_H // <freetype/ftsizes.h>
#include FT_SYNTHESIS_H // <freetype/ftsynth.h>
+#include FT_TRUETYPE_TABLES_H // <freetype/ttables.h>
// Handle LunaSVG and PlutoSVG
#if defined(IMGUI_ENABLE_FREETYPE_LUNASVG) && defined(IMGUI_ENABLE_FREETYPE_PLUTOSVG)
@@ -454,11 +455,38 @@ bool ImGui_ImplFreeType_FontBakedInit(ImFontAtlas* atlas, ImFontConfig* src, ImF
{
// Read metrics
FT_Size_Metrics metrics = bd_baked_data->FtSize->metrics;
+
+ baked->Ascent = metrics.ascender / 64.0f;
+ baked->Descent = metrics.descender / 64.0f;
+ baked->LineHeight = metrics.height / 64.0f;
+
+ const float y_scale = (metrics.y_scale / 65536.0f) / 64.0f;
+ TT_OS2* os2 = (TT_OS2*)FT_Get_Sfnt_Table(bd_font_data->FtFace, ft_sfnt_os2);
+ if (os2 && os2->sTypoAscender && y_scale > 0.0f)
+ {
+ baked->LineHeight = (os2->sTypoAscender - os2->sTypoDescender + os2->sTypoLineGap) * y_scale;
+
+ // Set maxAscent/Descent from the sTypo* fields instead of hhea if the OS/2 fsSelection USE_TYPO_METRICS bit is set
+ const uint16_t kUseTypoMetricsMask = 1 << 7;
+ if (os2->fsSelection & kUseTypoMetricsMask)
+ {
+ // Fixes Noto Sans being 1px taller
+ baked->Ascent = ImFloor((os2->sTypoAscender * y_scale) + 0.5f);
+ baked->Descent = ImCeil((os2->sTypoDescender * y_scale) - 0.5f);
+ }
+ }
+
+ const float max_height = baked->Ascent - baked->Descent;
+ baked->LineHeight = ImFloor(ImMax(baked->LineHeight, max_height) + 0.5f);
+
+ const float internal_gap = ImFloor(max_height - baked->Size + 0.5f);
+ const float external_gap = baked->LineHeight - internal_gap - baked->Size;
+ baked->Ascent += external_gap;
+
const float scale = 1.0f / rasterizer_density;
- baked->Ascent = (float)FT_CEIL(metrics.ascender) * scale; // The pixel extents above the baseline in pixels (typically positive).
- baked->Descent = (float)(metrics.descender >> 6) * scale; // The extents below the baseline in pixels (typically negative).
- baked->LineHeight = src->SizePixels > 0.0f ? baked->Size : baked->Ascent - baked->Descent; // metrics.height also includes the font's suggested line gap
- //MaxAdvanceWidth = (float)FT_CEIL(metrics.max_advance) * scale; // This field gives the maximum horizontal cursor advance for all glyphs in the font.
+ baked->Ascent *= scale;
+ baked->Descent *= scale;
+ baked->LineHeight *= scale;
}
return true;
}With that, Noto Sans at the bottom also matches (otherwise same as #8857 (comment)). 
(Not sure if the complexity for that extra accuracy is really worth it though, or even if Firefox truly is a gold standard to strive for, versus just picking one of the simpler "good enough" approaches from above...) EDIT: Improved version of the 'E' path from above, identical results to #8857 (comment) (= Noto Sans sometimes 1px taller than Firefox depending on the size): else if (LOGIC == 'F')
{
const float ascent_a = (float)FT_CEIL(metrics.height + metrics.descender);
const float ascent_b = (float)FT_CEIL(metrics.ascender);
baked->Ascent = ImMax(ascent_a, ascent_b) * scale;
baked->LineHeight = baked->Ascent - baked->Descent;
}Aka this PR currently (D, not |
|
At 12px (G=Firefox's logic, h=too high, l=too low, H=way too high, dot=OK):
screencap4.mp4EDIT: F vs G at increasing font sizes w/ Noto SansNotably, at 14px, Firefox'x logic (G) looks visually too low in my opinion. |
|
Here are some immediate tasks I want to suggest you could look at: [01]
We above all need an implementation for stb_truetype (nb: if you compile both in you can dynamically change backend). But while we're there you could do a casual compare with e.g. Chrome, mostly as a sanity check. (FYI part of the goal of the font loader refactor was to eventually allow for a DirectWrite backend, but I haven't started to write this at all. For reference I guess we'd pull things from [2]
Both to sort of have an easy transition/backup plan and to facilitate compact UI which is desirable for some users in imgui land, I think it would be good if we developed a feature that allows to dynamically cancel out the "extra" spacing that is PR is introducing. My current vision is that we would have a floating point factor, 0.0f to 1.0f, which is part of the style. Where 1.0f would be the new default and behave like in the current PR, and 0.0f would ensure that g.FontSize == g.FontLineHeight, and anything between is smooth. For some fonts it may not make a difference. I'm not sure yet if implementing this may be done from the current Ascent/Descent/LineHeight values. We currently bake font_baked->Ascent into ImFontGlyph::, so at minimum, that new feature would likely introduce a Y offset on each new line in CalcTextSize/RenderText function. What do you think?
|
I thought about this again and I believe it is best if part of the font, not the style. |
This pull request fixes the inconsistent font sizes (between fonts and between ImGui and other software) caused by ImGui treating the font size as the total pixel height instead of the size of the font's em square. (#4780, #8822)
Noto Sans vs Helvetica vs Segoe UI at 12px (FreeType):
Before:
After:
Firefox:
Baseline alignment when mixing multiple line heights in the same line is not handled (can't adjust already-drawn items after pushing a taller font later in the line...).
The first commit only adds the concept of line height. It's set to the font size so there are no visual changes with it alone.
I sifted through and individually checked all usages of the font size.
Tab close buttons look better at the font size (while the titlebar's look best using the line height) so I tweakedCloseButtonto allow any size.I was uncertain about these non-rendering-related uses that also affect the X axis (left them unchanged):
ImGuiWindow::FontRefSizeref_unitinBeginMenuExscroll_r.ExpandinEndBoxSelectThe second commit sets the font size to be the em square size and the line
ascender - descender. (FreeType'smetrics.heightincludesline_gapwhich is undesirable.)The old size behavior remains when
ImFontCfg::SizePixelsis greater than zero (for legacy backends, bitmap fonts like the default one...).