emoon / ProDBG

Debugging the way it's meant to be done
Other
513 stars 31 forks source link

Fix text width calculation #304

Closed SlNPacifist closed 7 years ago

SlNPacifist commented 8 years ago

screenshot from 2016-08-23 15-45-35

For some reason ImGui calcs text size as follows:

ImVec2 ImGui::CalcTextSize(const char* text, const char* text_end, bool hide_text_after_double_hash, float wrap_width)
{
    ImGuiContext& g = *GImGui;

    const char* text_display_end;
    if (hide_text_after_double_hash)
        text_display_end = FindRenderedTextEnd(text, text_end);      // Hide anything after a '##' string
    else
        text_display_end = text_end;

    ImFont* font = g.Font;
    const float font_size = g.FontSize;
    if (text == text_display_end)
        return ImVec2(0.0f, font_size);
    ImVec2 text_size = font->CalcTextSizeA(font_size, FLT_MAX, wrap_width, text, text_display_end, NULL);

    // Cancel out character spacing for the last character of a line (it is baked into glyph->XAdvance field)
    const float font_scale = font_size / font->FontSize;
    const float character_spacing_x = 1.0f * font_scale;
    if (text_size.x > 0.0f)
        text_size.x -= character_spacing_x;
    text_size.x = (float)(int)(text_size.x + 0.95f);

    return text_size;
}

After getting text size from font (which is totally correct) it just loses any accuracy. Due to this, monospace fonts with non-integer width are just not usable at all. Rendering 1 + 3, 2 + 2 and 3 + 1 symbols give absolutely different results.

Those last operations on result need investigation. Without them:

ImVec2 ImGui::CalcTextSize(const char* text, const char* text_end, bool hide_text_after_double_hash, float wrap_width)
{
    ImGuiContext& g = *GImGui;

    const char* text_display_end;
    if (hide_text_after_double_hash)
        text_display_end = FindRenderedTextEnd(text, text_end);      // Hide anything after a '##' string
    else
        text_display_end = text_end;

    ImFont* font = g.Font;
    const float font_size = g.FontSize;
    if (text == text_display_end)
        return ImVec2(0.0f, font_size);
    ImVec2 text_size = font->CalcTextSizeA(font_size, FLT_MAX, wrap_width, text, text_display_end, NULL);

    // Cancel out character spacing for the last character of a line (it is baked into glyph->XAdvance field)
//    const float font_scale = font_size / font->FontSize;
//    const float character_spacing_x = 1.0f * font_scale;
//    if (text_size.x > 0.0f)
//        text_size.x -= character_spacing_x;
//    text_size.x = (float)(int)(text_size.x + 0.95f);

    return text_size;
}

screenshot from 2016-08-23 15-58-18

emoon commented 8 years ago

cc @ocornut any idea about this?

ocornut commented 8 years ago

The text_size.x -= character_spacing_x; is definitively needed.

Lots of areas in imgui assume that widgets/text elements starts on pixel boundaries, even if within an individual text strings can advance with sub-pixel.

It might make more sense to move the text_size.x = (float)(int)(text_size.x + 0.95f); out of here to another spot but I'll have to investigate the side effects caused by that.

(Workaround now is to set PixelSnapH=true in the ImFontConfig structure, which will lose your non-integer sizes and enlarge text a little.)

ocornut commented 8 years ago

Do you mind filling a bug in imgui repo? (just a link to here is fine)

SlNPacifist commented 8 years ago

I'll do it right now. I guess separate bug report is better than link to here.

ocornut commented 8 years ago

It's just for my tracking really, else I may forget it. No extra details needed.

SlNPacifist commented 8 years ago

I want to rephrase issue to be more clear. As I see, problem is in rounding to pixel border which makes monospace font unalignable (using just spaces). I'll reformulate it and make a link here.

SlNPacifist commented 8 years ago

The text_size.x -= character_spacing_x; is definitively needed.

With this code:

ImVec2 ImGui::CalcTextSize(const char* text, const char* text_end, bool hide_text_after_double_hash, float wrap_width)
{
    ImGuiContext& g = *GImGui;

    const char* text_display_end;
    if (hide_text_after_double_hash)
        text_display_end = FindRenderedTextEnd(text, text_end);      // Hide anything after a '##' string
    else
        text_display_end = text_end;

    ImFont* font = g.Font;
    const float font_size = g.FontSize;
    if (text == text_display_end)
        return ImVec2(0.0f, font_size);
    ImVec2 text_size = font->CalcTextSizeA(font_size, FLT_MAX, wrap_width, text, text_display_end, NULL);

    // Cancel out character spacing for the last character of a line (it is baked into glyph->XAdvance field)
    const float font_scale = font_size / font->FontSize;
    const float character_spacing_x = 1.0f * font_scale;
    if (text_size.x > 0.0f)
        text_size.x -= character_spacing_x;
//    text_size.x = (float)(int)(text_size.x + 0.95f);

    return text_size;
}

I get output like this: screenshot from 2016-08-23 16-57-47

Amount of symbols in each line is the same.

emoon commented 8 years ago

I wonder if we do something bad that would cause this behavior? (maybe bad font or such? I'm pretty such we just call directly down to Imgui from our Rust wrapper so I doubt something could go wrong there but might be good to repro in the example code in Imgui to see if the same issue is there also)

ocornut commented 8 years ago

That's weird. I'll investigate too. Basically this +1 if baked into XAdvance to make the calculation loop faster, that's just aiming to cancel out the +1 of the last character.

Are those only successive Text() or draw_list->AddText() calls? or is there any cursor positioning involved?

If you can post the .TTF + code to load it (ImFontConfig is any) I can test it on same font also. Thanks!

SlNPacifist commented 8 years ago

Only Text() and SameLine(0, 0) calls are involved. No direct draw_list->AddText(). All unaligned text is drawn inside of tree node. Added related issue: https://github.com/ocornut/imgui/issues/792.

SlNPacifist commented 8 years ago

@ocornut see related issue from my previous message. It could be that I misunderstand how text rendering works and using fixed-width font is not as simple as I assume.

ocornut commented 8 years ago

Monospace non-pixel aligned fonts are indeed an issue right now. I'll look into both of your reports but I'm not entirely sure yet it'll fully be fixed easily for your use case.

You can always set PixelSnapH=true in ImFontConfig before setting up the font to make the glyphs pixel-aligned.

SlNPacifist commented 8 years ago

Thanks for advice, I will definitely try it out. I am just worried that my first assumption ("ab" and "a" + "b" should be of equal length in pixels) is not correct and even monospace font assume some kind of space between characters.

ocornut commented 8 years ago

I am just worried that my first assumption ("ab" and "a" + "b" should be of equal length in pixels) is not correct

It should in theory but because imgui expect all items to start from pixel aligned position we need to do that rounding somewhere. So in your case using glyphs that aren't pixel aligned gets broken and perhaps not trivial to fix. I will investigate and see the extent of issues caused by allowing items to start at non-aligned position. The most common case is that people use non-pixel-aligned fonts that are NOT monospace, and then don't care so much about a +0.5 ish offset.

The other bug may be something else and easier to fix (still won't fully solve your issue).

emoon commented 7 years ago

Switching Qt and C++ (for now, will be reevaluated when there are some good Rust bindings for Qt) so closing.