mosra / magnum-integration

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

ImGui IdxOffset not honored correctly during frame drawing #86

Closed jvannugteren closed 2 years ago

jvannugteren commented 2 years ago

Using the magnum-integration I get the following graphical glitch with ImGui: https://github.com/ocornut/imgui/releases/tag/v1.86 (see first section)

I have created an issue there. However, I was pointed out by Ocornut, creator of imgui, it is an issue with the backend (i.e. Magnum) not honoring the ImDrawCmd::IdxOffset correctly.

See the following ImGui issues (all the same): https://github.com/ocornut/imgui/issues/4887 (this one is mine) https://github.com/ocornut/imgui/issues/4863 https://github.com/ocornut/imgui/issues/4845

I think the (easy) fix is to change src/Magnum/ImGuiIntegration/Context.cpp line 328 to:

_mesh.setIndexBuffer(_indexBuffer, pcmd->IdxOffset*sizeof(ImDrawIdx),
                sizeof(ImDrawIdx) == 2
                ? GL::MeshIndexType::UnsignedShort
                : GL::MeshIndexType::UnsignedInt);

Lines 310 and 333 then become obsolete. This way the IdxOffset of ImGui is respected and not re-calculated by magnum. With this change the glitch goes away at least in my code. Please check what I did as I'm not fully familiar with your code.

Thanks for looking at this.

pezcode commented 2 years ago

I have a patch for this, just stashed it because there was some other work I originally wanted to do on ImGuiIntegration. I can send a PR tomorrow ☺️

jvannugteren commented 2 years ago

Thanks. Did you happen to also fix the key events? Then I won't need to make another issue.

pezcode commented 2 years ago

Thanks. Did you happen to also fix the key events? Then I won't need to make another issue.

Hm no, not aware of key event issues. Feel free to file an issue.

mosra commented 2 years ago

Thanks! The code here originates way before 1.71 was a thing and since I'm not using ImGui myself, I'm not really reading the changelogs and so I missed this, sorry -- glad you've discovered the issue instead.

88 got merged as 3e7b175385b85fba8eeb0cbdb8051a59f3e075b6, so this can be closed.