Cleanup OpenGL 3 loader detection

[funchal]

OpenGL 3 backend header has some logic to auto-detect the GL loader, however that is only necessary for the implementation in the cpp, not in the header. This change moves the code from the header to the cpp. This avoids unnecessary global macro pollution as well as potential for mistakes since previously IMGUI_IMPL_OPENGL_LOADER_* and IMGUI_IMPL_OPENGL_* would have to have consistent values every time imgui_impl_opengl3.h was included.

[ocornut]

Hello @funchal and thanks for the PR.

You are right most of this doesn't need to be in the header file. Prior to #2798 this block only had a few #ifdef and #defines.

Could you:

1. Keep the OpenGL ES commented defines in the .h (instead of .cpp), so they are visible and helpful as documentation.

2. Add commented #define for each of the 6 desktop loaders (likewise, for documentation) along with a comment redirecting to the cpp file for details on loaders.

Thank you!

[funchal]

Thank you so much for taking the time to review this. I have applied your suggestions.

dear imgui is awesome, I've been a user for absolutely ages and I'm happy that I finally found a little time to try to contribute.

[ocornut]

Thank you!

Well, bad news ahead :/
Looking at the final patch, it looks like the need to add the define to the main.cpp files defeat the purpose of this unfortunately, so I don't think it is worth it anymore (and would technically constitute a breakage, even if I'm not too fussed by small breakage in back-end api).

Also note that imgui_impl_opengl3.h is unlikely to need to be included from more than 1 compile-unit from user point of view.

[funchal]

Yes, I agree.

However, note that when IMGUI_IMPL_OPENGL_ES2/3 is defined, we ignore these settings. See below:

imgui/examples/imgui_impl_opengl3.cpp

Lines 124 to 131 in 2e2c962

	#if defined(IMGUI_IMPL_OPENGL_ES2) || defined(IMGUI_IMPL_OPENGL_ES3)
	#undef IMGUI_IMPL_OPENGL_LOADER_GL3W
	#undef IMGUI_IMPL_OPENGL_LOADER_GLEW
	#undef IMGUI_IMPL_OPENGL_LOADER_GLAD
	#undef IMGUI_IMPL_OPENGL_LOADER_GLBINDING2
	#undef IMGUI_IMPL_OPENGL_LOADER_GLBINDING3
	#undef IMGUI_IMPL_OPENGL_LOADER_CUSTOM
	#endif

This is what bothered me originally. In my case I use ES2 so I had to define IMGUI_IMPL_OPENGL_ES2 as well as IMGUI_IMPL_OPENGL_LOADER_CUSTOM otherwise the autodetection would (incorrectly) kick in and #include the wrong thing.

So let me suggest one more thing. Perhaps what we should do is move the code for IMGUI_IMPL_OPENGL_ES2/3 detection from cpp to h in the other direction, so that it happens before the __has_include auto-detection. In this case if IMGUI_IMPL_OPENGL_ES2 is defined by a user (or autodetected) then IMGUI_IMPL_OPENGL_LOADER_CUSTOM doesn't have to also be provided.

imgui/examples/imgui_impl_opengl3.cpp

Lines 115 to 122 in 2e2c962

	// Auto-enable GLES on matching platforms
	#if !defined(IMGUI_IMPL_OPENGL_ES2) && !defined(IMGUI_IMPL_OPENGL_ES3)
	#if (defined(__APPLE__) && (TARGET_OS_IOS || TARGET_OS_TV)) || (defined(__ANDROID__))
	#define IMGUI_IMPL_OPENGL_ES3 // iOS, Android -> GL ES 3, "#version 300 es"
	#elif defined(__EMSCRIPTEN__)
	#define IMGUI_IMPL_OPENGL_ES2 // Emscripten -> GL ES 2, "#version 100"
	#endif
	#endif

In other words, basically the header file would have this (in order):

// Auto-enable GLES on matching platforms 
#if !defined(IMGUI_IMPL_OPENGL_ES2) && !defined(IMGUI_IMPL_OPENGL_ES3) 

#if (defined(__APPLE__) && (TARGET_OS_IOS || TARGET_OS_TV)) || (defined(__ANDROID__)) 
#define IMGUI_IMPL_OPENGL_ES3           // iOS, Android  -> GL ES 3, "#version 300 es" 
#elif defined(__EMSCRIPTEN__) 
#define IMGUI_IMPL_OPENGL_ES2           // Emscripten    -> GL ES 2, "#version 100" 
#else

// Desktop OpenGL: attempt to detect default GL loader based on available header files.
// If auto-detection fails or doesn't select the same GL loader file as used by your application,
// you are likely to get a crash in ImGui_ImplOpenGL3_Init().
// You can explicitly select a loader by using '#define IMGUI_IMPL_OPENGL_LOADER_XXX' in imconfig.h or compiler command-line.
    #if !defined(IMGUI_IMPL_OPENGL_LOADER_GL3W) \
     && !defined(IMGUI_IMPL_OPENGL_LOADER_GLEW) \
     && !defined(IMGUI_IMPL_OPENGL_LOADER_GLAD) \
     && !defined(IMGUI_IMPL_OPENGL_LOADER_GLBINDING2) \
     && !defined(IMGUI_IMPL_OPENGL_LOADER_GLBINDING3) \
     && !defined(IMGUI_IMPL_OPENGL_LOADER_CUSTOM)
        #if defined(__has_include)
            #if __has_include(<GL/glew.h>)
                #define IMGUI_IMPL_OPENGL_LOADER_GLEW
            #elif __has_include(<glad/glad.h>)
                #define IMGUI_IMPL_OPENGL_LOADER_GLAD
            #elif __has_include(<GL/gl3w.h>)
                #define IMGUI_IMPL_OPENGL_LOADER_GL3W
            #elif __has_include(<glbinding/glbinding.h>)
                #define IMGUI_IMPL_OPENGL_LOADER_GLBINDING3
            #elif __has_include(<glbinding/Binding.h>)
                #define IMGUI_IMPL_OPENGL_LOADER_GLBINDING2
            #else
                #error "Cannot detect OpenGL loader!"
            #endif
        #else
            #define IMGUI_IMPL_OPENGL_LOADER_GL3W       // Default to GL3W
        #endif
    #endif
#endif

#if defined(IMGUI_IMPL_OPENGL_ES2) || defined(IMGUI_IMPL_OPENGL_ES3) 
#undef IMGUI_IMPL_OPENGL_LOADER_GL3W
#undef IMGUI_IMPL_OPENGL_LOADER_GLEW
#undef IMGUI_IMPL_OPENGL_LOADER_GLAD
#undef IMGUI_IMPL_OPENGL_LOADER_GLBINDING2
#undef IMGUI_IMPL_OPENGL_LOADER_GLBINDING3
#undef IMGUI_IMPL_OPENGL_LOADER_CUSTOM
#endif 

[ocornut]

I agree yes. Seems like this was overlooked when adding the auto-detection macros.

• Note that <TargetConditionals.h> would need to be included on OS X (this is fine)
• I think the auto-detection block could be omitted if either of the ES define are set, by adding two extra #ifndef in that block.
• And then it means we can delete the #undef block perhaps?

[funchal]

Yes, I think this is better. Updated.

[ocornut]

Merged with minor tweaks (moved comments below to prioritize important contents of imgui_impl_opengl3.h)
Thanks a lot for your help!