Font atlas builder is off by one in some font configurations #7399

New issue

Closed

opened 2024-03-13 19:14:49 +00:00 by GamingMinds-DanielC · 0 comments

GamingMinds-DanielC commented

2024-03-13 19:14:49 +00:00

(Migrated from github.com)

Version/Branch of Dear ImGui:

Version 1.90.5 WIP ()19045), Branch: master/docking

Back-ends:

imgui_impl_win32.cpp + imgui_impl_dx11/dx12.cpp

Compiler, OS:

Windows 10/11 + MSVC 2019/2022

Full config/build information:

Dear ImGui 1.90.5 WIP (19045)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=199711
define: _WIN32
define: _WIN64
define: _MSC_VER=1929
define: _MSVC_LANG=201703
--------------------------------
io.BackendPlatformName: imgui_impl_win32
io.BackendRendererName: imgui_impl_dx11
io.ConfigFlags: 0x00000001
 NavEnableKeyboard
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x0000000E
 HasMouseCursors
 HasSetMousePos
 RendererHasVtxOffset
--------------------------------
io.Fonts: 2 fonts, Flags: 0x00000000, TexSize: 512,128
io.DisplaySize: 1264.00,761.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

Details:

I have seen the notice to use the discussions for font loading issues. This is not a font loading question, just a problem with the font atlas builder with a fix included.

