D3D12 implementation

[onatto]

I have some flickering errors for the first frames or so but it's working.

[ocornut]

Thanks! Looks like a really tedious API to work with.
Would you mind changing the license to MIT to be on par with the rest of ImGui+examples for simplicity? Or you can leave out the license and it'll follow the rest of the code.

[onatto]

Happy to contribute! :) I don't think any licence is necessary, it can follow the rest of the code.

[onatto]

Oh and also I forgot to implement changing texture id's between draw calls.

[mmozeiko]

This D3D12 example doesn't work on my machine (Win 10 Pro, i7-4790K, GTX 970). Debug output shows error:

D3D12 ERROR: ID3D12Device::CreateGraphicsPipelineState: Root Signature doesn't match Vertex Shader: Shader CBV descriptor range (RegisterSpace=0, NumDescriptors=1, BaseShaderRegister=0) is not fully bound in root signature
 [ STATE_CREATION ERROR #688: CREATEGRAPHICSPIPELINESTATE_VS_ROOT_SIGNATURE_MISMATCH]

This happens if you build 64-bit code. Then this initialization is wrong:

    { D3D12_ROOT_PARAMETER_TYPE_CBV, {0,0}, D3D12_SHADER_VISIBILITY_ALL },

{0,0} initializes DescriptorTable member of rootParameter[1], but it should initialize Constants member. Why this works on 32-bit code? Because DescriptorTable and Constants are in union. And in 32-bit code members of these structures align on each just fine. But in 64-bit code there is 32-bit padding after first member DescriptorTable. So second 0 doesn't initialize second member of Constants structure correctly and everything fails later.

Oh and dxgidll variable is not used - you are linking with dxgi.lib file. I would suggest to do same thing with d3d12.dll file and its functions.

[onatto]

Sorry for the late response, just saw this, thanks for the info, those dang unions!

[ghost]

Is there a reason this hasn't been merged? I ask because I wish to use it and just wanted to make sure there were no known issues I should be aware of beforehand. Thanks so much for contributing this, it's a massive help.

[ocornut]

Hasn't been merged because 1/ I didn't have time to look, 2/ comment above suggest there are issues 3/ frankly I am not super convinced of the value of a DX12 example. DX12 is rather complex and anybody using that would have their own high-level layer/engine over it, at which point it would be preferable to just call your own functions and not native DX12 ones.

The additions of examples may be noise more than anything, it is preferable to look at the simplest OpenGL example and adapt that to your own renderer architecture rather than use the raw render API.

[ocornut]

So as per you and Julian interventions in others I understand why this may be of use as is.
I'd imagine you can grab the files before it is merged and give it a go and post the results here.

(The reason I'm a bit slow and reluctant to add new examples is that even minimal, they all requires some amount of maintenance/sync at some point).

[ghost]

Ah yes, I misread the comments sorry. I thought the flickering issue was resolved but it is not. I made a (very short, due to my awful upload speed) video which shows it. https://youtu.be/DNBiMhEWb9A

I was able to work around it by removing the DXGI_PRESENT_RESTART flag (simply passing 0 instead) on the call to Present on line 511 of main.cpp, however that dropped the FPS down to double my refresh rate, instead of what it was before which was about 8x.

[onatto]

Yes, thank you for the video, maybe it will help identify the problem. Unfortunately I haven't found a workaround either except for rendering with vsync. There is a fence before Present() which should prevent this behavior, so I am very curious why this happens.

[ratchetfreak]

Any reason why you shared a single CommandBuffer between RenderDrawLists and the main loop?

It sounds simpler if the render target and the commandAllocator are the only things shared and the hypothetical game code can then use an independent set of commandbuffers.

[ratchetfreak]

This for loop can be merged with the previous one. So you copy the render data in parallel with filling the command buffer.

also you never unmapped g_uploadBuffer

[mmozeiko]

also you never unmapped g_uploadBuffer

g_uploadBuffer is on D3D12_HEAP_TYPE_UPLOAD heap. Resources on upload heap can be permanently mapped. No need to call unmap. This is in the docs: https://msdn.microsoft.com/en-us/library/windows/desktop/dn788712.aspx#Advanced_Usage_Models

[ratchetfreak]

I think you can stop the flickering if you have proper buffering

https://github.com/Microsoft/DirectX-Graphics-Samples/tree/master/Samples/D3D12HelloWorld/src/HelloFrameBuffering

There microsoft has a created a CommandAllocator per frame in the swapchain and only resets the allocator when it's time to render to the frame it's connected to.

In addition you'll need to round robin buffer the upload heap.

[jdm3]

Why is the root signature created and set in main.cpp instead of imgui_impl_dx12.cpp?

Also, commandList->SetGraphicsRootDescriptorTable(0, ...); needs to be set per-draw in ImGui_ImplDX12_RenderDrawLists() in order to support more than one texture (not once for the entire GUI). This doesn't happen in the example app but is necessary in general.

This implementation will only support rendering one frame at a time, as you need to version your geometry data in order to support multiple concurrent frames in flight. main.cpp indeed waits for each render to complete before starting the next frame, but this is a pretty bad constraint for performance and probably isn't appropriate for all imgui users.

IMO, it would be nice if the imgui_impl_dx12.h API matched the other API implementations more-closely to help integrate it with existing imgui applications (e.g. _Init, _Shutdown, _NewFrame, _InvalidateDeviceObjects, etc).

[sherief]

Is there still interest in merging this in?

[ocornut]

@sherief I wouldn't mind merging a DX12 sample but it looks like this one has several pending questions/issues to be looked at. The PR code is maybe usable locally (I don't know) but isn't mergable quality. If someone knowledgeable of DX12 want to have a serious look at some of the things discussed. They may also do line-by-line compare of the code with the DX11 version to be more strict about coding-style and mirroring other examples structures.

[jdm3]

FWIW, I also have a DX12 implementation that I wrote because I didn't notice that this pull request existed yet:

https://github.com/jdm3/imgui

It may serve as an interesting comparison to the above.

[RomanTruhanov]

DirectX 12 example build failed, error C2338: Can't pack descriptor handle into TexID

[mmozeiko]

You should compile as 64-bit code (amd64). Example code assumes void* can store UINT64 type (ptr member of D3D12_GPU_DESCRIPTOR_HANDLE structure).

[erictuvesson]

@jdm3

should be called before drawing so we set the font texture.
g_pd3dDevice->CreateShaderResourceView(g_pFontTextureResource, NULL, g_hFontSrvCpuDescHandle); here

And the back buffer format should be changeable, for example I am using RGB and not BGR as you are. here

Edit: I am not sure about setting the texture like that thought.

[CrohnsAndMe]

Hi Guys, I put this version into my project and noticed when i call to render the ImGui it makes my screen brighter overall. Would there be any reason for this to happen?

[jdm3]

I assume you mean the window IMGUI is rendering is brighter and not the whole screen right? Also, is this brighter than a different backend like D3D11? Check to make sure you are using the same rendertarget/swapchain formats (in particular, SRGB vs. RGB).

[jdm3]

@ChillyFlashER: The font texture's SRV is created in ImGui_ImplDX12_CreateFontsTexture().

@ChillyFlashER: Is there IMGUI API for changing the backbuffer format? I believe the expectation is that you just change the backend to your desired the swapchain format.

[ocornut]

@onatto @jdm3 Thanks, this is merged now! Better late than never :)
I applied various minor tweaks/fixes to match recent changes in the example, some coding style tweaks, and called WaitForLastSubmittedFrame() in main.cpp before the shutdown so debug layer won't error (as we discussed privately).

