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

Android compilation broken after sRGB additions #1079

Closed
gordonmcshane opened this Issue Apr 4, 2016 · 16 comments

Comments

@gordonmcshane

gordonmcshane commented Apr 4, 2016

Workaround:
gordonmcshane@c89d1a9

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 4, 2016

Member

Would been nice if you add some more information or even better follow the Contribution Guidelines...

Member

eXpl0it3r commented Apr 4, 2016

Would been nice if you add some more information or even better follow the Contribution Guidelines...

@gordonmcshane

This comment has been minimized.

Show comment
Hide comment
@gordonmcshane

gordonmcshane Apr 5, 2016

Apologies. I don't spend a lot of time in SFML code base and didn't want to be presumptuous in thinking my solution was sufficiently correct enough not to break other aspects of SFML which is why there was no pull request. My intent was just to unblock anyone while a better fix was worked out. I should have left a log though:

[ 79%] Building CXX object src/SFML/Graphics/CMakeFiles/sfml-graphics.dir/RenderTextureImplDefault.cpp.o
In file included from /Users/gordon/Projects/SFML/src/SFML/Graphics/GLCheck.hpp:32:0,
                 from /Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp:30:
/Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp: In member function 'bool sf::Texture::create(unsigned int, unsigned int)':
/Users/gordon/Projects/SFML/src/SFML/Graphics/GLExtensions.hpp:118:55: error: 'GL_EXT_sRGB' was not declared in this scope
     #define GLEXT_texture_sRGB                        GL_EXT_sRGB
                                                       ^
/Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp:190:31: note: in expansion of macro 'GLEXT_texture_sRGB'
     static bool textureSrgb = GLEXT_texture_sRGB;
                               ^
In file included from /Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp:30:0:
/Users/gordon/Projects/SFML/src/SFML/Graphics/GLExtensions.hpp:119:55: error: 'GL_SRGB8_ALPHA8_EXT' was not declared in this scope
     #define GLEXT_GL_SRGB8_ALPHA8                     GL_SRGB8_ALPHA8_EXT
                                                       ^
/Users/gordon/Projects/SFML/src/SFML/Graphics/GLCheck.hpp:51:28: note: in definition of macro 'glCheck'
     #define glCheck(expr) (expr)
                            ^
/Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp:213:54: note: in expansion of macro 'GLEXT_GL_SRGB8_ALPHA8'
     glCheck(glTexImage2D(GL_TEXTURE_2D, 0, (m_sRgb ? GLEXT_GL_SRGB8_ALPHA8 : GL_RGBA), m_actualSize.x, m_actualSize.y, 0, GL_RGBA, GL_UNSIGNED_BYTE, NULL));
                                                      ^
make[2]: *** [src/SFML/Graphics/CMakeFiles/sfml-graphics.dir/Texture.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [src/SFML/Graphics/CMakeFiles/sfml-graphics.dir/all] Error 2
make: *** [all] Error 2

If you think my changes are at least a decent starting point, let me know and I will submit a pull request per the contribution guidelines to be reviewed.

gordonmcshane commented Apr 5, 2016

Apologies. I don't spend a lot of time in SFML code base and didn't want to be presumptuous in thinking my solution was sufficiently correct enough not to break other aspects of SFML which is why there was no pull request. My intent was just to unblock anyone while a better fix was worked out. I should have left a log though:

[ 79%] Building CXX object src/SFML/Graphics/CMakeFiles/sfml-graphics.dir/RenderTextureImplDefault.cpp.o
In file included from /Users/gordon/Projects/SFML/src/SFML/Graphics/GLCheck.hpp:32:0,
                 from /Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp:30:
/Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp: In member function 'bool sf::Texture::create(unsigned int, unsigned int)':
/Users/gordon/Projects/SFML/src/SFML/Graphics/GLExtensions.hpp:118:55: error: 'GL_EXT_sRGB' was not declared in this scope
     #define GLEXT_texture_sRGB                        GL_EXT_sRGB
                                                       ^
/Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp:190:31: note: in expansion of macro 'GLEXT_texture_sRGB'
     static bool textureSrgb = GLEXT_texture_sRGB;
                               ^
In file included from /Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp:30:0:
/Users/gordon/Projects/SFML/src/SFML/Graphics/GLExtensions.hpp:119:55: error: 'GL_SRGB8_ALPHA8_EXT' was not declared in this scope
     #define GLEXT_GL_SRGB8_ALPHA8                     GL_SRGB8_ALPHA8_EXT
                                                       ^
/Users/gordon/Projects/SFML/src/SFML/Graphics/GLCheck.hpp:51:28: note: in definition of macro 'glCheck'
     #define glCheck(expr) (expr)
                            ^
/Users/gordon/Projects/SFML/src/SFML/Graphics/Texture.cpp:213:54: note: in expansion of macro 'GLEXT_GL_SRGB8_ALPHA8'
     glCheck(glTexImage2D(GL_TEXTURE_2D, 0, (m_sRgb ? GLEXT_GL_SRGB8_ALPHA8 : GL_RGBA), m_actualSize.x, m_actualSize.y, 0, GL_RGBA, GL_UNSIGNED_BYTE, NULL));
                                                      ^
make[2]: *** [src/SFML/Graphics/CMakeFiles/sfml-graphics.dir/Texture.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [src/SFML/Graphics/CMakeFiles/sfml-graphics.dir/all] Error 2
make: *** [all] Error 2

If you think my changes are at least a decent starting point, let me know and I will submit a pull request per the contribution guidelines to be reviewed.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 5, 2016

Member

That's why encourage people in the Contribution Guidelines to first use the forum and reserve the issue tracker for confirmed issues.

What Android NDK are you using? What OS are you on? etc.

Member

eXpl0it3r commented Apr 5, 2016

That's why encourage people in the Contribution Guidelines to first use the forum and reserve the issue tracker for confirmed issues.

What Android NDK are you using? What OS are you on? etc.

@gordonmcshane

This comment has been minimized.

Show comment
Hide comment
@gordonmcshane

gordonmcshane Apr 5, 2016

Host: OSX 10.11.4
Target: armeabi-v7a (no thumb)

NDK r11b (which removed 4.8 and clang 3.5)
CMake 3.5.1

Toolchain: prebuilt gcc 4.9
STL: gnustl_shared

CMake command line:

cmake -DANDROID_ABI="armeabi-v7a" -DANDROID_FORCE_ARM_BUILD=ON -DANDROID_NATIVE_API_LEVEL=19 -DCMAKE_TOOLCHAIN_FILE=../../cmake/toolchains/android.toolchain.cmake ../..

Additionally, the android.toolchain provided with SFML in master doesn't support r11b (the SDK layout has changed completely) and additionally has incompatibilities with homebrew on OSX. I patched it here: gordonmcshane@9919e55

gordonmcshane commented Apr 5, 2016

Host: OSX 10.11.4
Target: armeabi-v7a (no thumb)

NDK r11b (which removed 4.8 and clang 3.5)
CMake 3.5.1

Toolchain: prebuilt gcc 4.9
STL: gnustl_shared

CMake command line:

cmake -DANDROID_ABI="armeabi-v7a" -DANDROID_FORCE_ARM_BUILD=ON -DANDROID_NATIVE_API_LEVEL=19 -DCMAKE_TOOLCHAIN_FILE=../../cmake/toolchains/android.toolchain.cmake ../..

Additionally, the android.toolchain provided with SFML in master doesn't support r11b (the SDK layout has changed completely) and additionally has incompatibilities with homebrew on OSX. I patched it here: gordonmcshane@9919e55

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 15, 2016

Member

@gordonmcshane Could you provide a PR for the 1st patch? (And only that one so that we can discuss, if needed, the other one separately. For the second one, you should probably explain exactly what's your setup and the issue you're facing on the forum.)

Member

mantognini commented Apr 15, 2016

@gordonmcshane Could you provide a PR for the 1st patch? (And only that one so that we can discuss, if needed, the other one separately. For the second one, you should probably explain exactly what's your setup and the issue you're facing on the forum.)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 13, 2016

Member

Finally had some time to check this. The linked patch is not needed.

The correct way to do this is forcing a minimum NDK API level of 12 (Android 3.1.x or later). What do you think?

If we're using the provided patch, we'll lose that support even for versions that support it.

Member

MarioLiebisch commented May 13, 2016

Finally had some time to check this. The linked patch is not needed.

The correct way to do this is forcing a minimum NDK API level of 12 (Android 3.1.x or later). What do you think?

If we're using the provided patch, we'll lose that support even for versions that support it.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 13, 2016

Member

The correct way to do this is forcing a minimum NDK API level of 12 (Android 3.1.x or later). What do you think?

#1085 seems to do that but with min = 21.

Member

mantognini commented May 13, 2016

The correct way to do this is forcing a minimum NDK API level of 12 (Android 3.1.x or later). What do you think?

#1085 seems to do that but with min = 21.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 13, 2016

Member

Which is overkill. API 21 is 5.0 and newer only.

Member

MarioLiebisch commented May 13, 2016

Which is overkill. API 21 is 5.0 and newer only.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 14, 2016

Member
diff --git a/src/SFML/Graphics/GLExtensions.hpp b/src/SFML/Graphics/GLExtensions.hpp
index c3333b7..c9ed451 100644
--- a/src/SFML/Graphics/GLExtensions.hpp
+++ b/src/SFML/Graphics/GLExtensions.hpp
@@ -34,6 +34,14 @@

     #include <SFML/OpenGL.hpp>

+    // Since SFML enforces Android API level 9 or greater,
+    // <GLES2/gl2ext.h> will always be available on Android
+    #ifdef SFML_SYSTEM_ANDROID
+
+        #include <GLES2/gl2ext.h>
+
+    #endif
+
     // SFML requires at a bare minimum OpenGL ES 1.0 capability
     // Some extensions only incorporated by 2.0 are also required
     // OpenGL ES 1.0 is defined relative to OpenGL 1.3
@@ -112,8 +120,13 @@
     #define GLEXT_GL_INVALID_FRAMEBUFFER_OPERATION    GL_INVALID_FRAMEBUFFER_OPERATION_OES

     // Core since 3.0 - EXT_sRGB
-    #define GLEXT_texture_sRGB                        GL_EXT_sRGB
-    #define GLEXT_GL_SRGB8_ALPHA8                     GL_SRGB8_ALPHA8_EXT
+    #ifdef GL_EXT_sRGB
+        #define GLEXT_texture_sRGB                        GL_EXT_sRGB
+        #define GLEXT_GL_SRGB8_ALPHA8                     GL_SRGB8_ALPHA8_EXT
+    #else
+        #define GLEXT_texture_sRGB                        false
+        #define GLEXT_GL_SRGB8_ALPHA8                     0
+    #endif

 #else

Member

binary1248 commented May 14, 2016

diff --git a/src/SFML/Graphics/GLExtensions.hpp b/src/SFML/Graphics/GLExtensions.hpp
index c3333b7..c9ed451 100644
--- a/src/SFML/Graphics/GLExtensions.hpp
+++ b/src/SFML/Graphics/GLExtensions.hpp
@@ -34,6 +34,14 @@

     #include <SFML/OpenGL.hpp>

+    // Since SFML enforces Android API level 9 or greater,
+    // <GLES2/gl2ext.h> will always be available on Android
+    #ifdef SFML_SYSTEM_ANDROID
+
+        #include <GLES2/gl2ext.h>
+
+    #endif
+
     // SFML requires at a bare minimum OpenGL ES 1.0 capability
     // Some extensions only incorporated by 2.0 are also required
     // OpenGL ES 1.0 is defined relative to OpenGL 1.3
@@ -112,8 +120,13 @@
     #define GLEXT_GL_INVALID_FRAMEBUFFER_OPERATION    GL_INVALID_FRAMEBUFFER_OPERATION_OES

     // Core since 3.0 - EXT_sRGB
-    #define GLEXT_texture_sRGB                        GL_EXT_sRGB
-    #define GLEXT_GL_SRGB8_ALPHA8                     GL_SRGB8_ALPHA8_EXT
+    #ifdef GL_EXT_sRGB
+        #define GLEXT_texture_sRGB                        GL_EXT_sRGB
+        #define GLEXT_GL_SRGB8_ALPHA8                     GL_SRGB8_ALPHA8_EXT
+    #else
+        #define GLEXT_texture_sRGB                        false
+        #define GLEXT_GL_SRGB8_ALPHA8                     0
+    #endif

 #else

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 16, 2016

Member

Is there a way to check the NDK API level and return a verbose message if the minimal version isn't met?

Member

eXpl0it3r commented May 16, 2016

Is there a way to check the NDK API level and return a verbose message if the minimal version isn't met?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 16, 2016

Member

CMake already does this. If the user specifies some value smaller than 9, configuration will fail.

Member

binary1248 commented May 16, 2016

CMake already does this. If the user specifies some value smaller than 9, configuration will fail.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 16, 2016

Member

Do we have any feature that requires the sRGB extension? If not, binary's solution would work (without including the v2 header).

Member

MarioLiebisch commented May 16, 2016

Do we have any feature that requires the sRGB extension? If not, binary's solution would work (without including the v2 header).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 18, 2016

Member

What do you mean by feature? Does the normal sRGB support not require the extension?

Member

eXpl0it3r commented May 18, 2016

What do you mean by feature? Does the normal sRGB support not require the extension?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jun 15, 2016

Member

There is no typical "feature" that requires sRGB support. sRGB support is the feature itself, when it is available. If the NDK against which the library is built doesn't expose it then it just won't be available for the user to use (i.e. calling the functions won't do anything).

Member

binary1248 commented Jun 15, 2016

There is no typical "feature" that requires sRGB support. sRGB support is the feature itself, when it is available. If the NDK against which the library is built doesn't expose it then it just won't be available for the user to use (i.e. calling the functions won't do anything).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jun 22, 2016

Member

Bump

Member

eXpl0it3r commented Jun 22, 2016

Bump

@h0tw1r3

This comment has been minimized.

Show comment
Hide comment
@h0tw1r3

h0tw1r3 Jul 7, 2016

Contributor

Here's what I am using.

diff --git a/include/SFML/OpenGL.hpp b/include/SFML/OpenGL.hpp
index a5d964f..a62ea81 100644
--- a/include/SFML/OpenGL.hpp
+++ b/include/SFML/OpenGL.hpp
@@ -67,7 +67,12 @@

     #include <GLES/gl.h>
     #include <GLES/glext.h>
+    #include <GLES2/gl2ext.h>

+    #if __ANDROID_API__ < 21
+        #define GL_EXT_sRGB false
+        #define GL_SRGB8_ALPHA8_EXT 0
+    #endif
 #endif

Contributor

h0tw1r3 commented Jul 7, 2016

Here's what I am using.

diff --git a/include/SFML/OpenGL.hpp b/include/SFML/OpenGL.hpp
index a5d964f..a62ea81 100644
--- a/include/SFML/OpenGL.hpp
+++ b/include/SFML/OpenGL.hpp
@@ -67,7 +67,12 @@

     #include <GLES/gl.h>
     #include <GLES/glext.h>
+    #include <GLES2/gl2ext.h>

+    #if __ANDROID_API__ < 21
+        #define GL_EXT_sRGB false
+        #define GL_SRGB8_ALPHA8_EXT 0
+    #endif
 #endif

MarioLiebisch added a commit that referenced this issue Jul 14, 2016

Fixed current Android compilation issues
* Updated the Android toolchain file to support NDKs up to the latest release (r12b; based on https://github.com/gongminmin/android-cmake).
* Fixed missing sRGB extension defines - also SFML once again compiles for older target API levels not having the sRGB extensions (fixes #1079, supersedes #1085).
* Changed SFML's default STL runtime to `stlport_shared`, since `c++_shared` is no longer supported.

eXpl0it3r added a commit that referenced this issue Jul 21, 2016

Fixed current Android compilation issues
* Updated the Android toolchain file to support NDKs up to the latest release (r12b; based on https://github.com/gongminmin/android-cmake).
* Fixed missing sRGB extension defines - also SFML once again compiles for older target API levels not having the sRGB extensions (fixes #1079, supersedes #1085).
* Changed SFML's default STL runtime to `stlport_shared`, since `c++_shared` is no longer supported.

iamPHEN added a commit to Bablawn3d5/SFML that referenced this issue Mar 11, 2017

Fixed current Android compilation issues
* Updated the Android toolchain file to support NDKs up to the latest release (r12b; based on https://github.com/gongminmin/android-cmake).
* Fixed missing sRGB extension defines - also SFML once again compiles for older target API levels not having the sRGB extensions (fixes SFML#1079, supersedes SFML#1085).
* Changed SFML's default STL runtime to `stlport_shared`, since `c++_shared` is no longer supported.

@eXpl0it3r eXpl0it3r added this to Done in Android Backlog Jan 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment