imgui icon indicating copy to clipboard operation
imgui copied to clipboard

Request for feedback: dynamic fonts & texture updates (v1.92)

Open ocornut opened this issue 10 months ago • 78 comments

Pushed an experimental features/dynamic_fonts branch.


2025/03/19: pushed a "big" change reverting the renaming ImTextureID to ImTextureUserID from the March 5 version. This is simpler for end-user, both in terms of breaking changes and in term of understanding the design. The structure previously called ImTextureID in the March 5 version is now called ImTextureRef.

2025/06/06: pushed enough changes that I am hopeful this could be merged this month. Feedback welcome.


I am looking for feedback / early adopters to try this as we will inevitably find bugs or use cases that are not well supported. The API are not 100% stabilized but it is expected they stay relatively close.

As of today, I only recommend experienced users to try this. Things should be simplified and clarified over the upcoming month.

WHEN YOU SUBMIT FEEDBACK PLEASE TRY TO INCLUDE RELEVANT CONTEXT. Things may look simple in the surface but there are lots of parameters under the hood.

Warning: Your first impression may be very dry: the examples application are using the default bitmap fonts and are still not DPI aware. (but it is now trivial to scale the font dynamically). You may tinker with changing FontSize or FontScale in Style Editor. It's going to make more sense in an established application. If you are using multiple fonts, large fonts, icon fonts, non-English languages, this will likely make things easier.

TL;DR; Add a font once, use it as any size.

https://github.com/user-attachments/assets/edf257b8-8433-456d-a2e0-5202713dead2

If you want to help testing this:

  • Read this :)
  • Branch your project and update to this features/dynamic_fonts branch of imgui.
  • Make changes (in required/desirable) in your branch.
  • If you find any issue you can report/discuss and return to your main branch. Your feedback will help improve the code but also comments/docs/examples.
  • If you start using this, please try to update and feedback regularly as we work on finalizing this.

I am interested in different situations:

  • Updating to latest code and using an old standard backend. Do you see any issue/breakage?
  • Updating to latest code and using the latest standard backend.
  • Updating to latest code and using a custom backend. Do you see any issue/breakage? How's your experience figuring out how to update to support ImGuiBackendFlags_RendererHasTextures ?
  • Users for AddCustomRectRegular() / AddCustomRectFontGlyph() api: I am creating a dedicated topic #8466 for feedback on those.

The API we are replacing to interface with renderer backends has been here since January 2015 (!), so is more 10 years old. I want to ensure that the new API will also last long.

THE PLAN

My hope is that I can finish and merge this to master in 1~2 months but it will depends on amount of feedback/bugs, and the remaining work on more "General Scaling".

  1. I am pushing a public branch today. It is temporarily based over docking. If you need to maintain changes on your side (custom engine etc.) you can use #ifdef IMGUI_HAS_TEXTURES.
  2. Now gathering a first wave of feedback and issues from early enthusiasts. I am creating two topics:
    • Topic #8465 (this one) for any general question and issue on this feature.
    • Topic #8466 solely related to support for CustomRect via AddCustomRect() API (previously AddCustomRectRegular()/AddCustomRectFontGlyph()). Your feedback, any sorts of feedback will help fixing bugs, improving documentation, comments and examples. Don't hesitate to share any sorts of feedback, or any report of what confused you, if even for a minute.
  3. The commit history will be reworked/sculpted, rebased and force-pushed for a little while, as I am treating this as short-term feature branch. So you will likely need to checkout when you update (regular merging will be harder). At some point when I am convinced enough that the commits are a good, I will stop sculpting history and start only adding to it.
  4. When this is deemed mature enough, it will be rebased on master and pushed as 1.92. We will open new topics if the current topics have had too many posts and many already solved issues.
  5. My expectation is that before 1.92 I will have also finished and added more features related to "general scaling" (current code only has "font scaling").

USEFUL REFERENCES COMMITS/DIFF

Public API

Backends:

  • Allegro5: c3f7d4c
  • DirectX9: dcef84a (+45 lines)
  • DirectX10: b3689ae
  • DirectX11: 434763a (+36 lines)
  • DirectX12: a2877e3 (+89 lines)
  • Metal: fa9c258 +51 lines
  • OpenGL2: c16e739 (+42 lines)
  • OpenGL3: 15ef13b (+42 lines)
  • SDL_Renderer2: 14cc874 (+15 lines)
  • SDL_Renderer3: 5d26c35 (+15 lines)
  • SDL_GPU: e5c9542 (+24 lines)
  • Vulkan: 2f8d076 (+29 lines)

Third Party Extensions

  • ImPlot fixes: https://github.com/epezent/implot/commit/193b9d8f92c4a437e84182b171f1ae266e72321f
  • ~~cimgui expose image functions with ImTextureUserID type to simplify function calls: https://github.com/cimgui/cimgui/issues/287~~

ocornut avatar Mar 05 '25 17:03 ocornut

GENERAL OVERVIEW OF THE CHANGES

As outlined in the '10 years of Dear ImGui' article there are many changes coming related to the font system and text shaping.

For the font system, I have decided to split the work in 2 distinct steps:

  • Step 1: dynamic font atlas & backend support for texture updates.
    • Fonts may be used at any size.
    • Backends can create, update, destroy textures.
    • Getting the low-level support working. As little user facing change as possible.
  • Step 2: rework and cleanup all high-level fonts api and general scaling system.
    • General scaling system (e.g. scaling style sizes).
    • Maybe revamp font loading API. Surface more features.

The code I am publishing today is mostly step 1.

What is part of what I am publishing (step 1)

  • Generally reworked the low-level font atlas system toward being more dynamic. Provided the backend support ImGuiBackendFlags_RendererHasTextures:
    • Fonts may be added and removed at any time.
    • One font may be used at any size.
    • Glyphs are added, and rasterized and packed on demand.
    • Atlas may be repacked, compacted etc.
    • A bunch of under the hood machinery ensure this is mostly transparent and efficient. Great care has been put at reducing the cost on lower-level text functions, but a general overall 1-2% slowdown may be expected.
  • Intentionally trying to keep user-facing changes to a strict minimum!
    • As such, many features are not exposed or surfaced in high-level API yet. (e.g. while it is now easy to use a font at any size, we don't yet provide a standard mechanism for general scaling of everything.) e.g. I very shyly added a minimum set of ImFontFlags values required for feature we need know, but it is intentionally buried in ImFontConfig struct until font loading API are redesigned better.
  • Added API for backends to support arbitrary texture creation and destruction and partial texture updates.
    • The backend API for this has not changed since v1.30 (February 2015!). We are changing it for the first time in ten years!
    • Hopefully the new API should last for a long time!
    • The old system required the Renderer Backend to build and upload texture during the backend NewFrame() call. The atlas texture identifier was locked past this point and used in draw commands.
    • The new system provides a list of textures for the backend to process at the end of the frame when it does its rendering. Low-level texture identifiers are not generally known until end of the the frame.
    • Textures blocks that have never been used may be written with new texels. Textures blocks that have been used will never be rewritten, which simplify needs for synchronization in rendering backends.
    • When the texture is out of capacity, a new texture is created. Previous glyphs are packed into it. Size variations that are not being used may be garbage collected. The texture may be larger or the same size, depending on which font size may be unused and were evicted. The previous texture may be freed by the backend after in-flight renderings are done.
    • During a frame where a new texture is created, some UI elements will use the old texture and some will use the new texture. This should be completely transparent to the user.
    • If you need fixed/known memory usage, you may decide to setup atlas texture to a fixed size by writing equal values to the atlas->TexMinWidth/TexMaxWidth, TexMinHeight/TexMaxHeight.
  • Internals have been rewritten, making it easier to later experiment with font backends others than stb_truetype or Freetype, and different ways to render fonts. Custom font loaders may be used.
  • Effective benefits, today:
    • Users of icons, Asian and non-English languages do not need to pre-build all glyphs ahead of time. Saving on loading time, memory, and also reducing issues with missing glyphs.
    • Specifying glyph ranges is not needed anymore.
    • **PushFontSize() may be used anytime. Adding/removing fonts is easier (but will be more easier later). **
    • It is easier to dynamically adjust variety font setting and change the font rendering backend at runtime while seeing result immediately.(Hard to see now as I didn't push many tools yet)
    • Packing custom rectangles is more convenient as pixels may be written to immediately.
    • Any update to fonts previously required backend specific calls to re-upload the texture, and said calls were not portable across backend. It is now possible to scale fonts etc. in a way that doesn't require you to make backend specific calls.
    • It is possible to plug a custom loader/backend to any font source.
    • General refactor should facilitate further experiments and improvements.

What is not part of this update yet (step 2, will come next)

Your first impression may be very dry: The examples are using the default bitmap fonts and are still not DPI aware.

  • Adding a more general scaling system that apply to e.g. style sizes.
  • Refactoring high-level fonts API: loading api, loading flags, sharing font sources and data between fonts, fonts fallbacks etc.
  • Reevaluating many other fonts features.
  • This is one of the backbone for better DPI and multi-DPI support, but still need to make all examples DPI aware.
  • Examples are still using the default pixelated font, "ProggyClean", also informally referred to as the Marmite/Vegemite of UI fonts: much loved and much hated.
  • Font sizes and many other aspects of layout are still pixel aligned. (#1858, #791) This might become optional, primarily to allow smoother zooming. Without the rounding it's possible to get incredibly smooth zooming. Note that Freetype's great Font Hinting feature by nature does align character widths to pixel, but we are aiming to design a system when animated/slow zooming could temporarily disable hinting.
  • CustomRects are already better as you can write your pixels immediately after registering a rectangle. But the whole system is still in flux as better multi-dpi support will likely need us to further redesign those API. Custom font glyphs are better handled by registering a custom ImFontLoader.
  • When the system is widely adopted, this should enable imgui code to easily bake custom symbols, shapes, icons and many other things on the fly. Because I am eager to be able to take advantage of this fully, my expectation is that we will accelerate phasing out support for old backends (normally we phase things out in 2 years, I may make this shorter).

Generally speaking, the design puzzle here has been to push toward improvements while providing a reasonable path for old code to function without major issues. I have iterated on this for countless hours to find what I hope is the best solution, but it's expected that people will run into issues.

ocornut avatar Mar 05 '25 17:03 ocornut

KNOWN ISSUES

  • [ ] General Scaling system is not yet solved/pushed. This only provides FONT SCALING. I am working on this, and the final solution may alter some details about font scaling.
  • [x] I haven't yet exposed many fonts features in examples and demo.
    • [x] I have NOT YET exposed the general satisfying "scale all the UI" widget in public code, because I want this to be tied to general scaling rather than font scaling.
    • But if you are curious you may toy with style.FontSize from inside Style Editor. Or bind io.FontGlobalScale to an interactive item and immediately see this in action: ImGui::DragFloat("io.FontGlobalScale", &ImGui::GetIO().FontGlobalScale, 0.05f, 0.5f, 5.0f);
  • [x] Examples are still not technically DPI aware yet (but it's much easier now). As I am still working on providing a transparent solution for general scaling, how much responsibility may be left to user is this undecided and being explored.
  • Using style.ScaleAllSizes() now applies to font to.
  • ~~Important: use of legacy io.FontGlobalScale, ImGuiConfigFlags_DpiEnableScaleFonts and SetWindowFontScale() should now all appear to work nicely. But be warned they can lead to confusion and feedback loop when using e.g. GetFontSize() as a base to request another size. This will be clarified and improved as I finish work on general scaling.~~
  • [x] Using multiple atlas simultaneously in the same context has not been tested yet.
  • Sharing a same atlas between two contexts is possible, with the following constraints:
    • Contexts sharing a same atlas must not be doing work in parallel.
    • Contexts sharing a same atlas must be using the same rendering backend.
  • [ ] No attempt to merge UpdateRects[] provided to backends yet. Should be easy.
  • [x] ImFont::AddRemapChar() doesn't work and may be obsoleted as I'm not sure anyone is using this.
  • [ ] Binding generators for C and other languages (dear_bindings, cimgui) may need to come up with a way to automatically turn ImTextureUserID into ImTextureID.
  • Backends (HELP WANTED)
    • [x] Backends: WGPU: Missing support (PR welcome!)
    • [ ] Backends: DX12, Vulkan: due to some legacy constructs, we are currently needlessly requiring for texture uploads to complete during rendering, which is not optimal. It may be hard to notice as texture uploads are rare. However, we can and should improve the backends to remove those blocking waits.
    • [x] Backends: SDLGPU: seems to glitch on OSX (bad sync?)

ocornut avatar Mar 05 '25 17:03 ocornut

Q: ImTextureID vs ImTextureRef ?

// ImTextureID = backend specific, low-level identifier for a texture uploaded in GPU/graphics system.
// [Compile-time configurable type]
// Overview:
// - When a Rendered Backend creates a texture, it store its native identifier into a ImTextureID value.
//   (e.g. Used by DX11 backend to a `ID3D11ShaderResourceView*`; Used by OpenGL backends to store `GLuint';
//         Used by SDLGPU backend to store a `SDL_GPUTextureSamplerBinding*`, etc.).
// - User may submit their own textures to e.g. ImGui::Image() function by passing the same type.
// - During the rendering loop, the Renderer Backend retrieve the ImTextureID, which stored inside
//   ImTextureRef, which is stored inside ImDrawCmd.
// Configuring the type:
// - To use something other than a 64-bit value: add '#define ImTextureID MyTextureType*' in your imconfig.h file.
// - This can be whatever to you want it to be! read the FAQ entry about textures for details.
// - You may decide to store a higher-level structure containing texture, sampler, shader etc. with various
//   constructors if you like. You will need to implement ==/!= operators.
// History:
// - In v1.91.4 (2024/10/08): the default type for ImTextureID was changed from 'void*' to 'ImU64'. This allowed backends requirig 64-bit worth of data to build on 32-bit architectures. Use intermediary intptr_t cast and read FAQ if you have casting warnings.
// - In v1.92.0 (2025/XX/XX): added ImTextureRef which carry either a ImTextureID either a pointer to internal texture atlas. All user facing functions taking ImTextureID changed to ImTextureRef
#ifndef ImTextureID
typedef ImU64 ImTextureID;      // Default: store up to 64-bits (any pointer or integer). A majority of backends are ok with that.
#endif

// Define this if you need 0 to be a valid ImTextureID for your backend.
#ifndef ImTextureID_Invalid
#define ImTextureID_Invalid     ((ImTextureID)0)
#endif

// ImTextureRef = higher-level identifier for a texture.
// The identifier is valid even before the texture has been uploaded to the GPU/graphics system.
// This is what gets passed to functions such as ImGui::Image(), ImDrawList::AddImage().
// This is what gets stored in draw commands (ImDrawCmd) to identify a texture during rendering.
// - When a texture is created by user code (e.g. custom images), we directly stores the low-level ImTextureID.
// - When a texture is created by the backend, we stores a ImTextureData* which becomes an indirection
//   to extract the ImTextureID value during rendering, after texture upload has happened.
// - There is no constructor to create a ImTextureID from a ImTextureData* as we don't expect this
//   to be useful to the end-user, and it would be erroneously called by many legacy code.
// - If you want to bind the current atlas when using custom rectangle, you can use io.Fonts->TexRef.
// - Binding generators for languages such as C (which don't have constructors), should provide a helper:
//      inline ImTextureRef ImTextureRefFromID(ImTextureID tex_id) { ImTextureRef tex_ref = { ._TexData = NULL, .TexID = tex_id }; return tex_ref; }
// In 1.92 we changed most drawing functions using ImTextureID to use ImTextureRef.
// We intentionally do not provide an implicit ImTextureRef -> ImTextureID cast operator because it is technically lossy to convert ImTextureRef to ImTextureID before rendering.
struct ImTextureRef
{
    ImTextureRef()                          { _TexData = NULL; _TexID = ImTextureID_Invalid; }
    ImTextureRef(ImTextureID tex_id)        { _TexData = NULL; _TexID = tex_id; }

    inline ImTextureID  GetTexID() const;   // == (_TexData ? _TexData->TexID : _TexID) // Implemented below in the file.

    // Members (either are set, never both!)
    ImTextureData*      _TexData;           //      A texture, generally owned by a ImFontAtlas. Will convert to ImTextureID during render loop, after texture has been uploaded.
    ImTextureID         _TexID;             // _OR_ Low-level backend texture identifier, if already uploaded or created by user/app. Generally provided to e.g. ImGui::Image() calls.
};

CHANGELOG

Moved to https://github.com/ocornut/imgui/releases/tag/v1.92.0

ocornut avatar Mar 05 '25 17:03 ocornut

Sounds great so far, but this one point worries me a little bit:

When the texture is out of capacity, a new texture is created and all previous glyphs packed into here. The texture may be larger or the same size, depending on which font size may be unused. The previous texture may be freed by the backend after in-flight renderings are done.

This sounds like glyphs (with "glyph" being glyph X of font Y at size Z) are always carried over, never discarded if unused. With zooming there will quickly be many different size variations, combined with scrolling through large texts (especially with Kanji since there are tons of them) this can spiral out of control fast and exceed maximum texture sizes.

I think it is necessary that glyphs that have not been submitted to any text function for at least N frames will be skipped when packing the new texture. But I'm pretty sure it is just the way you worded it and your actual implementation already takes care of this.

GamingMinds-DanielC avatar Mar 05 '25 19:03 GamingMinds-DanielC

It is the way I have worded it indeed, I will amend the description paragraph. When there is no room anymore, size variations that are not used may be evicted during repacking.

ocornut avatar Mar 05 '25 19:03 ocornut

As well as various small fixes/addition, I have temporarily added a scale widget + direction in the demo to make experimenting less dry. Image

ocornut avatar Mar 06 '25 19:03 ocornut

This is really great! It lets me delete a lot of the code I had written to deal with creating and using fonts of different sizes, and it's much faster and more flexible on top of that.

So far I've noticed only two issues (for which I can also create separate GitHub issues if you prefer):

  1. I create normal/bold/italic/bold+italic versions of all the fonts I use. This makes it so that I can quickly do e.g. PushFont(active_fonts.bold); [...] PopFont(); to have a heading text be bold. Right now this resets the font size back to what I initially specified when creating the font though.
PushFont(active_fonts.regular, 16.0f);
[...] // Later, in a nested function call:
PushFont(active_fonts.bold);
Text("This text gets shown with size 14.0f, which I created the font with originally.");
PopFont();
[...]
PopFont();

I saw that this is because of a temporary backwards compatibility measure in PushFont: font_size = font->Sources[0].SizePixels; // g.FontSize; If I change this line to font_size = g.FontSize;, the bold text is also shown at the new size (i.e. 16.0f in the example above).

  1. I have multiple OS windows, each with its own ImGuiContext, and I'm sharing the font atlas between all of these contexts. When I close one of the windows, I destroy its context and also call ImGui_ImplOpenGL3_Shutdown. This then deletes all the textures that are still in use in the other OS windows' contexts, which leads to the following assertion:
ImTextureUserID ImDrawCmd::GetTexUserID() const: Assertion `tex_id != ((ImTextureUserID)0) && "ImDrawCmd is referring to ImTextureData that wasn't uploaded to graphics system. Backend must call ImTextureData::SetTexUserID()!"' failed.

with the following stack:

[...]
#5  0x0000000001854a00 in ImDrawCmd::GetTexUserID (this=0x784a810) at imgui.h:3829
#6  0x00000000018fb67e in ImGui_ImplOpenGL3_RenderDrawData (draw_data=0x7b56bd0) at imgui_impl_opengl3.cpp:656
[...]

If I comment out the call to ImGui_ImplOpenGL3_Shutdown, the problem disappears, but then I'm leaking resources on the GPU.

bratpilz avatar Mar 07 '25 13:03 bratpilz

Thank you @bratpilz for your feedback.

(1) I haven't figured out the right design for this yet. To be forward-facing, it seems evident that PushFont(xxx) by default should preserve current font size, but this creates a problem with legacy code which defacto often used this to change font for the purpose of changing size. We can distinguish font_size ==0 from font_size < 0 to allow for two different behavior, but again the tricky part is to decide on the default one. I'll work on this.

(2) I think we could solve this by adding a reference count inside ImFontAtlas and having backend check this. I'll work on this too.

ocornut avatar Mar 07 '25 14:03 ocornut

I have pushed 3eefa4e which should solve the problem with sharing an atlas between context. For simplicity I had to take a shortcut of assuming that when multiple contexts are created, they all have their backend initialized/bound to the context.

ocornut avatar Mar 07 '25 15:03 ocornut

Question r.e supporting in custom backends, would it be possible for the pixel data to be kept alive until the backend agrees the texture is destroyed? Currently the data is freed as soon as the texture enters the ImTextureStatus_WantDestroy state, but its possible that the data could still be in use asynchronously (obviously relies on the whole "Textures blocks that have been used will never be rewritten" guarantee). Also fine if not! Just requires buffering

ZingBallyhoo avatar Mar 07 '25 19:03 ZingBallyhoo

Question r.e supporting in custom backends, would it be possible for the pixel data to be kept alive until the backend agrees the texture is destroyed? Currently the data is freed as soon as the texture enters the ImTextureStatus_WantDestroy state, but its possible that the data could still be in use asynchronously (obviously relies on the whole "Textures blocks that have been used will never be rewritten" guarantee). Also fine if not! Just requires buffering

Out of curiosity could you describe your use case? Dx12/Vulkan tend to use upload buffer which would require a copy anyhow. Trying to get as much data as possible to take on better decision (eg should this be opt-in for certain backends, or not).

ocornut avatar Mar 07 '25 20:03 ocornut

but this creates a problem with legacy code which defacto often used this to change font for the purpose of changing size.

Instead of modifying PushFont to not change the font size, maybe it could be done from the font configuration end instead? ImFontConfig::SizePixels is more of a default size now, right? Maybe if a font has a size of 0 that means it doesn't have one and it should use whatever is current.

As far as the default font size goes: Maybe a new style variable can be added for that? Or maybe there's just enforcement that the default font must always have an explicit size. (Specifying font size to AddFontFromFileTTF feels legacy with dynamic fonts so the former might make the most sense IMO.)

PathogenDavid avatar Mar 07 '25 20:03 PathogenDavid

Out of curiosity could you describe your use case?

I'm not @ZingBallyhoo and none of these situations are important to me personally, but I can imagine at least a few scenarios where someone might want the texture data to stick around after it's marked as WantDestroy:

  • Support for UMA GPU architectures (where the separate upload buffer can be skipped, EG: using ID3D12Resource::WriteToSubresource)
    • (Examples of UMA GPUs are integrated GPUs, consoles, modern MacBooks, and I think most/all embedded GPUs-EG: phones/tablets)
  • Engines where uploading a texture kicks off a job (and the copying to the upload buffer is done asynchronously)
  • Engines which keep the CPU-side memory around for reuploads (EG: to recover from device removal or as part of a manual memory eviction strategy)

(That being said I don't think it's totally unreasonable to tell backends that they don't own the texture memory and therefore shouldn't hold onto it, but it also currently looks like it wouldn't be that hard to say it's an (opt-in?) responsibility of backends to call ImTextureData::DestroyPixels.)

PathogenDavid avatar Mar 07 '25 21:03 PathogenDavid

Out of curiosity could you describe your use case?

Main thread -> render thread syncronization. Only the render thread can manage GPU resources, so in order to upload data it either needs to be buffered into per-frame allocs or be (effectively) immutable & outlive the lifetime of the render frame. Buffering is fine of course and happens everywhere (e.g imgui vtx/idx data), but the setup is very close to not requiring it which is why I ask.

it wouldn't be that hard to say it's an (opt-in?) responsibility of backends to call ImTextureData::DestroyPixels.)

My thought was to just move it to be deleted upon reaching the Destroyed state (decided by the backed) instead of the WantDestroy state (decided by imgui usually). Most backends will be immediately setting textures to that state when requested anyway so no semantic change there.

ZingBallyhoo avatar Mar 08 '25 00:03 ZingBallyhoo

Here's my function (Go language) to load a text font merged with an icons font. Icons font is given a smaller font size, but it ignores it and uses the text font size. Both text and icons being the same font size, makes icons too big when used within the same text line.


//go:embed embed/fonts/RobotoCondensed-Medium.ttf
var embededFontRegularBytes []byte

//go:embed embed/fonts/fa-light.ttf
var embededIconsFontBytes []byte

func createFonts() {
	fontSize := float32(20)
	iconsFontSize := fontSize * 0.6

	// Create font config, which will merge icons font with a previous text font
	fontIconsConfig := NewFontConfig()
	fontIconsConfig.SetMergeMode(true)
	fontIconsConfig.SetName("Font Awesome 5 Icons")
	defer fontIconsConfig.Delete()

	imguiIO.Fonts().AddFontFromMemoryTTFV(embededFontRegularBytes, fontSize, 0)
	imguiIO.Fonts().AddFontFromMemoryTTFV(embededIconsFontBytes, iconsFontSize, fontIconsConfig)
}

the-goodies avatar Mar 09 '25 19:03 the-goodies

@PathogenDavid

Instead of modifying PushFont to not change the font size, maybe it could be done from the font configuration end instead? ImFontConfig::SizePixels is more of a default size now, right? Maybe if a font has a size of 0 that means it doesn't have one and it should use whatever is current.

Merged fonts may need different base scale so that needs to be expressed in some way of another (it could be a scale factor relative to main font). That's exactly the case in the issue that just got reported. I'll come up with something.

@ZingBallyhoo I'll find a solution to your problem.

@the-goodies:

Here's my function (Go language) to load a text font merged with an icons font. Icons font is given a smaller font size, but it ignores it and uses the text font size. Both text and icons being the same font size, makes icons too big when used within the same text line.

Thank you for reporting this. It's indeed a bug that happened during a rework last month. I've pushed a quick fix 24be548 now (I will rework that fix and merge it into earlier commits later).

ocornut avatar Mar 09 '25 20:03 ocornut

I have pushed 3eefa4e which should solve the problem with sharing an atlas between context. For simplicity I had to take a shortcut of assuming that when multiple contexts are created, they all have their backend initialized/bound to the context.

Thanks! It works without asserting now.

bratpilz avatar Mar 10 '25 07:03 bratpilz

@ZingBallyhoo I have confirmed that I see no issue deferring destruction of the pixel buffer to after backend has acknowledged it, so pushed that change 0f7c7bc.

ocornut avatar Mar 10 '25 09:03 ocornut

just tried this branch and encountered a crash that I'm not sure if it is by design or not. Call fonts.Clear() and followed by load font causes a crash, like this:

Image

(this happened in the old logic, where clear and reload fonts if needed)

stacktrace, but line numbers seem inaccurate
 reframework::global_exception_handler (C:\Projects\REFramework\src\ExceptionHandler.cpp:56)
  e:\steamlibrary\steamapps\common\monsterhunterwilds\_storage_\dinput8.dll + 0x8a4d7

 UnhandledExceptionFilter
  C:\Windows\System32\KERNELBASE.dll + 0x134027

 memset
  C:\Windows\SYSTEM32\ntdll.dll + 0xa5818

 _C_specific_handler
  C:\Windows\SYSTEM32\ntdll.dll + 0x8ce46

 _chkstk
  C:\Windows\SYSTEM32\ntdll.dll + 0xa28bf

 RtlRaiseException
  C:\Windows\SYSTEM32\ntdll.dll + 0x52554

 KiUserExceptionDispatcher
  C:\Windows\SYSTEM32\ntdll.dll + 0xa13ce

 ImFontAtlasBuildUpdateLinesTexData (C:\Projects\REFramework\dependencies\imgui\imgui_draw.cpp:3528)
  e:\steamlibrary\steamapps\common\monsterhunterwilds\_storage_\dinput8.dll + 0x83fdfd

 ImFontAtlasBuildInit (C:\Projects\REFramework\dependencies\imgui\imgui_draw.cpp:4161)
  e:\steamlibrary\steamapps\common\monsterhunterwilds\_storage_\dinput8.dll + 0x83e663

 ImFontAtlas::AddFont (C:\Projects\REFramework\dependencies\imgui\imgui_draw.cpp:3002)
  e:\steamlibrary\steamapps\common\monsterhunterwilds\_storage_\dinput8.dll + 0x837788

 ImFontAtlas::AddFontFromMemoryTTF (C:\Projects\REFramework\dependencies\imgui\imgui_draw.cpp:3144)
  e:\steamlibrary\steamapps\common\monsterhunterwilds\_storage_\dinput8.dll + 0x838461

 ImFontAtlas::AddFontFromFileTTF (C:\Projects\REFramework\dependencies\imgui\imgui_draw.cpp:3129)
  e:\steamlibrary\steamapps\common\monsterhunterwilds\_storage_\dinput8.dll + 0x837f92

 REFramework::update_fonts (C:\Projects\REFramework\src\REFramework.cpp:1431)
  e:\steamlibrary\steamapps\common\monsterhunterwilds\_storage_\dinput8.dll + 0xd518c

lingsamuel avatar Mar 11 '25 01:03 lingsamuel

I've noticed another small issue with how I'm handling my contexts. I have a main context that owns the font atlas, but which does not have an actual OS window associated with it. All of the actual OS windows' contexts share the font atlas from the main context. Previously I had not been calling NewFrame and EndFrame on the main context since it was not necessary, but now I have to do so, because otherwise old font textures don't get deleted, since the other contexts all don't own the atlas.

SetCurrentContext(main_context);
GetIO().DisplaySize = { 1.0f, 1.0f }; // Avoid assert.
NewFrame();
EndFrame();

Having to set the DisplaySize here is a bit annoying, but other than that it's not a big workaround.

It looks like an alternative solution would be for me to call ImFontAtlasUpdateNewFrame manually, but then I have to set atlas->RendererHasTextures like in UpdateTexturesNewFrame:

auto &io = GetIO();
io.Fonts->RendererHasTextures = (io.BackendFlags & ImGuiBackendFlags_RendererHasTextures) != 0;
static int frame = 1;
ImFontAtlasUpdateNewFrame(io.Fonts, frame++);

bratpilz avatar Mar 11 '25 08:03 bratpilz

I've noticed another small issue with how I'm handling my contexts. I have a main context that owns the font atlas, but which does not have an actual OS window associated with it. All

EDIT That's similar to the shutdown issue I referring to with: "For simplicity I had to take a shortcut of assuming that when multiple contexts are created, they all have their backend initialized/bound to the context.". I hoped it won't happen. A better fix is not trivial for now, I'll keep a note of it but it doesn't seem top priority for now. Thanks for letting me know!

ocornut avatar Mar 11 '25 10:03 ocornut

No problem! I also just realized I lied a little bit. The main context does in fact have the glfw and opengl3 backends initialized, and there is a permanently hidden OS window attached to it (just to have a GL context to perform texture operations with).

bratpilz avatar Mar 11 '25 11:03 bratpilz

@lingsamuel

just tried this branch and encountered a crash that I'm not sure if it is by design or not. Call fonts.Clear() and followed by load font causes a crash, like this:

I have pushed a fix e9b4e5255f613300d51751ce1abe15a8d1499263. Thanks for reporting!

ocornut avatar Mar 11 '25 18:03 ocornut

I am updating the backend from an old version to the latest to get rid of glyph range. And I encountered crashes within ImGui_ImplDX12_UpdateTexture, at about:

cmdQueue->ExecuteCommandLists(1, (ID3D12CommandList* const*)&cmdList);

After debugging, I found that the legacy ImGui_ImplDX12_Init signature appears to create its own command queue:

HRESULT hr = device->CreateCommandQueue(&queueDesc, IID_PPV_ARGS(&init_info.CommandQueue));
IM_ASSERT(SUCCEEDED(hr));

So I basically copy the init info logic in the obsolete function:

    ImGui_ImplDX12_Init(device, 3, bb_vr_desc.Format, m_d3d12.srv_desc_heap.Get(),
            m_d3d12.get_cpu_srv(device, D3D12::SRV::IMGUI_FONT_VR), m_d3d12.get_gpu_srv(device, D3D12::SRV::IMGUI_FONT_VR))

// ↑ old usage, ↓ changed

    ImGui_ImplDX12_InitInfo init_info = {};
    init_info.Device = device;
    init_info.CommandQueue = g_framework->get_d3d12_hook()->get_command_queue(); // <- use own command queue
    init_info.NumFramesInFlight = 3;
    init_info.RTVFormat = bb_desc.Format;
    init_info.DSVFormat = DXGI_FORMAT_UNKNOWN;
    init_info.SrvDescriptorHeap = m_d3d12.srv_desc_heap.Get();
    init_info.LegacySingleSrvCpuDescriptor = m_d3d12.get_cpu_srv(device, D3D12::SRV::IMGUI_FONT_BACKBUFFER);
    init_info.LegacySingleSrvGpuDescriptor = m_d3d12.get_gpu_srv(device, D3D12::SRV::IMGUI_FONT_BACKBUFFER);

    ImGui_ImplDX12_Init(&init_info)

The crash is gone.

lingsamuel avatar Mar 12 '25 01:03 lingsamuel

@lingsamuel Can you clarify why it would crash with the command-queue created by legacy ImGui_ImplDX12_Init() ? Please note that using LegacySingleSrvCpuDescriptor mode you won't be able to create multiple textures.

I am pushing extra comments de4f77b and making the legacy call disable ImGuiBackendFlags_RendererHasTextures: d19fdc6

ocornut avatar Mar 12 '25 14:03 ocornut

I'm sorry, but I'm not very familiar with imgui internal implement, so I'm afraid I can't find the root cause of the issue.

lingsamuel avatar Mar 14 '25 10:03 lingsamuel

Where it is crashing, and in dx12 debug mode do you see any errror message in the console etc?

ocornut avatar Mar 14 '25 10:03 ocornut

Huge thanks for this beta branch! I'm one of those people that really have a huge need for dynamic glyph rasterization (and also nagged you in this PR ;) The mangled text and crashes with some glyphs seem to be fixed compared to the mentioned PR.

After migrating to this branch and adapting some code, all seems fine. One thing I've noticed is that ImGui::InputText with ImGuiInputTextFlags_Password seems to cause a crash, specifically at font_size = font->Sources[0].SizePixels; // g.FontSize;, in ImGui::PushFont() Steps to reproduce: Run DX9 example, navigate to Widgets->Text Input->Password Input, crash occurs.

That's the only issue I found so far. Again, HUGE thanks for this branch, it's very very much appreciated! :)

Stifler9000 avatar Mar 16 '25 17:03 Stifler9000

@Stifler9000 - I apologize I broke the password fields again (even my tests were failing). Pushed a fix now.

ocornut avatar Mar 17 '25 16:03 ocornut

@rodrigorc this is a reply for https://github.com/ocornut/imgui/issues/8466#issuecomment-2730578186

Thanks this is great feedback (although we ended up chatting in the wrong thread for it ^^)

  • On master I pushed a small change a7dc18477 moving ImTextureID to its own section of imgui.h. The point was that in the new branch the arguably more verbose section is now clearly scoped, which I believe can encourage people to read it.
  • I have pushed a bunch of comments and small tweaks in 9b30b07a1e64851f66b7631730ed3c5c830c66d9 (if you feel like reviewing them it'd be great).

In details:

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

I moved them out of the internal section.

When drawing an Image from the font atlas, the demo does basically: ImGui::Image(io.Fonts->TexData->GetTexID(), ...);

I think it was misleading that the demo did that. I tweaked the code to use Fonts->TexID and reworked comments.

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?

Yes, atlas->TexID is here for legacy reason. Conceptually it also align with the idea that user code (apart from backend) should never need to care/know about ImTextureData. The demo exceptionally uses ImTextureData::Width/Height because it aims to display the entire atlas, but it is not something needed in normal. Using atlas->TexID is however necessary and useful when using custom rects.

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:

As both are never set together I believe the test is fine? I added a comment.

ImTextureUserID ImTextureID::GetTexUserID(), but alas that doesn't exist. There is instead ImTextureUserID ImDrawCmd::GetTexUserID(), that I find a bit out of place.

You are correct.

  • A function somehow needs to exists in ImDrawCmd for legacy reason.
  • It would make sense to also add it to ImTextureID if only because it makes sense structurally and enhance self documentation.
  • My general reluctance was because over the years I converged over favoring the use of raw structures as much as possible. It's not entirely raw since there's a constructor, but I don't want methods scattered in two many types. It gives a particular flavor to the library, is harder to extend, and doesn't always map well to other languages etc.

Last but now least, what I believe is the most important thing:

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.

I agree. I actually spend a long time mulling over this and even tho I choose the current direction, it kept rubbing me off until now and I should consider your gut feeling reaction on this as valuable. In my old notes I saved, I'm noticing now that part of my reasoning was muddled by (1) the problem for language bindings, for which I originally thought users would always want to pass the lower-level value (but I debunked this yesterday realizing it was incompatible with using custom rect packed into atlas) and (2) not finding a great name for the structure.

I'm totally open to reworking this. Current:

  ImTextureID     = higher-level identifier to a texture (already or about to be uploaded)
  ImTextureUserID = backend low-level identifier for uploaded texture
  ImTextureData*  = texture specs, pixels, status etc. // for backend and core

The name I used in older version of the branch was:

  ImTexture       = higher-level identifier to a texture (already or about to be uploaded)
  ImTextureID     = backend low-level identifier for uploaded texture, same name as before
  ImTextureData*  = texture specs, pixels, status etc. // for backend and core

For some reason I'm not too happy about ImTexture because it seems like a "heavy" struct, and it makes the relationship with ImTextureData confusing. Maybe ImTextureRef would be adequate? I'm also not against renaming ImTextureData if needed.

ocornut avatar Mar 17 '25 21:03 ocornut