Merge overlapping font ranges

codecat commented

2018-03-08 13:00:20 +00:00

(Migrated from github.com)

So, I am running into this fun issue today. I am loading some fonts and including Font Awesome in them by using the MergeMode option when loading fonts. This works, generally. However, FA has multiple font files for different styles now, so they have a separate ttf file for solid & brand icons.

Loading them separately when merging in the current version of Imgui will cause it to discard many icons because they've already been added from the first range (since the ranges of brands & solid icons overlap).

As an attempt to fix this in a quick way, I've modified ImFontAtlasBuildWithStbTruetype to not include glyphs in the font that don't advance a character, like this:

stbtt_aligned_quad q;
float dummy_x = 0.0f, dummy_y = 0.0f;
stbtt_GetPackedQuad(range.chardata_for_range, atlas->TexWidth, atlas->TexHeight, char_idx, &dummy_x, &dummy_y, &q, 0);
if (dummy_x > 0.0f) { // Openplanet hack: Don't add glyph if it doesn't exist
	dst_font->AddGlyph((ImWchar)codepoint, q.x0 + off_x, q.y0 + off_y, q.x1 + off_x, q.y1 + off_y, q.s0, q.t0, q.s1, q.t1, pc.xadvance);
}

I don't know if this is the right approach, but it seems to work fine for me and there are no weird artefacts or missing characters as far as I can tell, and still allows me to merge overlapping ranges from 2 separate fonts.

Perhaps this would be nice as a setting? (Or maybe the check is safe enough?)

So, I am running into this fun issue today. I am loading some fonts and including Font Awesome in them by using the `MergeMode` option when loading fonts. This works, generally. However, FA has multiple font files for different styles now, so they have a separate ttf file for solid & brand icons. Loading them separately when merging in the current version of Imgui will cause it to discard many icons because they've already been added from the first range (since the ranges of brands & solid icons overlap). As an attempt to fix this in a quick way, I've modified `ImFontAtlasBuildWithStbTruetype` to not include glyphs in the font that don't advance a character, like this: ```c++ stbtt_aligned_quad q; float dummy_x = 0.0f, dummy_y = 0.0f; stbtt_GetPackedQuad(range.chardata_for_range, atlas->TexWidth, atlas->TexHeight, char_idx, &dummy_x, &dummy_y, &q, 0); if (dummy_x > 0.0f) { // Openplanet hack: Don't add glyph if it doesn't exist dst_font->AddGlyph((ImWchar)codepoint, q.x0 + off_x, q.y0 + off_y, q.x1 + off_x, q.y1 + off_y, q.s0, q.t0, q.s1, q.t1, pc.xadvance); } ``` I don't know if this is the right approach, but it seems to work fine for me and there are no weird artefacts or missing characters as far as I can tell, and still allows me to merge overlapping ranges from 2 separate fonts. Perhaps this would be nice as a setting? (Or maybe the check is safe enough?) Related: https://github.com/juliettef/IconFontCppHeaders/pull/5

ocornut commented

2018-03-08 13:50:41 +00:00

(Migrated from github.com)

It looks like we could indeed add flags for that purpose and/or glyph with no advance could be treated differently (overwrite by default, etc.). I'll look into it.

ocornut commented

2018-03-08 17:06:25 +00:00

(Migrated from github.com)

I had a quick look, I don't think the AdvanceX == 0.0f check is correct but it'll work in that case FontAwesome5.

There's an issue with the font code we don't properly handle glyphs that are missing from the font. One of the inner stb_truetype.h call is leading us to pack and rasterize the first glyph of the font as many times as they are missing glyphs. Likewise with the truetype path. It's fixable but it's not a 1 hour fix unfortunately so I don't yet know when I'll be able to do it.

Your local workaround should be fine until then.

I had a quick look, I don't think the AdvanceX == 0.0f check is correct but it'll work in that case FontAwesome5. There's an issue with the font code we don't properly handle glyphs that are missing from the font. One of the inner stb_truetype.h call is leading us to pack and rasterize the first glyph of the font as many times as they are missing glyphs. Likewise with the truetype path. It's fixable but it's not a 1 hour fix unfortunately so I don't yet know when I'll be able to do it. Your local workaround should be fine until then.

codecat commented

2018-03-08 21:21:21 +00:00

(Migrated from github.com)

Sounds good, thanks for checking. Will be using this workaround for the time being.

unravel-dev commented

2018-03-09 19:23:35 +00:00

(Migrated from github.com)

Yup same issue here. After I updated to latest master merged font awesome seem not to work anymore. Removing this line that you added also works for me. Though I doubt its ok to do that.

