WIN32 + OpenGL 3: Allow WndProcHandler to receive contexts as argument so they do not rely in global variables and becomes thread safe

[juanmanzanero]

Hello,

I would like to first thank you for the hard work you've put in this very useful project.

I propose a small edit on the interfaces of the Win32 WndProc handler, to accept an arbitrary ImGui Context as input so they do not rely on the current context and it is thread safe. This allows to render multiple independent windows with their own context from one thread. This is mandatory, for example, to host ImGui windows from Windows Presentation Foundation (WPF), which results in a very powerful tool.

Here I attach a screenshot of the resulting UI you can get by combining WPF and ImGui. It renders 4 fully independent ImGui applications, using a single auxiliary thread (e.g. a RenderThread).

I need this change to make it to work since at the time the WndProc function is called, it is possible that the RenderThread is currently drawing its frame, and the WndProc method would access an incorrect ImGuiIO or BackendData.

Thank you again.

Juan

[PathogenDavid]

Maybe I'm missing something, but is there a reason you can't just call ImGui::SetCurrentContext to the appropriate context before each of your ImGui_ImplWin32_WndProcHandler calls?

and becomes thread safe

Dear ImGui is not thread safe in general. If your Dear ImGui UI thread and OS UI threads are separate you're going to have race conditions between one thread updating ImGuiIO and the other one reading it.

(IIRC you can actually get away with protecting ImGui::NewFrame and your message pump with a mutex, but this isn't something I'd rely on. In the past I've used a custom backend that marshals UI events between threads avoid relying on this.)

It doesn't seem like it, but if your only goal is to have multiple contexts across multiple threads then it's generally recommended to make the context variable a thread local.

[juanmanzanero]

Hello David, thank you for answeing.

So current set up is: I want to render one (or more) instances of ImGui from WPF (C# .NET Core). So there are two threads involved, both created from the C# side: the Render thread that does all the ImGui (NewFrame, Begin, etc) calls, and the WPF UI thread that owns the Win32 window and hence receives the notifications and does the WndProc calls.

So I cannot set the current context in the WndProc callback since the render thread might be in the middle of a NewFrame/EndFrame operation (it crashes, leading to ImGui exceptions)

I cannot use the lock/mutex either since is C# who manages the threads and not C++ (leads to segfault in a very simple hello world program).

So far the only solution I've found (it works amazingly) is to pass the context as a input parameter to the ImGui function.

Cheers!

Juan

[ocornut]

So I cannot set the current context in the WndProc callback since the render thread might be in the middle of a NewFrame/EndFrame operation (it crashes, leading to ImGui exceptions)

If you #define GImGui to be a TLS variable you may use a different one per thread.

So far the only solution I've found (it works amazingly) is to pass the context as a input parameter to the ImGui function.

The problem is that you are sitting on a thin line of luck / undefined behavior. At it happens, the IO functions are currently not accessing the global context variable (see #6895 (comment)) but it has never been a thorough guarantee.

I see that I could add a patch similar to your to reduce access to this from ImGui_ImplWin32_WndProcHandler but:

• it would make the backend code inconsistent (some path not used in the handler haven't been changed).
• maintenance would be a little more difficult and error-prone. Any further addition to the WndProc handler would need to carry that responsibility. Had to find a solution for ImGui_ImplWin32_UpdateMouseCursor(io, imguiCxt->MouseCursor) line in WM_SETCURSOR handler which uses imgui_internal.h. Have to make sure that it works with multi-viewport feature in docking branch? See below.
• mostly it would start suggesting and offering a guarantee which I am not 100% sure we want or can honor forever.

I can make a tweaked version of your patch for master, but docking does this in the other WndProc handler:

static LRESULT CALLBACK ImGui_ImplWin32_WndProcHandler_PlatformWindow(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
{
    // Allow secondary viewport WndProc to be called regardless of current context
    ImGuiContext* hwnd_ctx = (ImGuiContext*)::GetPropA(hWnd, "IMGUI_CONTEXT");
    ImGuiContext* prev_ctx = ImGui::GetCurrentContext();
    if (hwnd_ctx != prev_ctx && hwnd_ctx != NULL)
        ImGui::SetCurrentContext(hwnd_ctx);

    LRESULT result = 0;
    if (ImGui_ImplWin32_WndProcHandler(hWnd, msg, wParam, lParam))
        result = true;
    else if (ImGuiViewport* viewport = ImGui::FindViewportByPlatformHandle((void*)hWnd))
    {
        switch (msg)
        {
        case WM_CLOSE:
            viewport->PlatformRequestClose = true;
            break;
        case WM_MOVE:
            viewport->PlatformRequestMove = true;
            break;
        case WM_SIZE:
            viewport->PlatformRequestResize = true;
            break;
        case WM_MOUSEACTIVATE:
            if (viewport->Flags & ImGuiViewportFlags_NoFocusOnClick)
                result = MA_NOACTIVATE;
            break;
        case WM_NCHITTEST:
            // Let mouse pass-through the window. This will allow the backend to call io.AddMouseViewportEvent() correctly. (which is optional).
            // The ImGuiViewportFlags_NoInputs flag is set while dragging a viewport, as want to detect the window behind the one we are dragging.
            // If you cannot easily access those viewport flags from your windowing/event code: you may manually synchronize its state e.g. in
            // your main loop after calling UpdatePlatformWindows(). Iterate all viewports/platform windows and pass the flag to your windowing system.
            if (viewport->Flags & ImGuiViewportFlags_NoInputs)
                result = HTTRANSPARENT;
            break;
        }
    }
    if (result == 0)
        result = DefWindowProc(hWnd, msg, wParam, lParam);
    if (hwnd_ctx != prev_ctx && hwnd_ctx != NULL)
        ImGui::SetCurrentContext(prev_ctx);
    return result;
}

We could remove the Get/SetCurrentContext(), but the problem here is that we cannot use FindViewportByPlatformHandle().

And of course we can reimplement it using ImGuiPlatformIO::Viewports[], but it raises again the issue of "how far can we go supporting this without encountering a problem?".
I unfortunately think it's not a responsibility that I want to carry right now, at least not before investigating parallel contexts in backends.

I am attaching my version of the patch (it's 80% the same as yours, doesn't use imgui_internal.h by calling ImGui_ImplWin32_UpdateMouseCursor() with bd->LastMouseCursor + add a ImGui_ImplWin32_GetBackendData(ImGuiIO& io) helper + add the param to other functions).

imgui-75a9107-Backends Win32 rework to add ImGui_ImplWin32_WndProcHandlerEx() not using current context. (#8069, #6293, #5856, #586).patch

[ocornut]

I'm caving in because I think we need a playground for this and I'd rather it be the win32 backend.

I have pushed 3b8c7d0 (master) + fedf45c (docking) + ee1decc (docking)

To be clear: I am not comfortable with this and it is undocumented, but it should help getting moving forward.

For your use case you will need to extern/forward declare ImGui_ImplWin32_WndProcHandlerEx().

[juanmanzanero]

Hello Omar,

Thank you very much for the update. It works perfectly!!

Cheers,

Juan

[ocornut]

Plot twist: it introduced a crash #8162 which I am now fixing.