Backends: DX12: Reuse command list and allocator for texture management

[RT2Code]

This PR reuses the command list and allocator instead of recreating them each time a texture needs to be updated. Command lists and allocators are not meant to be created frequently, they should be reused whenever possible for optimal performance.

Additional clarifications (from removed comments):

Can upload buffer be reused?
Upload buffers cannot be resized, if a different size is required, a new buffer must be created.

Can we use the command list from user? (pass ID3D12GraphicsCommandList* to ImGui_ImplDX12_UpdateTextures)
No. ImGui_ImplDX12_RenderDrawData only appends commands to the user-provided command list. The actual rendering happens later, when the user executes that command list. However, ImGui_ImplDX12_UpdateTexture needs to render immediately to the texture, which requires its own command list and allocator that can be submitted to the command queue right away.

[ocornut]

Thanks for your PR (and thanks for splitting them in small chunks. As DX12/Vulkan are notoriously tricky to review and get right).

Can upload buffer be reused?
Upload buffers cannot be resized, if a different size is required, a new buffer must be created.

The intent was: can keep a same upload buffer and only recreate one when we need to grow it?

However, ImGui_ImplDX12_UpdateTexture needs to render immediately to the texture, which requires its own command list and allocator that can be submitted to the command queue right away.

Why does it need to render immediately to the texture? can't this be queued in the same command list what will do the rendering?

I think I can merge this either way now, but interested in your answer for those.

[ocornut]

Merged with minor tweaks as 217bc44. I merged in master branch.

[RT2Code]

The intent was: can keep a same upload buffer and only recreate one when we need to grow it?

Yes, it's possible. It's also probably not necessary to unmap it until destruction, I'll do some tests and make a new PR.

Why does it need to render immediately to the texture? can't this be queued in the same command list what will do the rendering?

Honestly, I only assumed it. This PR is a byproduct of #8961, I wasn't trying to fix texture updates initially. Since DX12 and Vulkan backends rendered immediately, I thought it was required for some reason.

Now that I have spent more time reading the code, I don't see why it wouldn't be possible to reuse the user command list here. Two upload buffers are required, which can be added to the frame render buffers. Then, ImGui_ImplDX12_UpdateTexture needs access to both the command list and the draw data (to retrieve the current viewport and frame context). The new signature would be ImGui_ImplDX12_UpdateTexture(ImTextureData* tex, ImDrawData* draw_data, ID3D12GraphicsCommandList* graphics_command_list). Is this signature change acceptable?

[ocornut]

The intent was: can keep a same upload buffer and only recreate one when we need to grow it?
Yes, it's possible. It's also probably not necessary to unmap it until destruction, I'll do some tests and make a new PR.

I kept the comment so to be honest this is super low priority to investigate: it's perfectly fine to wait until this is detected as a problem (which might never be the case) before investigating. Since texture updates are rare enough it would be only worthwhile to bother only if creating an upload buffer may be slow, and if reusing a bigger than necessary upload buffer is not a problem.

The new signature would be ImGui_ImplDX12_UpdateTexture(ImTextureData* tex, ImDrawData* draw_data, ID3D12GraphicsCommandList* graphics_command_list). Is this signature change acceptable?

Hmm, I don't like it very much that it makes them different from other backends.
I guess it would only make sense then if we detect inefficiencies. Note that the backend already has fixmes next to the SetEventOnCompletion/WaitForSingleObject combo. Rather than reusing the command buffer for the sake of it we should simply have a look in a profiler and see if there's no excessive undesirable contention somewhere.

A good test may be to use a different PushFont(font, size) value every frame with an atlas small enough that it keeps reuploading changes (which is a worst case scenario already) and see if there's perf waste in the backend. But likewise I'd consider it low priority unless someones complain. But if you are in a DX12 mood feel free to investigate :)

[RT2Code]

I am pretty much in a DX12 mood, as I am switching my game engine from DX11. :)

I’ll review texture update performance. Using a second command list/allocator is not an issue and should have no performance impact at all. The only potential concern is the forced wait (effectively a GPU flush), but if texture updates are infrequent this should be acceptable.