// Third pass: setup ImFont and glyphs for runtime
    for (int input_i = 0; input_i < atlas->ConfigData.Size; input_i++)
    {
        ImFontConfig& cfg = atlas->ConfigData[input_i];
        ImFontTempBuildData& tmp = tmp_array[input_i];
        ImFont* dst_font = cfg.DstFont; // We can have multiple input fonts writing into a same destination font (when using MergeMode=true)

        // THIS here
        //if (cfg.MergeMode)
        //    dst_font->BuildLookupTable();

Yup same issue here. After I updated to latest master merged font awesome seem not to work anymore. Removing this line that you added also works for me. Though I doubt its ok to do that. ``` // Third pass: setup ImFont and glyphs for runtime for (int input_i = 0; input_i < atlas->ConfigData.Size; input_i++) { ImFontConfig& cfg = atlas->ConfigData[input_i]; ImFontTempBuildData& tmp = tmp_array[input_i]; ImFont* dst_font = cfg.DstFont; // We can have multiple input fonts writing into a same destination font (when using MergeMode=true) // THIS here //if (cfg.MergeMode) // dst_font->BuildLookupTable(); ```

ocornut commented

2018-05-07 16:05:03 +00:00

(Migrated from github.com)

@codecat @volcoma This should be fixed in master now, could you let me know if it works for you?
I submitted my modification to nothings/stb, I'm not sure it'll be merged as is (perhaps as an option to the packer) but either way I'll follow on it and hopefully the next version of stb_truetype will match our mod.

@codecat @volcoma This should be fixed in master now, could you let me know if it works for you? I submitted my modification to nothings/stb, I'm not sure it'll be merged as is (perhaps as an option to the packer) but either way I'll follow on it and hopefully the next version of stb_truetype will match our mod.

codecat commented

2018-05-07 17:13:52 +00:00

(Migrated from github.com)

Great, thank you! Is there any set up necessary for this fix? Some setting? Or do I just use it as-is and load both fonts?

ocornut commented

2018-05-07 17:16:49 +00:00

(Migrated from github.com)

You can load both fonts. Currently if a character already exist it doesn't overwrite it, may add the option later. It should matter for FA5 Solid + FA5 Brand. If you want FA5 Regular showing when available and fallback to FA5 Solid, you want to merge Regular first then Solid.

codecat commented

2018-05-07 17:20:42 +00:00

(Migrated from github.com)

Perfect, thanks!

codecat commented

2018-05-10 18:03:01 +00:00

(Migrated from github.com)

I don't exactly know if this is related to this issue at all, but it also involves FontAwesome in my case so I'll just post it here.

I'm storing a font awesome font (that's not using merging in this case) somewhere in my initialization:

m_test = io.Fonts->AddFontFromFileTTF(FromAppFolder("Openplanet/Fonts/fa-solid.ttf"), 36.0f);

Then, in my rendering, I try to render an icon from the font:

auto dl = ImGui::GetOverlayDrawList();
dl->AddText(m_test, 36.0f, ImVec2(30, 30), 0xFFFFFFFF, ICON_FAS_ENVELOPE);

But there is nothing getting drawn. I've also had some stranger issues besides it not drawing, like in ImDrawList::AddText it would crash on font->ContainerAtlas being null. Any other font works fine. Is merging required for fonts like FontAwesome?

Edit: Oh, this happens because Glyphs is empty..
Edit 2: Okay, I traced it down back to me not providing a glyph range.. surprised that a font without any glyphs doesn't trigger a failed assertion..

I don't exactly know if this is related to this issue at all, but it also involves FontAwesome in my case so I'll just post it here. I'm storing a font awesome font (that's not using merging in this case) somewhere in my initialization: ```cpp m_test = io.Fonts->AddFontFromFileTTF(FromAppFolder("Openplanet/Fonts/fa-solid.ttf"), 36.0f); ``` Then, in my rendering, I try to render an icon from the font: ```cpp auto dl = ImGui::GetOverlayDrawList(); dl->AddText(m_test, 36.0f, ImVec2(30, 30), 0xFFFFFFFF, ICON_FAS_ENVELOPE); ``` But there is nothing getting drawn. I've also had some stranger issues besides it not drawing, like in `ImDrawList::AddText` it would crash on `font->ContainerAtlas` being null. Any other font works fine. Is merging required for fonts like FontAwesome? **Edit**: Oh, this happens because `Glyphs` is empty.. **Edit 2**: Okay, I traced it down back to me not providing a glyph range.. surprised that a font without any glyphs doesn't trigger a failed assertion..

ocornut commented

2018-05-10 21:16:53 +00:00

(Migrated from github.com)

Font merging isn’t required. Use the font browser inside the style editor to figure out if fonts are loaded as expected, if you have a crash/assert a more detailed explanation + repro would be useful too.

ocornut commented

2018-05-14 17:11:54 +00:00

(Migrated from github.com)

@codecat

Edit 2: Okay, I traced it down back to me not providing a glyph range.. surprised that a font without any glyphs doesn't trigger a failed assertion..

The code fallback on the default ASCII range:

if (!cfg.GlyphRanges)
    cfg.GlyphRanges = atlas->GetGlyphRangesDefault();

Closing this now, yeah!

(The way AddFontXXX/ImFontConfig are designed is a prime candidate for minor refactors perhaps for 1.70. May may ImFontConfig mandatory and take a copy/ownership of the font range then.)

@codecat >Edit 2: Okay, I traced it down back to me not providing a glyph range.. surprised that a font without any glyphs doesn't trigger a failed assertion.. The code fallback on the default ASCII range: ``` if (!cfg.GlyphRanges) cfg.GlyphRanges = atlas->GetGlyphRangesDefault(); ``` Closing this now, yeah! (The way AddFontXXX/ImFontConfig are designed is a prime candidate for minor refactors perhaps for 1.70. May may ImFontConfig mandatory and take a copy/ownership of the font range then.)

Rows
Columns

Merge overlapping font ranges #1671