I just started looking into custom icon fonts (and UI scaling, but that's not the issue) to improve our internal tools and promptly ran into an imprecision. I made a test font with a single glyph for now. It has an ascent of 1024 and a descent of 0 in a 1024 grid, the gear icon touches the limits exactly but doesn't exceed them. So the expected behavior is to load an icon font at a specific size and get that exact size. If loading at 32 pixels, the font should have an ascent of 32 pixels and a descent of 0, but it actually gets 33 and -1. The ascent being too big then results in the glyphs being offset vertically. Since I am kind of a pixel perfectionist, I investigated further.

I have identified the problem to be here:
ocornut/imgui@da29b776ee/imgui_draw.cpp (L2986-L2987)

As long as the ascent is > 0 and the descent is <= 0, these lines are just overly complicated versions of ceilf and floorf that are off by one if the calculated float precisely hits an integer value. If ascent is <= 0 or descent is > 0 (I don't know if that can ever be the case), these lines would even break a bit, the calculated ascent and descent would then no longer be guaranteed to cover the glyphs.

Replacing the lines with these simpler versions fixes the issue:

        const float ascent = ImCeil(unscaled_ascent * font_scale);
        const float descent = ImFloor(unscaled_descent * font_scale);

With this, the calculated ascent and descent are correct and the glyph is vertically centered on the button. The default font is not affected at all since it doesn't hit exact integer values, so no side effects except other custom fonts and font scalings potentially being a bit more precise. I know that the placement could also be adjusted with a glyph offset in the font config, but that would fix the rendering only and leave the font metrics with their slightly off values, making them less useful for other stuff.

Bonus issue (related, but less important): a second inconvenience I ran into is merging with another font. Differences in ascent and descent to the merged into font will be visible unless compensated with a glyph offset. But that offset can be calculated only if the ascent of the other font is known in advance, since those metrics are not filled in before the atlas is done building. If the font is configurable by the user, this value is not known and I don't have a good solution yet. Building the atlas just to extract some metrics, discarding it again and then building it again with merged icons would work, but is far from a good solution. I need to think about this one a bit more or maybe discard custom font selection for users.

Screenshots/Video:

When looking closely, the glyph is placed one pixel too low.

Minimal, Complete and Verifiable Example code:

ImFont* g_IconsBig = nullptr;

#define GM_ICONSBIG_RANGE_START 0xE000
#define GM_ICONSBIG_RANGE_END   0xE000

#define GM_ICONSBIG_GEAR u8"\uE000"

// call once before first frame
void fontTestInit()
{
	ImGuiIO& io = ImGui::GetIO();

	io.Fonts->AddFontDefault();

	// a bit unrelated, but without a way to read metrics of the previous font, merging into custom fonts is not ideal
	ImFontConfig config1;

	config1.SizePixels       = 13.0f;
	config1.MergeMode        = true;
	config1.GlyphOffset.y    = 3.0f; // must be known in advance since the last added font doesn't have metrics filled out yet
	config1.GlyphMinAdvanceX = config1.SizePixels;

	static const ImWchar iconRanges1[] = { GM_ICONSBIG_RANGE_START, GM_ICONSBIG_RANGE_END, 0 };

	io.Fonts->AddFontFromFileTTF( "GmNavIcons.ttf", config1.SizePixels, &config1, iconRanges1 );

	// icon font for this issue
	ImFontConfig config2;

	config2.SizePixels       = 32.0f;
	config2.GlyphMinAdvanceX = config2.SizePixels;

	static const ImWchar iconRanges2[] = { GM_ICONSBIG_RANGE_START, GM_ICONSBIG_RANGE_END, 0 };

	g_IconsBig = io.Fonts->AddFontFromFileTTF( "GmNavIcons.ttf", config2.SizePixels, &config2, iconRanges2 );
}

// call once per update loop
void fontTestGui()
{
	if ( ImGui::Begin( "Test Fonts" ) )
	{
		ImGui::TextUnformatted( "Big button, separate font:" );

		ImVec2 imgButtonSize = ImVec2( 32, 32 );

		imgButtonSize.x += ImGui::GetStyle().FramePadding.x * 2;
		imgButtonSize.y += ImGui::GetStyle().FramePadding.y * 2;

		ImGui::PushFont( g_IconsBig );
		ImGui::Button( GM_ICONSBIG_GEAR, imgButtonSize );
		ImGui::PopFont();

		ImGui::Spacing();

		// bonus issue
		ImGui::TextUnformatted( "Normal buttons, merged font:" );

		ImGui::Button( GM_ICONSBIG_GEAR " Button" );
		ImGui::SmallButton( GM_ICONSBIG_GEAR " SmallButton" );
	}
	ImGui::End();
}

Here is the font file in case you want to test it yourself (must be unzipped and placed in the working directory):
GmNavIcons.zip

### Version/Branch of Dear ImGui: Version 1.90.5 WIP ()19045), Branch: master/docking ### Back-ends: imgui_impl_win32.cpp + imgui_impl_dx11/dx12.cpp ### Compiler, OS: Windows 10/11 + MSVC 2019/2022 ### Full config/build information: ``` Dear ImGui 1.90.5 WIP (19045) -------------------------------- sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20 define: __cplusplus=199711 define: _WIN32 define: _WIN64 define: _MSC_VER=1929 define: _MSVC_LANG=201703 -------------------------------- io.BackendPlatformName: imgui_impl_win32 io.BackendRendererName: imgui_impl_dx11 io.ConfigFlags: 0x00000001 NavEnableKeyboard io.ConfigInputTextCursorBlink io.ConfigWindowsResizeFromEdges io.ConfigMemoryCompactTimer = 60.0 io.BackendFlags: 0x0000000E HasMouseCursors HasSetMousePos RendererHasVtxOffset -------------------------------- io.Fonts: 2 fonts, Flags: 0x00000000, TexSize: 512,128 io.DisplaySize: 1264.00,761.00 io.DisplayFramebufferScale: 1.00,1.00 -------------------------------- style.WindowPadding: 8.00,8.00 style.WindowBorderSize: 1.00 style.FramePadding: 4.00,3.00 style.FrameRounding: 0.00 style.FrameBorderSize: 0.00 style.ItemSpacing: 8.00,4.00 style.ItemInnerSpacing: 4.00,4.00 ``` ### Details: I have seen the notice to use the discussions for font loading issues. This is not a font loading question, just a problem with the font atlas builder with a fix included. I just started looking into custom icon fonts (and UI scaling, but that's not the issue) to improve our internal tools and promptly ran into an imprecision. I made a test font with a single glyph for now. It has an ascent of 1024 and a descent of 0 in a 1024 grid, the gear icon touches the limits exactly but doesn't exceed them. So the expected behavior is to load an icon font at a specific size and get that exact size. If loading at 32 pixels, the font should have an ascent of 32 pixels and a descent of 0, but it actually gets 33 and -1. The ascent being too big then results in the glyphs being offset vertically. Since I am kind of a pixel perfectionist, I investigated further. I have identified the problem to be here: https://github.com/ocornut/imgui/blob/da29b776eed289db16a8527e5f16a0e1fa540251/imgui_draw.cpp#L2986-L2987 As long as the ascent is > 0 and the descent is <= 0, these lines are just overly complicated versions of `ceilf` and `floorf` that are off by one if the calculated float precisely hits an integer value. If ascent is <= 0 or descent is > 0 (I don't know if that can ever be the case), these lines would even break a bit, the calculated ascent and descent would then no longer be guaranteed to cover the glyphs. Replacing the lines with these simpler versions fixes the issue: ```c++ const float ascent = ImCeil(unscaled_ascent * font_scale); const float descent = ImFloor(unscaled_descent * font_scale); ``` With this, the calculated ascent and descent are correct and the glyph is vertically centered on the button. The default font is not affected at all since it doesn't hit exact integer values, so no side effects except other custom fonts and font scalings potentially being a bit more precise. I know that the placement could also be adjusted with a glyph offset in the font config, but that would fix the rendering only and leave the font metrics with their slightly off values, making them less useful for other stuff. Bonus issue (related, but less important): a second inconvenience I ran into is merging with another font. Differences in ascent and descent to the merged into font will be visible unless compensated with a glyph offset. But that offset can be calculated only if the ascent of the other font is known in advance, since those metrics are not filled in before the atlas is done building. If the font is configurable by the user, this value is not known and I don't have a good solution yet. Building the atlas just to extract some metrics, discarding it again and then building it again with merged icons would work, but is far from a good solution. I need to think about this one a bit more or maybe discard custom font selection for users. ### Screenshots/Video: ![font_test](https://github.com/ocornut/imgui/assets/77451134/873c9f1f-b936-4b3d-b6e9-a8d71050e255) ![button_zoomed](https://github.com/ocornut/imgui/assets/77451134/846b89a2-9d65-4ebf-b538-62c19306f2e0) When looking closely, the glyph is placed one pixel too low. ### Minimal, Complete and Verifiable Example code: ```cpp ImFont* g_IconsBig = nullptr; #define GM_ICONSBIG_RANGE_START 0xE000 #define GM_ICONSBIG_RANGE_END 0xE000 #define GM_ICONSBIG_GEAR u8"\uE000" // call once before first frame void fontTestInit() { ImGuiIO& io = ImGui::GetIO(); io.Fonts->AddFontDefault(); // a bit unrelated, but without a way to read metrics of the previous font, merging into custom fonts is not ideal ImFontConfig config1; config1.SizePixels = 13.0f; config1.MergeMode = true; config1.GlyphOffset.y = 3.0f; // must be known in advance since the last added font doesn't have metrics filled out yet config1.GlyphMinAdvanceX = config1.SizePixels; static const ImWchar iconRanges1[] = { GM_ICONSBIG_RANGE_START, GM_ICONSBIG_RANGE_END, 0 }; io.Fonts->AddFontFromFileTTF( "GmNavIcons.ttf", config1.SizePixels, &config1, iconRanges1 ); // icon font for this issue ImFontConfig config2; config2.SizePixels = 32.0f; config2.GlyphMinAdvanceX = config2.SizePixels; static const ImWchar iconRanges2[] = { GM_ICONSBIG_RANGE_START, GM_ICONSBIG_RANGE_END, 0 }; g_IconsBig = io.Fonts->AddFontFromFileTTF( "GmNavIcons.ttf", config2.SizePixels, &config2, iconRanges2 ); } // call once per update loop void fontTestGui() { if ( ImGui::Begin( "Test Fonts" ) ) { ImGui::TextUnformatted( "Big button, separate font:" ); ImVec2 imgButtonSize = ImVec2( 32, 32 ); imgButtonSize.x += ImGui::GetStyle().FramePadding.x * 2; imgButtonSize.y += ImGui::GetStyle().FramePadding.y * 2; ImGui::PushFont( g_IconsBig ); ImGui::Button( GM_ICONSBIG_GEAR, imgButtonSize ); ImGui::PopFont(); ImGui::Spacing(); // bonus issue ImGui::TextUnformatted( "Normal buttons, merged font:" ); ImGui::Button( GM_ICONSBIG_GEAR " Button" ); ImGui::SmallButton( GM_ICONSBIG_GEAR " SmallButton" ); } ImGui::End(); } ``` Here is the font file in case you want to test it yourself (must be unzipped and placed in the working directory): [GmNavIcons.zip](https://github.com/ocornut/imgui/files/14592357/GmNavIcons.zip)

👍 1 ❤️ 1