Shaders: avoid including GL/Shader.h in headers.

The class is rather heavy (strings, STL vector) and it'll stay heavier than strictly needed even after the planned STL cleanup -- shader users should not bear the overhead of Array, StringView etc. that it needs in order to compile the shader sources. I might eventually come to a different conclusion (maybe separating GL::Shader population and usage like doing in Vulkan with CreateInfos), but right now this commit has the best available solution -- converting the instance to a lightweight class containing just ID and type, and then converting that back to a GL::Shader upon checking compilation/link status. While at it, also removed the not-strictly-needed Optional usage from the header. It wouldn't work with forward-declared GL::Shader anyway.
mosra · Sep 7, 2022 · eede671 · eede671
1 parent a494b47
commit eede671
Show file tree

Hide file tree

Showing 18 changed files with 396 additions and 34 deletions.
diff --git a/src/Magnum/Shaders/CMakeLists.txt b/src/Magnum/Shaders/CMakeLists.txt
@@ -42,7 +42,9 @@ set(MagnumShaders_GracefulAssert_SRCS
     MeshVisualizerGL.cpp
     PhongGL.cpp
     VectorGL.cpp
-    VertexColorGL.cpp)
+    VertexColorGL.cpp
+
+    glShaderWrapper.cpp)
 
 set(MagnumShaders_HEADERS
     DistanceFieldVector.h
@@ -60,6 +62,7 @@ set(MagnumShaders_HEADERS
     VectorGL.h
     VertexColorGL.h
 
+    glShaderWrapper.h
     visibility.h)
 
 if(MAGNUM_BUILD_DEPRECATED)

diff --git a/src/Magnum/Shaders/DistanceFieldVectorGL.cpp b/src/Magnum/Shaders/DistanceFieldVectorGL.cpp
@@ -33,6 +33,7 @@
 
 #include "Magnum/GL/Context.h"
 #include "Magnum/GL/Extensions.h"
+#include "Magnum/GL/Shader.h"
 #include "Magnum/GL/Texture.h"
 #include "Magnum/Math/Color.h"
 #include "Magnum/Math/Matrix3.h"
@@ -171,7 +172,7 @@ template<UnsignedInt dimensions> DistanceFieldVectorGL<dimensions>::DistanceFiel
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({GL::Shader(state._vert), GL::Shader(state._frag)}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

diff --git a/src/Magnum/Shaders/DistanceFieldVectorGL.h b/src/Magnum/Shaders/DistanceFieldVectorGL.h
@@ -33,8 +33,8 @@
 
 #include "Magnum/DimensionTraits.h"
 #include "Magnum/GL/AbstractShaderProgram.h"
-#include "Magnum/GL/Shader.h"
 #include "Magnum/Shaders/GenericGL.h"
+#include "Magnum/Shaders/glShaderWrapper.h"
 #include "Magnum/Shaders/visibility.h"
 
 namespace Magnum { namespace Shaders {
@@ -690,7 +690,7 @@ template<UnsignedInt dimensions> class DistanceFieldVectorGL<dimensions>::Compil
 
     explicit CompileState(DistanceFieldVectorGL<dimensions>&& shader, GL::Shader&& vert, GL::Shader&& frag, GL::Version version): DistanceFieldVectorGL<dimensions>{std::move(shader)}, _vert{std::move(vert)}, _frag{std::move(frag)}, _version{version} {}
 
-    GL::Shader _vert, _frag;
+    Implementation::GLShaderWrapper _vert, _frag;
     GL::Version _version;
 };
 

diff --git a/src/Magnum/Shaders/FlatGL.cpp b/src/Magnum/Shaders/FlatGL.cpp
@@ -33,6 +33,7 @@
 
 #include "Magnum/GL/Context.h"
 #include "Magnum/GL/Extensions.h"
+#include "Magnum/GL/Shader.h"
 #include "Magnum/GL/Texture.h"
 #include "Magnum/Math/Color.h"
 #include "Magnum/Math/Matrix3.h"
@@ -246,7 +247,7 @@ template<UnsignedInt dimensions> FlatGL<dimensions>::FlatGL(CompileState&& state
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({GL::Shader(state._vert), GL::Shader(state._frag)}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

diff --git a/src/Magnum/Shaders/FlatGL.h b/src/Magnum/Shaders/FlatGL.h
@@ -33,8 +33,8 @@
 
 #include "Magnum/DimensionTraits.h"
 #include "Magnum/GL/AbstractShaderProgram.h"
-#include "Magnum/GL/Shader.h"
 #include "Magnum/Shaders/GenericGL.h"
+#include "Magnum/Shaders/glShaderWrapper.h"
 #include "Magnum/Shaders/visibility.h"
 
 namespace Magnum { namespace Shaders {
@@ -1101,7 +1101,7 @@ template<UnsignedInt dimensions> class FlatGL<dimensions>::CompileState: public
 
     explicit CompileState(FlatGL<dimensions>&& shader, GL::Shader&& vert, GL::Shader&& frag, GL::Version version): FlatGL<dimensions>{std::move(shader)}, _vert{std::move(vert)}, _frag{std::move(frag)}, _version{version} {}
 
-    GL::Shader _vert, _frag;
+    Implementation::GLShaderWrapper _vert, _frag;
     GL::Version _version;
 };
 

diff --git a/src/Magnum/Shaders/MeshVisualizerGL.cpp b/src/Magnum/Shaders/MeshVisualizerGL.cpp
@@ -517,7 +517,7 @@ MeshVisualizerGL2D::CompileState MeshVisualizerGL2D::compile(const Flags flags
 
     out.submitLink();
 
-    return CompileState{std::move(out), std::move(vert), std::move(frag), std::move(geom), flags, version};
+    return CompileState{std::move(out), std::move(vert), std::move(frag), geom ? &*geom : nullptr, flags, version};
 }
 
 MeshVisualizerGL2D::MeshVisualizerGL2D(const Flags flags): MeshVisualizerGL2D{compile(flags)} {}
@@ -537,10 +537,10 @@ MeshVisualizerGL2D::MeshVisualizerGL2D(CompileState&& state): MeshVisualizerGL2D
     if(!id()) return;
     #endif
 
-    if(state._geom)
-        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag, *state._geom}));
+    if(state._geom.id)
+        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({GL::Shader(state._vert), GL::Shader(state._frag), GL::Shader(state._geom)}));
     else
-        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
+        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({GL::Shader(state._vert), GL::Shader(state._frag)}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;
@@ -906,7 +906,7 @@ MeshVisualizerGL3D::CompileState MeshVisualizerGL3D::compile(Flags flags
 
     out.submitLink();
 
-    return CompileState{std::move(out), std::move(vert), std::move(frag), std::move(geom), flags, version};
+    return CompileState{std::move(out), std::move(vert), std::move(frag), geom ? &*geom : nullptr, flags, version};
 }
 
 MeshVisualizerGL3D::MeshVisualizerGL3D(CompileState&& state): MeshVisualizerGL3D{static_cast<MeshVisualizerGL3D&&>(std::move(state))} {
@@ -916,10 +916,10 @@ MeshVisualizerGL3D::MeshVisualizerGL3D(CompileState&& state): MeshVisualizerGL3D
     if(!id()) return;
     #endif
 
-    if(state._geom)
-        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag, *state._geom}));
+    if(state._geom.id)
+        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({GL::Shader(state._vert), GL::Shader(state._frag), GL::Shader(state._geom)}));
     else
-        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
+        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({GL::Shader(state._vert), GL::Shader(state._frag)}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

diff --git a/src/Magnum/Shaders/MeshVisualizerGL.h b/src/Magnum/Shaders/MeshVisualizerGL.h
@@ -33,11 +33,10 @@
 
 #include <Corrade/Utility/Utility.h>
 
-#include <Corrade/Containers/Optional.h>
 #include "Magnum/DimensionTraits.h"
 #include "Magnum/GL/AbstractShaderProgram.h"
-#include "Magnum/GL/Shader.h"
 #include "Magnum/Shaders/GenericGL.h"
+#include "Magnum/Shaders/glShaderWrapper.h"
 #include "Magnum/Shaders/visibility.h"
 
 namespace Magnum { namespace Shaders {
@@ -928,12 +927,13 @@ class MeshVisualizerGL2D::CompileState: public MeshVisualizerGL2D {
     /* Everything deliberately private except for the inheritance */
     friend class MeshVisualizerGL2D;
 
-    explicit CompileState(NoCreateT): MeshVisualizerGL2D{NoCreate}, _vert{NoCreate}, _frag{NoCreate} {}
+    explicit CompileState(NoCreateT): MeshVisualizerGL2D{NoCreate}, _vert{NoCreate}, _frag{NoCreate}, _geom{NoCreate} {}
 
-    explicit CompileState(MeshVisualizerGL2D&& shader, GL::Shader&& vert, GL::Shader&& frag, Containers::Optional<GL::Shader>&& geom, Flags flags, GL::Version version): MeshVisualizerGL2D{std::move(shader)}, _vert{std::move(vert)}, _frag{std::move(frag)}, _geom{std::move(geom)}, _flags{flags}, _version{version} {}
+    explicit CompileState(MeshVisualizerGL2D&& shader, GL::Shader&& vert, GL::Shader&& frag, GL::Shader* geom, Flags flags, GL::Version version): MeshVisualizerGL2D{std::move(shader)}, _vert{std::move(vert)}, _frag{std::move(frag)}, _geom{NoCreate}, _flags{flags}, _version{version} {
+        if(geom) _geom = Implementation::GLShaderWrapper{std::move(*geom)};
+    }
 
-    GL::Shader _vert, _frag;
-    Containers::Optional<GL::Shader> _geom;
+    Implementation::GLShaderWrapper _vert, _frag, _geom;
     Flags _flags;
     GL::Version _version;
 };
@@ -2465,12 +2465,13 @@ class MeshVisualizerGL3D::CompileState: public MeshVisualizerGL3D {
     /* Everything deliberately private except for the inheritance */
     friend class MeshVisualizerGL3D;
 
-    explicit CompileState(NoCreateT): MeshVisualizerGL3D{NoCreate}, _vert{NoCreate}, _frag{NoCreate} {}
+    explicit CompileState(NoCreateT): MeshVisualizerGL3D{NoCreate}, _vert{NoCreate}, _frag{NoCreate}, _geom{NoCreate} {}
 
-    explicit CompileState(MeshVisualizerGL3D&& shader, GL::Shader&& vert, GL::Shader&& frag, Containers::Optional<GL::Shader>&& geom, Flags flags, GL::Version version): MeshVisualizerGL3D{std::move(shader)}, _vert{std::move(vert)}, _frag{std::move(frag)}, _geom{std::move(geom)}, _flags{flags}, _version{version} {}
+    explicit CompileState(MeshVisualizerGL3D&& shader, GL::Shader&& vert, GL::Shader&& frag, GL::Shader* geom, Flags flags, GL::Version version): MeshVisualizerGL3D{std::move(shader)}, _vert{std::move(vert)}, _frag{std::move(frag)}, _geom{NoCreate}, _flags{flags}, _version{version} {
+        if(geom) _geom = Implementation::GLShaderWrapper{std::move(*geom)};
+    }
 
-    GL::Shader _vert, _frag;
-    Containers::Optional<GL::Shader> _geom;
+    Implementation::GLShaderWrapper _vert, _frag, _geom;
     Flags _flags;
     GL::Version _version;
 };

diff --git a/src/Magnum/Shaders/PhongGL.cpp b/src/Magnum/Shaders/PhongGL.cpp
@@ -39,6 +39,7 @@
 
 #include "Magnum/GL/Context.h"
 #include "Magnum/GL/Extensions.h"
+#include "Magnum/GL/Shader.h"
 #include "Magnum/GL/Texture.h"
 #include "Magnum/Math/Color.h"
 #include "Magnum/Math/Matrix3.h"
@@ -356,7 +357,7 @@ PhongGL::PhongGL(CompileState&& state): PhongGL{static_cast<PhongGL&&>(std::move
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({GL::Shader(state._vert), GL::Shader(state._frag)}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

diff --git a/src/Magnum/Shaders/PhongGL.h b/src/Magnum/Shaders/PhongGL.h
@@ -32,8 +32,8 @@
  */
 
 #include "Magnum/GL/AbstractShaderProgram.h"
-#include "Magnum/GL/Shader.h"
 #include "Magnum/Shaders/GenericGL.h"
+#include "Magnum/Shaders/glShaderWrapper.h"
 #include "Magnum/Shaders/visibility.h"
 
 namespace Magnum { namespace Shaders {
@@ -1847,7 +1847,7 @@ class PhongGL::CompileState: public PhongGL {
 
     explicit CompileState(PhongGL&& shader, GL::Shader&& vert, GL::Shader&& frag, GL::Version version): PhongGL{std::move(shader)}, _vert{std::move(vert)}, _frag{std::move(frag)}, _version{version} {}
 
-    GL::Shader _vert, _frag;
+    Implementation::GLShaderWrapper _vert, _frag;
     GL::Version _version;
 };
 

diff --git a/src/Magnum/Shaders/Test/CMakeLists.txt b/src/Magnum/Shaders/Test/CMakeLists.txt
@@ -30,6 +30,7 @@ set(CMAKE_FOLDER "Magnum/Shaders/Test")
 corrade_add_test(ShadersDistanceFieldVectorTest DistanceFieldVectorTest.cpp LIBRARIES MagnumShaders)
 corrade_add_test(ShadersFlatTest FlatTest.cpp LIBRARIES MagnumShaders)
 corrade_add_test(ShadersGenericTest GenericTest.cpp LIBRARIES MagnumShaders)
+corrade_add_test(ShadersGLShaderWrapperTest GLShaderWrapperTest.cpp LIBRARIES MagnumShaders)
 corrade_add_test(ShadersMeshVisualizerTest MeshVisualizerTest.cpp LIBRARIES MagnumShaders)
 corrade_add_test(ShadersPhongTest PhongTest.cpp LIBRARIES MagnumShaders)
 corrade_add_test(ShadersVectorTest VectorTest.cpp LIBRARIES MagnumShaders)
@@ -164,6 +165,8 @@ if(MAGNUM_BUILD_GL_TESTS)
         endif()
     endif()
 
+    corrade_add_test(ShadersGLShaderWrapperGLTest GLShaderWrapperGLTest.cpp LIBRARIES MagnumShaders MagnumOpenGLTester)
+
     set(ShadersMeshVisualizerGLTest_SRCS MeshVisualizerGLTest.cpp)
     if(CORRADE_TARGET_IOS)
         list(APPEND ShadersMeshVisualizerGLTest_SRCS FlatTestFiles MeshVisualizerTestFiles)