segross / UnrealImGui

Unreal plug-in that integrates Dear ImGui framework into Unreal Engine 4.
MIT License
666 stars 212 forks source link

Stale pointer causing random crashes #22

Closed Jay2645 closed 5 years ago

Jay2645 commented 5 years ago

In UImGuiInputHandler::Initialize, the InputState member caches the value from the indexed ContextProxy like so:

auto* ContextProxy = ModuleManager->GetContextManager().GetContextProxy(ContextIndex);
checkf(ContextProxy, TEXT("Missing context during initialization of input handler: ContextIndex = %d"), ContextIndex);
InputState = &ContextProxy->GetInputState();

However, it seems like the InputState pointer can become stale. This may be due to a resize of the container holding all the content proxies -- a newly constructed ContextProxy can be created along with a new InputState (or at least a copy), which invalidates the cached UImGuiInputHandler->InputState pointer. On my machine, I was seeing this when running 4 PIE sessions plus a dedicated server (plus the editor itself). My guess is that the container holding the content proxies was set to hold 5 elements, but then one of the clients caused the container to be resized (or something along those lines). The result is that InputState becomes a stale pointer pointing to some random piece of memory.

When input gets cleared, the input arrays get wiped... which wipes data from that random piece of memory. Sometimes this causes weird side effects, sometimes you don't notice, sometimes it results in a crash. Changing code changes where things are mapped out in memory, so the crash could go away for a bit and then come back when you write some more code.

I've come up with a hacky solution by updating the InputState every frame during UImGuiInputHandler::OnPostImGuiUpdate, using the same code that UImGuiInputHandler::Initialize uses (the bit I pasted above).

It may be that it only needs to be done on the first frame, but I've honestly spent about a week trying to debug this code (which is frustrating since it doesn't happen every time or on every machine, even with the same codebase) and the hacky solution seems to work. However, as I mentioned, the bug is intermittent and changes based on code changes elsewhere in the project.

I don't know if my hacky solution is the right way to do things, but it might be worth taking a look at how that stuff gets stored.

segross commented 5 years ago

Hey, that's a good point. I have moved the input state to the proxy some time ago and completely forgot (or overlooked) that they are stored in an array that can move them. The simplest fix is to change the input state in proxy to a unique pointer, so it moves without re-allocations and create it dynamically in the constructor. However, it might be even better to make sure that the whole context proxy cannot move. I'll do one or both changes.

segross commented 5 years ago

I’ve pushed the fix. I never managed to crash it but I’ve seen input not working after triggering re-allocation in the manager, which is not happening any-more.

Thanks for pointing out that issue.