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

Update to use AddKeyEvent, supporting the full key range from v1.87 #197

Merged
merged 6 commits into from
Apr 17, 2022

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Mar 12, 2022

Dear ImGui v1.87 has expanded its support for keyboard and gamepad buttons, and replaced the interface for backends to report the keys. The old way keeps working OK but is deprecated and has big limitations, thus already making "imgui-demo" run with some missing functionality when it's run through "imgui-sfml".

This change implements the new approach. So, (almost) all the keyboard keys and possible gamepad events will be properly reported to ImGui and possible to query.
Fixes #196.
Fixes #174.

I implemented this by following what changes the SDL backend has undergone and trying to make equivalent changes here.


Here you can observe the missing events:
One can just run the default "imgui-demo" and in the current state of imgui-sfml it is missing a lot of information in the "Keyboard, Gamepad & Navigation State".
After this change it matches the SDL backend's behavior.

before after
https://user-images.githubusercontent.com/371383/158024702-86a606c2-aea7-4718-a494-6e5ec8b06510.mp4 https://user-images.githubusercontent.com/371383/158024705-83840c9a-26ad-41ad-86a5-073bf89f51fd.mp4

This is backwards-compatible in the sense that I'm pretty sure all user code will continue working as is.
In particular, both of these keep working the same:

ImGui::IsKeyDown(sf::Keyboard::Z) == ImGui::IsKeyDown(ImGui::GetKeyIndex(ImGuiKey_Z))

In the table below, "before" means without this change and "after" means with this change.

  ImGui 1.86 before ImGui 1.87 before ImGui 1.87 after
ImGui::IsKeyDown(sf::Keyboard::Z) Works Works Works
ImGui::IsKeyDown(ImGui::GetKeyIndex(ImGuiKey_Z)) Works Works Works
ImGui::IsKeyDown(ImGuiKey_Z) Always false Works Works

But Z was one of the keys that was possible to make ImGui aware of.
Q was not one of such keys, hence:

  ImGui 1.86 before ImGui 1.87 before ImGui 1.87 after
ImGui::IsKeyDown(sf::Keyboard::Q) Works Works Works
ImGui::IsKeyDown(ImGui::GetKeyIndex(ImGuiKey_Q)) Invalid code Always false Works
ImGui::IsKeyDown(ImGuiKey_Q) Invalid code Always false Works

From a different point of view, this is not backwards-compatible because it requires ImGui 1.87 to compile.
If the tables above had a column "ImGui 1.86 after", that would all be "imgui-sfml can't compile".


I also change the deadzone for sticks from 5% to 15%, because that seems much closer to what the SDL backend does.


Additions to the API are also up for discussion.

 IMGUI_SFML_API void SetJoytickDPadThreshold(float threshold);
 IMGUI_SFML_API void SetJoytickLStickThreshold(float threshold);
+IMGUI_SFML_API void SetJoytickRStickThreshold(float threshold);
+IMGUI_SFML_API void SetJoytickLTriggerThreshold(float threshold);
+IMGUI_SFML_API void SetJoytickRTriggerThreshold(float threshold);

In particular:
Should we keep the typo "Joytick" consistent?
Should we perhaps merge the L + R pairs of these functions into one function?

@oprypin oprypin changed the title Update to use AddEventKey, supporting the full key range from v1.87 Update to use AddKeyEvent, supporting the full key range from v1.87 Mar 12, 2022
@eliasdaler
Copy link
Contributor

eliasdaler commented Mar 28, 2022

Hello.
Wow, this is a fantastic PR, thanks a lot!

I'll take a look at it soon as I'm currently very busy with real life stuff... (from what I've seen, it looks correct, just needs some manual testing).

Which OSes did you test this on?

As for the "Joytick" - feel free to rename it to "Joystick". I have no idea how no one noticed that one before. API/ABI break is okay in our case (since Dear ImGui and SFML don't have make any ABI promises)

Should we perhaps merge the L + R pairs of these functions into one function?

Two functions are better than one, imo.

@oprypin
Copy link
Member Author

oprypin commented Mar 28, 2022

Which OSes did you test this on?

Only on Arch Linux (KDE, Xorg, NVIDIA)

how no one noticed that one before

I definitely noticed, but didn't say anything

@eliasdaler
Copy link
Contributor

Only on Arch Linux (KDE, Xorg, NVIDIA)

Okay, I see. I'll test it on Ubuntu and Windows 10 a bit later!

Please rebase your branch onto the latest master, I'll approve the Gitlab Actions runs (CI was broken)

@eliasdaler
Copy link
Contributor

Please sync with master again - there are some merge conflicts now. :)

@eliasdaler
Copy link
Contributor

Tested on Ubuntu 20.04 and Windows 10 - works fine.
The one failing job is CI's issues, not caused by this branch.
Merging... thank you!

@eliasdaler eliasdaler merged commit 9c545ad into SFML:master Apr 17, 2022
oprypin added a commit to oprypin/crystal-imgui-sfml that referenced this pull request Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing support for new events in ImGui v1.87 State of modifier keys is wrong in certain scenarios
2 participants