Challenges with ImDrawCallback design

[pdoane]

Version/Branch of Dear ImGui:

Version: 1.89.9
Branch: master

Back-end/Renderer/Compiler/OS

Design issue that applies to most backends.

My Issue/Question:

This issue started when implementing an ImGui backend in Zig that had challenges with the ImDrawCallback_ResetRenderState definition. While Zig has very good interop with C, it is pickier than typical C compilers about tricks that are out of spec. In particular, the C ABI requires that function pointers are aligned, so this definition is flagged as an error -- which in a sense is true as there cannot be a function pointer at -1.

Reviewing the Reset implementations, most require additional state objects:

static void ImGui_ImplDX11_SetupRenderState(ImDrawData* draw_data, ID3D11DeviceContext* ctx)

In this case, ctx can be derived from the BackendData and is not included in the entrypoint:

IMGUI_IMPL_API void     ImGui_ImplDX11_RenderDrawData(ImDrawData* draw_data);

Other APIs require additional variables to RenderDrawData:

IMGUI_IMPL_API void     ImGui_ImplDX12_RenderDrawData(ImDrawData* draw_data, ID3D12GraphicsCommandList* graphics_command_list);
IMGUI_IMPL_API void     ImGui_ImplWGPU_RenderDrawData(ImDrawData* draw_data, WGPURenderPassEncoder pass_encoder);

And this requirement is I believe how we landed on the design for ImDrawCallback_ResetRenderState. The inner rendering loop has access to all the information and can pass it on to the backend-specific reset implementation.

But there's a potentially bigger design issue - user callbacks need this same information! I've had various hacks to workaround this in the past, and it can be challenging/annoying to figure how to get the same values into the callbacks that are passed to RenderDrawData.

Here's a possible way to improve on both of these issues:

In imgui.h:

// backend-specific variables needed to render ImDrawData
struct ImRendererDrawContext;

// extra parameter added to ImDrawCallback
typedef void (*ImDrawCallback)(const ImDrawList* parent_list, const ImDrawCmd* cmd, ImRendererDrawContext* ctx);

struct ImGuiIO {
    // reset function is made available to application
    ImDrawCallback BackendRendererReset;
};

And a backend example:

// draw callbacks can cast ImRendererDrawContext to this...
struct ImGui_ImplDX12_DrawContext {
    ID3D12GraphicsCommandList* graphics_command_list;
};

void ImGui_ImplDX12_ResetRenderState(ImDrawData* draw_data, ImGui_ImplDX12_DrawContext* ctx)
{
    ImGui_ImplDX12_Data* bd = ImGui_ImplDX12_GetBackendData();
    ImGui_ImplDX12_RenderBuffers* fr = &bd->pFrameResources[bd->frameIndex % bd->numFramesInFlight];
    ImGui_ImplDX12_SetupRenderState(draw_data, ctx->graphics_command_list, fr);
}

This would be a breaking change, but is easy to fix and would likely simplify callback implementations for users.

[GamingMinds-DanielC]

Since the backend specific data you need to give to those backends is under your control, it is your responsibility to forward them to user callbacks if you need this data there. There already is a mechanism for that, the userdata pointer. You can use that to forward a pointer to whatever context is required. If you already use it for something else, you can pack that something together with a pointer to the extra context into another structure and forward a pointer to a filled instance of that structure.

As for the alignment issue... maybe a modified special value would help?

#define ImDrawCallback_ResetRenderState (ImDrawCallback)(-(int)sizeof(void*))

[pdoane]

Since the backend specific data you need to give to those backends is under your control, it is your responsibility to forward them to user callbacks if you need this data there

Yes, I agree this works - assuming that I have the data where I'm making the callback (which is where the challenging/annoying part comes in). I might be N levels deep in some ImGui code or off in another library. That code may know what the backend is but not have easy access to what render pass encoder it happens to be executing in.

This was just an idea that came up when considering alternative Reset designs that improves ergonomics elsewhere.

#define ImDrawCallback_ResetRenderState (ImDrawCallback)(-(int)sizeof(void*))

Yeah, I've tried that and it works. :)

Here's another approach. I'm not a fan of it as it's also not backwards compatible and doesn't help with getting access to the backend data.

// return true if you want the backend to reset state
typedef bool (*ImDrawCallback)(const ImDrawList* parent_list, const ImDrawCmd* cmd);

[ocornut]

I am right that there are two separate issues here?

(1)
ResetRenderState is a backend-agnostic convenience. I am happy to change those values to be pointer aligned if is fixes the immediate issue for a Zig compiler (assuming there's no easy and harmless way to keep the -1 in Zig).
(Not sure then of the value of storing the callback in io.BackendRendererReset ? I suppose if we had myriad of backend-agnostic features like ResetRenderState we would consider a different design, but if we can just "fix" that special pointer value we should be good?)

Are you happy that I push that change before solving the second, wider problem?

(2)
As for more general access to backend and render context data.

I think we can accept to schedule a breakage of ImDrawCallback signature at some point as the unbreaking fix is easy to apply to a custom backend and if it come bundled with recommended backend upgrade (probably along with the texture-update api in backends).

An hypothetical "fast-track" workaround could be to add a ImGuiContext* pointer in ImDrawList, from where advanced users could pull ImGuiIO fields including io.BackendRendererUserData today, that's not breaking and pretty easy/cheap to do.

The main problem that remain is how what data to expose.
While it is practically not a problem for custom-backend user to cast BackendRendererUserData to their known type and access whatever they need, it's a bigger problem for stock backends: what we currently store there is too fluctuating, private and implementation dependent to be sanely moved to the .h of all backends.

We could decide on a subset of that data and store that in a structure (what you called ImRendererDrawContext), and add this extra data as ImGuiIO::BackendRendererContext now and in ImDrawCallback later. We can then decide to copy whichever _RenderDrawData() parameters make sense to that structure as well. The structure becomes part of backend-specific public API.

Heck, instead of pulling draw_list->Context->IO->BackendRendererContext it may make even more sense to make it draw_list->BackendRendererContext:

imgui_impl_dx11.h

// For users of ImDrawList::AddCallback(): retrieve backend-specific renderer context in draw_list->BackendRendererContext
struct ImGui_ImplDX11_RendererContext
{
    ID3D11Device* pd3dDevice;
    ID3D11DeviceContext* pd3dDeviceContext;
};

However I'm presently not sure I can tell the desirable subset for each backend, since callbacks could do practically anything I'd be happier to move this forward based on precise needs. It could be that we first add support this mechanism for custom backends user, and shyly make that small struct visible at the top of each backends .cpp, waiting for actual users to state their needs.

[ocornut]

Linking to #5834

[pdoane]

I am right that there are two separate issues here?

Yes. Trying to solve the reset problem presented design options that might be useful in other ways.

I am happy to change those values to be pointer aligned if is fixes the immediate issue for a Zig compiler (assuming there's no easy and harmless way to keep the -1 in Zig)

There's no way to do it with -1 as a constant expression. Another workaround in Zig would be to "hide" it behind a C function - e.g. ImDrawCallback_GetResetRenderStateFn() which returns the magic value.

Not sure then of the value of storing the callback in io.BackendRendererReset ?

Then it could be an actual function, and not some magic value... but that line of thinking led to the second part of this discussion (how to access to the backend context).

Are you happy that I push that change before solving the second, wider problem?

I hadn't thought of the function call workaround until this morning - which I think seems better. Changing the magic value might just create more confusion for people wondering why it's happening.

An hypothetical "fast-track" workaround could be to add a ImGuiContext* pointer in ImDrawList
Heck, instead of pulling draw_list->Context->IO->BackendRendererContext it may make even more sense to make it draw_list->BackendRendererContext

Oh that's a good idea! I like the latter.

However I'm presently not sure I can tell the desirable subset for each backend

A reasonable starting point would be any additional parameters to the RenderDrawData() function. Those are the ones that have presented harder structural challenges across library boundaries.

[ocornut]

Changing the magic value might just create more confusion for people wondering why it's happening

As a immediate first step solution I don’t see any issue with it. We never provided ABI backward compat, and i’d argue very few people use this feature anyhow.

it may make even more sense to make it draw_list->BackendRendererContext

A small niggle is that would make backend write to ImDrawList which is a little odd, whereas writing to IO is very standard. Given the small amount of users I’m not sure whats best.

(For my general understanding of use cases, I’m curious about your specific use of callbacks. you may reach privately if its easier for you to share it that way)

[pdoane]

(For my general understanding of use cases, I’m curious about your specific use of callbacks. you may reach privately if its easier for you to share it that way)

The most common use case for me is rendering textures with premultiplied alpha (e.g. render target results from a 3d engine). I need to temporarily switch blend state and then restore back to the backend default when I'm done.

I've also toyed around with higher quality font rendering (e.g. with a gaussian filter in the pixel shader). I didn't continue with that path because of shader switching overhead, but I also didn't know about channels at the time which I think could make that reasonably efficient.

[ocornut]

I have pushed a change e94f95d (+ f890d85 to fix typos in the comments) to start exposing selected backend specific render state.
This should hopefully enable callbacks to do more things. I've added a comment in there to suggest open an issue if something useful is missing. e.g. DX11:

// [BETA] Selected render state data shared with callbacks.
// This is temporarily stored in platform_io.Renderer_RenderState during the ImGui_ImplDX11_RenderDrawData() call.
// (Please open an issue if you feel you need access to more data)
struct ImGui_ImplDX11_RenderState
{
    ID3D11Device*           Device;
    ID3D11DeviceContext*    DeviceContext;
    ID3D11SamplerState*     SamplerDefault;
};

Access with:

ImGui_ImplDX11_RenderState* render_state = (ImGui_ImplDX11_RenderState*)ImGui::GetPlatformIO().Renderer_RenderState;

[ocornut]

I will close this issue and other related ones to focus on whatever arise from this.

[ocornut]

I realize this issue is more general than e.g. changing sampler state, but more visibility: I have added a "Modifying Render State" section in the Texture/Image wiki page:
https://github.com/ocornut/imgui/wiki/Image-Loading-and-Displaying-Examples#modifying-render-state
I can imagine that maybe once we prove that all backends can do this, we can even consider adding some helper callbacks in the backends themselves... e.g. the ImDrawCallback_ImplDX11_SetSampler() function provided in that wiki page.

[ocornut]

With 98d52b7, AddCallback() now supports passing any amount of data that will be copied internally inside a buffer inside DrawList. This should make it easier to use callback for variety of usages.