Skip to content
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

Fix for OpenGL<=3.2 + GLEW #3467

Closed
wants to merge 2 commits into from
Closed

Fix for OpenGL<=3.2 + GLEW #3467

wants to merge 2 commits into from

Conversation

jjwebb
Copy link
Contributor

@jjwebb jjwebb commented Sep 10, 2020

I'm using OpenGL 3.1 ES on a Raspberry Pi 4 and was getting a segfault at glBindSampler(). Using GLEW, #ifdef GL_SAMPLER_BINDING always evaluates to true because it is defined in glew.h even if sampler binding is not supported. In glew.h:

/* ------------------------- GL_ARB_sampler_objects ------------------------ */

#ifndef GL_ARB_sampler_objects
#define GL_ARB_sampler_objects 1

#define GL_SAMPLER_BINDING 0x8919

Testing against imgui's own g_GlVersion variable to make sure OpenGL version is above 3.2 (after which sampler binding is supported) clears up this error.

I'm using OpenGL 3.1 ES on a Raspberry Pi 4 and was getting a segfault at glBindSampler(). Using GLEW, #ifdef GL_SAMPLER_BINDING always evaluates to true because it is defined in glew.h even if sampler binding is not supported. In glew.h: 

/* ------------------------- GL_ARB_sampler_objects ------------------------ */

#ifndef GL_ARB_sampler_objects
#define GL_ARB_sampler_objects 1

#define GL_SAMPLER_BINDING 0x8919

Testing against imgui's own g_GlVersion variable to make sure OpenGL version is above 3.2 (after which sampler binding is supported) clears up this error.
Add additional precompiler check for emscripten
@ocornut
Copy link
Owner

ocornut commented Sep 16, 2020

Hello and thanks for this PR,
I am a little bit confused as to why you are using BOTH a check on g_GlVersion and a check on IMGUI_IMPL_OPENGL_ES2/ES3 defines. Wouldn't only checking g_GlVersion be enough?

@jjwebb
Copy link
Contributor Author

jjwebb commented Sep 16, 2020

Hi Omar,
That's what I had figured -- the first commit has just the check against g_Glversion, but the build check for Emscripten failed because the code got executed. I can't speak to why that is because I'm not using Emscripten, but the redundant check cleared up the problem. Admittedly it's a bit inelegant, but I figured since the first check is a preprocessor directive, it's not adding any additional complexity to the binary and might even remove some.

ocornut pushed a commit that referenced this pull request Sep 17, 2020
…<= 3.2 (#3467, #1985)

(nb: GLEW sets the define we previously used)
ocornut added a commit that referenced this pull request Sep 17, 2020
@ocornut
Copy link
Owner

ocornut commented Sep 17, 2020

Thank you @jjwebb, this is now merged! (I added an intermediary define in there to mimic the vtxoffset one too)

@ocornut ocornut closed this Sep 17, 2020
ocornut added a commit that referenced this pull request Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants