Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an ability to update font atlas texture #3761

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

thedmd
Copy link
Contributor

@thedmd thedmd commented Jan 26, 2021

PR add an ability to update font atlas texture. Based on feature/shadows.

There is an information about overall progress:

  • SDL, GLUT, GLFW3 and Allegro 5 backends were tested on Windows.
  • Metal backends was tested on macOS Big Sur.
  • Emscripten was tested on emcc 2.0.12.
  • Marmalade backend and example is not supported.

Backend

Progress: 15/15

Backend Status Comment
imgui_impl_win32.h No change required.
imgui_impl_vulkan.h There is ImGui_ImplVulkan_UpdateFontsTexture user can need to use in own code.
imgui_impl_sdl.h No change required.
imgui_impl_osx.h No change required.
imgui_impl_opengl3.h Done.
imgui_impl_opengl2.h Done.
imgui_impl_metal.h Done.
imgui_impl_marmalade.h Not supported.
imgui_impl_glut.h No change required.
imgui_impl_glfw.h No change required.
imgui_impl_dx12.h There is ImGui_ImplDX12_UpdateFontsTexture user need to call in own code.
imgui_impl_dx11.h Done.
imgui_impl_dx10.h Done.
imgui_impl_dx9.h Done.
imgui_impl_allegro5.h Done.

Examples

Progress: 20/20

Example Status Comment
example_allegro5 Works.
example_apple_metal Works.
example_apple_opengl2 Works.
example_emscripten_opengl3 Works.
example_glfw_metal Works.
example_glfw_opengl2 Works.
example_glfw_opengl3 Works.
example_glfw_vulkan Works. Use ImGui_ImplVulkan_UpdateFontsTexture and explicitly set ImGuiBackendFlags_RendererHasTexReload flag.
example_glut_opengl2 Works.
example_marmalade Not supported.
example_null No change required.
example_sdl_directx11 Works.
example_sdl_metal Works.
example_sdl_opengl2 Works.
example_sdl_opengl3 Works.
example_sdl_vulkan Works. Use ImGui_ImplVulkan_UpdateFontsTexture and explicitly set ImGuiBackendFlags_RendererHasTexReload flag.
example_win32_directx9 Works.
example_win32_directx10 Works.
example_win32_directx11 Works.
example_win32_directx12 Works. Use ImGui_ImplDX12_UpdateFontsTexture and explicitly set ImGuiBackendFlags_RendererHasTexReload flag.

@ocornut
Copy link
Owner

ocornut commented Feb 3, 2021

Thank you for the amazing work reviving this @thedmd.
Some quick thoughts (and we can discuss details in our chat later):

Given the simplified specs for "full reload" this looks good, but I would like us to make sure we have a sensible path for when we'll want to transition this to the fuller version (https://github.com/ocornut/imgui_private/issues/3)
They are two main difference:

1. Textures may have to be uploaded in _Render (to support dynamic glyphes).

I think it may be simpler/favorable to only try to upload at the end of the frame rather than also support the optimal timing for the initialization scenario or rare best-case scenario where we can do a full upload around NewFrame().

So presumably, the first ImplXXX_RenderDrawData() function call of each standard backend may be the one doing the upload if required.

(Advanced: to reduce latency on high-performing application, we want to permit custom backend to start uploading the texture after EndFrame() - without waiting for Render() or even ImplXXX_RenderDrawData(). This shouldn't be a problem at all from ImGui:: point of view. To test this scenario, I imagine some default backend may want expose a ~ImplXXX_UpdateTexture function which ImplXXX_RenderDrawData would call is texture is dirty, but we allow user to call that function. It's not 100% required that all default backend support it, but I still it would help convey the possibility and and test the scenario)

Timeline for Frame N:

  • Old/existing backends will still attempt to build the atlas in ImplXXX_NewFrame() - before ImGui::NewFrame() - and then upload texture. Should not be a problem.
  • ImGui::NewFrame() will need to build the atlas data if dirty because we need glyphs UV ready. (with new backends, ImGui::NewFrame will be the first piece of code updating this, with old backends, this is likely to be already built). Technically we don't even need the pixels to be rasterized at this point (but it's simpler to rasterize them).
  • As a result of forcing the atlas build in NewFrame(), the very useful assert IM_ASSERT(g.IO.Fonts->Fonts[0]->IsLoaded() in NewFrame() doesn't make sense anymore. Instead we can try to replace the assert with one that checks that if the texture was dirty on NewFrame..EndFrame N, someone should have called XXXGetTexData by the time NewFrame N+1 is called (because with new backends the right time for texture upload will be after ImGui::EndFrame or ImGui::Render)
  • Anytime after EndFrame(), the backend is allowed to get texture data and upload it. In default backends this will generally be done in ImplXXX_RenderDrawData().

2. We'll want (in future) to notify the back-end of partial update.

This was partially (and bit awkwardly) covered in the original version. I don't think we need to have it 100% ready now but we should explore the proof of concept API at leaast, to ensure a smooth transition from this PR version. Maybe we can even come to the realization that we can implement the API completely today, and have the back-end not fully honor the partial update, or something (not sure that's a good idea).
Maybe we need to separate into two flags ImGuiBackendFlags_RendererHasTexReload and ImGuiBackendFlags_RendererHasTexPartialUpdate ?

Unrelated (minor): why does ClearInputData() remove CustomRects.clear() and PackIdXXX clearing?

@rokups
Copy link
Contributor

rokups commented Feb 8, 2021

  1. Textures may have to be uploaded in _Render (to support dynamic glyphs).

Incremental atlas baking branch i am working on updates font atlas texture outside of imgui frame. Which means that for very first time an unknown glyph is encountered, a fallback character is rendered. It is invisible to naked eye at 60fps so probably is good enough.

@ocornut
Copy link
Owner

ocornut commented Feb 8, 2021

Which means that for very first time an unknown glyph is encountered, a fallback character is rendered. It is invisible to naked eye at 60fps so probably is good enough.

Sorry but this is not good enough.

We're working out a scheme with @thedmd to push this forward, it's going to work out :)

@rokups
Copy link
Contributor

rokups commented Mar 1, 2021

Turns out rasterizing glyphs immediately wasnt that big of a deal. I am not quite sure if locking font atlas during frame is warranted. It rebuilds just fine, granted it happens on main thread. The only issue i noticed is that build step intends to clear texture id, which is undesirable as ImDrawList::AddText() does IM_ASSERT(font->ContainerAtlas->TexID == _CmdHeader.TextureId). I am not sure whether this detail was overlooked or not, but apparently if we request font atlas update during this frame and we would like to use new atlas by the time this frame is rendering on GPU - we will have to update ImDrawCmd::TextureId of all draw commands with a new texture id.

@ocornut
Copy link
Owner

ocornut commented Mar 2, 2021

I am not quite sure if locking font atlas during frame is warranted.

Probably not anymore.

We will have to update ImDrawCmd::TextureId of all draw commands with a new texture id.

Yes this is why we changed all the render loop to use draw_cmd->GetTexID().

@@ -484,7 +516,7 @@ bool ImGui_ImplDX11_CreateDeviceObjects()
g_pd3dDevice->CreateDepthStencilState(&desc, &g_pDepthStencilState);
}

ImGui_ImplDX11_CreateFontsTexture();
ImGui_ImplDX11_UpdateFontsTexture();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we doing same in NewFrame, shouldn't we delete this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Work is being done on DX9 backend. Rest of them will be brought to sync after dust settle.

@@ -97,11 +97,15 @@ int main(int, char**)
if (done)
break;

ImGui::UpdateFontDemo();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like u forgotten to remove this, because it getting exposed only in imgui_demo.cpp, even as static

Copy link

@rollraw rollraw Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it working after some changes on UpdateFontDemo, seems it still under construction for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test with DX9. As it is the only one that was changed to match latest changes.

@thedmd
Copy link
Contributor Author

thedmd commented Jul 30, 2021

Well, status table went out of date after introducing ability to update texture mid-frame.

@ocornut
Copy link
Owner

ocornut commented Jul 30, 2021

Technically we also moved this to a private branch for now (but thedmd: you could push that branch back into your public copy)

@thedmd
Copy link
Contributor Author

thedmd commented Jul 31, 2021

I pushed private branch here. I expect most backends will not pass build test. DX9 however should build fine. This is still work in progress.

@ocornut ocornut force-pushed the master branch 2 times, most recently from 0c1e5bd to bb6a60b Compare August 27, 2021 19:10
@maximegmd
Copy link

Hello, we are interested in this PR, we are a bit stuck on a feature until this is merged maximegmd/CyberEngineTweaks#840

We cannot rebuild textures currently, we also believe we are experiencing a GPU crash because of fonts when there is a d3d12 reset, this PR should also address this issue.

Is there something we can do to help move this forward?

@vkedwardli
Copy link

@maximegmd Based on the commit history of https://github.com/ocornut/imgui/commits/features/range_select, I guess when range select is ready, v1.90 will be shipped and this PR would also be included.

@ocornut
Copy link
Owner

ocornut commented Jul 18, 2023

Unfortunately the 1.90 milestone goal was probably too optimistic for this so it’ll probably be a bit later. Right now indeed focusing on range-select and after 1.90 is shipped we can look at this.

@ocornut
Copy link
Owner

ocornut commented Jul 25, 2023

Rebased this on v1.89.8 WIP (18974), done only basic testing. (was a bit tedious because of the NULL->nullptr change in backend but should be all done now).
Briefly noticed that both DX10/DX11 have issue with "Rebuild every frame" in demo but this already happened before I rebased, we may have just left it as is. Won't look at it until post 1.90.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants