dearimgui / dear_bindings

C header (and language binding metadata) generator for Dear ImGui
MIT License
221 stars 12 forks source link

ResetRenderState #56

Closed pdoane closed 8 months ago

pdoane commented 8 months ago

I noticed that ImDrawCallback_ResetRenderState is removed from the bindings - I'm not sure why?

One challenge is that the definition isn't legal for the C ABI so Zig complains about it - pointers need to be properly aligned so -1 won't compile (-8 would though!).

Maybe we could add a new function instead? e.g. ImDrawList_AddResetCallback(ImDrawList* self)

ShironekoBen commented 8 months ago

ImDrawCallback_ResetRenderState is actually still present in the C header, but as you observe it's removed from the JSON metadata.

The main reason I did that was because it felt like it didn't really make sense to metadata consumers - as it's a define it just gets reported as a raw string, which means in order to do anything with it you have to be able to parse (ImDrawCallback)(-1) into something meaningful for your language, which is rather messy if you try to do it properly rather than just hardcoding the value. Plus as you observe the whole notion of "-1 as a function pointer" is a pretty foreign concept to a lot of languages.

And on top of that it's (almost) entirely a user-side construct (i.e. ImGui doesn't actually do anything with it, although a couple of the backends do), so I felt that it was simplest just to remove it and let the user define their own constant if they needed it.

Adding a function would be one option, although it should really be added to the main ImGui API for consistency, I suspect... another would be to quietly rewrite the define so that it is at least reported as "-1" (without the cast), giving the user a much easier road to parse it.

I'm not too familiar with Zig but would I be right in thinking that adding a function is the preferred (only?) way to handle this there if the limitations on pointers means that -1 can't be represented in one?

pdoane commented 8 months ago

Generally using this forum to discuss the bindings challenges - even if the right solution in the end would be an ImGui change. Apologies if that's out of scope.

An additional challenge that I didn't think about is that my backend is now also written in Zig, so even if there was an AddResetCallback, my backend wouldn't know how to use it.

If there's interest in improving this at the ImGui level, a different solution would be to add to ImGuiIO:

    ImDrawCallback BackendResetRenderState;

and bypass the whole magic value entirely. I can workaround this by just providing a function in my backend API for use with AddDrawCallback.

Also, I do agree this is a little out of scope to ImGui, and I think the whole ResetRenderState mechanism may have come out of an old discussion Omar and I had years ago. Use of draw callbacks typically requires user code to know something about the backend implementation choices (e.g. i'm using DX11), but not how/where that ImGui backend implementation exists in the code base. So it helps a bit on structure that user code doesn't need to have a dependency on the backend ImGui renderer.

pdoane commented 8 months ago

Okay, I'm starting to remember some of the issues this was trying to solve as I tried implementing the above idea... a backend implementation like DX11 has this signature:

IMGUI_IMPL_API void     ImGui_ImplDX11_RenderDrawData(ImDrawData* draw_data);

So it is possible to provide a backend function that can be used as a DrawCallback. But other backends have additional state, e.g.:

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

So an implementation of Reset needs access to the graphics_command_list and that's how we landed on the magic value. But - a DrawCallback probably needs this too! I'll open an issue on the ImGui side as this definitely is getting out of bindings territory :)

pdoane commented 8 months ago

ImGui issue here - https://github.com/ocornut/imgui/issues/6969