I'm currently working on another branch where the examples/ files are being redesigned so we can easily separate the platform (win32/glfw etc.) from the renderer (dx12/vulkan) so I wanted this in the master branch.

The 32-bit ImTextureId is still an open problem, will look at that soon.

[ocornut]

@jdm3 Would using D3D12_GPU_DESCRIPTOR_HANDLE* instead of D3D12_GPU_DESCRIPTOR_HANDLE be a more elegant solution? Would it be a problem for users (who might not always have trivial access to a handle value stored in a place that would persist for 1-2 frames?)

Also see #1641

[jdm3]

That definitely sounds like a lower-touch option. imgui_impl_dx12.cpp already has storage for the D3D12_GPU_DESCRIPTOR_HANDLE so it would just be a matter of storing &g_hFontSrvGpuDescHandle into the TexID instead of g_FontSrvGpuDescHandle and then dereferencing it for SetGraphicsRootDescriptorTable(...).

[ocornut]

We don’t really care about the font texture, the question is will it be sufficiently practical for user displaying their own texture to pass a pointer there (aka their descriptor can’t be on the stack etc and must have a lifetime of one frame).

[jdm3]

Ah, ok, my typical imgui usage is more basic than that I guess :) IMO 64-bit TextureId is a more-ideal solution, but requiring the user to maintain storage and taking a pointer to that is a reasonable, easier change. Freeing the memory too early is a risk, and would be hard to debug, but hopefully clear usage documentation is enough to prevent that. Vulkan/D3D12 users need to be pretty aware of these parallel execution and lifetime issues anyway, so it shouldn't be too painful to add descriptor handles into whatever their "in-flight" data structure is.

[pdoane]

My recommendation is to keep it as void* but consider adding a frame allocator to ImGui (maybe on the draw list?). Most larger projects will already have one, but for smaller things I've run into it a few times. E.g. passing data to a draw list callback. If someone had a descriptor on the stack and knew that it would be valid for the frame, they could use the ImGui frame allocator as a convenience.

[ratchetfreak]

a frame allocator that clears on newFrame sounds like a good idea, preallocate some memory for it (configurable) and you can allocate some more when the allocation gets too big,

though will you add running destructors on clear to that?

Unless it's clearly documented that it's a flat clear I can foresee a lot of leaks because someone thought destructors of pushed types would also run.

[ocornut]

Thanks for your feedback all.

It feels like we can keep the void* and use D3D12_GPU_DESCRIPTOR_HANDLE * at a first step, but ultimately I guess we could/should rework the examples API to allow easily replacing the "texture binding" function. I'll post about that in the topic #1641 soon.

I have pushed an experimental Viewport branch (#1542) which includes two things:

• Started refactoring the examples, splitting the platform and renderer parts (e.g. "win32" and "dx12" are two separate files, which allows combining variants easily).
• The bindings for platform and renderer can optional provide extra function to allow imgui to create additional viewport, which means that the renderer binding in this situation needs to create rendering context / framebuffer / whatever resources applies. Effectively (and you can try this today if you build the DX11 example from that branch) you can extract imgui windows outside the main window seamlessly. Down the line it will combine very well with Docking/Tabs features.

The branch is at:
https://github.com/ocornut/imgui/tree/viewport
(This is experimental at this point, exact API to be determined, right now I'll focused on implementing the features then we can rework the syntax and API)

The reason I'm posting here is that both the DX12 and Vulkan renderer in this branch don't have support for the "viewport renderer" api yet.

In:
https://github.com/ocornut/imgui/blob/viewport/examples/imgui_impl_dx12.cpp

From Line 640 I have pushed an empty skeleton of what would be needed to support it in DX12, there are basically 5~6 functions to fill.

static void ImGui_ImplDX12_CreateViewport(ImGuiViewport* viewport)
static void ImGui_ImplDX12_DestroyViewport(ImGuiViewport* viewport)
static void ImGui_ImplDX12_ResizeViewport(ImGuiViewport* viewport, int w, int h)
static void ImGui_ImplDX12_RenderViewport(ImGuiViewport* viewport)
static void ImGui_ImplDX12_SwapBuffers(ImGuiViewport* viewport)

I left commented out code based on the DX11 renderer to make it clearer what is expected here.

I may try to look at it at some point later, but if someone knows DX12 and is excited by this you may want to see how to add the missing code for DX12 and Vulkan and see if the current API hold or how we should improve it.

[giladreich]

This is probably obvious to some of you :) But I'll add this info here in case someone looking for this in the future.
As discussed in #1641 for alternative solutions, to allow x32bit compilation at the current state of imgui(v1.69), add the following definition before the imgui headers includes:

#define ImTextureID D3D12_GPU_DESCRIPTOR_HANDLE *

Then remove the static_assert from here:
https://github.com/ocornut/imgui/blob/master/examples/imgui_impl_dx12.cpp#L372

and it should compile perfectly fine.

There are few other issues with imgui-dx12-example, when resizing the back-buffer of the main-window. it kind of squeezing the fonts and the imgui windows until the mouse is released. Also multi-view support still not available. Other than that, it's quiet usable :)

[sonictk]

...There are few other issues with imgui-dx12-example, when resizing the back-buffer of the main-window. it kind of squeezing the fonts and the imgui windows until the mouse is released. Also multi-view support still not available. Other than that, it's quiet usable :)

I'm actually experiencing these problems right now (I'm using ResizeBuffers on my swap chain rather than re-creating the swap chain itself on every resize like how the example is doing it). How do people get around this? I'm not super up-to-date on all the stuff available within ImGui; is there something quick and easy to 1. tell ImGui it needs to auto-resize the widgets to fit the new client width/height 2. update the hot zones for widgets?

[ocornut]

Closing this old issue :)

• I made the examples compile in 32-bits and documented the way to do it in various place d8d58b0

• "when resizing the back-buffer of the main-window. it kind of squeezing the fonts and the imgui windows until the mouse is released

This is a general issue with many/most windows application, the way the Win32 API works is that during a resize it goes into a modal loop inside Windows code and the application cannot react, so your notice most back-ends are doing that. There are several ways to fix it, e.g. using a WM_TIMER, and it's well documented on the internet but right now out of the scope of your examples application to avoid dragging in noise into the application.

Resizing programmatically (e.g. via dear imgui decorations in a multi-viewport context) doesn't suffer from it and should be totally smooth.

Also multi-view support still not available.

This has been added since.