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

CMake: Add option to use system GLEW 2.0+ #3059

Merged
merged 3 commits into from
Jan 12, 2018

Conversation

akien-mga
Copy link
Contributor

@akien-mga akien-mga commented Nov 22, 2017

The option is enabled by default, but the system version will only be used if found and newer than 2.0.0 (to ensure that https://sourceforge.net/p/glew/patches/40/ is fixed). When missing, we silently fallback to the vendored source code, so there is no change for e.g. Windows compilation.

If prefered, I can also make it opt-in (disabled by default) and throw out an error when GLEW can't be found (like done currently for the AngelScript option), but IMO it's more user-friendly this way.

I plan another PR (Edit: #3060) to update the vendored GLEW code to the latest (unmodified) upstream version. AIUI you had patched GLEW to workaround the above-mentioned issue, but this should no longer be necessary, so it's better to stick to the unmodified code to ease maintenance.

The option is enabled by default, but the system version will only be used
if found and newer than 2.0.0 (to ensure that https://sourceforge.net/p/glew/patches/40/
is fixed). When missing, we silently fallback to the vendored source code,
so there is no change for e.g. Windows compilation.
@deveee
Copy link
Member

deveee commented Nov 22, 2017

One more thing is that glew is used for wayland session too (not enabled by default yet). I didn't check latest glew version, but I suppose that it doesn't dynamically detect if GLX/EGL is available and it will fail when loading glx extensions. At least it was happeining in older versions.

Currently I still use glXGetProcAddress for wayland (it should work as long as we are linking with x11) and there is a patch in glew that checks if glx is available:
961ac4d#diff-b488a83d07787d37ac8d832df8f66b85

And tbh. I wouldn't care that much about the upgrade... Does it fix some actual issues for us? At least for current stable version I would suggest to keep our glew in the package, because nobody tested it with newer glew.

@akien-mga
Copy link
Contributor Author

akien-mga commented Nov 22, 2017

One more thing is that glew is used for wayland session too (not enabled by default yet). I didn't check latest glew version, but I suppose that it doesn't dynamically detect if GLX/EGL context was created and it will fail when loading glx extensions. At least it was happeining in older versions.

I'm not a GLEW expert, but GLEW 2.0.0+ ships with a new GL/eglew.h header, which is included conditionally:

// glew.c
#if defined(GLEW_OSMESA)
#  define GLAPI extern
#  include <GL/osmesa.h>
#elif defined(GLEW_EGL)
#  include <GL/eglew.h>
#elif defined(_WIN32)
/*
 * If NOGDI is defined, wingdi.h won't be included by windows.h, and thus
 * wglGetProcAddress won't be declared. It will instead be implicitly declared,
 * potentially incorrectly, which we don't want.
 */
#  if defined(NOGDI)
#    undef NOGDI
#  endif
#  include <GL/wglew.h>
#elif !defined(__ANDROID__) && !defined(__native_client__) && !defined(__HAIKU__) && (!defined(__APPLE__) || defined(GLEW_APPLE_GLX))
#  include <GL/glxew.h>
#endif

GLEW_EGL doesn't seem to be defined conditionally by GLEW itself, so you'd have to add it to your CMakeLists.txt. That sounds like a better approach than your current patch IMO.

And tbh. I wouldn't care that much about the upgrade... Does it fix some actual issues for us?

Well that's your call, I don't care much either way. Here's the upstream changelog: http://glew.sourceforge.net/log.html
As long as you don't need OpenGL 4.6 or extensions from the last 5 years you should be fine. There might be bug fixes, but as I mentioned I don't have much experience with it and can't judge.
It does however fix all the issues you had patched as far as I can tell:

At least for current stable version I would suggest to keep our glew in the package, because nobody tested it with newer glew.

Note however that the current stable version was just released AFAIK, so it seems like the good time to upgrade libraries and see how that works out for the next stable.

@deveee
Copy link
Member

deveee commented Nov 22, 2017

I know that there is an #ifdef for EGL, but it's probably not used in system glew (?). And anyway I need dynamically detect in STK if user runs it from wayland session or from xorg session. Ideally I should use eglGetProcAddress instead of glXGetProcAddress when wayland session was detected. But it requires more modifications in glew... (or loading all extensions manually).

I will check the current glew version, maybe it doesn't crash without glx anymore.

@deveee
Copy link
Member

deveee commented Jan 12, 2018

I just checked glew 2.1 and it fails to init if glx display is not available:

  /* check for a display */
  display = glXGetCurrentDisplay();
  if (display == NULL) return GLEW_ERROR_NO_GLX_DISPLAY;

I opened a ticket for it:
nigels-com/glew#172

@deveee
Copy link
Member

deveee commented Jan 12, 2018

Actually if current version doesn't crash, then I can just ignore the GLEW_ERROR_NO_GLX_DISPLAY error.

@deveee deveee merged commit 84459c6 into supertuxkart:master Jan 12, 2018
@deveee
Copy link
Member

deveee commented Jan 12, 2018

"Resolve conflicts" on github was doing something strange, but it looks that it's merged correctly :P Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants