From 713407e1593fdf19a9553b09597f488edf5ba9ba Mon Sep 17 00:00:00 2001 From: Maximilian Wagenbach Date: Sat, 19 Jul 2014 01:10:27 +0200 Subject: [PATCH 1/3] Fixed a bug where calling Texture::getMaximumSize() before any GlResource is created would break context management. --- src/SFML/Graphics/Text.cpp | 9 ++++----- src/SFML/Graphics/Texture.cpp | 23 +++++++++++++++++++---- src/SFML/Window/GlContext.cpp | 1 - 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/SFML/Graphics/Text.cpp b/src/SFML/Graphics/Text.cpp index 90208cf69..e415728fe 100644 --- a/src/SFML/Graphics/Text.cpp +++ b/src/SFML/Graphics/Text.cpp @@ -28,13 +28,12 @@ #include #include #include -#include namespace sf { //////////////////////////////////////////////////////////// -Text::Text() : +Text::Text() : m_string (), m_font (NULL), m_characterSize (30), @@ -42,14 +41,14 @@ m_style (Regular), m_color (255, 255, 255), m_vertices (Triangles), m_bounds (), -m_geometryNeedUpdate(false) +m_geometryNeedUpdate(false) { } //////////////////////////////////////////////////////////// -Text::Text(const String& string, const Font& font, unsigned int characterSize) : +Text::Text(const String& string, const Font& font, unsigned int characterSize) : m_string (string), m_font (&font), m_characterSize (characterSize), @@ -57,7 +56,7 @@ m_style (Regular), m_color (255, 255, 255), m_vertices (Triangles), m_bounds (), -m_geometryNeedUpdate(true) +m_geometryNeedUpdate(true) { } diff --git a/src/SFML/Graphics/Texture.cpp b/src/SFML/Graphics/Texture.cpp index 86f1fdbe8..2e93a7c32 100644 --- a/src/SFML/Graphics/Texture.cpp +++ b/src/SFML/Graphics/Texture.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -522,12 +523,26 @@ void Texture::bind(const Texture* texture, CoordinateType coordinateType) //////////////////////////////////////////////////////////// unsigned int Texture::getMaximumSize() { - ensureGlContext(); + static unsigned int size = 0; + static bool checked = false; - GLint size; - glCheck(glGetIntegerv(GL_MAX_TEXTURE_SIZE, &size)); + // Make sure we only have to check once + if (!checked) + { + // Create a temporary context in case the user queries + // the size before a GlResource is created, thus + // initializing the shared context + Context context; - return static_cast(size); + GLint glSize; + glCheck(glGetIntegerv(GL_MAX_TEXTURE_SIZE, &glSize)); + + size = static_cast(glSize); + + checked = true; + } + + return size; } diff --git a/src/SFML/Window/GlContext.cpp b/src/SFML/Window/GlContext.cpp index f76f6c526..7ecc524c2 100644 --- a/src/SFML/Window/GlContext.cpp +++ b/src/SFML/Window/GlContext.cpp @@ -32,7 +32,6 @@ #include #include #include -#include #ifdef SFML_SYSTEM_IOS #include #else From 7defb17e8c95c86221fc07c76180d713d20d749a Mon Sep 17 00:00:00 2001 From: binary1248 Date: Wed, 13 Aug 2014 08:44:48 +0200 Subject: [PATCH 2/3] Made a few functions in Texture and Shader a bit more thread-safe. --- src/SFML/Graphics/Shader.cpp | 65 ++++++++++++++++++++++------------- src/SFML/Graphics/Texture.cpp | 41 +++++++++++----------- 2 files changed, 62 insertions(+), 44 deletions(-) diff --git a/src/SFML/Graphics/Shader.cpp b/src/SFML/Graphics/Shader.cpp index c5fa4b6a3..0af485a9f 100644 --- a/src/SFML/Graphics/Shader.cpp +++ b/src/SFML/Graphics/Shader.cpp @@ -31,6 +31,8 @@ #include #include #include +#include +#include #include #include #include @@ -40,11 +42,25 @@ namespace { + sf::Mutex mutex; + + GLint checkMaxTextureUnits() + { + GLint maxUnits = 0; + + glCheck(glGetIntegerv(GL_MAX_TEXTURE_COORDS_ARB, &maxUnits)); + + return maxUnits; + } + // Retrieve the maximum number of texture units available GLint getMaxTextureUnits() { - GLint maxUnits = 0; - glCheck(glGetIntegerv(GL_MAX_TEXTURE_COORDS_ARB, &maxUnits)); + // TODO: Remove this lock when it becomes unnecessary in C++11 + sf::Lock lock(mutex); + + static GLint maxUnits = checkMaxTextureUnits(); + return maxUnits; } @@ -86,6 +102,24 @@ namespace buffer.push_back('\0'); return success; } + + bool checkShadersAvailable() + { + // Create a temporary context in case the user checks + // before a GlResource is created, thus initializing + // the shared context + sf::Context context; + + // Make sure that extensions are initialized + sf::priv::ensureExtensionsInit(); + + bool available = GLEW_ARB_shading_language_100 && + GLEW_ARB_shader_objects && + GLEW_ARB_vertex_shader && + GLEW_ARB_fragment_shader; + + return available; + } } @@ -378,7 +412,7 @@ void Shader::setParameter(const std::string& name, const Texture& texture) if (it == m_textures.end()) { // New entry, make sure there are enough texture units - static const GLint maxUnits = getMaxTextureUnits(); + GLint maxUnits = getMaxTextureUnits(); if (m_textures.size() + 1 >= static_cast(maxUnits)) { err() << "Impossible to use texture \"" << name << "\" for shader: all available texture units are used" << std::endl; @@ -438,27 +472,10 @@ void Shader::bind(const Shader* shader) //////////////////////////////////////////////////////////// bool Shader::isAvailable() { - static bool available = false; - static bool checked = false; + // TODO: Remove this lock when it becomes unnecessary in C++11 + Lock lock(mutex); - // Make sure we only have to check once - if (!checked) - { - // Create a temporary context in case the user checks - // before a GlResource is created, thus initializing - // the shared context - Context context; - - // Make sure that extensions are initialized - priv::ensureExtensionsInit(); - - available = GLEW_ARB_shading_language_100 && - GLEW_ARB_shader_objects && - GLEW_ARB_vertex_shader && - GLEW_ARB_fragment_shader; - - checked = true; - } + static bool available = checkShadersAvailable(); return available; } @@ -603,7 +620,7 @@ int Shader::getParamLocation(const std::string& name) // Not in cache, request the location from OpenGL int location = glGetUniformLocationARB(m_shaderProgram, name.c_str()); m_params.insert(std::make_pair(name, location)); - + if (location == -1) err() << "Parameter \"" << name << "\" not found in shader" << std::endl; diff --git a/src/SFML/Graphics/Texture.cpp b/src/SFML/Graphics/Texture.cpp index 2e93a7c32..81387c783 100644 --- a/src/SFML/Graphics/Texture.cpp +++ b/src/SFML/Graphics/Texture.cpp @@ -40,16 +40,31 @@ namespace { + sf::Mutex mutex; + // Thread-safe unique identifier generator, // is used for states cache (see RenderTarget) sf::Uint64 getUniqueId() { - static sf::Uint64 id = 1; // start at 1, zero is "no texture" - static sf::Mutex mutex; - sf::Lock lock(mutex); + + static sf::Uint64 id = 1; // start at 1, zero is "no texture" + return id++; } + + unsigned int checkMaximumTextureSize() + { + // Create a temporary context in case the user queries + // the size before a GlResource is created, thus + // initializing the shared context + sf::Context context; + + GLint size; + glCheck(glGetIntegerv(GL_MAX_TEXTURE_SIZE, &size)); + + return static_cast(size); + } } @@ -523,24 +538,10 @@ void Texture::bind(const Texture* texture, CoordinateType coordinateType) //////////////////////////////////////////////////////////// unsigned int Texture::getMaximumSize() { - static unsigned int size = 0; - static bool checked = false; + // TODO: Remove this lock when it becomes unnecessary in C++11 + Lock lock(mutex); - // Make sure we only have to check once - if (!checked) - { - // Create a temporary context in case the user queries - // the size before a GlResource is created, thus - // initializing the shared context - Context context; - - GLint glSize; - glCheck(glGetIntegerv(GL_MAX_TEXTURE_SIZE, &glSize)); - - size = static_cast(glSize); - - checked = true; - } + static unsigned int size = checkMaximumTextureSize(); return size; } From 6959c1a826693017a26336fc1814593f6199ed3d Mon Sep 17 00:00:00 2001 From: binary1248 Date: Fri, 15 Aug 2014 13:39:06 +0200 Subject: [PATCH 3/3] Added notes to documentation and adjusted OpenGL example as a workaround for the side effects of making static functions that require a context work. If the user wants to make use of both sfml-graphics and OpenGL, they should make sure sf::Texture::getMaximumSize() and/or sf::Shader::isAvailable() are called at least once before setting their final context active as those functions will cause a context switch the first time they are called. --- examples/opengl/OpenGL.cpp | 10 +++++++--- include/SFML/Graphics/Shader.hpp | 3 +++ include/SFML/Graphics/Texture.hpp | 3 +++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/examples/opengl/OpenGL.cpp b/examples/opengl/OpenGL.cpp index 639254bc0..ccb1d34da 100644 --- a/examples/opengl/OpenGL.cpp +++ b/examples/opengl/OpenGL.cpp @@ -22,9 +22,6 @@ int main() sf::RenderWindow window(sf::VideoMode(800, 600), "SFML graphics with OpenGL", sf::Style::Default, contextSettings); window.setVerticalSyncEnabled(true); - // Make it the active window for OpenGL calls - window.setActive(); - // Create a sprite for the background sf::Texture backgroundTexture; if (!backgroundTexture.loadFromFile("resources/background.jpg")) @@ -39,6 +36,13 @@ int main() text.setColor(sf::Color(255, 255, 255, 170)); text.setPosition(250.f, 450.f); + // Make the window the active target for OpenGL calls + // Note: If using sf::Texture or sf::Shader with OpenGL, + // be sure to call sf::Texture::getMaximumSize() and/or + // sf::Shader::isAvailable() at least once before calling + // setActive(), as those functions will cause a context switch + window.setActive(); + // Load an OpenGL texture. // We could directly use a sf::Texture as an OpenGL texture (with its Bind() member function), // but here we want more control on it (generate mipmaps, ...) so we create a new one from an image diff --git a/include/SFML/Graphics/Shader.hpp b/include/SFML/Graphics/Shader.hpp index 99f86dbb1..028dd7270 100644 --- a/include/SFML/Graphics/Shader.hpp +++ b/include/SFML/Graphics/Shader.hpp @@ -486,6 +486,9 @@ public : /// the shader features. If it returns false, then /// any attempt to use sf::Shader will fail. /// + /// Note: The first call to this function, whether by your + /// code or SFML will result in a context switch. + /// /// \return True if shaders are supported, false otherwise /// //////////////////////////////////////////////////////////// diff --git a/include/SFML/Graphics/Texture.hpp b/include/SFML/Graphics/Texture.hpp index 83a016dfe..8fde8ec0a 100644 --- a/include/SFML/Graphics/Texture.hpp +++ b/include/SFML/Graphics/Texture.hpp @@ -460,6 +460,9 @@ public : /// You can expect a value of 512 pixels for low-end graphics /// card, and up to 8192 pixels or more for newer hardware. /// + /// Note: The first call to this function, whether by your + /// code or SFML will result in a context switch. + /// /// \return Maximum size allowed for textures, in pixels /// ////////////////////////////////////////////////////////////