ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
59.38k stars 10.11k forks source link

IMGUI_IMPL_OPENGL_LOADER_CUSTOM isn't included anymore #4810

Open ShadowNinja opened 2 years ago

ShadowNinja commented 2 years ago

Environment

ImGui Version: 1.85 (master) Platform Backend: imgui_impl_sdl.cpp Render Backend: imgui_impl_opengl3.cpp Operating System: Linux Desktop: Plasma Wayland

Issue

ImGui checks for an IMGUI_IMPL_OPENGL_LOADER_CUSTOM define, and it used to import it if it was defined, but after the ImGL3W loader changes it no longer does that, so it simply fails to compile because it doesn't include any loader, unless you build it as an amalgam that includes a loader.

The reason I'm using a custom loader is because I want my application to have native Wayland support. The included gl3w-based loader uses GLX, which is an X11-specific API. This seems to work because GLX is still available on my system and glxGetProcAddress happens to still work, but only EGL is officially supported under Wayland. I can use my own loader (GLAD with SDL_GL_GetProcAddress which supports EGL), but I can't get ImGui to use that loader because of this issue.

Proposed Solution

I solved the issue with the following patch:

--- imgui_impl_opengl3.cpp
+++ imgui_impl_opengl3.cpp
@@ -113,7 +113,9 @@
 #else
 #include <GLES3/gl3.h>          // Use GL ES 3
 #endif
-#elif !defined(IMGUI_IMPL_OPENGL_LOADER_CUSTOM)
+#elif defined(IMGUI_IMPL_OPENGL_LOADER_CUSTOM)
+#include IMGUI_IMPL_OPENGL_LOADER_CUSTOM
+#else
 // Modern desktop OpenGL doesn't have a standard portable header file to load OpenGL function pointers.
 // Helper libraries are often used for this purpose! Here we are using our own minimal custom loader based on gl3w.
 // In the rest of your app/engine, you can use another loader of your choice (gl3w, glew, glad, glbinding, glext, glLoadGen, etc.).

My problem would also be solved by adding EGL support to the default loader so I don't need a custom loader (which should be done anyways, but that's really a gl3w issue). But I'd still like IMGUI_IMPL_OPENGL_LOADER_CUSTOM to work without using an amalgam.

ocornut commented 2 years ago

We're only keeping IMGUI_IMPL_OPENGL_LOADER_CUSTOM to support case of single-compilation-unit build (perhaps that macro should be renamed).

My problem would also be solved by adding EGL support to the default loader so I don't need a custom loader (which should be done anyways, but that's really a gl3w issue).

We should do this, you can attempt patches to https://github.com/dearimgui/imgl3w

This seems to work because GLX is still available on my system and glxGetProcAddress happens to still work, but only EGL is officially supported under Wayland.

However it seems that things already work for you and you are only fixing a theoretical problem.

ShadowNinja commented 2 years ago

We're only keeping IMGUI_IMPL_OPENGL_LOADER_CUSTOM to support case of single-compilation-unit build (perhaps that macro should be renamed).

Ah, it should work for that, although it would be nice if it also worked when the backend was compiled on its own.

We should [support EGL], you can attempt patches to https://github.com/dearimgui/imgl3w

I'm working on a patch for gl3w.

One issue is that the GetProcAddress implementation could end up using EGL while other parts of your application, (or even other loaders) use GLX (or vice-versa). I don't know if this would actually be an issue in practice though, it seems like it still works when using the GLVND implementations of GLX/EGL, even when GLX and EGL dispatch to different vendor OpenGL drivers (I'm not sure if it would work in this case).

However it seems that things already work for you and you are only fixing a theoretical problem.

Yes, it works on my machine since GLX is still installed and glXGetProcAddressARB in particular still returns valid addresses under Wayland, but that could be different on other systems.

ocornut commented 2 years ago

glXGetProcAddressARB in particular still returns valid addresses under Wayland, but that could be different on other systems.

if I were you i would wait to actually run into a problem before blindly solving this.

PathogenDavid commented 2 years ago

I dug into this a little bit out of curiosity since I've been messing with Wayland lately, and while I'm not a huge fan of mixing GLX and EGL either I don't think it's really an issue of immediate concern. (Although I do worry it's something that could go bang after a driver update though since it's not exactly typical.)


When it comes to supporting EGL in the first place, one practical concern I do have though is that our current loader implementation is implicitly causing anyone using the OpenGL backend to depend on X since libGL depends on (is?) GLX which depends on X. (This is the whole reason Wayland uses EGL instead of libGL in the first place -- "Why does Wayland use EGL?")

In 2021 I don't think it's actually practical to have a useful Linux system running Wayland without XWayland yet. Maybe you can with Weston?


One issue is that the GetProcAddress implementation could end up using EGL while other parts of your application, (or even other loaders) use GLX (or vice-versa).

Could we use RTLD_NOLOAD with dlopen to test which one was previously loaded and have it prefer that one? (And if neither is loaded just try to load whatever's present.)

I don't think that'd be appropriate for gl3w upstream, but it'd make sense for a sibling GL loader like Dear ImGui's.

ShadowNinja commented 2 years ago

Could we use RTLD_NOLOAD with dlopen to test which one was previously loaded and have it prefer that one? (And if neither is loaded just try to load whatever's present.)

I don't think that'd be appropriate for gl3w upstream, but it'd make sense for a sibling GL loader like Dear ImGui's.

Yes, I considered this. It's a bit of a hack, but it seems to work. It might make sense to have it upstream too, since you still want the loader to match what the windowing library (SDL/glfw/glut/etc) is using.

I've made a PR here: skaslev/gl3w#74

ocornut commented 2 years ago

Thanks! We'll merge it into our fork (need to do a little uneasy rebasing as we moved a template to a file, may submit a PR to gl3w upstream too).