Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra keys #2625

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add extra keys #2625

wants to merge 5 commits into from

Conversation

@thedmd
Copy link
Contributor

@thedmd thedmd commented Jun 14, 2019

This PR add definitions for keys A-Z, 0-9, F1-F12 and support for them in all platform backends.

Since support for every key is optional and depends on content of io.KeyMap it should not introduce any incompatibilities.

I found myself adding support for extra keys in every project that used keyboard shortcuts. So maybe it will be useful also for others.

Note: Marmalade goes up to F10. I didn't found constants for F11 and F12.

@ocornut ocornut added the inputs label Jun 14, 2019
@ocornut
Copy link
Owner

@ocornut ocornut commented Jun 14, 2019

Some question:
A-Z keys are not localized and this has been the main issue that has been blocking me (see #456, and from glfw/glfw#672 (comment)). How are you using them, I assume you are using them in a context where you don't cater for multiple keyboard layout?

I've been thinking about removing the indirection completely. While the intent was initially to allow people using their native defines to access keyboard functions, this bring on two issues: the code becomes not portable accross back-ends, and this is not workable when the underlying value are not small values (which is the case for SDL keycodes (not scancodes) and OSX inputs). So perhaps the solution would be to just get rid of that indirection and embrace providing a large ImGuiKey_ enum like most frameworks are using. I'd like to think about it discuss the design of that path and how it would break the IO api. I'd like to have this discussion because if we establish this is too big of a change, I'll leave it for 1.80 or even maybe 2.00 and maybe merge a PR like yours soon. If we establish there is a path for changing that IO system that wouldn't be too destructive for users, we may aim for that path directly.

( Linking to #2005, #1924 - I think I'll close #1924 as this pr is more suitable anyway )

@ocornut
Copy link
Owner

@ocornut ocornut commented Jun 14, 2019

Taking a tangeant of solving the localization problem I am seeing:
I looked into what it would take to get things right with GLFW (see new issue glfw/glfw#1502)

Since we are dealing with UI, I would like to clarify that I think the ImGuiKey_XXX values should correspond to translated keys, and we'll figure out a glfwGetKeyName()-based workaround for GLFW or simply will use untranslated keys.
But since the IO layer of dear imgui is convenient for many use, including uses that could want to rely on physical layout for some use of keyboard (e.g. WASD) we might want to actually switch to using two arrays, one for translated keys, one for untranslated ones?

@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Jun 14, 2019

I'm using Windows WM_* messages and Qt to feed IO with input data, so I never encountered problem with different keyboard layouts with ImGui.

That being said, there are some my initial thought on this topic.

I skewing into 'backend did the right thing' camp by feeding io.KeyMap with translated keys. Indirection doesn't look like an issue. Actually I find it handy, because I sometimes feed IO with events from Qt, sometimes from WM_* messages directly.

Access to physical keyboard layout for WASD seems to be out of the scope of the UI. It may be tackled by providing an optional IO extension + some convenience API on top and leaving heavy lifting on backend side.

I read related all linked issues and never encountered pattern already present in ImGui:

#ifdef __APPLE__
ImGui::SetNextShortcut(ImGuiKey_A, ImGuiModKey_Cmd);
#else
ImGui::SetNextShortcut(ImGuiKey_A, ImGuiModKey_Ctrl);
#endif
if (ImGui::Buton("Select All"))
{
    // ...
}

One may argue that define is not a convenient solution. I agree that it is not typing friendly. Another approach may be more convenient:

// after creating context
#ifdef __APPLE__
ImGui::DefineAction("SelectAll", ImGuiKey_A, ImGuiModKey_Cmd);
#else
ImGui::DefineAction("SelectAll", ImGuiKey_A, ImGuiModKey_Ctrl);
#endif

ImGui::SetNextAction("SelectAll");
if (ImGui::Buton("Select All"))
{
    // ...
}

Or pushing things even further:

// after creating context
// void DefineAction(const char* id, const char* label, int key, int mods);
#ifdef __APPLE__
ImGui::DefineAction("SelectAll", "Select All", ImGuiKey_A, ImGuiModKey_Cmd);
#else
ImGui::DefineAction("SelectAll", "Select All", ImGuiKey_A, ImGuiModKey_Ctrl);
#endif

// `##!<id>` Will make button query action details.
if (ImGui::Buton("##!SelectAll"))
{
    // ...
}

In any case I don't know for sure if there is a reasonable way of getting rid of platform dependent action descriptions. Maybe if ImGui predefine some common ones. User may always add more.

@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Jun 20, 2019

Should I move shortcut discussion elsewhere?

@ocornut
Copy link
Owner

@ocornut ocornut commented Jun 21, 2019

You may post the shortcut specific stuff in #456
(I'm terribly late with other work I'd like to make progress on soon - tables - so I'll leave this topic dangling but I think the key enum is something we can be looking at in the near/mid future).

@thedmd thedmd mentioned this pull request Jun 24, 2019
7 tasks
thedmd added a commit to thedmd/imgui-node-editor that referenced this pull request Jul 2, 2019
ImGuiKey_D and ImGuiKey_F are used in node editor only if they
are defined in ImGuiKey_ enum.

This commit can be removed when/if ocornut/imgui#2625
PR will be merged.
thedmd added a commit to thedmd/imgui-node-editor that referenced this pull request Jul 2, 2019
ImGuiKey_D and ImGuiKey_F are used in node editor only if they
are defined in ImGuiKey_ enum.

This commit can be removed when/if ocornut/imgui#2625
PR will be merged.
@ocornut ocornut force-pushed the ocornut:master branch to 54c49b5 Jul 2, 2019
@ocornut ocornut force-pushed the ocornut:master branch from 51085b5 to baae057 Jul 23, 2019
@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Nov 6, 2019

Green again.

@thedmd thedmd force-pushed the thedmd:feature/extra-keys branch from a2d8fa1 to e53b02c Nov 25, 2019
@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Nov 25, 2019

Rebased on 1.74

@ocornut
Copy link
Owner

@ocornut ocornut commented Nov 25, 2019

@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Nov 26, 2019

Sounds great!

Issue on GLFW does not look like moving forward. Are you planning to address the issue in ImGui or to allow GLFW to be whacky until fixed?

@ocornut
Copy link
Owner

@ocornut ocornut commented Nov 26, 2019

We'll do both, find the workaround on imgui_impl_glfw and pushes PR to GLFW for future versions.

fdfxalex added a commit to fdfxalex/imgui-node-editor that referenced this pull request Dec 31, 2019
ImGuiKey_D and ImGuiKey_F are used in node editor only if they
are defined in ImGuiKey_ enum.

This commit can be removed when/if ocornut/imgui#2625
PR will be merged.
@thedmd thedmd force-pushed the thedmd:feature/extra-keys branch from e53b02c to 0b6e3cb Apr 13, 2020
@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Apr 17, 2020

Rebased on 1.76.

Issues with GLFW and shortcuts don't seems to be around the corner. Ability to bind more keys to ImGui is something I find useful all the time regardless of translation issues.
Do you think this PR can get a pass?

@ocornut
Copy link
Owner

@ocornut ocornut commented Apr 18, 2020

Hello,

I will try to evaluate it. I don't necessarily need to have the GLFW etc patches in to merge it, but I'd like to know we have a safe and forward-compatible way to eventually allow both types of key mapping.

Because Dear ImGui apis are used for so many things including custom widgets, it seems viable to lay out a plan where both translated and untranslated keys are somehow available. When the use of a key relates strongly to the actual letter (e.g. shortcuts CTRL+S = Save) we'd want to use translated key. Then the use of a key relates to the physical position on the keyboard (QWSD) we want to use untranslated keys.

Without necessarily making a PR for it, if you can help me come up with a design that would satisfy this, we'll faster have the visibility we need to merge this. (If I merge this today I think we'll also increase the incentive for people to stop using their native keys indexes. Which is good because native keys indexes are problematic for portability.)

@thedmd thedmd force-pushed the thedmd:feature/extra-keys branch from 0b6e3cb to 3b067b8 May 23, 2020
@thedmd thedmd force-pushed the thedmd:feature/extra-keys branch from 3b067b8 to 6d91336 Jul 22, 2020
@ocornut ocornut force-pushed the ocornut:master branch from 09caf50 to 13258f5 Nov 12, 2020
@thedmd thedmd force-pushed the thedmd:feature/extra-keys branch from 6d91336 to 459469c Nov 28, 2020
@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Nov 28, 2020

Make it green again.

@thedmd thedmd force-pushed the thedmd:feature/extra-keys branch 3 times, most recently from 776c7d1 to 770a5ac Jan 5, 2021
@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Jan 6, 2021

Event more extra keys were added:

    ImGuiKey_None,
...
    ImGuiKey_Apostrophe,     // '
    ImGuiKey_Comma,          // ,
    ImGuiKey_Minus,          // -
    ImGuiKey_Period,         // .
    ImGuiKey_Slash,          // /
    ImGuiKey_Semicolon,      // ;
    ImGuiKey_Equal,          // =
    ImGuiKey_LeftBracket,    // [
    ImGuiKey_Backslash,      // \ (this text inhibit multiline comment caused by backlash)
    ImGuiKey_RightBracket,   // ]
    ImGuiKey_GraveAccent,    // `
    ImGuiKey_CapsLock,
    ImGuiKey_ScrollLock,
    ImGuiKey_NumLock,
    ImGuiKey_PrintScreen,
    ImGuiKey_Pause,
    ImGuiKey_KeyPad0,
    ImGuiKey_KeyPad1,
    ImGuiKey_KeyPad2,
    ImGuiKey_KeyPad3,
    ImGuiKey_KeyPad4,
    ImGuiKey_KeyPad5,
    ImGuiKey_KeyPad6,
    ImGuiKey_KeyPad7,
    ImGuiKey_KeyPad8,
    ImGuiKey_KeyPad9,
    ImGuiKey_KeyPadDecimal,
    ImGuiKey_KeyPadDivide,
    ImGuiKey_KeyPadMultiply,
    ImGuiKey_KeyPadSubtract,
    ImGuiKey_KeyPadAdd,
    ImGuiKey_KeyPadEnter,
    ImGuiKey_KeyPadEqual,
    ImGuiKey_LeftShift,
    ImGuiKey_LeftControl,
    ImGuiKey_LeftAlt,
    ImGuiKey_LeftSuper,
    ImGuiKey_RightShift,
    ImGuiKey_RightControl,
    ImGuiKey_RightAlt,
    ImGuiKey_RightSuper,
    ImGuiKey_Menu,

Summary of the support for the backends:

Backend Platform Comment
imgui_impl_allegro5.cpp Windows Print Screen is mapped, but key down is not reported. Windows do not send this event and Allegro does not try to fix it.
imgui_impl_glut.cpp Windows / Freeglut Support for: ',-./;=[\], grave key, modifiers (Freeglut extension) Not supported: Caps Lock, Print Screen, Scroll Lock, Num Lock, most of the numerical keyboard (digits are mapped), digits when Shift is pressed.
imgui_impl_glfw.cpp Windows / GLFW 3.3.2 Print Screen down/up is reported in very same frame, so ImGui does not have a chance to react to that. "AltGr" is untranslated to "Right Alt".
imgui_impl_marmalade.cpp - New keys are mapped to constants found on 'The Internet'. Marmalade is dead, SDK is an unobtanium.
imgui_impl_osx.mm macOS Support for ',-./;=[\], grave key, Num Lock, Scroll Lock, Print Screen, Pause, Menu Not supported: Numpad, modifiers, Caps Lock
imgui_impl_sdl.cpp Windows / SDL2 2.0.14 AltGr is reported as combination of Left Control and Right Alt, which is how Windows reports it.
imgui_impl_win32.cpp Windows Print Screen down event is not generated by system.
@thedmd
Copy link
Contributor Author

@thedmd thedmd commented Jan 6, 2021

As an extra extra new API function were introduced to help with testing:

    IMGUI_API const char*   GetKeyName(ImGuiKey imgui_key);                                     // returns English name of the key
    IMGUI_API int           FindImGuiKey(int user_key_index);                                   // finds ImGuiKey_* associated to specified user_key_index. Returns ImGuiKey_None if not found.

They may not stay and are kept in separate commits. They served they purpose so I can see what keys are pressed and if they are mapped correctly to ImGuiKey.
image

I imagine returning key name may be useful. : )

@maxint
Copy link

@maxint maxint commented Jul 14, 2021

@thedmd Thanks for this pull requesst! I just need to use extra keys (e.g. F1-F12) for my shortcut management plugin.
#456 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants