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

ImGuiIntegration: fix indexing into KeysDown #89

Closed
wants to merge 1 commit into from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Jan 12, 2022

Somewhat bandaid-y fix for #87. The tl;dr is that with imgui >1.86 ImGuiKey_* don't start at 0, so we need to subtract the first value to index io.KeysDown correctly.

This isn't a long-term fix since the whole KeyMap + KeysDown thing is deprecated and will be removed in the future.

Beginning with imgui 1.87 ImGuiKey_* values start at 512, which means we can't use them directly to index io.KeysDown (which has size 512, not coincidentally). This is a quick fix to make the legacy code work. Long-term we need to add support for the new key system introduced in 1.87 which lets us queue a full range of backend-agnostic ImGuiKey with AddKeyEvent().
@jvannugteren
Copy link

This fix works fine for my code.

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #89 (1205b87) into master (da3c1ab) will decrease coverage by 0.36%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   77.25%   76.88%   -0.37%     
==========================================
  Files          21       21              
  Lines         941      926      -15     
==========================================
- Hits          727      712      -15     
  Misses        214      214              
Impacted Files Coverage Δ
src/Magnum/ImGuiIntegration/Context.cpp 89.13% <100.00%> (-1.13%) ⬇️
src/Magnum/ImGuiIntegration/Context.hpp 85.07% <100.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da3c1ab...1205b87. Read the comment docs.

@ocornut
Copy link

ocornut commented Jan 13, 2022

I'm particularly interested in this as our change ocornut/imgui#4858 was designed to completely backward compatible, if any code change is required by a backend we have an issue on our side.

Seeing the PR I realize how this specific use would indeed cause a problem:

io.KeyMap[ImGuiKey_Tab] = ImGuiKey_Tab;

I will add an assert on dear imgui side if a legacy KeyMap[] entry is >=512 and document this specific use a breaking change on our side.

@ocornut
Copy link

ocornut commented Jan 13, 2022

I will add an assert on dear imgui side if a legacy KeyMap[] entry is >=512 and document this specific use a breaking change on our side.

I tested this and there was already a triggering assert (good), still going to update documentation to mark this use as breaking.

ocornut added a commit to ocornut/imgui that referenced this pull request Jan 14, 2022
@mosra mosra added this to the 2022.0a milestone Feb 3, 2022
@mosra mosra added this to TODO in GUI via automation Feb 3, 2022
@mosra
Copy link
Owner

mosra commented Feb 3, 2022

Hmm, so if I understand correctly, this is obsoleted by #93 (which is still a draft), right? Thus nothing to do for me with this one.

@pezcode
Copy link
Contributor Author

pezcode commented Feb 3, 2022

Hmm, so if I understand correctly, this is obsoleted by #93 (which is still a draft), right? Thus nothing to do for me with this one.

Correct, I'll close this to avoid any confusion 🙇‍♂️

@pezcode pezcode closed this Feb 3, 2022
GUI automation moved this from TODO to Done Feb 3, 2022
This was linked to issues Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GUI
  
Done
Development

Successfully merging this pull request may close these issues.

ImGUI IO assertion failure Key Events have changed in ImGui
4 participants