From a1a59ec4ea9e81f7ccd83714fc6806ebc291b707 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 1 Oct 2019 21:48:45 +0200 Subject: [PATCH] Shaders: assert on the wireframe flag when calling related setters. This is to be consistent with other shaders -- failing loudly is better than habing to spend ages wondering why it doesn't render the thing. --- doc/changelog.dox | 4 ++++ src/Magnum/Shaders/CMakeLists.txt | 2 +- src/Magnum/Shaders/MeshVisualizer.cpp | 17 ++++++++++---- .../Shaders/Test/MeshVisualizerGLTest.cpp | 23 ++++++++++++++++++- 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index e24bcc993..4d923e651 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -781,6 +781,10 @@ See also: to indicate an invalid format, better catching accidentally forgotten initialization. Valid code shouldn't be affected by this change, but broken code that seemingly worked before might start throwing assertions now. +- @ref Shaders::MeshVisualizer now asserts if its wireframe-related setters + are called when the @ref Shaders::MeshVisualizer::Flag::Wireframe flag was + not set, consistently with other shaders. This might cause failures in code + that was calling them unnecessarily before. @section changelog-2019-01 2019.01 diff --git a/src/Magnum/Shaders/CMakeLists.txt b/src/Magnum/Shaders/CMakeLists.txt index 6a5c30a8e..eb98f515a 100644 --- a/src/Magnum/Shaders/CMakeLists.txt +++ b/src/Magnum/Shaders/CMakeLists.txt @@ -33,7 +33,6 @@ set_target_properties(MagnumShaders_RCS-dependencies PROPERTIES FOLDER "Magnum/S set(MagnumShaders_SRCS AbstractVector.cpp DistanceFieldVector.cpp - MeshVisualizer.cpp Vector.cpp VertexColor.cpp @@ -41,6 +40,7 @@ set(MagnumShaders_SRCS set(MagnumShaders_GracefulAssert_SRCS Flat.cpp + MeshVisualizer.cpp Phong.cpp) set(MagnumShaders_HEADERS diff --git a/src/Magnum/Shaders/MeshVisualizer.cpp b/src/Magnum/Shaders/MeshVisualizer.cpp index f3f794cbc..98992cb18 100644 --- a/src/Magnum/Shaders/MeshVisualizer.cpp +++ b/src/Magnum/Shaders/MeshVisualizer.cpp @@ -164,6 +164,8 @@ MeshVisualizer& MeshVisualizer::setTransformationProjectionMatrix(const Matrix4& } MeshVisualizer& MeshVisualizer::setViewportSize(const Vector2& size) { + /* Not asserting here, since the relation to wireframe is a bit vague. + Also it's an ugly hack that should be removed, ideally. */ if(_flags & Flag::Wireframe && !(_flags & Flag::NoGeometryShader)) setUniform(_viewportSizeUniform, size); return *this; @@ -175,18 +177,25 @@ MeshVisualizer& MeshVisualizer::setColor(const Color4& color) { } MeshVisualizer& MeshVisualizer::setWireframeColor(const Color4& color) { - if(_flags & Flag::Wireframe) setUniform(_wireframeColorUniform, color); + CORRADE_ASSERT(_flags & Flag::Wireframe, + "Shaders::MeshVisualizer::setWireframeColor(): the shader was not created with wireframe enabled", *this); + setUniform(_wireframeColorUniform, color); return *this; } MeshVisualizer& MeshVisualizer::setWireframeWidth(const Float width) { - if(_flags & Flag::Wireframe) setUniform(_wireframeWidthUniform, width); + CORRADE_ASSERT(_flags & Flag::Wireframe, + "Shaders::MeshVisualizer::setWireframeWidth(): the shader was not created with wireframe enabled", *this); + setUniform(_wireframeWidthUniform, width); return *this; } MeshVisualizer& MeshVisualizer::setSmoothness(const Float smoothness) { - if(_flags & Flag::Wireframe) - setUniform(_smoothnessUniform, smoothness); + /* This is a bit vaguely related too, but less vague than setViewportSize() + so asserting. */ + CORRADE_ASSERT(_flags & Flag::Wireframe, + "Shaders::MeshVisualizer::setSmoothness(): the shader was not created with wireframe enabled", *this); + setUniform(_smoothnessUniform, smoothness); return *this; } diff --git a/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp b/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp index ae9dd42c9..38fc2c643 100644 --- a/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp +++ b/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp @@ -24,9 +24,11 @@ */ #include +#include #include #include #include +#include #include #include "Magnum/DebugTools/CompareImage.h" @@ -67,6 +69,8 @@ struct MeshVisualizerGLTest: GL::OpenGLTester { void constructMove(); + void setWireframeNotEnabled(); + void renderSetup(); void renderTeardown(); @@ -121,7 +125,9 @@ MeshVisualizerGLTest::MeshVisualizerGLTest() { #endif &MeshVisualizerGLTest::constructWireframeNoGeometryShader, - &MeshVisualizerGLTest::constructMove}); + &MeshVisualizerGLTest::constructMove, + + &MeshVisualizerGLTest::setWireframeNotEnabled}); addTests({&MeshVisualizerGLTest::renderDefaults, #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) @@ -218,6 +224,21 @@ void MeshVisualizerGLTest::constructMove() { CORRADE_VERIFY(!b.id()); } +void MeshVisualizerGLTest::setWireframeNotEnabled() { + std::ostringstream out; + Error redirectError{&out}; + + MeshVisualizer shader; + shader.setWireframeColor({}) + .setWireframeWidth({}) + .setSmoothness({}); + + CORRADE_COMPARE(out.str(), + "Shaders::MeshVisualizer::setWireframeColor(): the shader was not created with wireframe enabled\n" + "Shaders::MeshVisualizer::setWireframeWidth(): the shader was not created with wireframe enabled\n" + "Shaders::MeshVisualizer::setSmoothness(): the shader was not created with wireframe enabled\n"); +} + constexpr Vector2i RenderSize{80, 80}; void MeshVisualizerGLTest::renderSetup() {