Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multiple GLFW contexts #3934

Closed
wants to merge 4 commits into from

Conversation

bear24rw
Copy link
Contributor

@bear24rw bear24rw commented Mar 17, 2021

I have the same situation as #3012 where I am integrating into a large existing code base that manages multiple windows via glfwCreateWindow and glfwMakeContextCurrent. ImGui is currently very close to supporting this out of the box, the only issue is the global state in imgui_impl_glfw.cpp.

The first patch removes the global g_Window in favor of glfwGetCurrentContext(). This allows the user to manage their own glfw and imgui contexts and the imgui glfw backend will use which ever one is currently active.

The second patch encapsulates the other cross-frame state and keeps an instance of it per glfw window in a ImguiStorage global. I'm not sure if this is the best data structure to use.

The more general solution would probably be related to the Viewports work but it was not obvious to me how to achieve my integration with that branch.

…fw window as global in order to better handle the situation where the caller might be self-managing multiple glfw contexts (ocornut#3012)
… context. This allows users to self-manage multiple glfw contexts (ocornut#3012)
@nicolasnoble
Copy link
Contributor

Don't forget the docking branch already has multi-windows capabilities, so this change likely would heavily conflict with it.

@bear24rw
Copy link
Contributor Author

Don't forget the docking branch already has multi-windows capabilities, so this change likely would heavily conflict with it.

Yeah, I did look at using that branch but it was unclear to me how I can integrate it with an existing application that manually uses glfwCreateWindow. For instance when I create a second window I register my own glfw callbacks, which then call into the imgui glfw backend. With viewports when you create a new viewport/window (which I also couldn't figure out how to do without creating an imgui window first) the backend registers its own callbacks. Its kinda an inverted ownership, I want to own the glfw window/context of all windows but in the viewport world, imgui owns it.

That branch also still has the issue of the global variables, specifically g_Window. In my application you can open two windows and then close the first one, this would invalidate g_Window.

I do think eventually the viewport feature would be the correct way to achieve multi-window support but I'm not sure it has all the features at the moment.

@rokups
Copy link
Contributor

rokups commented Mar 24, 2021

You do not really need to do this for docking branch. I integrated docking branch to a larger codebase without having to do any changes. I used SDL backend, but its not that different from GLFW. example_glfw_opengl3/main.cpp creates a new GLFW context and passes it to backend. You can do same in your application and pass your own GLFW context to the backend. As for secondary viewports - you do not really care what backend does. Sure it creates another GLFW window, but library manages all that. All you do is render your UI in that window. There are no issues with rendering your scene into a texture and displaying such texture in a detached imgui viewport as graphics APIs allow such resource sharing.

@bear24rw
Copy link
Contributor Author

bear24rw commented Mar 24, 2021

You do not really need to do this for docking branch. I integrated docking branch to a larger codebase without having to do any changes. I used SDL backend, but its not that different from GLFW. example_glfw_opengl3/main.cpp creates a new GLFW context and passes it to backend. You can do same in your application and pass your own GLFW context to the backend.

Yeah, the docking branch for sure works great if your application is only creating one initial GLFWwindow and passing that to the backend (the normal case). Imgui creates/manages additional GLFWwindow's as needed for viewports. I have no issue with that flow and it also "just works" for my application.

As for secondary viewports - you do not really care what backend does. Sure it creates another GLFW window, but library manages all that.

Correct, but in my case my application is creating its own GLFWwindows that are not imgui viewports. They are full platform windows with their own GLFW callbacks.

All you do is render your UI in that window.

When you say window do you mean platform window (like a GLFWwindow), imgui window (ImGui::Begin), or an imgui viewport?

There are no issues with rendering your scene into a texture and displaying such texture in a detached imgui viewport as graphics APIs allow such resource sharing.

For a large existing application changing to rendering into a texture is not always feasible.

I believe the change I am requesting with this pull request is orthogonal and complimentary to the viewport feature (albeit not implemented on the docking branch). Imgui can still manage the creation/destruction of its own platform windows for viewport usage, but it would be nice to also support allowing the user to manage creation/destruction of platform windows.

@rokups
Copy link
Contributor

rokups commented Mar 25, 2021

When you say window do you mean platform window (like a GLFWwindow), imgui window (ImGui::Begin), or an imgui viewport?

I mean ImGui::Begin. Ideally you should never concern yourself with platform details. All you do is create imgui windows and let the backend handle them however appropriate.

Correct, but in my case my application is creating its own GLFWwindows that are not imgui viewports. They are full platform windows with their own GLFW callbacks.

And problem is that you want to render imgui ui in those windows as well? As it might not be trivial. This library started as a debug UI for games, so due to legacy reasons it is tied to a main viewport (think game window). Docking/viewports came later and even though now we can detach imgui windows from main viewport - we still need to keep main viewport around. I have a feeling what you actually need sort of like rendering into multiple main viewports, which would not be easy way to achieve. I am a bit confused what this patch achieves, because g_Window is directly tied to main viewport concept. Now that it is gone and there may be multiple such windows - how is rendering handled? How imgui window knows to which glfw window it should render? Also how such windows are rendered? Do they act as a space for multiple windows like main viewport does now? Do they act as a single window container like now secondary viewport windows do?

@ocornut
Copy link
Owner

ocornut commented Mar 25, 2021

I have a feeling what you actually need sort of like rendering into multiple main viewports, which would not be easy way to achieve. I

I think we'll get there. Some of the work that has been done recently with backporting and clarifying the concept of "main viewport" in master branch was tied to the necessity to also support 0 main viewport, and down the line we will also support "N user created viewport" which will serve as host viewports for imgui windows.

(Also linking to #2004)

@bear24rw
Copy link
Contributor Author

And problem is that you want to render imgui ui in those windows as well?

Correct.

I am a bit confused what this patch achieves, because g_Window is directly tied to main viewport concept.

That's why this patch removed the g_Window concept :) (specifically it changed it from a single global to a hash map of glfwWindow's).

Now that it is gone and there may be multiple such windows - how is rendering handled?

GLFW allows you to share the opengl context between platform windows (GLFWwindow*). You simply activate the context for your window before rendering and then the default imgui opengl3 backend "just works".

I just add two calls in the render loop for each window to set the context's:

  bool AppWindow::render()
  {
      glfwMakeContextCurrent(m_glfw_window);       // Added
      ImGui::SetCurrentContext(m_imgui_context);   // Added

      ImGui_ImplOpenGL3_NewFrame();
      ImGui_ImplGlfw_NewFrame();
      ImGui::NewFrame();

      draw_all_the_stuff();

      ImGui::Render();
      int display_w, display_h;
      glfwGetFramebufferSize(m_glfw_window, &display_w, &display_h);
      glViewport(0, 0, display_w, display_h);
      glClearColor(0.18f, 0.18f, 0.18f, 1.0f);
      glClear(GL_COLOR_BUFFER_BIT);
      ImGui_ImplOpenGL3_RenderDrawData(ImGui::GetDrawData());
      glfwSwapBuffers(m_glfw_window);
  }

How imgui window knows to which glfw window it should render?

In the glfw backend we look up the current context via glfwGetCurrentContext()
In the opengl3 backend, it doesn't need to know which platform window has the current gl context since all resources are shared.

Also how such windows are rendered? Do they act as a space for multiple windows like main viewport does now? Do they act as a single window container like now secondary viewport windows do?

They act as totally separate host platform windows. As if you launched another instance of your application, you will have two windows in the windows task bar for example.

What really sent me down this path was that macOS really does not want you to launch separate instances of your app. It very much wants to launch multiple platform windows within the same app instance. So to handle this I need the imgui glfw backend to allow switching between them via glfwMakeContextCurrent

@bear24rw
Copy link
Contributor Author

I just discovered that glfwGetCurrentContext() only works when using glfw with an opengl context. So it does not work when using the metal imgui backend (and maybe others?). Given that we should not merge this patch and think if there is alternate solution.

…and Shutdown instead of determining it from glfwCurrentContext since glfw contexts are only valid when using opengl.
@bear24rw
Copy link
Contributor Author

I have pushed a new patch which removes the dependency on glfwGetCurrentContext(). I now allow the user to optionally pass a GLFWwindow* to ImGui_ImplGlfw_NewFrame and ImGui_ImplGlfw_Shutdown. To retain backwards compatibility I've defaulted the arguments to NULL. If they are NULL it will use g_DefaultWindow which is set to the first window passed to ImGui_ImplGlfw_Init.

@rokups
Copy link
Contributor

rokups commented Mar 26, 2021

I suppose i worded my questions wrong. What i was really getting at is that handling of main viewport (main application window) is hardcoded in imgui internals. Switching window and OpenGL context should be breaking a lot of stuff because from perspective of library you would be still rendering into same main viewport except its size would change when you switch between windows. Although if you submitted a PR i suppose it works to some degree, which is a surprising luck.

…rder to get properly constructed State objects with the correct member initialization
ocornut added a commit that referenced this pull request Jun 29, 2021
…multi-contexts. (#586, #1851, #2004, #3012, #3934, #4141)

This is NOT enable multi-contexts for any backends
- in order to make this commit as harmless as possible, while containing all the cruft/renaming
-
@ocornut
Copy link
Owner

ocornut commented Jun 29, 2021

Hello,

(Will paste same messages in a few threads)

With 70c6038 i have rearranged all backends to pull their data from structures.
This commit is rather large and noisy so I tried to make it as harmless a possible and therefore the commit as-is tries to NOT change anything in the backends, which include NOT enabling support for multiple imgui-context.

This support in theory can be enabled by changing a small portion of the code:
e.g. for GLFW

Remove those 4 lines:

// Wrapping access to backend data (to facilitate multiple-contexts stored in io.BackendPlatformUserData)
static ImGui_ImplGlfw_Data* g_Data;
static ImGui_ImplGlfw_Data* ImGui_ImplGlfw_CreateBackendData()  { IM_ASSERT(g_Data == NULL); g_Data = IM_NEW(ImGui_ImplGlfw_Data); return g_Data; }
static ImGui_ImplGlfw_Data* ImGui_ImplGlfw_GetBackendData()     { return ImGui::GetCurrentContext() != NULL ? g_Data : NULL; }
static void                 ImGui_ImplGlfw_DestroyBackendData() { IM_DELETE(g_Data); g_Data = NULL; }

Replace with those 3 lines:

static ImGui_ImplGlfw_Data* ImGui_ImplGlfw_CreateBackendData()  { return IM_NEW(ImGui_ImplGlfw_Data)(); }
static ImGui_ImplGlfw_Data* ImGui_ImplGlfw_GetBackendData()     { return (ImGui_ImplGlfw_Data*)ImGui::GetIO().BackendPlatformUserData; }
static void                 ImGui_ImplGlfw_DestroyBackendData() { IM_DELETE(ImGui_ImplGlfw_GetBackendData()); }

(For platform backend, BackendPlatformUserData, for renderer backends use BackendRendererUserData)

  • I think for many backends this may work as-is
  • But this is not thoroughly tested
  • Some backends are manipulating global state (e.g. os mouse cursor),
  • I don't have a solid enough testing platform for them. I'm building one but it's rather tricky to test for everything.
  • So we can enable them one by one if they are confirmed to work.

Current change being harmless and healthy I don't mind it but I'm not very excited with promoting and encouraging support of multiple-imgui-context-each-in-their-own-backend-context. I think that feature is likely to be incompatible with multi-viewports (single imgui context + multiple backend windows) and I think instead we should aim at reworking this to let user register any existing window.

@bear24rw Since you submitted this PR for GLFW, could you pull latest, apply the 3-4 lines change above and see if this work for your usage?

Thank you!

ocornut added a commit that referenced this pull request Jun 29, 2021
#1851, #2004, #3012, #3934, #4141)

I believe more renderer backends should work. GLFW/Win32/SDL/Vulkan probably have many issues.
@ocornut
Copy link
Owner

ocornut commented Jun 29, 2021

@bear24rw I have enabled support with 4cec3a0 as it seemed to more or less work in my testbed but I'm keeping this advertised as barely tested.

(Difference over your version: all data stored in imgui context so only SetCurrentContext() required, no hashing/lookup required)

It should have the same functionalities and the same issues (shared resources such as gamepad or mouse cursor shape may not work well).

Current change being generaly healthy I don't mind it but I'm not very excited with promoting and encouraging support of multiple-imgui-context-each-in-their-own-backend-context. I think that feature is likely to be incompatible with multi-viewports (single imgui context + multiple backend windows) and I think instead we should aim at reworking this to let user register any existing window.

@ocornut ocornut closed this Jun 29, 2021
@bear24rw
Copy link
Contributor Author

bear24rw commented Jul 2, 2021

@bear24rw I have enabled support with 4cec3a0 as it seemed to more or less work in my testbed but I'm keeping this advertised as barely tested.

I just tested on my project and it works great! Definitely a cleaner solution than my original PR. I think the only thing users need to watch out for is that they need to register their own glfw callbacks to ensure the imgui context is set correctly before fowarding the call to the imgui backend. I believe this is needed because you dont know what window is active when glfw is triggering the callbacks.

For example:

PlatformWindow::PlatformWindow()
{
    m_imgui_context = ImGui::CreateContext();
    ImGui::SetCurrentContext(m_imgui_context);

    const bool install_callbacks = false;
    ImGui_ImplGlfw_InitForOther(m_glfw_window, install_callbacks);
    ImGui_ImplMetal_Init(s_device);

    auto char_callback_func = [](GLFWwindow* w, unsigned int c) { static_cast<PlatformWindow*>(glfwGetWindowUserPointer(w))->char_callback(c); };

    glfwSetCharCallback(m_glfw_window, char_callback_func);
    glfwSetWindowUserPointer(m_glfw_window, this);
}

void PlatformWindow::char_callback(unsigned int c)
{
    ImGui::SetCurrentContext(m_imgui_context);
    ImGui_ImplGlfw_CharCallback(m_glfw_window, c);
}

@ocornut
Copy link
Owner

ocornut commented Jul 7, 2021

Thanks for the clarification!

Another solution would be to rework the backend to store an additional GLFWwindow* -> ImGuiContext* map, and in all the callback set the context based on that (and maybe restore it afterward). This would unfortunately be a little messy :(

For now I'll add additional commentary about that.

ocornut added a commit that referenced this pull request Jul 7, 2021
…() for forward compatibility with docking branch.

+ Comments (#3934)
AnClark added a commit to AnClark/amsynth that referenced this pull request Sep 5, 2022
On Windows, when I've used dedicated thread for ImGui rendering, and
used TLS for GImGui, there's still a bug:

When I move mouse into editor window, plugin crashes. GDB shows that it
crashes when ImGui's GLFW callback function is accessing backend data
via ImGui_ImplGlfw_GetBackendData() (returns current ImGui context).

This is because GLFW queries window events in main thread, not in
drawing thread. Since the two threads have their own GImGui instance,
GImGui will be null in main thread.

@bear24rw (Max Thurn) provided a workaround: register our own glfw
callbacks to ensure the imgui context is set correctly before fowarding
the call to the imgui backend.

This workaround doesn't aim at multithreading, but works well for me!

Reference: ocornut/imgui#3934 (comment)
AnClark added a commit to AnClark/amsynth that referenced this pull request Sep 6, 2022
On Windows, when I've used dedicated thread for ImGui rendering, and
used TLS for GImGui, there's still a bug:

When I move mouse into editor window, plugin crashes. GDB shows that it
crashes when ImGui's GLFW callback function is accessing backend data
via ImGui_ImplGlfw_GetBackendData() (returns current ImGui context).

This is because GLFW queries window events in main thread, not in
drawing thread. Since the two threads have their own GImGui instance,
GImGui will be null in main thread.

@bear24rw (Max Thurn) provided a workaround: register our own glfw
callbacks to ensure the imgui context is set correctly before fowarding
the call to the imgui backend.

This workaround doesn't aim at multithreading, but works well for me!

Reference: ocornut/imgui#3934 (comment)

WARNING: This commit is not suitable for Linux (with X11).

On Linux, it will cause "Forgot to call Render() or EndFrame() at
the end of the previousframe?" error.
AnClark added a commit to AnClark/amsynth that referenced this pull request Sep 7, 2022
On Windows, when I've used dedicated thread for ImGui rendering, and
used TLS for GImGui, there's still a bug:

When I move mouse into editor window, plugin crashes. GDB shows that it
crashes when ImGui's GLFW callback function is accessing backend data
via ImGui_ImplGlfw_GetBackendData() (returns current ImGui context).

This is because GLFW queries window events in main thread, not in
drawing thread. Since the two threads have their own GImGui instance,
GImGui will be null in main thread.

@bear24rw (Max Thurn) provided a workaround: register our own glfw
callbacks to ensure the imgui context is set correctly before fowarding
the call to the imgui backend.

This workaround doesn't aim at multithreading, but works well for me!

Reference: ocornut/imgui#3934 (comment)

WARNING: This commit is not suitable for Linux (with X11).

On Linux, it will cause "Forgot to call Render() or EndFrame() at
the end of the previousframe?" error.
AnClark added a commit to AnClark/amsynth that referenced this pull request Sep 7, 2022
On Windows, when I've used dedicated thread for ImGui rendering, and
used TLS for GImGui, there's still a bug:

When I move mouse into editor window, plugin crashes. GDB shows that it
crashes when ImGui's GLFW callback function is accessing backend data
via ImGui_ImplGlfw_GetBackendData() (returns current ImGui context).

This is because GLFW queries window events in main thread, not in
drawing thread. Since the two threads have their own GImGui instance,
GImGui will be null in main thread.

@bear24rw (Max Thurn) provided a workaround: register our own glfw
callbacks to ensure the imgui context is set correctly before fowarding
the call to the imgui backend.

This workaround doesn't aim at multithreading, but works well for me!

Reference: ocornut/imgui#3934 (comment)

WARNING: This workaround is not suitable for Linux (with X11).

On Linux, it will cause "Forgot to call Render() or EndFrame() at
the end of the previousframe?" error.

So, only apply it in Windows.
AnClark added a commit to AnClark/amsynth that referenced this pull request Sep 8, 2022
On Windows, when I've used dedicated thread for ImGui rendering, and
used TLS for GImGui, there's still a bug:

When I move mouse into editor window, plugin crashes. GDB shows that it
crashes when ImGui's GLFW callback function is accessing backend data
via ImGui_ImplGlfw_GetBackendData() (returns current ImGui context).

This is because GLFW queries window events in main thread, not in
drawing thread. Since the two threads have their own GImGui instance,
GImGui will be null in main thread.

@bear24rw (Max Thurn) provided a workaround: register our own glfw
callbacks to ensure the imgui context is set correctly before fowarding
the call to the imgui backend.

This workaround doesn't aim at multithreading, but works well for me!

Reference: ocornut/imgui#3934 (comment)

WARNING: This workaround is not suitable for Linux (with X11).

On Linux, it will cause "Forgot to call Render() or EndFrame() at
the end of the previousframe?" error.

So, only apply it in Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants