dearimgui / dear_bindings

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

GetKeyIndex is defined regardless of IMGUI_DISABLE_OBSOLETE_KEYIO in cimgui.h #32

Closed JaedanC closed 1 year ago

JaedanC commented 1 year ago

In imgui.h

namespace ImGui
{
#ifndef IMGUI_DISABLE_OBSOLETE_KEYIO
    IMGUI_API ImGuiKey     GetKeyIndex(ImGuiKey key);  // map ImGuiKey_* values into legacy native key index. == io.KeyMap[key]
#else
    static inline ImGuiKey GetKeyIndex(ImGuiKey key)   { // ...impl }
#endif
}

When defining IMGUI_DISABLE_OBSOLETE_KEYIO, the GetKeyIndex function is not exposed to the API 😀.

However, in dear_binding's cimgui.h:

#ifndef IMGUI_DISABLE_OBSOLETE_KEYIO
CIMGUI_API ImGuiKey GetKeyIndex(ImGuiKey key);  // map ImGuiKey_* values into legacy native key index. == io.KeyMap[key]
#else
CIMGUI_API ImGuiKey GetKeyIndex(ImGuiKey key);
#endif  // #ifndef IMGUI_DISABLE_OBSOLETE_KEYIO

Even if IMGUI_DISABLE_OBSOLETE_KEYIO is defined the function is exposed with CIMGUI_API. Shouldn't the function be omitted if IMGUI_DISABLE_OBSOLETE_KEYIO is defined? This could also pose a problem if someone is compiling imgui as a dll (even though this isn't recommended), as the function will not be found when linking cimgui with the imgui dll.

My suggestion is to update the autogeneration script to exclude the GetKeyIndex function if IMGUI_DISABLE_OBSOLETE_KEYIO is defined similar to below.

#ifndef IMGUI_DISABLE_OBSOLETE_KEYIO
CIMGUI_API ImGuiKey GetKeyIndex(ImGuiKey key);  // map ImGuiKey_* values into legacy native key index. == io.KeyMap[key]
#endif // #ifndef IMGUI_DISABLE_OBSOLETE_KEYIO
ShironekoBen commented 1 year ago

Thanks for the report and apologies for the slow response!

I've been pondering this for the last few days and whilst I definitely agree that it's a bit confusing, I suspect the current behavior may actually be the "least wrong" way to handle this.

Basically, from what I can see, having GetKeyIndex() present when IMGUI_DISABLE_OBSOLETE_KEYIO is defined shouldn't cause any issues even if ImGui is in a DLL, as what will happen is that the C++ header definition will get inlined into cimgui.cpp (and thus function correctly without needing IMGUI_API), and the exported C API will have CIMGUI_API and therefore also work correctly.

As you say, it would be perfectly possible to exclude the function from the C API - and indeed I initially wrote some code that does exactly that - but on reflection I felt that whilst this would make things cleaner in some respects, it was also probably going to violate the principle of least surprise in that for anyone not aware of the background around this decision it would simply appear that there was a perfectly usable (albeit depreciated) function that exists in the C++ header but was for unknown reasons missing from the C API.

Conversely, as long as it actually works in the same manner as it does in C++ the presence of this function doesn't feel like it causes too big an issue (it's not recommended to use it, but the #define and comments make that clear to the user)... is there some case you've come across where it misbehaves or otherwise causes a problem?

JaedanC commented 1 year ago

Thanks for the response and for the clarification. I'm currently not experiencing any issues whether the function is there, however it was a touch confusing at first as to why the function appeared regardless of defining the macro.

I guess this just comes down to trying to keep dear_bindings as predictable as possible, and realistically, bindings built on top of dear_bindings can simply choose to omit the deprecated functions themselves. Your reasoning is very fair. I'm glad it's a feature, not a bug! 😅. And at the end of the day when imgui.h finally removes the deprecated functions, this will cease to be a concern at all.

Thanks for your clarification. Happy to close.