Request for feedback: custom rect / custom glyphs / custom loaders with dynamic_fonts branch.

[ocornut]

March 31: reworked API.

• Most code only need AddCustomRect(), GetCustomRect(), ImFontAtlasRect now.

    // - (Pre-1.92 names) ------------> (1.92 names)
    //   - GetCustomRectByIndex()   --> Use GetCustomRect()
    //   - CalcCustomRectUV()       --> Use GetCustomRect() and read uv0, uv1 fields.
    //   - AddCustomRectRegular()   --> Renamed to AddCustomRect()
    //   - AddCustomRectFontGlyph() --> Prefer using custom ImFontLoader inside ImFontConfig
    //   - ImFontAtlasCustomRect    --> Renamed to ImFontAtlasRect

Before this branch:

const ImFontAtlasCustomRect* r = atlas->GetCustomRectByIndex(custom_rect_id);
ImVec2 uv0, uv1;
atlas->CalcCustomRectUV(r, &uv0, &uv1);
ImGui::Image(atlas->TexRef, ImVec2(r->w, r->h), uv0, uv1);

Before March 31:

const ImTextureRect* r = atlas->GetCustomRect(custom_rect_id);
ImVec2 uv0, uv1;
atlas->GetCustomRectUV(r, &uv0, &uv1);
ImGui::Image(atlas->TexRef, ImVec2(r->w, r->h), uv0, uv1);

From March 31:

ImFontAtlasRect r;
atlas->GetCustomRect(custom_rect_id, &r);
ImGui::Image(atlas->TexRef, ImVec2(r.w, r.h), r.uv0, r.uv1);

---

March 5:

This is an extension to #8465 solely dedicated to feedback and questions related to:

• AddCustomRectRegular()
• AddCustomRectFontGlyph() (now made obsolete)

Because the atlas may be compacted/shuffled, and because fonts have dynamic size, those API cannot be necessarily used in the same way.

In particular, while AddCustomRectRegular() may be of use, AddCustomRectFontGlyph() doesn't make sense very much if the font can be rescaled. The legacy function assume a "default" font size which is the one passed to AddFontXXX function, but that concept is likely to be lifted.

Instead, using a custom ImFontLoader source is likely to be a preferred solution, but that solution is still in fix, and ImFontLoader is still only in imgui_internal.h.

Linking to #8107, #7962, #1282.

Here's an example of how to create a "fully procedural" font using imgui_internal.h API:

void LoadProceduralFont(float scale)
{
    ImGuiIO& io = ImGui::GetIO();
    static ImFontLoader custom_loader;
    custom_loader.Name = "Procedural";
    custom_loader.FontSrcContainsGlyph = [](ImFontAtlas* atlas, ImFontConfig* src, ImWchar codepoint)
    {
        IM_UNUSED(atlas);
        IM_UNUSED(src);
        if (codepoint >= 'A' && codepoint <= 'Z')
            return false;
        return true;
    };
    custom_loader.FontBakedLoadGlyph = [](ImFontAtlas* atlas, ImFontConfig* src, ImFontBaked* baked, void*, ImWchar codepoint)
    {
        if (codepoint >= 'A' && codepoint <= 'Z')
            return (ImFontGlyph*)NULL;

        int w = ImMax(1, (int)(baked->Size * 0.5f));
        int h = ImMax(1, (codepoint >= 'a' && codepoint <= 'z') ? (int)(baked->Size * 0.7f) : (int)(baked->Size * 0.9f));
        ImFontAtlasRectId pack_id = ImFontAtlasPackAddRect(atlas, w, h);
        ImFontAtlasRect* r = ImFontAtlasPackGetRect(atlas, pack_id);

        ImFontGlyph glyph_in = {};
        ImFontGlyph* glyph = &glyph_in;
        glyph->Codepoint = codepoint;
        glyph->AdvanceX = (float)w + 1.0f;
        glyph->X0 = 0;
        glyph->Y0 = baked->Size - h;
        glyph->X1 = (float)w;
        glyph->Y1 = (float)baked->Size;
        glyph->Visible = true;
        glyph->Colored = true;
        glyph->PackId = pack_id;

        ImFontGlyph* out_glyph = ImFontAtlasBakedAddFontGlyph(atlas, baked, src, &glyph_in);

        ImU32 col = IM_COL32(55 + (codepoint % 3) * 100, 55 + (codepoint % 5) * 50, 55 + ((codepoint + 2) % 3) * 100, 255);
        ImTextureData* tex = atlas->TexData;
        ImFontAtlasTextureBlockFill(tex, r->x, r->y, r->w - 2, r->h, col);
        ImFontAtlasTextureBlockQueueUpload(atlas, tex, r->x, r->y, r->w, r->h);

        return out_glyph;
    };

    ImFontConfig empty_font;
    strcpy(empty_font.Name, "procedural");
    empty_font.SizePixels = 20.0f * scale;
    empty_font.FontLoader = &custom_loader;
    io.Fonts->AddFont(&empty_font);
}

In particular, the question e.g. #2127 which is essentially using a custom bitmap loader could use this technique, possibly coupled with "locking" the font to a fixed size:

myfont->GetFontBaked(16.0f);
myfont->Flags |= ImFontFlags_LockBakedSizes; // prevent further sizes from being used.

If you try using this at another size e.g. 20.0f, it will use the closest size (16.0f) and rely on bilinear filtering.

[rodrigorc]

I am updating my Rust bindings and backend (easy-imgui-rs) and it goes pretty well.

However I have some issues with CustomRects, that I think are reproducible in C++ too.

1. Calling ImFontAtlas::AddCustomRectRegular() at the beginning of the program, without any loaded font, crashes. Calling ImFontAtlas::AddFontDefault just avoids the crash.

The crash is in imgui_draw.cpp:4262 (ImFontAtlasPackAddRect):

builder->MaxRectSize.x = ImMax(builder->MaxRectSize.x, w);

because builder is NULL.

2. If I load a few custom rects, and then click the demo button "Clear Output", that calls ImFontAtlasBuildClearTexture the rects are invalidated, but my code can't know that, it calls GetCustomRectByIndex() with a now invalid index, and crashes.
   It would be nice that GetCustomRectByIndex() returned NULL when called with an invalid index.
   AFAIK, ImFontAtlasBuildClearTexture is not public, but I'm not sure if there are other public ways to invalidate the rects.

3. If I create a Image based on a custom rect, and then move the font-scale slider in the demo window like crazy, when the atlas is destroyed and recreated, sometimes there is one single frame where the Image is drawn black. Not a big deal, but it may be a symptom of something subtle.

[ocornut]

Thank you Rodrigo for your feedback, this is very useful!

(1) I have fixed this now.

(2) The function is not public, however there are other factors which could lead to ImFontAtlasBuildDestroy() being called, one of them being changing the font loader. I would like to rework the system further to avoid invalidating custom rects in that situation.

• I have pushed some small changes now to stop suggesting that the return value of AddCustomRectXXX was an index,
• Clarified that the only invalid value is -1 (previously it was <= 0).
• My plan is pack both index and atlas "generation" in that identifier, so regardless of me finding a way to preserve custom rect on a backend change, if we decide to expose a function to remove a custom rect, or simply a clear function which intently invalidate them, the GetCustomRect() functions will be able to safely return NULL without asserting. I will work on that later.
  (note that I just renamed GetCustomRectByIndex() to GetCustomRect()).

(3) I tried to build a repro and couldn't reproduce this behavior.
I have a suspicion it could be due to you passing an incorrect ImTextureID value to the Image() call. If you pull atlas->TexID then you get a ImTextureID which can represent the texture before it's been created by the backend. Whereas if you create one from a ImTextureUserID it could be zero on the initial frame. Could it be your issue?

if (ImGui::Button("Add Image"))
{
    data->CustomRectImage = atlas->AddCustomRectRegular(200, 200);
    const ImTextureRect* r = atlas->GetCustomRect(data->CustomRectImage);
    unsigned char* pixels = atlas->TexData->GetPixelsAt(r->x, r->y);
    for (int y = 0; y < r->h; y++)
    {
        for (int x = 0; x < r->w; x++)
        {
            float d = ImSqrt(ImLengthSqr({ ImFabs(r->w * 0.5f - x), ImFabs(r->h * 0.5f - y) }));
            int alpha = (int)ImLinearRemapClamp(80, 95, 255, 0, d);
            ((ImU32*)pixels)[x] = IM_COL32(120, 255, 120, alpha);
        }
        pixels += atlas->TexData->GetPitch();
    }
}
if (data->CustomRectImage != -1)
{
    const ImTextureRect* r = atlas->GetCustomRect(data->CustomRectImage);
    ImVec2 uv0, uv1;
    atlas->GetCustomRectUV(r, &uv0, &uv1);
    ImGui::Image(atlas->TexID, ImVec2(r->w, r->h), uv0, uv1);
}

[rodrigorc]

Hi again!

I have additional feedback.

(1). This works now, thanks!

(2). A function to remove a rect would be nice although I don't really need it yet. But as long as I will be able to avoid crashes I'll be happy.

(3). After some intense debugging I've seen that indeed I'm setting the texture_id to 0, but not on the first frame...

The problem came from this comment in imgui.h about ImTextureID.

// If you wrap the library from another language than C++: the functions taking ImTextureID would directly
// better take ImTextureUserID in your language binding.

So when writing the Rust binding to void Image(ImTextureID user_texture_id, ...) I wrote it as taking a ImTextureUserID instead.

When drawing an Image from the font atlas, the demo does basically:

ImGui::Image(io.Fonts->TexData->GetTexID(), ...);

That in my Rust binding becomes something equivalent to:

let tex_user_id = io.Fonts->TexID._TexUserID;
// call my Rust image(tex_user_id, ...), that in turn calls:
ImGui::Image(ImTextureID(tex_user_id), ...)

This usually works just fine, but sometimes when the atlas texture is about to be rebuilt, the value of io.Fonts->TexID._TexUserID is temporarily 0 while building the UI. This is when I see a black flicker during just 1 frame.

I've just disobeyed the comment and wrote the Image binding as taking a ImTextureID, passing io.Fonts->TexID and the problem vanished. I'm not sure if this is the correct solution though. I admit that I find the ImTextureID vs ImTextureUserID vs ImTextureData a bit confusing.

BTW, ImFontAtlas::TexData and TexID are marked as internal. It would be nice to have some public function to get this value.

[ocornut]

(2). A function to remove a rect would be nice although I don't really need it yet. [...]

I'll add one.

(3) I've just disobeyed the comment and wrote the Image binding as taking a ImTextureID, passing io.Fonts->TexID and the problem vanished. I'm not sure if this is the correct solution though.

Right. The suggestion in comment makes sense if only using images that you loaded yourself, for which you'd immediately have a valid ImTextureUserID. My assumption is that drawing using the font atlas texture only made sense for debugging.... but that reasoning is entirely flawed when we consider the use of custom rect, which we aim to facilitate and promote going onward.

We need a different strategy and should tell non-C bindings to allow constructing a full-fledged ImTextureID.

I admit that I find the ImTextureID vs ImTextureUserID vs ImTextureData a bit confusing.

ImTextureUserID (which was previously called ImTextureID) is the lower level identifier for backend to identify a texture (e.g. GLuint texture_identifier, DirectX* texture etc.). Because that value cannot be known until the end of the frame where atlas textures are uploaded, ImTextureID is the indirection that store an optional pointer to ImTextureData and use that to retrieve the latest low-leve ID during render loop.

ImTextureData is the struct and only core and backends should care about it.

I will add comments and work on improving it.

If it is still fresh in your mind, can you share more about about your confusion/assumption on those types, so I can tailor comments to reduce that confusion for future users?

[rodrigorc]

... can you share more about about your confusion/assumption on those types...

Sure... Well, I think I actually understand them, but I keep on re-reading my notes...

After some thought, I think the confusion comes down to these two points.

First, the name. The old ImTextureID becomes the new ImTextureUserID, and now there is a new struct named ImTextureID. I understand that this is probably so that the signature of Image/ImageButton is kept the same, together with the implicit constructor ImTextureID::ImTextureID(ImTextureUserID tex_user_id). But having the old name with a new meaning is a bit disorienting.

Second, the invariants of ImTextureID are not clear. First I thought that this should always be true:

_TexData == NULL || _TexData->TexUserID == _TexUserID

But that is clearly impossible to maintain if _TexData points to the atlas and the atlas texture is rebuilt.

Then I thought that if _TexData != NULL then _TexUserID is meaningless and should not be used. But if this is the case, then the compare overloads are wrong, because they check _TexUserID even if _TexData != NULL:

static inline bool operator==(const ImTextureID& lhs, const ImTextureID& rhs)
{
    return lhs._TexUserID == rhs._TexUserID && lhs._TexData == rhs._TexData;
}
// ditto for operator!=

I think they should be instead:

static inline bool operator==(const ImTextureID& lhs, const ImTextureID& rhs)
{
    if (lhs._TexData || rhs._TexData)
        return lhs._TexData == rhs._TexData;
    else
        return lhs._TexUserID == rhs._TexUserID;
}
static inline bool operator!=(const ImTextureID& lhs, const ImTextureID& rhs)
{
    return !(lhs == rhs);
}

If that is the case I would expect _TexUserID to be private and to have a getter function
ImTextureUserID ImTextureID::GetTexUserID(), but alas that doesn't exist. There is instead ImTextureUserID ImDrawCmd::GetTexUserID(), that I find a bit out of place.

And finally there is the fact that ImFontAtlas contains two internal members:

    ImTextureID                 TexID;              // Current texture identifier == TexData->GetTexID().
    ImTextureData*              TexData;            // Current texture

The TexID seems totally redundant. Or isn't it? Which one should I use? I guess that it is there to make the legacy functions work, maybe?

[ocornut]

Thanks! I'm moving this discussion to the other thread #8465 (comment)

[ocornut]

The TexID seems totally redundant. Or isn't it? Which one should I use? I guess that it is there to make the legacy functions work, maybe?

Just to clarify on that one point: the TexID is required to users to use e.g. ImGui::Image() calls with their own images created independently. My recent pushes/comments hopefully clarified that.

