-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Automatically include the available gl loader header #2798
Conversation
Note that __has_include is also a GCC extension, so it will work for that compiler too. |
@corentin-plouet oh year sure, it also is supported now on MSVC ... I just wanted to point out in case the extension isn't available everything will be fine still ... 😉 |
Hello @o-micron, Unfortunately I am not sure this is a desirable change to make. Multiple headers may be available on the system, header availability doesn't mean that said loader is being used by the application. While in the examples's main.cpp case we rely on the same If a used tries to integrate the OpenGL back-end in their app which already uses and initialize glad, but the system has header files for |
I get your point, I also thought that would cause problems in case someone has multiple gl loader headers in the same project. But why would I ever need to even put multiple gl loaders in the same project ? I mean, I am not sure about other projects, but for me, I only use a single header, glew for example, and that's all I need, there is no reason to use glad for example while I already have glew ... I thought I would add some automatic header inclusion in case the user didn't actually specify one, which is the default behaviour ... you have a single opengl header and you want imgui to use it without the need to actually define a macro to say that ... I dunno ... I thought that would make things easier 😃 |
I agree there's little point using multiple loaders simultaneously. That's the issue I highlighted... with this autodection now it is more likely to happen by accident. You are also correct this is merely a default behavior, but this new potential issue it can create may be extremely difficult to understand for the end-user? Effectively it will manifest as a crash when calling GL functions in the renderer. Previously it would more likely fail to build and requires a loader to be selected explicitely. Now it will success to build but may lead to crash that will appear mysterious to the user. Tricky trade-off ? (EDIT: typos) |
@ocornut yeah, I think if the user already is having multiple gl loaders in his/her project, then probably it would eventually crash with or without imgui ... his/her own code would eventually crash like you said ... that isn't imgui's problem in this case I guess ... (EDIT) |
It's not a problem if the user is having multiple GL loader in their projects, it's a problem if the user have multiple GL loaders installed on their system (more likely) and the one they already use for their application doesn't match the one that those auto-detection statements will select. |
In addition there's not a single line of code in We can minimize this issue by adding code under |
Yeah, usually the user would have one and use one ... ? |
I don't think you are understanding my point here.
Previously, compilation would fail and force the user to select Glad. With the change, compilation will succeed and leave the user with a really hard to comprehend error. |
So my suggestion is to add this in the Init function: (I have intentionally taken a screenshot of the code to show syntax coloring) |
I think this also would fix this issue at compile time, leading to no surprises at runtime ... // to avoid problem with non-clang compilers not having this macro.
#if defined(__has_include)
// check if the header exists, then automatically define the macros ..
#if __has_include(<GL/glew.h>)
#if __has_include(<glad/glad.h>)
#error you have multiple headers in your system (GLEW + GLAD)
#else
#define IMGUI_IMPL_OPENGL_LOADER_GLEW
#endif
#elif __has_include(<glad/glad.h>)
#if __has_include(<GL/glew.h>)
#error you have multiple headers in your system (GLEW + GLAD)
#else
#define IMGUI_IMPL_OPENGL_LOADER_GLAD
#endif
#else
#define IMGUI_IMPL_OPENGL_LOADER_GL3W
#endif
#else
#define IMGUI_IMPL_OPENGL_LOADER_GL3W
#endif |
…e to the user which gl loader headers is being used
Merged now with a few changes in an extra commit (9769164), looks like our commits crossed.. I haven't used your second commit, partly because in theory we ought to check for more combinations (incl gl3w, in my change I am also explicitely using __has_include for gl3w) and I don't want to make that block in imgui_impl_opengl3.h too unwelcoming. The only reason this block is in the .h file is for the benefit of the example's main.cpp files but it should be required for the user. I think with those changes we'll got a good solution in, what do you thnik? |
Cool, I was adding the dummy constructs you wanted to make it more visible ... |
Thanks for your help with this, great idea :) Passing tests on CI machines too: https://travis-ci.com/ocornut/imgui |
…le to match linking settings (otherwise if another loader such as Glew is accessible, the opengl3 backend might automatically use it). [ocornut#2919, ocornut#2798]
This change is additive and doesn't change the existing code.
It handles the case when we don't define any of these macros
{
IMGUI_IMPL_OPENGL_LOADER_GL3W
,IMGUI_IMPL_OPENGL_LOADER_GLEW
,IMGUI_IMPL_OPENGL_LOADER_GLAD
,IMGUI_IMPL_OPENGL_LOADER_CUSTOM
]
Using the
__has_include
clang extension, we can automatically know which header is available,then we use it directly without predefined macros or anything, this fits naturally in the code without having to use gl3w or supply any macros ...