Closed Froyok closed 4 years ago
Sorry, I cannot help you much with that because this is something that happens inside of the ImGui demo and on the Linux platform where I cannot run it. This call stack alone doesn't tell that much and on the Windows, I don't have that crash. The fact that it works fine outside of the UE might have something to do with the type of TCHAR that UE uses on Linux, or not... not sure.
Is the input sharing essential? Although I don't think it should be.
And on a side note, you know that there are commands to toggle those properties and that input sharing can be configured from settings?
Looking at the line 3689 and one thing I would check is the state
. Did you try to put a break-point and see what is there?
Using SetKeyboardInputShared( false)
didn't help. So I ran my project in debug this time which gave me a more precise callstack:
#0 0x00007fff5beb2df1 in ImGui::IsKeyPressed (user_key_index=1073741904, repeat=true) at /mnt/dev/Perforce/Exedre/Plugins/ImGui/Source/ThirdParty/ImGuiLibrary/Private/imgui.cpp:4263
#1 0x00007fff5bf3494f in ImGui::IsKeyPressedMap (key=1, repeat=true) at /mnt/dev/Perforce/Exedre/Plugins/ImGui/Source/ThirdParty/ImGuiLibrary/Private/imgui_internal.h:1694
#2 0x00007fff5bef5c80 in ImGui::InputTextEx (label=0x7fff5be451b7 "##MyStr", hint=0x0, buf=0x2f5fdf0 "", buf_size=1, size_arg=..., flags=1310720,
callback=0x7fff5bf17480 <ShowDemoWindowWidgets()::Funcs::MyResizeCallback(ImGuiInputTextCallbackData*)>, callback_user_data=0x7fff5bfa78c0 <ShowDemoWindowWidgets()::my_str>)
I don't know yet why, but when IsKeypresse() is called, user_key_index
has a value of 1073741904 which seems way too big for an array index, leading to the crash in the function below (at const float t = g.IO.KeysDownDuration[user_key_index];
). I'm also not sure why the assert in the line just above doesn't trigger :
bool ImGui::IsKeyPressed(int user_key_index, bool repeat)
{
ImGuiContext& g = *GImGui;
if (user_key_index < 0)
return false;
IM_ASSERT(user_key_index >= 0 && user_key_index < IM_ARRAYSIZE(g.IO.KeysDown));
const float t = g.IO.KeysDownDuration[user_key_index];
if (t == 0.0f)
return true;
if (repeat && t > g.IO.KeyRepeatDelay)
return GetKeyPressedAmount(user_key_index, g.IO.KeyRepeatDelay, g.IO.KeyRepeatRate) > 0;
return false;
}
I will continue to investigate, but if you have any suggestions I'm all ears.
[EDIT]
I wonder if for some reason the mouse click actually triggers IsKeyPressed()
(because that's what I do: I just click the text field to get the focus to be able to type). It seems the KeyMap array is only for keyboard shortcuts, which maybe is why the index doesn't make sense (and is way beyond the max 512).
I did a little hack by adding
if( user_key_index > IM_ARRAYSIZE(g.IO.KeysDown) )
return false;
and it's not crashing anymore, I'm even able to type text in the text fields. :thinking:
user_key_index = key_index
where key_index = g.IO.KeyMap[key]
and key = 1
in that instance.
So, it looks like the IO.KeyMap
may be bad. This map is copied to IO after context creation: ImGuiInterops::SetUnrealKeyMap(IO)
in the ImGuiContextProxy.cpp, line 101.
The void ImGuiInterops::SetUnrealKeyMap(ImGuiIO&)
is declared in the ImGuiInteroperability.h, line 37 and defined in the ImGuiInteroperability.cpp, line 88.
It creates a local variable static const FUnrealToImGuiMapping Mapping
with a map that is initialised using the uint32 GetKeyIndex(const FKey&)
function. So I would check the state of that Mapping
variable and what is copied to the ImGuiIO
. If the mapping is wrong then perhaps the uint32 GetKeyIndex(const FKey&)
doesn't work correctly on Linux. In this case, we care about element 1, but if the mapping is wrong I would expect that other entries will be also bad. And I would check that during creation and later when copying.
I would also make sure that the type of Mapping.KeyMap
is exactly the same as the type of IO.KeyMap
(in the void ImGuiInterops::SetUnrealKeyMap(ImGuiIO&)
function).
And about the mouse click, I would think that it can just activate that logic (but I might be wrong). So, right now I think that it must be something with the mapping.
Did you find out what is the state of the Mapping.KeyMap
or IO.KeyMap
?
Hadn't the chance to look into it further yet, quite a bit busy last week unfortunately. I will try to look into it again soon.
+1 on this. I hit this in our application, and the next two levels in the callstack above are actually IsKeyPressedMap and IsKeyPressed due to this line in InputTextEx:
if (IsKeyPressedMap(ImGuiKey_LeftArrow)) { state->OnKeyPressed((is_startend_key_down ? STB_TEXTEDIT_K_LINESTART : is_wordmove_key_down ? STB_TEXTEDIT_K_WORDLEFT : STB_TEXTEDIT_K_LEFT) | k_mask); }
I did dig into the root cause. It's the values of the keys at the platform-specific level and their use as an index as noted by @Froyok . On Linux, the underlying implementation is using SDL, and not so coincidentally, 1073741904 is 0x40000050, which maps to SDLK_LEFT. Here's a copy/paste from an internal Jira where I dumped my analysis:
"Ok, yeah, this is a Linux-specific issue. That 1073741904 value is 0x40000050, which maps to SDLK_LEFT (arrow key) in the SDL library (which is what UE4 uses for its underlying Linux implementation), as can be seen here: https://wiki.libsdl.org/SDLKeycodeLookup
They are using that value as a lookup into an array of size 512, so this is going way past valid memory of the array. On Windows, that left arrow key index value is 37, so a safe value.
37 is VK_LEFT: https://docs.microsoft.com/en-us/windows/win32/inputdev/virtual-key-codes .. mapped in UE4 here for EKeys::Left: https://github.com/EpicGames/UnrealEngine/blob/release/Engine/Source/Runtime/InputCore/Private/Windows/WindowsPlatformInput.cpp#L34 : ADDKEYMAP( VK_LEFT, TEXT("Left") );.
0x40000050, on Linux, mapped here as SDKL_LEFT: https://github.com/EpicGames/UnrealEngine/blob/release/Engine/Source/Runtime/InputCore/Private/Linux/LinuxPlatformInput.cpp#L31 : ADDKEYMAP(SDLK_LEFT, TEXT("Left"));
In the ImGui code: KeyMap[ImGuiKey_LeftArrow] = GetKeyIndex(EKeys::Left);
So, this all aligns to end up in the crash we are seeing."
Looking more into this, a possible quick-ish fix, and maybe a reasonable fix to future-proof any other platforms that might have crazy values for some of the keys, would be to make KeysDown, KeysDuration, and KeysDurationPrev a std::map (or ImMap, if one were to be implemented, maybe a super-minimal implementation for adding/indexing/iterating; first pass could just be 'typedef ImMap std::map', though), instead of the [512] array they currently are.
Most of the existing code may work as-is (the stuff that adds keys and indexes them) but some adjustments would be needed such as looping through them.
Hey @jbuckags. Thank you for looking into this and for your analysis. It helps a lot. It seems like potentially a really nasty bug.
So, the very fast fix for Linux would be to transform the current index to something like this: (Index < 512) ? Index : 256 + Index % 256
. Transforming the SDL key-codes this way should be conflicts-free and should solve the problem on Linux. It should also not require any other changes than that.
But I agree that a safer solution for any platform will be to provide a custom mapping. As much as I don't like making another map I also thought about going this direction. For consistency I'd go with the TMap as I adopted a convention that I'll stick to Unreal data structures whenever possible. The benefit of custom mapping might be that I can make it more compact.
Most of the existing code may work as-is (the stuff that adds keys and indexes them) but some adjustments would be needed such as looping through them.
Not sure whether I got that last part. What I would think is that I would still produce an index, but instead of using raw key-codes I'd do a mapping from that to something more compact. Since ImGui only is interested with ImGuiKey_
keys, I would map only those, which would the mapping small enough.
If "Index % 0xff" (or & 0xff) doesn't end up conflicting with others keys' codes, then yeah, we should be good there.
As for the bit that I meant about looping through.. if a TMap (or whatever) were to be used, there are current places where there is an actual array-based loop going through all the elements of the array and would need to be changed to map-based begin()/end() kind of construct.
OMG, yes you are right '&'. I've rushed with an answer and didn't check it properly. I've corrected my previous post to avoid confusion.
Regardless of the conflicts - yes in case of the SDL key-codes it should be fine. From the wiki page, the first group will be mapped directly to the first part of the array and the second group is a sequence that when taken modulo will fit nicely in the second part.
So, any number in range 0-511 and SDL key-codes should be fine but to make it more portable and not to worry about random platforms I might need an own mapping. Not sure yet.
there are current places where there is an actual array-based loop going through all the elements of the array
There are some old enough bits of code, so I might not remember it well. Do you mean array-based loops in the plugin's code or in the imgui code from the Third Party folder?
Hah, well, '& 0xff' is equivalent to '% 0x100', so you were close. ;)
Doing a quick search on the codebase for 'KeysDown' (not-whole-word search):
ImGui\Private\ImGuiInputState.cpp(24): if (KeyIndex < Utilities::GetArraySize(KeysDown))
ImGui\Private\ImGuiInputState.cpp(26): if (KeysDown[KeyIndex] != bIsDown)
ImGui\Private\ImGuiInputState.cpp(28): KeysDown[KeyIndex] = bIsDown;
ImGui\Private\ImGuiInputState.cpp(66): fill(KeysDown, &KeysDown[Utilities::GetArraySize(KeysDown)], false);
ImGui\Private\ImGuiInputState.h(46): const FKeysArray& GetKeys() const { return KeysDown; }
ImGui\Private\ImGuiInputState.h(225): FKeysArray KeysDown;
ImGui\Private\ImGuiInteroperability.cpp(295): Copy(InputState.GetKeys(), IO.KeysDown, InputState.GetKeysUpdateRange());
ImGui\Private\ImGuiInteroperability.h(25): using FKeysArray = decltype(ImGuiIO::KeysDown);
ThirdParty\ImGuiLibrary\Docs\CHANGELOG.txt(1048): - Set io.ConfigFlags |= ImGuiConfigFlags_NavEnableKeyboard to enable. NewFrame() will automatically fill io.NavInputs[] based on your io.KeysDown[] + io.KeyMap[] arrays.
ThirdParty\ImGuiLibrary\Include\imgui.h(674): IMGUI_API bool IsKeyDown(int user_key_index); // is key being held. == io.KeysDown[user_key_index]. note that imgui doesn't know the semantic of each entry of io.KeysDown[]. Use your own indices/enums according to how your backend/engine stored them into io.KeysDown[]!
ThirdParty\ImGuiLibrary\Include\imgui.h(944):// User fill ImGuiIO.KeyMap[] array with indices into the ImGuiIO.KeysDown[512] array
ThirdParty\ImGuiLibrary\Include\imgui.h(973):// Keyboard: Set io.ConfigFlags |= ImGuiConfigFlags_NavEnableKeyboard to enable. NewFrame() will automatically fill io.NavInputs[] based on your io.KeysDown[] + io.KeyMap[] arrays.
ThirdParty\ImGuiLibrary\Include\imgui.h(997): // Keyboard behavior that have no corresponding gamepad mapping (e.g. CTRL+TAB) will be directly reading from io.KeysDown[] instead of io.NavInputs[].
ThirdParty\ImGuiLibrary\Include\imgui.h(1011): ImGuiConfigFlags_NavEnableKeyboard = 1 << 0, // Master keyboard navigation enable flag. NewFrame() will automatically fill io.NavInputs[] based on io.KeysDown[].
ThirdParty\ImGuiLibrary\Include\imgui.h(1366): int KeyMap[ImGuiKey_COUNT]; // <unset> // Map of indices into the KeysDown[512] entries array which represent your "native" keyboard state.
ThirdParty\ImGuiLibrary\Include\imgui.h(1429): bool KeysDown[512]; // Keyboard keys that are pressed (ideally left in the "native" order your engine has access to keyboard keys, so you can use your own defines/enums for keys).
ThirdParty\ImGuiLibrary\Include\imgui.h(1472): float KeysDownDuration[512]; // Duration the keyboard key has been down (0.0f == just pressed)
ThirdParty\ImGuiLibrary\Include\imgui.h(1473): float KeysDownDurationPrev[512]; // Previous duration the key has been down
ThirdParty\ImGuiLibrary\Private\imgui.cpp(329): NewFrame() will automatically fill io.NavInputs[] based on your io.KeysDown[] + io.KeyMap[] arrays.
ThirdParty\ImGuiLibrary\Private\imgui.cpp(1056): for (int i = 0; i < IM_ARRAYSIZE(KeysDownDuration); i++) KeysDownDuration[i] = KeysDownDurationPrev[i] = -1.0f;
ThirdParty\ImGuiLibrary\Private\imgui.cpp(3514): IM_ASSERT(g.IO.KeyMap[n] >= -1 && g.IO.KeyMap[n] < IM_ARRAYSIZE(g.IO.KeysDown) && "io.KeyMap[] contains an out of bound value (need to be 0..512, or -1 for unmapped key)");
ThirdParty\ImGuiLibrary\Private\imgui.cpp(3634): memcpy(g.IO.KeysDownDurationPrev, g.IO.KeysDownDuration, sizeof(g.IO.KeysDownDuration));
ThirdParty\ImGuiLibrary\Private\imgui.cpp(3635): for (int i = 0; i < IM_ARRAYSIZE(g.IO.KeysDown); i++)
ThirdParty\ImGuiLibrary\Private\imgui.cpp(3636): g.IO.KeysDownDuration[i] = g.IO.KeysDown[i] ? (g.IO.KeysDownDuration[i] < 0.0f ? 0.0f : g.IO.KeysDownDuration[i] + g.IO.DeltaTime) : -1.0f;
ThirdParty\ImGuiLibrary\Private\imgui.cpp(4219):// Note that imgui doesn't know the semantic of each entry of io.KeysDown[]. Use your own indices/enums according to how your back-end/engine stored them into io.KeysDown[]!
ThirdParty\ImGuiLibrary\Private\imgui.cpp(4225): IM_ASSERT(user_key_index >= 0 && user_key_index < IM_ARRAYSIZE(g.IO.KeysDown));
ThirdParty\ImGuiLibrary\Private\imgui.cpp(4226): return g.IO.KeysDown[user_key_index];
ThirdParty\ImGuiLibrary\Private\imgui.cpp(4252): IM_ASSERT(key_index >= 0 && key_index < IM_ARRAYSIZE(g.IO.KeysDown));
ThirdParty\ImGuiLibrary\Private\imgui.cpp(4253): const float t = g.IO.KeysDownDuration[key_index];
ThirdParty\ImGuiLibrary\Private\imgui.cpp(4262): IM_ASSERT(user_key_index >= 0 && user_key_index < IM_ARRAYSIZE(g.IO.KeysDown));
ThirdParty\ImGuiLibrary\Private\imgui.cpp(4263): const float t = g.IO.KeysDownDuration[user_key_index];
ThirdParty\ImGuiLibrary\Private\imgui.cpp(4275): IM_ASSERT(user_key_index >= 0 && user_key_index < IM_ARRAYSIZE(g.IO.KeysDown));
C:\dev\pavilion\AdnPavilionProject\PavilionProject\Plugins\ImGui\Source\ThirdParty\ImGuiLibrary\Private\imgui.cpp(4276): return g.IO.KeysDownDurationPrev[user_key_index] >= 0.0f && !g.IO.KeysDown[user_key_index];
ThirdParty\ImGuiLibrary\Private\imgui.cpp(7829): if (IsKeyPressedMap(ImGuiKey_C)) { g.NavMoveDirLast = (ImGuiDir)((g.NavMoveDirLast + 1) & 3); g.IO.KeysDownDuration[g.IO.KeyMap[ImGuiKey_C]] = 0.01f; }
ThirdParty\ImGuiLibrary\Private\imgui_demo.cpp(2956): ImGui::Text("Keys down:"); for (int i = 0; i < IM_ARRAYSIZE(io.KeysDown); i++) if (io.KeysDownDuration[i] >= 0.0f) { ImGui::SameLine(); ImGui::Text("%d (0x%X) (%.02f secs)", i, i, io.KeysDownDuration[i]); }
ThirdParty\ImGuiLibrary\Private\imgui_demo.cpp(2957): ImGui::Text("Keys pressed:"); for (int i = 0; i < IM_ARRAYSIZE(io.KeysDown); i++) if (ImGui::IsKeyPressed(i)) { ImGui::SameLine(); ImGui::Text("%d (0x%X)", i, i); }
ThirdParty\ImGuiLibrary\Private\imgui_demo.cpp(2958): ImGui::Text("Keys release:"); for (int i = 0; i < IM_ARRAYSIZE(io.KeysDown); i++) if (ImGui::IsKeyReleased(i)) { ImGui::SameLine(); ImGui::Text("%d (0x%X)", i, i); }
.. the lines that immediately stand out as needing to go from array-based indexing to map-based begin/end (assuming you went in that direction [and I think you should :)]), are lines like:
for (int i = 0; i < IM_ARRAYSIZE(KeysDownDuration); i++)
and
for (int i = 0; i < IM_ARRAYSIZE(KeysDown); i++)
Also, not sure your timeline on fixes of this nature, but if I end up fixing this in the coming week (and there is probably some pressure for me to do so where I work! :) ), let me know if there's a way hereon github to pitch in. (I'm a long-time developer, since the mid-90s, but other than grabbing code from github, I'm a github n00b when it comes to developing/collaborating/submitting patches/etc. on here.)
Well yes, I was hesitating which operator will be better for readability sake and at the end missed the number.
Going to the main problem. Yes, I asked because I suspected that you might be thinking about the actual Dear ImGui code (the one in the ThirdParty\ImGuiLibrary directory). Those few cases in the ImGui\Private are just a consequence. They also contain a range check when writing the key state, which is good because for the moment I worried that I might be writing out of bounds. But the problem is only with the mapping that is used to retrieve key states.
I don't want to branch from the original Dear ImGui, so as a policy, I don't change it.
But the bug will be purely in the plugin's code (ImGui\Private directory) and more specifically, in the ImGuiInterops::GetKeyIndex(...)
. The transform like above should fix it but for the peace of mind I might also make a completely custom mapping from Unreal key name to the custom index in the KeyDown.
Ahh, ok, so the idea is to come up with a fix that doesn't touch the core ImGui code.. hmm.. just a quick idea since I don't have more than a second right now to look further (maybe later), in SetUnrealKeyMap::FUnrealToImGuiMapping::FUnrealToImGuiMapping(), all those 'KeyMap[etc..] = GetKeyIndex(etc.)'...maybe GetKeyIndex could do some translation magic. Instead of returning the *pKeyCode, it could create some internal values that keep it below a certain value (512, for example) and just return that value instead. It could even memoize values as they are called upon with an internal hash map.
Ahh, ok, so the idea is to come up with a fix that doesn't touch the core ImGui code..
Yes, and as I said, I'm pretty sure that this is the plugin's code that is at fault. I think, that basically imgui library expects you to have a reasonable mapping.
... could do some translation magic. Instead of returning the *pKeyCode,...
Yes, that is what I meant above. I had to correct my original formula, but I'm pretty sure that now it should work, at least in cases that we discussed. Anything below 511 should remain unaltered and SDL key-codes should be transformed to cover the whole array without stomping on each other. Not sure, but maybe that was the reason of picking 512 elements long array. But as I also mentioned and as I think you also pointed, alternatively it could use a completely custom mapping. It would need an extra lookup every time when registering a key but on the bright side information could be more packed and would fit in twenty something entries.
I don't have more than a second right now to look further
No worries, it is a weekend after all :) I just came home now and now quickly finishing this post before my wife gives me a look for sitting again with my computer ;)
Ahh, wives and husbands-using-computers never mix well. :)
If you have a change and would be up for someone to give it a test, let me know the code change, and I could get our Linux guys to give it a whirl on my end. (I myself do all my dev on Windows but eyeballed the code to work out the Linux-specific issue that our Linux users hit.)
I would think that the latest version should have it fixed. Please let me know whether it works for you or you still have issues. I also mostly develop on Windows, so Linux is quite under-tested from my side.
https://github.com/segross/UnrealImGui/commit/ea74c3c872c9fb80bd50b73661cafb8f2a262637
OH, NICE! Thanks for that! Ok, my first work task tomorrow is to update to latest! :) It will also fix another issue we had when upgrading to UE4.25. (This: https://github.com/segross/UnrealImGui/commit/16056820e991d62004de6614cb262ff0368f7520 ). We are currently on the version based on June 10th's commit, so this will be nice to move up from there.
Great, hopefully it will be good. IF it works then I'll close the bug.
And if you are on the version from around June 10th's then yes, I highly recommend the latest changes. Most of that was to handle various maintenance and stability issues, like enforcing IWYU, better handling of hot-reloading, etc. And I recently discovered that my old context mapping broke in 4.25, so debugging multi-PIE should be now also better.
I'm not able to run client but I run a server using WSL and printed values in map. All seem good so closing the bug but feel free to reopen, if you still have problems with this. Thank you for assistance guys.
Circling back, since it took a bit of time to get a build finally into Linux-using hands, but we confirm the fix works, and everything works a-ok on Windows still as well (so, no regression issues)! Thanks for kicking this one out, man!
Every time I try to get the focus a text field to type something, I crash in ImGui function
ImGui::InputTextEx
. I easily reproduce it with the Demo Window and using the Widgets > Text Input > Resize Callstack example.Looks like it's crashing on this line:
if (IsKeyPressedMap(ImGuiKey_LeftArrow)) { state->OnKeyPressed((is_startend_key_down ? STB_TEXTEDIT_K_LINESTART : is_wordmove_key_down ? STB_TEXTEDIT_K_WORDLEFT : STB_TEXTEDIT_K_LEFT) | k_mask); }
I gave a try at the standalone ImGui versions 1.78 and 1.74 (via the example
example_sdl_opengl2
) and it's not crashing. Any idea what that might be ? I'm using the following by the way :