From 1436b7bc86f51c543633e54da850520ae54be199 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 19 Jun 2020 13:28:06 +0200 Subject: [PATCH] Shaders: fix various WebGL issues with new MeshVisualizer features. * Shader compilation failed with vertex, object and primitive ID enabled due to the NO_GEOMETRY_SHADER define not being correctly propagated * Enabling just vertex ID visualization on WebGL caused an assert in constructor, complaining that "at least one visualization feature has to be enabled", which is wrong * Defaults were not correctly set up for vertex ID rendering, causing all-black render when setColor() wasn't called * Forgot to list/bundle some ground truth test images for the test case, causing the test to fail due to files not found * The test asserted when generating mesh data due to an unhandled corner case * The test expected an ES2 assertion message on WebGL 2 * Flag::Wireframe now implicitly enables Flag::NoGeometryShader also on WebGL. This was done only for ES2 previously, but WebGL doesn't have (and won't have) geometry shaders, so it makes sense to do the same there. --- doc/changelog.dox | 4 +++ src/Magnum/Shaders/MeshVisualizer.cpp | 31 ++++++++++++++----- src/Magnum/Shaders/MeshVisualizer.h | 27 ++++++++-------- src/Magnum/Shaders/Test/CMakeLists.txt | 2 ++ .../Shaders/Test/MeshVisualizerGLTest.cpp | 9 ++++-- .../Shaders/Test/MeshVisualizerTest.cpp | 4 +-- 6 files changed, 50 insertions(+), 27 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 83050843f..b638df17c 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -340,6 +340,10 @@ See also: `Magnum/version.h` header. This header is not included by any other header to avoid trigerring a full rebuild when Git commit changes. If Git is not found, only the first two defines are present. +- @ref Shaders::MeshVisualizer3D::Flag::Wireframe now implicitly enables + @ref Shaders::MeshVisualizer3D::Flag::NoGeometryShader also on WebGL in + addition to ES2, since this platform doesn't have a possibility to have + geometry shaders either. Same is done for @ref Shaders::MeshVisualizer2D. @subsubsection changelog-latest-changes-audio Audio library diff --git a/src/Magnum/Shaders/MeshVisualizer.cpp b/src/Magnum/Shaders/MeshVisualizer.cpp index e42c96572..f563f37e1 100644 --- a/src/Magnum/Shaders/MeshVisualizer.cpp +++ b/src/Magnum/Shaders/MeshVisualizer.cpp @@ -125,7 +125,6 @@ GL::Version MeshVisualizerBase::setupShaders(GL::Shader& vert, GL::Shader& frag, #endif ; frag.addSource(_flags & FlagBase::Wireframe ? "#define WIREFRAME_RENDERING\n" : "") - .addSource(_flags & FlagBase::NoGeometryShader ? "#define NO_GEOMETRY_SHADER\n" : "") #ifndef MAGNUM_TARGET_GLES2 .addSource(_flags & FlagBase::InstancedObjectId ? "#define INSTANCED_OBJECT_ID\n" : "") .addSource(_flags & FlagBase::VertexId ? "#define VERTEX_ID\n" : "") @@ -200,12 +199,18 @@ MeshVisualizer2D::MeshVisualizer2D(const Flags flags): Implementation::MeshVisua vert.addSource("#define TWO_DIMENSIONS\n") /* Pass NO_GEOMETRY_SHADER not only when NoGeometryShader but also when nothing actually needs it, as that makes checks much simpler in - the vertex shader code */ + the shader code */ .addSource((flags & Flag::NoGeometryShader) || !(flags & Flag::Wireframe) ? "#define NO_GEOMETRY_SHADER\n" : "") .addSource(rs.get("generic.glsl")) .addSource(rs.get("MeshVisualizer.vert")); - frag.addSource(rs.get("generic.glsl")) + frag + /* Pass NO_GEOMETRY_SHADER not only when NoGeometryShader but also when + nothing actually needs it, as that makes checks much simpler in + the shader code */ + .addSource((flags & Flag::NoGeometryShader) || !(flags & Flag::Wireframe) ? + "#define NO_GEOMETRY_SHADER\n" : "") + .addSource(rs.get("generic.glsl")) .addSource(rs.get("MeshVisualizer.frag")); #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) @@ -292,7 +297,7 @@ MeshVisualizer2D::MeshVisualizer2D(const Flags flags): Implementation::MeshVisua setTransformationProjectionMatrix({}); if(flags & (Flag::Wireframe #ifndef MAGNUM_TARGET_GLES2 - |Flag::InstancedObjectId|Flag::PrimitiveIdFromVertexId + |Flag::InstancedObjectId|Flag::VertexId|Flag::PrimitiveIdFromVertexId #endif )) setColor(Color3(1.0f)); @@ -340,7 +345,7 @@ MeshVisualizer3D::MeshVisualizer3D(const Flags flags): Implementation::MeshVisua CORRADE_ASSERT(!(flags & Flag::BitangentDirection && flags & Flag::BitangentFromTangentDirection), "Shaders::MeshVisualizer3D: Flag::BitangentDirection and Flag::BitangentFromTangentDirection are mutually exclusive", ); #elif !defined(MAGNUM_TARGET_GLES2) - CORRADE_ASSERT(flags & ((Flag::Wireframe|Flag::InstancedObjectId|Flag::PrimitiveIdFromVertexId) & ~Flag::NoGeometryShader), + CORRADE_ASSERT(flags & ((Flag::Wireframe|Flag::InstancedObjectId|Flag::VertexId|Flag::PrimitiveIdFromVertexId) & ~Flag::NoGeometryShader), "Shaders::MeshVisualizer3D: at least one visualization feature has to be enabled", ); #else CORRADE_ASSERT(flags & (Flag::Wireframe & ~Flag::NoGeometryShader), @@ -377,6 +382,14 @@ MeshVisualizer3D::MeshVisualizer3D(const Flags flags): Implementation::MeshVisua .addSource(rs.get("generic.glsl")) .addSource(rs.get("MeshVisualizer.vert")); frag + /* Pass NO_GEOMETRY_SHADER not only when NoGeometryShader but also when + nothing actually needs it, as that makes checks much simpler in + the vertex shader code */ + .addSource((flags & Flag::NoGeometryShader) || !(flags & (Flag::Wireframe + #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) + |Flag::TangentDirection|Flag::BitangentDirection|Flag::BitangentFromTangentDirection|Flag::NormalDirection + #endif + )) ? "#define NO_GEOMETRY_SHADER\n" : "") #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) .addSource(flags & (Flag::TangentDirection|Flag::BitangentFromTangentDirection|Flag::BitangentDirection|Flag::NormalDirection) ? "#define TBN_DIRECTION\n" : "") #endif @@ -504,7 +517,7 @@ MeshVisualizer3D::MeshVisualizer3D(const Flags flags): Implementation::MeshVisua setProjectionMatrix({}); if(flags & (Flag::Wireframe #ifndef MAGNUM_TARGET_GLES2 - |Flag::InstancedObjectId|Flag::PrimitiveIdFromVertexId + |Flag::InstancedObjectId|Flag::VertexId|Flag::PrimitiveIdFromVertexId #endif )) setColor(Color3(1.0f)); @@ -652,7 +665,8 @@ Debug& operator<<(Debug& debug, const MeshVisualizer3D::Flag value) { Debug& operator<<(Debug& debug, const MeshVisualizer2D::Flags value) { return Containers::enumSetDebugOutput(debug, value, "Shaders::MeshVisualizer2D::Flags{}", { MeshVisualizer2D::Flag::Wireframe, - /* Wireframe contains this on ES2 so it's not reported there */ + /* Wireframe contains this on ES2 and WebGL 1 so it's not reported + there */ MeshVisualizer2D::Flag::NoGeometryShader, #ifndef MAGNUM_TARGET_GLES2 MeshVisualizer2D::Flag::InstancedObjectId, @@ -668,7 +682,8 @@ Debug& operator<<(Debug& debug, const MeshVisualizer2D::Flags value) { Debug& operator<<(Debug& debug, const MeshVisualizer3D::Flags value) { return Containers::enumSetDebugOutput(debug, value, "Shaders::MeshVisualizer3D::Flags{}", { MeshVisualizer3D::Flag::Wireframe, - /* Wireframe contains this on ES2 so it's not reported there */ + /* Wireframe contains this on ES2 and WebGL 1 so it's not reported + there */ MeshVisualizer3D::Flag::NoGeometryShader, #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) MeshVisualizer3D::Flag::TangentDirection, diff --git a/src/Magnum/Shaders/MeshVisualizer.h b/src/Magnum/Shaders/MeshVisualizer.h index 9ac652d38..cf3222d13 100644 --- a/src/Magnum/Shaders/MeshVisualizer.h +++ b/src/Magnum/Shaders/MeshVisualizer.h @@ -43,11 +43,10 @@ namespace Implementation { class MAGNUM_SHADERS_EXPORT MeshVisualizerBase: public GL::AbstractShaderProgram { protected: enum class FlagBase: UnsignedShort { - #ifndef MAGNUM_TARGET_GLES2 + /* Unlike the public Wireframe flag, this one doesn't include + NoGeometryShader on ES2 as that would make the checks too + complex */ Wireframe = 1 << 0, - #else - Wireframe = (1 << 0) | (1 << 1), - #endif NoGeometryShader = 1 << 1, #ifndef MAGNUM_TARGET_GLES2 InstancedObjectId = 1 << 2, @@ -169,10 +168,10 @@ class MAGNUM_SHADERS_EXPORT MeshVisualizer2D: public Implementation::MeshVisuali */ enum class Flag: UnsignedShort { /** - * Visualize wireframe. On OpenGL ES 2.0 this also enables - * @ref Flag::NoGeometryShader. + * Visualize wireframe. On OpenGL ES 2.0 and WebGL this also + * enables @ref Flag::NoGeometryShader. */ - #ifndef MAGNUM_TARGET_GLES2 + #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) Wireframe = 1 << 0, #else Wireframe = (1 << 0) | (1 << 1), @@ -181,8 +180,8 @@ class MAGNUM_SHADERS_EXPORT MeshVisualizer2D: public Implementation::MeshVisuali /** * Don't use a geometry shader for wireframe visualization. If * enabled, you might need to provide also the @ref VertexIndex - * attribute in the mesh. In OpenGL ES 2.0 enabled alongside - * @ref Flag::Wireframe. + * attribute in the mesh. On OpenGL ES 2.0 and WebGL enabled + * alongside @ref Flag::Wireframe. */ NoGeometryShader = 1 << 1, @@ -589,10 +588,10 @@ class MAGNUM_SHADERS_EXPORT MeshVisualizer3D: public Implementation::MeshVisuali */ enum class Flag: UnsignedShort { /** - * Visualize wireframe. On OpenGL ES 2.0 this also enables - * @ref Flag::NoGeometryShader. + * Visualize wireframe. On OpenGL ES 2.0 and WebGL this also + * enables @ref Flag::NoGeometryShader. */ - #ifndef MAGNUM_TARGET_GLES2 + #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) Wireframe = 1 << 0, #else Wireframe = (1 << 0) | (1 << 1), @@ -601,8 +600,8 @@ class MAGNUM_SHADERS_EXPORT MeshVisualizer3D: public Implementation::MeshVisuali /** * Don't use a geometry shader for wireframe visualization. If * enabled, you might need to provide also the @ref VertexIndex - * attribute in the mesh. In OpenGL ES 2.0 enabled alongside - * @ref Flag::Wireframe. + * attribute in the mesh. On OpenGL ES 2.0 and WebGL enabled + * alongside @ref Flag::Wireframe. * * Mutually exclusive with @ref Flag::TangentDirection, * @ref Flag::BitangentFromTangentDirection, diff --git a/src/Magnum/Shaders/Test/CMakeLists.txt b/src/Magnum/Shaders/Test/CMakeLists.txt index 19b5b96a1..7919b6eea 100644 --- a/src/Magnum/Shaders/Test/CMakeLists.txt +++ b/src/Magnum/Shaders/Test/CMakeLists.txt @@ -174,6 +174,8 @@ if(BUILD_GL_TESTS) MeshVisualizerTestFiles/vertexid3D.tga MeshVisualizerTestFiles/wireframe-nogeo2D.tga MeshVisualizerTestFiles/wireframe-nogeo3D.tga + MeshVisualizerTestFiles/wireframe-objectid2D.tga + MeshVisualizerTestFiles/wireframe-objectid3D.tga MeshVisualizerTestFiles/wireframe-nogeo-objectid2D.tga MeshVisualizerTestFiles/wireframe-nogeo-objectid3D.tga MeshVisualizerTestFiles/wireframe-perspective.tga diff --git a/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp b/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp index 804722bdf..c24439e28 100644 --- a/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp +++ b/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp @@ -207,7 +207,7 @@ constexpr struct { } ConstructInvalidData2D[] { {"no feature enabled", MeshVisualizer2D::Flag::NoGeometryShader, /* not a feature flag */ - #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) + #ifndef MAGNUM_TARGET_GLES2 "2D: at least one visualization feature has to be enabled" #else "2D: at least Flag::Wireframe has to be enabled" @@ -230,7 +230,7 @@ constexpr struct { } ConstructInvalidData3D[] { {"no feature enabled", MeshVisualizer3D::Flag::NoGeometryShader, /* not a feature flag */ - #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) + #ifndef MAGNUM_TARGET_GLES2 "3D: at least one visualization feature has to be enabled" #else "3D: at least Flag::Wireframe has to be enabled" @@ -1583,8 +1583,11 @@ void MeshVisualizerGLTest::renderObjectVertexPrimitiveId2D() { if(data.flags2D >= MeshVisualizer2D::Flag::PrimitiveIdFromVertexId) circleData = MeshTools::generateIndices(circleData); if(data.flags2D >= MeshVisualizer2D::Flag::PrimitiveIdFromVertexId || - data.flags2D & MeshVisualizer2D::Flag::NoGeometryShader) + data.flags2D & MeshVisualizer2D::Flag::NoGeometryShader) { + if(circleData.primitive() != MeshPrimitive::Triangles) + circleData = MeshTools::generateIndices(circleData); circleData = MeshTools::duplicate(circleData); + } GL::Mesh circle = MeshTools::compile(circleData); diff --git a/src/Magnum/Shaders/Test/MeshVisualizerTest.cpp b/src/Magnum/Shaders/Test/MeshVisualizerTest.cpp index 5a8d54995..763f8947c 100644 --- a/src/Magnum/Shaders/Test/MeshVisualizerTest.cpp +++ b/src/Magnum/Shaders/Test/MeshVisualizerTest.cpp @@ -124,7 +124,7 @@ void MeshVisualizerTest::debugFlags2D() { std::ostringstream out; Debug{&out} << (MeshVisualizer2D::Flag::Wireframe|MeshVisualizer2D::Flag::NoGeometryShader) << MeshVisualizer2D::Flags{}; - #ifndef MAGNUM_TARGET_GLES2 + #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) CORRADE_COMPARE(out.str(), "Shaders::MeshVisualizer2D::Flag::Wireframe|Shaders::MeshVisualizer2D::Flag::NoGeometryShader Shaders::MeshVisualizer2D::Flags{}\n"); #else CORRADE_COMPARE(out.str(), "Shaders::MeshVisualizer2D::Flag::Wireframe Shaders::MeshVisualizer2D::Flags{}\n"); @@ -135,7 +135,7 @@ void MeshVisualizerTest::debugFlags3D() { std::ostringstream out; Debug{&out} << (MeshVisualizer3D::Flag::Wireframe|MeshVisualizer3D::Flag::NoGeometryShader) << MeshVisualizer3D::Flags{}; - #ifndef MAGNUM_TARGET_GLES2 + #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) CORRADE_COMPARE(out.str(), "Shaders::MeshVisualizer3D::Flag::Wireframe|Shaders::MeshVisualizer3D::Flag::NoGeometryShader Shaders::MeshVisualizer3D::Flags{}\n"); #else CORRADE_COMPARE(out.str(), "Shaders::MeshVisualizer3D::Flag::Wireframe Shaders::MeshVisualizer3D::Flags{}\n");