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

Viewports for win32+opengl backend (fix #2600) #5170

Closed

Conversation

theOtherMichael
Copy link
Contributor

Adds support for Win32 + OpenGL backends. Previously, this combination didn't work correctly because spawned windows weren't given rendering contexts. This PR makes it so OpenGL contexts are created and destroyed correctly for each new window. Additionally, it links the contexts to the main one via wglShareLists().

The only API change is the addition of an optional parameter, hglrc, in ImGui_ImplWin32_Init(). Passing in the main window's render context handle here will engage the new code. If only the first parameter is passed, a non-OpenGL API will be inferred. This makes it so existing projects using the Win32 backend will be able to merge this change without issue.

When an OpenGL render context is specified, four WGL functions, wglCreateContext(), wglDeleteContext(), wglMakeCurrent(), and wglShareLists() will be loaded dynamically to enable the context management. The OpenGL library will not be loaded if no hglrc is specified.

Each window created in imgui_impl_win32.cpp now creates an openGL
context which shares its display list space with the primary window's.
IMGUI_IMPL_WIN32_OPENGL3 must be defined to use this fix.
ImGui_ImplWin32_Init() now optionally accepts a render context handle,
and IMGUI_IMPL_WIN32_OPENGL3 is no longer a necessary define.
Copy link
Contributor

@PathogenDavid PathogenDavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on whether this is the right thing for the Win32 backend, but this definitely seems pretty close to being as low impact as it could be.

I mainly reviewed this from the perspective of how this will affect DirectX developers consuming/maintaining this backend.

It'd probably be a good idea to read all of the review comments before making changes since some of them cancel other ones out.

backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
backends/imgui_impl_win32.h Outdated Show resolved Hide resolved
backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
backends/imgui_impl_win32.cpp Outdated Show resolved Hide resolved
@ocornut ocornut changed the title Fix issue #2600 Viewports for win32+opengl backend (fix #2600) Apr 7, 2022
Device Contexts are no longer maintained for the application lifetime.
::GetDC() is now never called when using DirectX backend.
Removed lambda callback and brought WGL function loading into
conformance with XInput DLL loading.
@theOtherMichael
Copy link
Contributor Author

I've just committed a change that should resolve all of these issues. We no longer maintain the device context when it's not in use. To make this possible, I did have to add some changes to imgui.h, but I don't believe the change is invasive.

@ocornut
Copy link
Owner

ocornut commented Jan 3, 2023

Hello,

There is a Platform_RenderWindow hook which could be used instead of Platform_PushOglContext/Platform_PopOglContext:

The examples don't expect current context to be unchanged after rendering platform windows, see e.g. example_opengl3_glfw:

        // Update and Render additional Platform Windows
        // (Platform functions may change the current OpenGL context, so we save/restore it to make it easier to paste this code elsewhere.
        //  For this specific demo app we could also call glfwMakeContextCurrent(window) directly)
        if (io.ConfigFlags & ImGuiConfigFlags_ViewportsEnable)
        {
            GLFWwindow* backup_current_context = glfwGetCurrentContext();
            ImGui::UpdatePlatformWindows();
            ImGui::RenderPlatformWindowsDefault();
            glfwMakeContextCurrent(backup_current_context);
        }

Therefore I don't think a "pop" is needed here and it looks like this could be simplified.

While I have no timeframe for merging this, some other feedback:

  • Make the change to use Platform_RenderWindow(). Ditch the two new hooks and "stashedXXX" variables.
  • Adding an example_win32_opengl3/ would be good.
  • Minor corrections: OglDLL -> OpenGlDLL. Both init function routed through a static/internal ImGui_ImplWin32_InitEx() so it's not confusing for users to find OpenGL in their callstack. Combine all asserts after DLL loading into 1. Refer to GetProcAddress as ::GetProcAddress for consistency. Keep ImGui_ImplWin32_ViewportData() a one-liner.

Thanks!

@ocornut
Copy link
Owner

ocornut commented Apr 7, 2023

Could you post your example app for this?
I'm trying to use #3218 one now but unsure how GL Context version affects code there.

@ocornut
Copy link
Owner

ocornut commented Apr 7, 2023

I rebased, added a (maybe) working example into:
https://github.com/ocornut/imgui/commits/features/win32_opengl_example
And now will try to dig into #5170 #3218 #2772 to find the best way to make this not affect Win32 backend so much.

The whole topic has been extremely confusing, even more so as if we 8 open issues/PR and none of them goes 100%.

#3218 seems particularly elegant, do you know if there's something that it doesn't do?
I feel the change in #3218 could be reworded as a ImGui_ImplWin32_InitForOpenGL().
One thing I'm not sure about #3218 is it use the "GL2" backend and not the GL3 one.

ocornut pushed a commit that referenced this pull request Apr 19, 2023
…2772, #2600, #2359, #2022, #1553)

Removed mulit-viewpot stuff in this commit on master, will be re-added separately.
@ocornut
Copy link
Owner

ocornut commented Apr 19, 2023

Closed/solved, see #3218 (comment)

@ocornut ocornut closed this Apr 19, 2023
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