Reset dx12 render state after returning from a user callback.

[kudaba]

This change allows for rendering my 3d scene interleaved with imgui rendering so I don't need to render to an offscreen buffer.

[ocornut]

Hello Chris,

This would unfortunately be a breaking change for people who rely on callbacks to change their render state. #1639 was proposing a similar thing for OpenGL, see comments and particularly #1639 (comment).

I think we should standardize dummy callback value for that purpose, e.g. something like:

#define ImDrawCallback_ResetAllGfxState   (ImDrawCallback)(-1)

(And possibly finer grained variations later.)

It would be up to the user to push this callback to request restoration of graphics state.
Otherwise your own callback could be responsible for backing up and restoring those state. (I'm also noticing that the current _dx12.cpp code makes no attempt to backup and restore those modified state, which is an issue we ought to fix.)

The work required would be to implement that for all/most current renderers, which would require a bit of code refactoring.

-Omar

[kudaba]

I kept it as a local solution, but manually triggered, so I don't risk breaking all other renderers. The pattern will need to be replicated in each one if support is desired.

[kudaba]

I updated it to match the comment in #2452 so you can simply write:
ImGui::GetWindowDrawList()->AddCallback(ImDrawCallback_ResetState, NULL);

I also added an optional descriptor heap to the render function so it will also be reset. I was already doing this manually.

[ocornut]

Thanks!
I'll keep that as a reference to work something out along with #1639 and #2452.

I would keep using a hardcoded-value-casted-into-pointer (defined in imgui.h) in order to allow any user code to use them without having to include binding headers. It also allow the rendering those to be performing those actions locally with less cruft (no setting of globals).

Among related tasks:

• I don't have a solution right now but I also agree if would be nice if user callbacks could trigger this "state reset" themselves, in order to not always require the end-user to push two callbacks. In practice it isn't a big problem since users of callback are fairly advanced and are going to wrap their callback in high-level concepts ("RenderScene").
  (One possibility would be to make callback return another callback.)

• I have been asked for a way for custom renderer to pass local/rendering data to the callback, which would possibly mean changing the signature of ImDrawCallback with an extra void*. Which would be a breaking change. Not eager to do soon that but if we bundle related changes it could make sense. There's no big pressure making this change since there are all sort of ways the user could pass down this void*.

• Would there be use for other standard callbacks? e.g. Reset specific part of the GPU pipeline?

(Edited with a third point)

[ocornut]

Another issue with the PR constructed as is, is that if a user callback is the first element of the list draw, it will be executed without having the GPU state setup.

Likewise for a CustomCallback/ResetState/CustomCallback sequence.

The main problem here is to find the most elegant, most consistent and least confusing way to handle that for every rendering back-ends. And we have to be particularly considerate that those rendering functions are de-facto documentations for a lot of users.

The cleanest way may actually be to move that init code into a function?
e.g. Below for DX11. I don't know if that would easily work for every renderer.

static void ImGui_ImplDX11_ResetGpuState(ImDrawData* draw_data, ID3D11DeviceContext* ctx)
{
    // Setup viewport
    D3D11_VIEWPORT vp;
    memset(&vp, 0, sizeof(D3D11_VIEWPORT));
    vp.Width = draw_data->DisplaySize.x;
    vp.Height = draw_data->DisplaySize.y;
    vp.MinDepth = 0.0f;
    vp.MaxDepth = 1.0f;
    vp.TopLeftX = vp.TopLeftY = 0;
    ctx->RSSetViewports(1, &vp);

    // Setup shader and vertex buffers
    unsigned int vertex_buffer_stride = sizeof(ImDrawVert);
    unsigned int vertex_buffer_offset = 0;
    ctx->IASetInputLayout(g_pInputLayout);
    ctx->IASetVertexBuffers(0, 1, &g_pVB, &vertex_buffer_stride, &vertex_buffer_offset);
    ctx->IASetIndexBuffer(g_pIB, sizeof(ImDrawIdx) == 2 ? DXGI_FORMAT_R16_UINT : DXGI_FORMAT_R32_UINT, 0);
    ctx->IASetPrimitiveTopology(D3D11_PRIMITIVE_TOPOLOGY_TRIANGLELIST);
    ctx->VSSetShader(g_pVertexShader, NULL, 0);
    ctx->VSSetConstantBuffers(0, 1, &g_pVertexConstantBuffer);
    ctx->PSSetShader(g_pPixelShader, NULL, 0);
    ctx->PSSetSamplers(0, 1, &g_pFontSampler);

    // Setup blend state
    const float blend_factor[4] = { 0.f, 0.f, 0.f, 0.f };
    ctx->OMSetBlendState(g_pBlendState, blend_factor, 0xffffffff);
    ctx->OMSetDepthStencilState(g_pDepthStencilState, 0);
    ctx->RSSetState(g_pRasterizerState);
}

[...]

    ImGui_ImplDX11_ResetGpuState(draw_data, ctx);

    // Render command lists
    int vtx_offset = 0;
    int idx_offset = 0;
    ImVec2 clip_off = draw_data->DisplayPos;
    for (int n = 0; n < draw_data->CmdListsCount; n++)
    {
        const ImDrawList* cmd_list = draw_data->CmdLists[n];
        for (int cmd_i = 0; cmd_i < cmd_list->CmdBuffer.Size; cmd_i++)
        {
            const ImDrawCmd* pcmd = &cmd_list->CmdBuffer[cmd_i];

            if (pcmd->UserCallback == ImDrawCallback_ResetGpuState)
            {
                // Special callback to reset graphics state
                ImGui_ImplDX11_ResetGpuState(draw_data, ctx);
            }
            else if (pcmd->UserCallback != NULL)
            {
                // User callback (registered via ImDrawList::AddCallback)
                pcmd->UserCallback(cmd_list, pcmd);
            }
            else
            {
                // Apply scissor/clipping rectangle
                const D3D11_RECT r = { (LONG)(pcmd->ClipRect.x - clip_off.x), (LONG)(pcmd->ClipRect.y - clip_off.y), (LONG)(pcmd->ClipRect.z - clip_off.x), (LONG)(pcmd->ClipRect.w - clip_off.y) };
                ctx->RSSetScissorRects(1, &r);

                // Bind texture, Draw
                ID3D11ShaderResourceView* texture_srv = (ID3D11ShaderResourceView*)pcmd->TextureId;
                ctx->PSSetShaderResources(0, 1, &texture_srv);
                ctx->DrawIndexed(pcmd->ElemCount, idx_offset, vtx_offset);
            }
            idx_offset += pcmd->ElemCount;
        }
        vtx_offset += cmd_list->VtxBuffer.Size;
    }

[ocornut]

Pushed a test:
https://github.com/ocornut/imgui/compare/features/drawcallback_reset_render_state

Seems fairly clean and not adding too much extra cognitive load to the casual reader?

I think we ought to answer this question:
"Would there be use for other standard callbacks? e.g. Reset specific part of the GPU pipeline?"

Ping @bear24rw, @xor2k (sorry for resurrecting #1639 so late!)

[kudaba]

Thanks for feature branch. The only suggestion I have is to move the check for ImDrawCallback_ResetGpuState inside the check to see if UserCallback is valid, just to get rid of the extra if on each iteration (even though I added one in my version...)

What are you thoughts about adding the HeapDescriptor to the DX12 Render call so it can be reset by default without extra user code? I was interleaving ImGui rendering with my own rendering to reduce the number of offscreen buffers which meant switching to my own HeapDescriptors.

[ocornut]

Thanks for feature branch. The only suggestion I have is to move the check for ImDrawCallback_ResetGpuState inside the check to see if UserCallback is valid, just to get rid of the extra if on each iteration (even though I added one in my version...)

You're right I'll do that, mostly to keep all that code in a single block and not give too much importance of the callback paths (as for the casual user callbacks are not relevant).

What are you thoughts about adding the HeapDescriptor to the DX12 Render call so it can be reset by default without extra user code? I was interleaving ImGui rendering with my own rendering to reduce the number of offscreen buffers which meant switching to my own HeapDescriptors.

I would need more details, and probably need a crash course about what a HeapDescriptor is and how they are used or can be used. Similar topics have been brushed in the past for DX12/Vulkan issues rarely with enough details to allow me to take good enough decision (both DX12/Vulkan still have unsorted issues regarding how ImTextureId is handled, on which people seemingly not agree, and I am unable to make a decision lacking a deep enough understanding of the situation).

[kudaba]

Now that I'm reading it more it looks like I set things up wrong by isolating ImGui resources into their own static heap. Everyone seems to agree that rebinding Heaps is very costly so I will still need some mechanism to ensure that ImGui's font resources are available on the global heap so I don't need to rebind it.

[ocornut]

You may want to investigate how would using Image() ImDrawList->AddImage() etc. fit to your case.
It allows you to emit draw call refering to your own textures. The exact interpretation of what a "texture" is up to the binding.

[kudaba]

Looking at that was part of what made me start to rethink the whole process. I feel like I would need a hook to replace:

ctx->SetGraphicsRootDescriptorTable(1, (D3D12_GPU_DESCRIPTOR_HANDLE)&pcmd->TextureId);

with my own method of binding resources. Generally the texture is held in a cpu only heap and needs to be copied to the currently active gpu heap before being set, but that gpu heap is controlled by the application, not by ImGui.

[ocornut]

Also see the end of #301 (and #914, #2251)

What I would like to determine is if we could provide a renderer-agnostic hook (e.g. an optional function pointer in ImGuiIO) or if it is best hooked at the renderer level.

The later would require a little more cruft but would make it easier for each renderer to pass on local information that may be useful to the "texture" change. e.g. in DX12's command-list.

[kudaba]

I pushed a change to address the issue.

https://github.com/kudaba/imgui/tree/features/drawcallback_reset_render_state

My main concern was making it so whatever ImTextureID is being used for is consistent for all calls. This meant that the Font texture has to be created and set by the user when using a callback to set textures during rendering.

[ocornut]

I think from the moment you specify a set_texture_function we can leave it to the user to call Fonts->SetTexID() so that would remove an extra parameters from the Init function.

PS: It would be good is you made sure your editor supported EditorConfig so make your patches use 4-spaces instead of tabs.

[kudaba]

I'll get another version uploaded tomorrow with a global set texture function. I'll put some assertions in there to make sure that the user is setting the texture function and font texture ID before calling dx12 init to make sure they are in sync and to prevent the dx12 init from creating its own texture.

I can go ahead and update all the example renderers to use the new texture resolution method, but I'm not currently setup to be able to test anything beyond dx12 at the moment.

PS: I'm using Visual Studio 2017 which does support it, but apparently has some holes in it's implementation. I'll make sure the next change doesn't have this problem.

[ocornut]

I can go ahead and update all the example renderers to use the new texture resolution method, but I'm not currently setup to be able to test anything beyond dx12 at the moment.

Don't worry about this. The important part is nailing the design, once I'm happy with a solution I can apply it to other back-ends (and it is definitively more important for DX12 and Vulkan than many other back-ends).

[kudaba]

The branch is updated. One thing I couldn't decide on was if I should be more strict about setting both the font texture and texture set functions consistently. I was thinking of adding asserts to make sure they were both set or both unset during initialization but felt that might be too restrictive so I left it up to the user.

[ocornut]

@kudaba I may start backporting some of the ImGuiPlatformIO stuff from the docking/viewport branch so if we add new functions they can be always kept in there.
This may be stalled for a bit.

[ocornut]

Branch has been merged in now.
Using ImDrawList::AddCallback(ImDrawCallback_ResetRenderState) it is possible to request the renderer to reset render state. This is now supported by all example renderers.

[ocornut]

@kudaba
Sorry I forgot to be specific as this thread went to talk about two different topics.
I merged the ResetRenderState feature, but you also discussed above the possibility of reworking "texture setting.". Would you mind opening a new issue and/or PR for that separate discussion, based on your current conclusion? (It's ok if you don't have a satisfying solution for it yet, it's an open problem)

[kudaba]

I'll try to get a fresh pr up for the texture set callback sometime this week. Thanks for getting this in.