Configurable OpenGL loader in the example bindings

jdumas commented

2018-08-04 01:01:39 +00:00

(Migrated from github.com)

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

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: ```cpp #ifdef IMGUI_OPENGL_LOADER #include IMGUI_OPENGL_LOADER #else #include <GL/gl3w.h> #endif ```

ocornut commented

2018-08-04 11:33:54 +00:00

(Migrated from github.com)

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

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 commented

2018-08-04 12:20:19 +00:00

(Migrated from github.com)

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.

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 commented

2018-08-04 12:33:41 +00:00

(Migrated from github.com)

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.

> 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 commented

2018-08-04 12:38:59 +00:00

(Migrated from github.com)

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

jdumas commented

2018-08-04 12:43:39 +00:00

(Migrated from github.com)

(Do you want glext as well?)

ocornut commented

2018-08-04 12:48:17 +00:00

(Migrated from github.com)

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!).

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 commented

2018-08-04 14:18:35 +00:00

(Migrated from github.com)

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.

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 commented

2018-08-04 15:07:01 +00:00

(Migrated from github.com)

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 https://github.com/ocornut/imgui/pull/1941#issuecomment-408846799, 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.

> 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 https://github.com/ocornut/imgui/pull/1941#issuecomment-408846799, 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 commented

2018-08-04 15:16:02 +00:00

(Migrated from github.com)

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.

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.

👍 1

ocornut commented

2018-08-04 15:27:38 +00:00

(Migrated from github.com)

(
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.
)

( 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 commented

2018-08-04 15:36:18 +00:00

(Migrated from github.com)

[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 ;)

[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 commented

2018-08-04 15:46:50 +00:00

(Migrated from github.com)

[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!)

[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 commented

2018-08-28 14:08:45 +00:00

(Migrated from github.com)

Closing this (merged #2002).

nickenchev commented

2019-04-28 19:06:24 +00:00

(Migrated from github.com)

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)

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)

Rows
Columns

Configurable OpenGL loader in the example bindings #2001