Configurable OpenGL loader in the example bindings

[jdumas]

Hi there,

In order to be able to use the example bindings as is (GLFW + OpenGL 3 as far as I am concerned), I'd suggest to make the OpenGL loader configurable (e.g. via a macro IMGUI_OPENGL_LOADER). For example, in imgui_impl_opengl3.cpp, you could have something along the lines of:

#ifdef IMGUI_OPENGL_LOADER
#include IMGUI_OPENGL_LOADER
#else
#include <GL/gl3w.h>
#endif

[ocornut]

I agree we should do that.

I think it would be nice to have explicitly named defines for the most-common 3/4 loaders and then a _CUSTOM version that does the include.
so e.g.

#define IMGUI_IMPL_OPENGL_LOADER_GL3W
//#define IMGUI_IMPL_OPENGL_LOADER_GLEW
//#define IMGUI_IMPL_OPENGL_LOADER_GLAD
//#define IMGUI_IMPL_OPENGL_LOADER_CUSTOM       <GL/myloader.h>

And it would need to default to something (GL3W probably.

The main.cpp in examples/ would also need to perform the initialization call for those common 3/4 loaders.

Let me know if you want to work on a PR for it, that would be helpful.

Thanks,
Omar

[jdumas]

I think it's probably enough to have a "custom" override, but we can do as you suggest. I mean the user probably already knows how to initialize their OpenGL loader, so I'm not sure if it's worth putting it into the main.cpp of each example.

I can work on a PR for this, but can only test on the Linux side.

[ocornut]

the user probably already knows how to initialize their OpenGL loader

If the questions posted on this Issue Tracker are any indication, lots of users are confused by most of the code involved in the back-ends. C++ is a tough ecosystem and OpenGL is messy (the loader situation itself being a good reminder of that). So generally, making things as obvious and clear as possible is valuable to reduce further support requests.

See #1988 for example.

I can work on a PR for this, but can only test on the Linux side.

That would be great, then I'll do some testing on Windows.

[jdumas]

Ok no prob, we can write loader code for glad/gl3w/glew :D

[jdumas]

(Do you want glext as well?)

[ocornut]

As many as possible provided they are mostly all one-liners (~1 line of include, ~1 of init) and not totally obscure.

To clarify, we won't provide the code/library for them in the repository, so those lines are only provided as reference.

Since you have to do some of it for your testing, I guess it wouldn't hurt to add comments e.g. in the Makefile describing the typical gcc/clang command-line options associated to a loader (both the -I and the -D part) but that's not mandatory. When we include cmake/premake files in the repo we'll include something to facilitate their use (even if it's just comments!).

[jdumas]

Ok I've open a PR with a WIP changes so we can comment on this.

One thing I've noticed with the glfw+opengl3 example: right now, example_glfw_opengl3/main.cpp loads OpenGL 3.0. However, if using glad with OpenGL 3.3, the macro GL_SAMPLER_BINDING will be defined and thus calls to glBindSampler() will be made, leading to a runtime error (since profile 3.2 was loaded by glfw). So probably testing for the presence of GL_SAMPLER_BINDING is not a very robust way of telling what version of OpenGL is available at runtime. I'm not sure what's the solution for this but I wanted to point it out.

[ocornut]

I'll start with gl3w/glad/glew for now, as they are the more popular.

Yes, this is fine and more than enough. We can perfectly add more later if someone mentions something.

For now I've only added the glad backend for glfw+opengl3. I believe sdl+opengl3 will be the only other example needing a custom OpenGL loader.

Correct. My suggestion is to ignore the sdl+opengl3 main.cpp for now. When we are happy with the change you or I can replicate them at the end.

One thing I've noticed with the glfw+opengl3 example: right now, example_glfw_opengl3/main.cpp loads OpenGL 3.0. However, if using glad with OpenGL 3.3, the macro GL_SAMPLER_BINDING will be defined and thus calls to glBindSampler() will be made, leading to a runtime error (since profile 3.2 was loaded by glfw). So probably testing for the presence of GL_SAMPLER_BINDING is not a very robust way of telling what version of OpenGL is available at runtime. I'm not sure what's the solution for this but I wanted to point it out.

Good point, it was discussed in #1985 and #1941 (comment), and I think that @ziocleto PR was faulty (see my last comment in #1985) and should be reverted.

Instead my suggestion would to be to keep the #ifdef GL_SAMPLER_BINDING and then also test for the function pointer inside that block, and see if it work with those loaders. Then we need to add #pragma warning push/ignore/pop block surrounding imgui_impl_opengl3, in order for GCC+Clang to disable those absolutely non-productive warnings that #1985 was trying to remove.

[ziocleto]

Yes we have warnings as errors everywhere, so if it's impracticable to detect if that function is defined and evaluating opengl es version is tricky then ignoring that warning but keeping the if should work.

[ocornut]

( Mildly derailing the topic here, I am always surprised that people use such aggressive warning settings for _third-party_ code. Surely you are not going to fix everyone’s else code, why not restrict those settings to your own code? GCC in particular likes to add new doses of warnings every version, it’s not practical to submit third-party repos code which may be stable/unchanging to those ever-evolving requirements. )

[ziocleto]

[OT] We usually link external libs as system libs so we do not have to worry about warnings as errors when building our source code. But for dear imgui I was just trying to play with it and noticed it won't compile ;) I didn't know what your build pipeline policies were :).

Generally I agree especially with GCC warnings some times are a PITA but it's also good practice IMO to at least know what they are because most often than not they can masquerade problems with your code.
Also having warnings produced really hampers the reading of the build pipeline logs, as you will be flooded with them.

[OT2] I saw you worked in Guildford I've worked there on the studio across the road for 10 years also ;)

[ocornut]

[OT] Don’t get me wrong, I also have a zero-warning policy on most project. In the particular case if imgui it’s tricky to allow users to compile with modern pedantic warnings as they tend to warn for stylistic things that we have choosen (C style cast) or things which are required for compatibility with older compilers (imgui in theory compiles with C++03, thought I am tempted to raise the bar to Visual Studio 2012 level of C++11 compat as we would benefit from a few things such as eg: forward defined enums). I am always interested in warning reports (in a new thread!) even if it means we take the decision to ignore warning xx in imgui codebase, at least the user doesn’t have to be bothered by it. (Not sure _which_ studio accross the road, there were several, but hi!)

[ocornut]

Closing this (merged #2002).

[nickenchev]

Just throwing my 2 cents in. I setup the loader via https://cmake.org/cmake/help/latest/command/add_compile_definitions.html#command:add_compile_definitions

add_compile_definitions(IMGUI_IMPL_OPENGL_LOADER_GLEW)