From f0a868da9cd256bdc17d3605e5106e56edc4d1b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 3 Aug 2018 12:35:38 +0200 Subject: [PATCH] Shaders: assert on Phong::bindTexture*() if no texture was enabled. --- doc/changelog.dox | 16 +++++++ src/Magnum/Shaders/CMakeLists.txt | 51 ++++++++++++++++++++--- src/Magnum/Shaders/Phong.cpp | 14 +++++-- src/Magnum/Shaders/Phong.h | 14 +++++-- src/Magnum/Shaders/Test/CMakeLists.txt | 2 +- src/Magnum/Shaders/Test/PhongGLTest.cpp | 55 ++++++++++++++++++++++++- src/Magnum/Shaders/visibility.h | 2 +- 7 files changed, 139 insertions(+), 15 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 5e30a7558..2ae116cac 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -153,6 +153,15 @@ See also: - @ref Platform::GlfwApplication now loads modifiers for mouse move and mouse scroll events lazily, only when needed +@subsubsection changelog-latest-changes-shaders Shaders library + +- @ref Shaders::Phong::bindAmbientTexture(), + @ref Shaders::Phong::bindDiffuseTexture(), + @ref Shaders::Phong::bindSpecularTexture() and + @ref Shaders::Phong::bindTextures() now assert that the shader was created + with the corresponding flag enabled to prevent accidental "black screen of + death" errors. + @subsubsection changelog-latest-changes-texturetools TextureTools library - Fixed @ref TextureTools::distanceField() to not require more than 8 texture @@ -245,6 +254,13 @@ See also: working to fail with an assertion. See the new @ref Math::Matrix3::rotationShear() and @ref Math::Matrix4::rotationShear() accessors for a possible solution. +- @ref Shaders::Phong::bindAmbientTexture(), + @ref Shaders::Phong::bindDiffuseTexture(), + @ref Shaders::Phong::bindSpecularTexture() and + @ref Shaders::Phong::bindTextures() now assert that the shader was created + with the corresponding flag enabled to prevent accidental "black screen of + death" errors. This might cause your application to abort if it was calling + these functions when not needed. - @ref Trade::PhongMaterialData::ambientColor(), @ref Trade::PhongMaterialData::diffuseColor() "diffuseColor()" and @ref Trade::PhongMaterialData::specularColor() "specularColor()" now return diff --git a/src/Magnum/Shaders/CMakeLists.txt b/src/Magnum/Shaders/CMakeLists.txt index b119bdac8..6d96f27d4 100644 --- a/src/Magnum/Shaders/CMakeLists.txt +++ b/src/Magnum/Shaders/CMakeLists.txt @@ -31,12 +31,14 @@ set(MagnumShaders_SRCS DistanceFieldVector.cpp Flat.cpp MeshVisualizer.cpp - Phong.cpp Vector.cpp VertexColor.cpp ${MagnumShaders_RCS}) +set(MagnumShaders_GracefulAssert_SRCS + Phong.cpp) + set(MagnumShaders_HEADERS DistanceFieldVector.h AbstractVector.h @@ -53,11 +55,25 @@ set(MagnumShaders_HEADERS # Header files to display in project view of IDEs only set(MagnumShaders_PRIVATE_HEADERS Implementation/CreateCompatibilityShader.h) -# Shaders library -add_library(MagnumShaders ${SHARED_OR_STATIC} +# Objects shared between main and test library +add_library(MagnumShadersObjects OBJECT ${MagnumShaders_SRCS} - ${MagnumShaders_HEADERS} - ${MagnumShaders_PRIVATE_HEADERS}) + ${MagnumShaders_HEADERS}) +target_include_directories(MagnumShadersObjects PUBLIC + $ + $) +if(NOT BUILD_STATIC) + target_compile_definitions(MagnumShadersObjects PRIVATE "MagnumShadersObjects_EXPORTS") +endif() +if(NOT BUILD_STATIC OR BUILD_STATIC_PIC) + set_target_properties(MagnumShadersObjects PROPERTIES POSITION_INDEPENDENT_CODE ON) +endif() +set_target_properties(MagnumShadersObjects PROPERTIES FOLDER "Magnum/Shaders") + +# Main Shaders library +add_library(MagnumShaders ${SHARED_OR_STATIC} + $ + ${MagnumShaders_GracefulAssert_SRCS}) set_target_properties(MagnumShaders PROPERTIES DEBUG_POSTFIX "-d" FOLDER "Magnum/Shaders") @@ -77,6 +93,31 @@ install(TARGETS MagnumShaders install(FILES ${MagnumShaders_HEADERS} DESTINATION ${MAGNUM_INCLUDE_INSTALL_DIR}/Shaders) if(BUILD_TESTS) + # Library with graceful assert for testing + add_library(MagnumShadersTestLib ${SHARED_OR_STATIC} + $ + ${MagnumShaders_GracefulAssert_SRCS}) + set_target_properties(MagnumShadersTestLib PROPERTIES + DEBUG_POSTFIX "-d" + FOLDER "Magnum/Shaders") + target_compile_definitions(MagnumShadersTestLib PRIVATE + "CORRADE_GRACEFUL_ASSERT" "MagnumShaders_EXPORTS") + if(BUILD_STATIC_PIC) + set_target_properties(MagnumShadersTestLib PROPERTIES POSITION_INDEPENDENT_CODE ON) + endif() + target_link_libraries(MagnumShadersTestLib PUBLIC + Magnum + MagnumGL) + + # On Windows we need to install first and then run the tests to avoid "DLL + # not found" hell, thus we need to install this too + if(CORRADE_TARGET_WINDOWS AND NOT CMAKE_CROSSCOMPILING AND NOT BUILD_STATIC) + install(TARGETS MagnumShadersTestLib + RUNTIME DESTINATION ${MAGNUM_BINARY_INSTALL_DIR} + LIBRARY DESTINATION ${MAGNUM_LIBRARY_INSTALL_DIR} + ARCHIVE DESTINATION ${MAGNUM_LIBRARY_INSTALL_DIR}) + endif() + add_subdirectory(Test) endif() diff --git a/src/Magnum/Shaders/Phong.cpp b/src/Magnum/Shaders/Phong.cpp index 1b5e3997b..24494ff81 100644 --- a/src/Magnum/Shaders/Phong.cpp +++ b/src/Magnum/Shaders/Phong.cpp @@ -125,21 +125,29 @@ Phong::Phong(const Flags flags): _flags(flags) { } Phong& Phong::bindAmbientTexture(GL::Texture2D& texture) { - if(_flags & Flag::AmbientTexture) texture.bind(AmbientTextureLayer); + CORRADE_ASSERT(_flags & Flag::AmbientTexture, + "Shaders::Phong::bindAmbientTexture(): the shader was not created with ambient texture enabled", *this); + texture.bind(AmbientTextureLayer); return *this; } Phong& Phong::bindDiffuseTexture(GL::Texture2D& texture) { - if(_flags & Flag::DiffuseTexture) texture.bind(DiffuseTextureLayer); + CORRADE_ASSERT(_flags & Flag::DiffuseTexture, + "Shaders::Phong::bindDiffuseTexture(): the shader was not created with diffuse texture enabled", *this); + texture.bind(DiffuseTextureLayer); return *this; } Phong& Phong::bindSpecularTexture(GL::Texture2D& texture) { - if(_flags & Flag::SpecularTexture) texture.bind(SpecularTextureLayer); + CORRADE_ASSERT(_flags & Flag::SpecularTexture, + "Shaders::Phong::bindSpecularTexture(): the shader was not created with specular texture enabled", *this); + texture.bind(SpecularTextureLayer); return *this; } Phong& Phong::bindTextures(GL::Texture2D* ambient, GL::Texture2D* diffuse, GL::Texture2D* specular) { + CORRADE_ASSERT(_flags & (Flag::AmbientTexture|Flag::DiffuseTexture|Flag::SpecularTexture), + "Shaders::Phong::bindTextures(): the shader was not created with any textures enabled", *this); GL::AbstractTexture::bind(AmbientTextureLayer, {ambient, diffuse, specular}); return *this; } diff --git a/src/Magnum/Shaders/Phong.h b/src/Magnum/Shaders/Phong.h index 595c66a52..c65f4a924 100644 --- a/src/Magnum/Shaders/Phong.h +++ b/src/Magnum/Shaders/Phong.h @@ -199,7 +199,8 @@ class MAGNUM_SHADERS_EXPORT Phong: public GL::AbstractShaderProgram { * @brief Bind ambient texture * @return Reference to self (for method chaining) * - * Has effect only if @ref Flag::AmbientTexture is set. + * Expects that the shader was created with @ref Flag::AmbientTexture + * enabled. * @see @ref bindTextures(), @ref setAmbientColor() */ Phong& bindAmbientTexture(GL::Texture2D& texture); @@ -231,7 +232,8 @@ class MAGNUM_SHADERS_EXPORT Phong: public GL::AbstractShaderProgram { * @brief Bind diffuse texture * @return Reference to self (for method chaining) * - * Has effect only if @ref Flag::DiffuseTexture is set. + * Expects that the shader was created with @ref Flag::DiffuseTexture + * enabled. * @see @ref bindTextures(), @ref setDiffuseColor() */ Phong& bindDiffuseTexture(GL::Texture2D& texture); @@ -264,7 +266,8 @@ class MAGNUM_SHADERS_EXPORT Phong: public GL::AbstractShaderProgram { * @brief Bind specular texture * @return Reference to self (for method chaining) * - * Has effect only if @ref Flag::SpecularTexture is set. + * Expects that the shader was created with @ref Flag::SpecularTexture + * enabled. * @see @ref bindTextures(), @ref setSpecularColor() */ Phong& bindSpecularTexture(GL::Texture2D& texture); @@ -284,7 +287,10 @@ class MAGNUM_SHADERS_EXPORT Phong: public GL::AbstractShaderProgram { * * A particular texture has effect only if particular texture flag from * @ref Phong::Flag "Flag" is set, you can use `nullptr` for the rest. - * More efficient than setting each texture separately. + * Expects that the shader was created with at least one of + * @ref Flag::AmbientTexture, @ref Flag::DiffuseTexture or + * @ref Flag::SpecularTexture enabled. More efficient than setting each + * texture separately. * @see @ref bindAmbientTexture(), @ref bindDiffuseTexture(), * @ref bindSpecularTexture() */ diff --git a/src/Magnum/Shaders/Test/CMakeLists.txt b/src/Magnum/Shaders/Test/CMakeLists.txt index 59b163af1..a0a604500 100644 --- a/src/Magnum/Shaders/Test/CMakeLists.txt +++ b/src/Magnum/Shaders/Test/CMakeLists.txt @@ -43,7 +43,7 @@ if(BUILD_GL_TESTS) corrade_add_test(ShadersDistanceFieldVectorGLTest DistanceFieldVectorGLTest.cpp LIBRARIES MagnumShaders MagnumOpenGLTester) corrade_add_test(ShadersFlatGLTest FlatGLTest.cpp LIBRARIES MagnumShaders MagnumOpenGLTester) corrade_add_test(ShadersMeshVisualizerGLTest MeshVisualizerGLTest.cpp LIBRARIES MagnumShaders MagnumOpenGLTester) - corrade_add_test(ShadersPhongGLTest PhongGLTest.cpp LIBRARIES MagnumShaders MagnumOpenGLTester) + corrade_add_test(ShadersPhongGLTest PhongGLTest.cpp LIBRARIES MagnumShadersTestLib MagnumOpenGLTester) corrade_add_test(ShadersVectorGLTest VectorGLTest.cpp LIBRARIES MagnumShaders MagnumOpenGLTester) corrade_add_test(ShadersVertexColorGLTest VertexColorGLTest.cpp LIBRARIES MagnumShaders MagnumOpenGLTester) diff --git a/src/Magnum/Shaders/Test/PhongGLTest.cpp b/src/Magnum/Shaders/Test/PhongGLTest.cpp index 26a129124..3bf17202b 100644 --- a/src/Magnum/Shaders/Test/PhongGLTest.cpp +++ b/src/Magnum/Shaders/Test/PhongGLTest.cpp @@ -23,7 +23,13 @@ DEALINGS IN THE SOFTWARE. */ +#include + +#include "Magnum/PixelFormat.h" +#include "Magnum/ImageView.h" #include "Magnum/GL/OpenGLTester.h" +#include "Magnum/GL/Texture.h" +#include "Magnum/GL/TextureFormat.h" #include "Magnum/Shaders/Phong.h" namespace Magnum { namespace Shaders { namespace Test { @@ -34,6 +40,9 @@ struct PhongGLTest: GL::OpenGLTester { void construct(); void constructMove(); + + void bindTextures(); + void bindTexturesNotEnabled(); }; constexpr struct { @@ -48,11 +57,15 @@ constexpr struct { {"ambient + specular texture", Phong::Flag::AmbientTexture|Phong::Flag::SpecularTexture}, {"diffuse + specular texture", Phong::Flag::DiffuseTexture|Phong::Flag::SpecularTexture}, {"ambient + diffuse + specular texture", Phong::Flag::AmbientTexture|Phong::Flag::DiffuseTexture|Phong::Flag::SpecularTexture}}; +}; PhongGLTest::PhongGLTest() { addInstancedTests({&PhongGLTest::construct}, Containers::arraySize(ConstructData)); - addTests({&PhongGLTest::constructMove}); + addTests({&PhongGLTest::constructMove, + + &PhongGLTest::bindTextures, + &PhongGLTest::bindTexturesNotEnabled}); } void PhongGLTest::construct() { @@ -86,6 +99,46 @@ void PhongGLTest::constructMove() { CORRADE_VERIFY(!b.id()); } +void PhongGLTest::bindTextures() { + char data[4]; + + GL::Texture2D texture; + texture + .setMinificationFilter(SamplerFilter::Linear, SamplerMipmap::Linear) + .setMagnificationFilter(SamplerFilter::Linear) + .setWrapping(SamplerWrapping::ClampToEdge) + .setImage(0, GL::TextureFormat::RGBA, ImageView2D{PixelFormat::RGBA8Unorm, {1, 1}, data}); + + MAGNUM_VERIFY_NO_GL_ERROR(); + + /* Test just that no assertion is fired */ + Phong shader{Phong::Flag::AmbientTexture|Phong::Flag::DiffuseTexture|Phong::Flag::SpecularTexture}; + shader.bindAmbientTexture(texture) + .bindDiffuseTexture(texture) + .bindSpecularTexture(texture) + .bindTextures(&texture, &texture, &texture); + + MAGNUM_VERIFY_NO_GL_ERROR(); +} + +void PhongGLTest::bindTexturesNotEnabled() { + std::ostringstream out; + Error redirectError{&out}; + + GL::Texture2D texture; + Phong shader; + shader.bindAmbientTexture(texture) + .bindDiffuseTexture(texture) + .bindSpecularTexture(texture) + .bindTextures(&texture, &texture, &texture); + + CORRADE_COMPARE(out.str(), + "Shaders::Phong::bindAmbientTexture(): the shader was not created with ambient texture enabled\n" + "Shaders::Phong::bindDiffuseTexture(): the shader was not created with diffuse texture enabled\n" + "Shaders::Phong::bindSpecularTexture(): the shader was not created with specular texture enabled\n" + "Shaders::Phong::bindTextures(): the shader was not created with any textures enabled\n"); +} + }}} CORRADE_TEST_MAIN(Magnum::Shaders::Test::PhongGLTest) diff --git a/src/Magnum/Shaders/visibility.h b/src/Magnum/Shaders/visibility.h index bb17b7563..69d5cb77e 100644 --- a/src/Magnum/Shaders/visibility.h +++ b/src/Magnum/Shaders/visibility.h @@ -31,7 +31,7 @@ #ifndef DOXYGEN_GENERATING_OUTPUT #ifndef MAGNUM_BUILD_STATIC - #ifdef MagnumShaders_EXPORTS + #if defined(MagnumShaders_EXPORTS) || defined(MagnumShadersObjects_EXPORTS) #define MAGNUM_SHADERS_EXPORT CORRADE_VISIBILITY_EXPORT #else #define MAGNUM_SHADERS_EXPORT CORRADE_VISIBILITY_IMPORT