mosra / magnum-integration

Integration libraries for the Magnum C++11 graphics engine
https://magnum.graphics/
Other
97 stars 44 forks source link

ImGuiIntegration: fix indexing into KeysDown #89

Closed pezcode closed 2 years ago

pezcode commented 2 years ago

Somewhat bandaid-y fix for https://github.com/mosra/magnum-integration/issues/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.

jvannugteren commented 2 years ago

This fix works fine for my code.

codecov[bot] commented 2 years ago

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%) :arrow_down:
src/Magnum/ImGuiIntegration/Context.hpp 85.07% <100.00%> (+0.22%) :arrow_up:

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 commented 2 years ago

I'm particularly interested in this as our change https://github.com/ocornut/imgui/issues/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 commented 2 years ago

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.

mosra commented 2 years ago

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 commented 2 years ago

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 🙇‍♂️