Shaders: assert on Phong::bindTexture*() if no texture was enabled.

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

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
+    $<TARGET_PROPERTY:Magnum,INTERFACE_INCLUDE_DIRECTORIES>
+    $<TARGET_PROPERTY:MagnumGL,INTERFACE_INCLUDE_DIRECTORIES>)
+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}
+    $<TARGET_OBJECTS:MagnumShadersObjects>
+    ${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}
+        $<TARGET_OBJECTS:MagnumShadersObjects>
+        ${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()
 

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;
 }

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()
          */

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)
 

src/Magnum/Shaders/Test/PhongGLTest.cpp

@@ -23,7 +23,13 @@
     DEALINGS IN THE SOFTWARE.
 */
 
+#include <sstream>
+
+#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)

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