[pozemka]

Hello, Omar! Hello everyone! We are now one step closer to produce these effects using MSDF.

I have created playground application to generate and display glyphs with distance-field data. The application is located in that repository. It uses msdfgen under the hood. You will need Vcpkg to compile it.

First few words about how it works:
(1) fonts.cpp contains font loader implementation. It generates glyph bitmaps and sets glyph placement information (X0,Y0, X1,Y1). For debugging purposes and to compare generated glyphs with FreeType implementation you can uncomment #define SYMBOLS_ONLY and it will produce glyphs without aliasing and SDF data (just white on black pixels). Generated glyphs are saved to disk for debug purposes. Change B_CHANNELS value to use different generateXXSDF functions.
(2) shader.cpp contains shader setup and shader code used to render outilne from SDF data. Outline parameters are set with thickness, outlineThickness etc.
(3) my_gui.cpp contains demo window and shader usage example. Parts of the callback code are borrowed from the imgui_impl_opengl3.cpp file.

Next some notes and thoughts
(4) Font glyphs for the same font size differ in pixel size between SDF and Imgui's FreeType. I am not a specialist in typography and font rendering but I found something interesting. There was a bug in msdfgen related to usage of divisor with value of 64. The same divisor is used in the ImGui_ImplFreeType_FontBakedLoadGlyph (look for FT_SCALEFACTOR in the imgui_freetype.cpp file). Since msdfgen is also using FreeType under the hood this could be related.
(5) Performance. SDF algorithms are not as fast as FreeType glyph rendering so for larger font sizes there is a visible lag when you request new symbols or change font size. MSDF could actually produce different font sizes using low-resolution MSDF data and appropriate shader, but it would require a different approach. I haven't thought about it much.
(6) Ease of use of the shader. When creating UI for a game (Let's forget that ImGui is not designed specifically for this) you change font styles quite often. Text on buttons, headers, counters, information, tooltips etc. could all have different sizes, fonts and styles. Using addCallback approach on every single one of them is not optimal from the points of convenience and performance. Currently we prepare whole render state inside the callback, but for the needs of styling it would be enough to only set the shader and its uniforms. Perhaps some kind of injection similar to addCallback but more limited would be possible?
(7) Support for kerning pairs would be awesome ;)

[rodrigorc]

Hello @ocornut!

When using a AddCustomRectRegular() with a non-square image, I get the end picture all squashed and with random pixels in the bottom.

I have traced the issue back to this line in ImFontAtlas::GetCustomRectUV (imgui_draw.cpp:3305):

-    *out_uv_max = ImVec2((float)(rect->x + rect->w) * TexUvScale.x, (float)(rect->y + rect->w) * TexUvScale.y);
+    *out_uv_max = ImVec2((float)(rect->x + rect->w) * TexUvScale.x, (float)(rect->y + rect->h) * TexUvScale.y);
//                                                                                           ^ here!

That is, the last rect->w must be a rect->h.

Nobody noticed because all images in the Demo are square. And all of mine too, except one.

[db48x]

Ooh, an excellent find. Should this code use vector operators more? If it were written as (position + size) * scale, where all three of those variables were ImVec2s, then there would be no way for a typo to slip in. It would also be more readable, and easier for the reader to visualize.

[ocornut]

@rodrigorc Oops! Fixed by ee36d5f.

@db48x They are user facing structures storing unsigned short values, there's no value in adding dedicated type, nor there is in general to make long-term user facing structures use high-level types. It's mostly showing that embarrassingly I haven't started writing tests for this.

[db48x]

I understand about the types, but can’t you get the readability and correctness benefits of vector operators even without changing them? Try this:

void ImFontAtlas::GetCustomRectUV(const ImTextureRect* rect, ImVec2* out_uv_min, ImVec2* out_uv_max) const
{
    IM_ASSERT(TexData->Width > 0 && TexData->Height > 0);   // Font atlas needs to be built before we can calculate UV coordinates
    auto position = ImVec2((float)rect->x, (float)rect->y);
    auto size = ImVec2((float)rect->w, (float)rect->h);
    *out_uv_min = position * TexUvScale;
    *out_uv_max = (position + size) * TexUvScale;
}

Note that I haven’t actually compiled it; that would take extra steps.

[db48x]

Heh, I just noticed that you have almost exactly the same code in ImFontAtlasGetMouseCursorTexData where you wrote (pos + size) * atlas->TexUvScale. See line 3321.

[ocornut]

I understand about the types, but can’t you get the readability and correctness benefits of vector operators even without changing them?

The typo was that we used rect->w instead of rect->h, it could have happened just as well with the alternative you suggested.

[db48x]

Yes, but if that typo had been in the line auto size = ImVec2((float)rect->w, (float)rect->h); then it would be far more obvious on inspection. And don’t forget that you already have exactly the same code a few lines further down in the file. Why not write it the same way just for consistency?

[ocornut]

Amended that change now.

[rodrigorc]

Hello @ocornut.

I've been pondering for a while about that TextureRef and why it has a pointer inside, when I don't see it necessary... and I think that I understand it now.

The thing is that now the font atlas grows dynamically, but if during a frame a quad is emitted, then that UVs and texture must be valid during the rendering of that frame... A new bigger texture may be created, but the old one can't be destroyed until the next frame.
And if the renderer uses the latest (that is, the current) atlas texture to render everything it will be wrong at lest of part of the frame during the frames where the atlas is resized.

I assume that the atlas can be extended any time during the frame building when adding a glyph or a custom-rect, although maybe that's not currently the case (but the API is as it were).

To put it in code, this would be correct:

a = io.Fonts->GetCustomRect(rectA);
io.Fonts->GetCustomRectUV(a, &uva0, &uva1);
ImGui::Image(io.Fonts->TexRef, size, uva0, uva1);

b = io.Fonts->GetCustomRect(rectB);
io.Fonts->GetCustomRectUV(b, &uvb0, &uvb1);
ImGui::Image(io.Fonts->TexRef, size, uvb0, uvb1);

But this would be wrong:

a = io.Fonts->GetCustomRect(rectA);
b = io.Fonts->GetCustomRect(rectB);

io.Fonts->GetCustomRectUV(a, &uva0, &uva1);
io.Fonts->GetCustomRectUV(b, &uvb0, &uvb1);

ImGui::Image(io.Fonts->TexRef, size, uva0, uva1);
ImGui::Image(io.Fonts->TexRef, size, uvb0, uvb1);

Because when getting the rectB the atlas could be extended and then the uva* textures no longer correspond to the current io.Fonts->TexRef.

I think that what confused me was that GetCustomRectByIndex() returns a ImTextureRect that only has the rectangle in pixels (x, y, w, h). The UV are obtained later with a call to CalcCustomRectUV(). But FindGlyph(), that gets just the same information, plus a few font stuff, returns a ImFontGlyph that has X0, Y1, X1, Y1 that I assume are the rectangle in pixels, plus U0, V0, U1, V1'. Moreover the fields in ImTextureRectareushortwhile those in inImFontGlypharefloat. But the texture where those rect/UV refer to is implicit in the current io.Fonts->TexRef`.

An API that I would find easier to grasp, would be like this (I guess would run into problems of backwards compatibility and renderers without ImGuiBackendFlags_RendererHasTextures):

struct ImTextureRect
{
    unsigned short x, y;
    unsigned short w, h;
    ImVec2 uv_min, uv_max;
    ImTextureRef texRef;
};

ImTextureRect  GetCustomRect(int id); //returns value not pointer
// No need for GetCustomRectUV() any more
// ImFontAtlas::TexRef isn't needed either, the user should always use the returned ImTextureRect::texRef.

struct ImFontGlyph
{
    ImTextureRect texRect;
    // other stuff, but no X,Y,W,H,U,V
};

With this API my wrong code above is now correct:

a = io.Fonts->GetCustomRect(rectA);
b = io.Fonts->GetCustomRect(rectB);

ImGui::Image(a.TexRef, size, a.uv0, a.uv1);
ImGui::Image(b.TexRef, size, b.uv0, b.uv1);

This example may look a bit contrieved, but I actually had this issue in one of my programs. I'm rendering a lot of text in an OpenGl FrameBufferObject, and for that I'm (ab)using the imgui atlas. Since the text changes infrequently I'm baking the quads into a VertexBufferObject, and then I just render it when I need it. When the text changes I just re-bake the text geometry.

With the current DearImGui version I can rebuild the geometry and/or do the FBO rendering whenever I want, it just works, because the atlas texture is always the same, and the glyph coordinates are invariant. But with these dynamic fonts things get trickier. Now the textures may not be valid until after ImGui::EndFrame. And the atlas textures may be invalidated at any time without me doing nothing. And I have to take into account that the TexRef may be different for each glyph.

And the last one is what makes me think that ImFontAtlas::TexRef should not exist: it is too easy to use it wrongly, but it will mostly work correctly most of the time anyway...

PS: Sorry for the long message, maybe I've thought too much about this...

[ocornut]

The thing is that now the font atlas grows dynamically, but if during a frame a quad is emitted, then that UVs and texture must be valid during the rendering of that frame... A new bigger texture may be created, but the old one can't be destroyed until the next frame.

Correct.

And if the renderer uses the latest (that is, the current) atlas texture to render everything it will be wrong at lest of part of the frame during the frames where the atlas is resized.

Correct.
I have experimented with rewriting/scaling UV coordinates of everything that has been submitted so far, but it gets tricky as user could submit custom UV coordinates within a custom rectangle. For this to be possible for each UV we would need to find the "before" rectangle (which is potentially an expensive query) and the "after" rectangle.

But this would be wrong:

a = io.Fonts->GetCustomRect(rectA);
b = io.Fonts->GetCustomRect(rectB);

io.Fonts->GetCustomRectUV(a, &uva0, &uva1);
io.Fonts->GetCustomRectUV(b, &uvb0, &uvb1);

ImGui::Image(io.Fonts->TexRef, size, uva0, uva1);
ImGui::Image(io.Fonts->TexRef, size, uvb0, uvb1);

Because when getting the rectB the atlas could be extended and then the uva* textures no longer correspond to the current io.Fonts->TexRef.

This is actually correct ;) but only because you used GetCustomRect() which is a harmless getter.

I believe the issue you wanted to showcase was:

a_idx = io.Fonts->AddCustomRectRegular(....);
a = GetCustomRect(a_idx);
io.Fonts->GetCustomRectUV(a, &uva0, &uva1); // Get UV

b_idx = io.Fonts->AddCustomRectRegular(....); // may invalidate UV above
b = io.Fonts->GetCustomRect(rectB);
io.Fonts->GetCustomRectUV(b, &uvb0, &uvb1); // Get UV

ImGui::Image(io.Fonts->TexRef, size, uva0, uva1);
ImGui::Image(io.Fonts->TexRef, size, uvb0, uvb1);

or even simpler:

a_idx = io.Fonts->AddCustomRectRegular(....);
a = GetCustomRect(a_idx);
io.Fonts->GetCustomRectUV(a, &uva0, &uva1); // Get UV
ImGui::Text("SOME TEST"); // may invalidate UV above!!
ImGui::Image(io.Fonts->TexRef, size, uva0, uva1);

I am adding more aggressive comments to outline that fact: c414104.

I think that what confused me was that GetCustomRectByIndex() returns a ImTextureRect that only has the rectangle in pixels (x, y, w, h). The UV are obtained later with a call to CalcCustomRectUV(). But FindGlyph(), that gets just the same information, plus a few font stuff, returns a ImFontGlyph that has X0, Y1, X1, Y1 that I assume are the rectangle in pixels, plus U0, V0, U1, V1'. Moreover the fields in ImTextureRect are ushortwhile those in inImFontGlypharefloat. But the texture where those rect/UV refer to is implicit in the current io.Fonts->TexRef`.

ImFontGlyph X0/Y0/X1/Y1 are not rectangle coordinates they are offsets from current layout/text position, and they need to be floats. It's however true that X1-X0 is going to be equal to the corresponding rectangle width, and likewise for height. But they are not pixel aligned. I would also want to reserve the possibility of storing those as half-precision fp16 or fixed-point eventually.

The U0/V0/U1/V1 are indeed cached versions of if you were to call GetCustomRectUV() with ImFontGlyph::PackId. They need to be cached for performances reason because text rendering is a rather hot path.
I added comments about this too in c414104.

An API that I would find easier to grasp, would be like this (I guess would run into problems of backwards compatibility and renderers without ImGuiBackendFlags_RendererHasTextures):

Breaking backward compatibility with old renderers would be a big no but then I am not sure I understand how this proposed change would affect backends? That said, your proposed change doesn't make sense once you understand they are different values and any extra indirection or touching of more memory in hot-loop is potentially a performance issues. (I have a FIXME next to PackID because ideally it could be moved in a separate array.)

With the current DearImGui version I can rebuild the geometry and/or do the FBO rendering whenever I want, it just works, because the atlas texture is always the same, and the glyph coordinates are invariant. But with these dynamic fonts things get trickier. Now the textures may not be valid until after ImGui::EndFrame. And the atlas textures may be invalidated at any time without me doing nothing.

I understand this create an extra challenge for cases were you were caching UV coordinates, but I don't see how your proposed potential change would change anything to it. Your baked VertexBufferObject would still refer to invalid coordiates.

You may use atlas->TexMinWidth/TexMinHeight to ensure a fixed texture size, e.g. if your feature uses a dedicated atlas that you have some controls of the contents.

And I have to take into account that the TexRef may be different for each glyph.

I don't understand what this means or imply, can you clarify?

PS: Sorry for the long message, maybe I've thought too much about this...

Feedback is always greatly useful, thanks.

[ocornut]

Further musing on those specific API:

(1) I think I should rename AddCustomRectRegular() to AddCustomRect() while cleaning up those API.

(2) Since underlying logic for ImTextureRect* GetCustomRect(int) is rather simple and the most frequent path is to get UV coordinates, I believe I might change GetCustomRectUV() API to take the integer ID rather than the rectangle pointer.

(3) I have considered, and still considering offering a single function that generate a struct that also carry UV coordinates. I don't like the two functions. But note that if I make the change highlighted in (2) then many codepaths who only need the UV coordinates would call the second function only, not both.
But returning a pointer as we do currently allow handling a fail state (as we aim to support returning NULL on invalid id in order to support a function to remove rectangles) which is convenient. If we dynamically generate a struct with UV it needs to be a stack/lvalue, if we returns a pointer to it to be able to return NULL then we need to manage lifetimes.

[ocornut]

I potential way to solve (2) and (3) would be:

struct ImFontAtlasRect
{
    unsigned short      x, y;
    unsigned short      w, h;
    ImVec2              uv0, uv1; // corresponding UV coordinates
};

bool ImFontAtlas::GetCustomRect(int id, ImFontAtlasRect* out_rect);

Typical call site:

const ImTextureRect* r = atlas->GetCustomRect(data->CustomRectImage);
ImVec2 uv0, uv1;
atlas->GetCustomRectUV(r, &uv0, &uv1);
ImGui::Image(atlas->TexRef, ImVec2(r->w, r->h), uv0, uv1);

Would become

ImFontAtlasRect r;
atlas->GetCustomRect(data->CustomRectImage, &r);
ImGui::Image(atlas->TexRef, ImVec2(r.w, r.h), r.uv0, r.uv1);

What do you think?

[rodrigorc]

This is actually correct ;) but only because you used GetCustomRect() which is a harmless getter.

Ah, I assumed GetCustomRect() did some copy behind the scenes and that is why it is not a const function. Maybe it should be const after all?

And I have to take into account that the TexRef may be different for each glyph.

I don't understand what this means or imply, can you clarify?

What I mean is that every time I get UV coordinates from the atlas, they refer to a specific texture. I think that currently that texture is the value of atlas->TexRef just when the function returns. If I need to use the returned UVs later, then I have to same them together with the corresponding TexRef or I risk using the wrong texture.

I like the new GetCustomRect(). But going back to my original idea, a more obvious API would be:

struct ImFontAtlasRect
{
    unsigned short      x, y;
    unsigned short      w, h;
    ImVec2              uv0, uv1; // corresponding UV coordinates
    ImTextureRef        texRef; // uv0, uv1 refer to this texture
};

And now to use it:

ImFontAtlasRect r;
atlas->GetCustomRect(data->CustomRectImage, &r);
ImGui::Image(r.texRef, ImVec2(r.w, r.h), r.uv0, r.uv1);

You could even have a Image(ImFontAtlasRect &atlasRect) overload that does everything, maybe with an additional float scale=1.0f parameter to alter the size.

And the same could be said about ImFontGlyph. Since the returned UV coordinates refer to one specific texture, it could be defined as:

struct ImFontGlyph
{
    float           U0, V0, U1, V1;     // Texture coordinates
    ImTextureRef    texRef;    // U0, V0, U1, V1 refer to this texture
};

I may want to create a button with a big $ sign doing this:

ImFontGlyph *glyph = io.Fonts->Fonts[0]->GetFontBaked(32.0f)->FindGlyph('$');
ImGui::ImageButton("$", glyph.texRef, ImVec2(32, 32), ImVec2(glyph->U0, glyph->V0), ImVec2(glyph->U1, glyph->V1));

I understand this create an extra challenge for cases were you were caching UV coordinates, but I don't see how your proposed potential change would change anything to it. Your baked VertexBufferObject would still refer to invalid coordiates.

My proposed change doesn't actually allows me to do anything new. I could just carefully read io.Fonts->TexRef whenever I need it and all would work just the same. I just did this proposal to check if I'm understanding the current API properly.

I don't actually care too much if I have to write r.texRef or io.Fonts->TexRef. The first one is simpler to understand, IMO, because the second one forces me to think about when io.Font->TexRef can change.

I think that a comment about FindGlyph() and GetCustomRect() saying "the returned UV values are for the current Fonts->TexRef, and note that any ImGui function can change the value of Fonts->TexRef.

About my baked VBOs, my current intention is to create a separate VBO per ImTextureData, probably indexing them by their UniqueID. Then for every frame I will check that all baked UniqueID values are still existing, and if not re-bake. And be careful not to render anything between BeginFrame/EndFrame because those textures may have not been updated yet by the backend. Then during the render for each VBO I will search for the keyed UniqueID in io.Fonts->TexList. I don't see how it could fail, but I'll let you know...

[ocornut]

Ah, I assumed GetCustomRect() did some copy behind the scenes and that is why it is not a const function. Maybe it should be const after all?

Yes, I'm changing it now.

I think that a comment about FindGlyph() and GetCustomRect() saying "the returned UV values are for the current Fonts->TexRef, and note that any ImGui function can change the value of Fonts->TexRef.

Adding those.

And the same could be said about ImFontGlyph. Since the returned UV coordinates refer to one specific texture, it could be defined as:

That would have undesirable performances side-effects as hot loops would be touching +35% more cache lines.

---

Here's the full reworked API

// An identifier to a rectangle in the atlas. -1 when invalid.
// The rectangle may move and UV may be invalidated, use GetCustomRect() to retrieve it.
typedef int ImFontAtlasRectId;
#define ImFontAtlasRectId_Invalid -1

// Output of ImFontAtlas::GetCustomRect() when using custom rectangles.
// Those values may not be cached/stored as they are only valid for the current value of atlas->TexRef
// (this is in theory derived from ImTextureRect but we use separate structures for reasons)
struct ImFontAtlasRect
{
    unsigned short  x, y;               // Position (in current texture)
    unsigned short  w, h;               // Size
    ImVec2          uv0, uv1;           // UV coordinates (in current texture)

    ImFontAtlasRect() { memset(this, 0, sizeof(*this)); }
};

ImFontAtlasRectId AddCustomRect(int width, int height);                               // Register a rectangle. Return -1 (ImFontAtlasRectId_Invalid) on error.
bool              GetCustomRect(ImFontAtlasRectId id, ImFontAtlasRect* out_r) const;  // Get rectangle coordinates for current texture. Valid immediately, never store this (read above)!

Legacy

inline ImFontAtlasRectId      AddCustomRectRegular(int w, int h)                            { return AddCustomRect(w, h); }                             // RENAMED in 1.92.X
inline const ImFontAtlasRect* GetCustomRectByIndex(ImFontAtlasRectId id)                    { return GetCustomRect(id, &TempRect) ? &TempRect : NULL; } // OBSOLETED in 1.92.X
inline void                   CalcCustomRectUV(const ImFontAtlasRect* r, ImVec2* out_uv_min, ImVec2* out_uv_max) const    { *out_uv_min = r->uv0; *out_uv_max = r->uv1; }             // OBSOLETED in 1.92.X
IMGUI_API ImFontAtlasRectId   AddCustomRectFontGlyph(ImFont* font, ImWchar codepoint, int w, int h, float advance_x, const ImVec2& offset = ImVec2(0, 0));                            // OBSOLETED in 1.92.X: Use custom ImFontLoader in ImFontConfig
IMGUI_API ImFontAtlasRectId   AddCustomRectFontGlyphForSize(ImFont* font, float font_size, ImWchar codepoint, int w, int h, float advance_x, const ImVec2& offset = ImVec2(0, 0));    // ADDED AND OBSOLETED in 1.92.X

Added recap of changes on top post.

[rodrigorc]

This new API with the comments are great, thanks.

I assume the same caveat applies to ImFontGlyph, although the comment still refers to GetCustomRectUV().

But this line doesn't say all the truth:

Those values may not be cached/stored as they are only valid for the current value of atlas->TexRef

because I intend to consider them valid for as long as atlas->TexData->UniqueID exists, and I think is correct.

But I don't think it's a use case frequent enough to deserve any change in your new comments. I'm an advanced user now! 🤓.

[ocornut]

I have added an optional output to AddCustomRect() since it is frequent to do both AddCustomRect and GetCustomRect() together.

I have also added a RemoveCustomRect() function but it only partially solve the problem you mention when using "Clear Output" / ImFontAtlasBuildClear() as mentioned in point (2) of #8466 (comment).

I have an (unpushed patch) for this, encoding clear/build regeneration in ImFontAtlasRectId to be fail-safe when doing queries on rects that have been invalidated due to clearing. But I'm bothered by this approach, I would rather never clear those rectangles.

Custom rectangles are cleared in ImFontAtlasBuildDestroy(), which can happen in 4 code paths:

• Calling ImFontAtlasBuildClear() which is currently exposed via the "Clear Output" button.
• Calling ClearFonts() - which for tricky reasons may be misleadingly misnamed now, but is mostly a legacy function at this point.
• A texture format change clears it. Which generally only happens at boot time by a backend.
• A font loader change clear it.

Now, the first 3 I don't care/mind so much: they are rare or explicit. And we can remove the "Clear Output" button from user-facing spots if it is problematic, or move it to a lower-level debug feature.

But the fact that changing font loader needs to clear custom rects really bothers me. Because I want to make it easy and natural to experiment with font settings and that include dynamically changing between e.g. stb_truetype and Freetype. And simultaneously the end goal is that custom rect should be first-class citizens used by custom widgets, and it would be unacceptable if their use constrained our ability to toy with font settings live.

So what I am going to do if to try working out a way for ImFontAtlasBuildSetupFontLoader() changes to preserve custom rectangles by using code closer to a discard+compact. It might be very easy, I'll try this week. If I can get this to work, then we can reconsider the "Clear Output" button and assuming custom rects are simply never destroyed unless you ask them too, which seems healthier.