ocornut/imgui

Fork 0

mirror of https://github.com/ocornut/imgui.git synced 2026-11-01 05:10:41 +00:00

ImGuiMod_Shortcut is not "portable" #5923

New issue

Closed

opened 2022-11-26 13:30:17 +00:00 by oprypin · 5 comments

oprypin commented

2022-11-26 13:30:17 +00:00

(Migrated from github.com)

Version/Branch of Dear ImGui:

Version: 1.89.1
Branch: master

My Issue/Question:

See here:
ocornut/imgui@a8df192df0/imgui.h (L1456-L1460)

In its header file, as part of the core API, imgui now has a value that depends on the system that it's being compiled on.

The first issue I see with this is that this might not be representable in bindings to other languages. Or if it is, the binding generators can be not smart enough to discern this and instead just paste the value of the system on which the generator happens to be running.

cimgui already straight up defines it as the non-Mac value, presumably because that's where it was generated.
cimgui/cimgui@68483775b3/cimgui.h (L643)
Maybe this is an oversight that can be fixed, so cc @sonoro1234. But still there's no good way to expose it in the JSON info files of cimgui.

Based on this, in crystal-imgui I intend to also ignore the Mac value for the foreseeable future. Though I could manually code the conditional into my generator.

The 2nd concern again comes from the fact that this is in a header file, so even in C++, the value gets baked into the destination executable, not into imgui (built as a shared library). Or well, it probably also gets baked into imgui, which is even less good.

Either way, because I believe this is the first ever occurrence of a platform-specific API value in a header, maybe it warrants some discussion or reconsideration.

Perhaps turning ImGuiMod_Shortcut into its own unique value (but then handling the difference on .cpp side) would be the approach that's less prone to cause issues down the line. Although... that's not possible to apply to equality checks against the value :/

**Version/Branch of Dear ImGui:** Version: 1.89.1 Branch: master **My Issue/Question:** See here: https://github.com/ocornut/imgui/blob/a8df192df022ed6ac447e7b7ada718c4c4824b41/imgui.h#L1456-L1460 In its header file, as part of the core API, _imgui_ now has a value that depends on the system that it's being compiled on. --- The first issue I see with this is that this might not be representable in bindings to other languages. Or if it is, the binding generators can be not smart enough to discern this and instead just paste the value of the system on which the generator happens to be running. _cimgui_ already straight up defines it as the non-Mac value, presumably because that's where it was generated. https://github.com/cimgui/cimgui/blob/68483775b3d7594eb14d296ec002ac11683882c9/cimgui.h#L643 Maybe this is an oversight that can be fixed, so cc @sonoro1234. But still there's no good way to expose it in the JSON info files of _cimgui_. Based on this, in _crystal-imgui_ I intend to also ignore the Mac value for the foreseeable future. Though I could manually code the conditional into my generator. --- The 2nd concern again comes from the fact that this is in a header file, so even in C++, the value gets baked into the destination executable, not into _imgui_ (built as a shared library). Or well, it probably *also* gets baked into _imgui_, which is even less good. --- Either way, because I believe this is the first ever occurrence of a platform-specific API value in a header, maybe it warrants some discussion or reconsideration. Perhaps turning `ImGuiMod_Shortcut` into its own unique value (but then handling the difference on `.cpp` side) would be the approach that's less prone to cause issues down the line. Although... that's not possible to apply to equality checks against the value :/

👍 1

sonoro1234 commented

2022-11-26 15:19:00 +00:00

(Migrated from github.com)

Perhaps turning ImGuiMod_Shortcut into its own unique value (but then handling the difference on .cpp side) would be the approach that's less prone to cause issues down the line.

> Perhaps turning `ImGuiMod_Shortcut` into its own unique value (but then handling the difference on `.cpp` side) would be the approach that's less prone to cause issues down the line. 1+

sonoro1234 commented

2022-11-26 15:36:11 +00:00

(Migrated from github.com)

But still there's no good way to expose it in the JSON info files of cimgui.

{
                    "name": "ImGuiMod_Shortcut",
                    "value": "ImGuiMod_Super",
                    "conditionals": [
                        {
                            "condition": "if",
                            "expression": "defined(__APPLE__)"
                        }
                    ]
                },
                {
                    "name": "ImGuiMod_Shortcut",
                    "value": "ImGuiMod_Ctrl",
                    "conditionals": [
                        {
                            "condition": "ifnot",
                            "expression": "defined(__APPLE__)"
                        }
                    ]
                },

It seems that dear_bindings is able to do it. The main reason is that does preprocessing itself with lex and yacc while cimgui takes the preprocessed output. That make things extraordinary more complex and prone to failures but is the way to go if you want to handle things like that. (That way still has to prove some things as imgui_internal, implot, etc parsing).
Initial cimgui_auto took less than a month to develop while dear_bindings seemed to take 18 months!!
And another question would be: Is it easy to use for a binding generator? (There are features in cimgui that seem too complicated for some binding generators so that not everybody use them)

The way to go for cimgui would be to make generation and compilation in the same machine. That is the same situation as with freetype, wchar32, obsolete functions or other preprocesor options (even options in imconfig.h)

> But still there's no good way to expose it in the JSON info files of _cimgui_. ```json { "name": "ImGuiMod_Shortcut", "value": "ImGuiMod_Super", "conditionals": [ { "condition": "if", "expression": "defined(__APPLE__)" } ] }, { "name": "ImGuiMod_Shortcut", "value": "ImGuiMod_Ctrl", "conditionals": [ { "condition": "ifnot", "expression": "defined(__APPLE__)" } ] }, ```` It seems that dear_bindings is able to do it. The main reason is that does preprocessing itself with lex and yacc while cimgui takes the preprocessed output. That make things extraordinary more complex and prone to failures but is the way to go if you want to handle things like that. (That way still has to prove some things as imgui_internal, implot, etc parsing). Initial cimgui_auto took less than a month to develop while dear_bindings seemed to take 18 months!! And another question would be: Is it easy to use for a binding generator? (There are features in cimgui that seem too complicated for some binding generators so that not everybody use them) The way to go for cimgui would be to make generation and compilation in the same machine. That is the same situation as with freetype, wchar32, obsolete functions or other preprocesor options (even options in imconfig.h)

sonoro1234 commented

2022-11-28 10:42:18 +00:00

(Migrated from github.com)

Just found https://github.com/ocornut/imgui/blob/master/imgui.h#L1928 where ConfigMacOSXBehaviors seemed to be the previous way of Dear ImGui handling MacOS differences.

Just found https://github.com/ocornut/imgui/blob/master/imgui.h#L1928 where `ConfigMacOSXBehaviors` seemed to be the previous way of Dear ImGui handling MacOS differences.

👍 1

ocornut commented

2022-11-28 13:47:13 +00:00

(Migrated from github.com)

Perhaps turning ImGuiMod_Shortcut into its own unique value (but then handling the difference on .cpp side) would be the approach that's less prone to cause issues down the line.

I will look into the possibility of making that change. I am not entirely sure it is easily .
I agree it can create complication for bindings users and generator so if we can simplify it it'd be better.

ConfigMacOSXBehaviors seemed to be the previous way of Dear ImGui handling MacOS differences.

Yes. This specific occurrence was more difficult to do at runtime. I agree it is inconsistent to use the define it was just much simpler. I realized that we almost never take advantage of the dynamic nature ConfigMacOSXBehaviors (for local or automated testing) so I thought I could take that shortcut.

> Perhaps turning ImGuiMod_Shortcut into its own unique value (but then handling the difference on .cpp side) would be the approach that's less prone to cause issues down the line. **I will look into the possibility of making that change.** I am not entirely sure it is easily . I agree it can create complication for bindings users and generator so if we can simplify it it'd be better. > ConfigMacOSXBehaviors seemed to be the previous way of Dear ImGui handling MacOS differences. Yes. This specific occurrence was more difficult to do at runtime. I agree it is inconsistent to use the define it was just much simpler. I realized that we almost never take advantage of the dynamic nature `ConfigMacOSXBehaviors` (for local or automated testing) so I thought I could take that shortcut.

ocornut commented

2022-11-29 18:16:30 +00:00

(Migrated from github.com)

I experimented with various approach, and decided toward implementing a redirect without storage:

Committed 1a497c2
It seemed simpler and less error-prone (in term of e.g. comparing mods flags).
io.KeyMods (which is technically commented as "internal") will never contains ImGuiMod_Shortcut. So code wishing to do a single check of io.KeyMods and support ImGuiMod_Shortcut would need to call ConvertShortcutMod(). Because ImGuiMod_Shortcut is new there's likely no regression.

The other approach, which I have also implemented and tried is to store the alias, it felt slightly more complicated and the only value of doing it was to make it easier to expose e.g. io.KeyMods with the 5-bits, but that can break some code. Perhaps exposing two variants of io.KeyMods with/without alias may be useful but now the first approach seems simpler.

(Also committed cc3a220 which is internal refactor only, designed to make it easy to switch between both approach)

Let me know if it works!

EDIT: Interestingly if we decide to store a variant of KeyMods with ImGuiMod_Shortcut inside, we can actually still do it with first approach anyway.

I experimented with various approach, and decided toward implementing a redirect without storage: - Committed 1a497c2 - It seemed simpler and less error-prone (in term of e.g. comparing mods flags). - `io.KeyMods` (which is technically commented as "internal") will never contains `ImGuiMod_Shortcut`. So code wishing to do a single check of `io.KeyMods` and support `ImGuiMod_Shortcut` would need to call `ConvertShortcutMod()`. Because `ImGuiMod_Shortcut` is new there's likely no regression. The other approach, which I have also implemented and tried is to store the alias, it felt slightly more complicated and the only value of doing it was to make it easier to expose e.g. `io.KeyMods` with the 5-bits, but that can break some code. Perhaps exposing two variants of `io.KeyMods` with/without alias may be useful but now the first approach seems simpler. (Also committed cc3a220 which is internal refactor only, designed to make it easy to switch between both approach) Let me know if it works! EDIT: Interestingly if we decide to store a variant of KeyMods with ImGuiMod_Shortcut inside, we can actually still do it with first approach anyway.

👍 1