From a494b47d4b1a7ccb289c67d11bc2ade75a04f0ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 6 Sep 2022 19:05:55 +0200 Subject: [PATCH] GL: add Shader::wrap() and release(). The whole time I thought this class doesn't need such APIs due to being rather short-lived. But now with async shader compilation it's no longer so short-lived. --- doc/changelog.dox | 1 + doc/opengl-wrapping.dox | 4 +-- src/Magnum/GL/Shader.cpp | 8 +++-- src/Magnum/GL/Shader.h | 46 +++++++++++++++++++++++++++-- src/Magnum/GL/Test/ShaderGLTest.cpp | 16 ++++++++++ 5 files changed, 67 insertions(+), 8 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 676d9749c..13826a031 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -412,6 +412,7 @@ See also: complement @relativeref{GL::Framebuffer::InvalidationAttachment,Depth} and @relativeref{GL::Framebuffer::InvalidationAttachment,Stencil} (see [mosra/magnum#554](https://github.com/mosra/magnum/pull/554)) +- Added @ref GL::Shader::wrap() and @relativeref{GL::Shader,release()} @subsubsection changelog-latest-changes-math Math library diff --git a/doc/opengl-wrapping.dox b/doc/opengl-wrapping.dox index 84db5b8fc..2efda4083 100644 --- a/doc/opengl-wrapping.dox +++ b/doc/opengl-wrapping.dox @@ -61,8 +61,8 @@ Magnum object instance using @cpp wrap() @ce: @snippet MagnumGL.cpp opengl-wrapping-transfer The @cpp wrap() @ce and @cpp release() @ce functions are available for all -OpenGL classes except for @ref GL::Shader, instances of which are rather -short-lived and thus wrapping external instances makes less sense. +OpenGL classes except for @ref GL::AbstractShaderProgram, where the desired +usage via subclassing isn't really suited for wrapping external objects. @attention Note that interaction with third-party OpenGL code as shown above usually diff --git a/src/Magnum/GL/Shader.cpp b/src/Magnum/GL/Shader.cpp index 40e5cf2dc..3167ffe0e 100644 --- a/src/Magnum/GL/Shader.cpp +++ b/src/Magnum/GL/Shader.cpp @@ -645,7 +645,7 @@ Int Shader::maxCombinedUniformComponents(const Type type) { } #endif -Shader::Shader(const Version version, const Type type): _type(type), _id(0) { +Shader::Shader(const Version version, const Type type): _type{type}, _flags{ObjectFlag::DeleteOnDestruction|ObjectFlag::Created} { _id = glCreateShader(GLenum(_type)); switch(version) { @@ -678,9 +678,11 @@ Shader::Shader(const Version version, const Type type): _type(type), _id(0) { CORRADE_ASSERT_UNREACHABLE("GL::Shader::Shader(): unsupported version" << version, ); } +Shader::Shader(const Type type, const GLuint id, ObjectFlags flags) noexcept: _type{type}, _id{id}, _flags{flags} {} + Shader::~Shader() { - /* Moved out, nothing to do */ - if(!_id) return; + /* Moved out or not deleting on destruction, nothing to do */ + if(!_id || !(_flags & ObjectFlag::DeleteOnDestruction)) return; glDeleteShader(_id); } diff --git a/src/Magnum/GL/Shader.h b/src/Magnum/GL/Shader.h index b9385e877..da8943c44 100644 --- a/src/Magnum/GL/Shader.h +++ b/src/Magnum/GL/Shader.h @@ -526,6 +526,23 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { static CORRADE_DEPRECATED("use either submitCompile() and checkCompile() or the zero-argument compile() instead") bool compile(std::initializer_list> shaders); #endif + /** + * @brief Wrap existing OpenGL shader object + * @param type Shader type + * @param id OpenGL shader ID + * @param flags Object creation flags + * @m_since_latest + * + * The @p id is expected to be of an existing OpenGL shader object. + * Unlike a shader created using a constructor, the OpenGL object is by + * default not deleted on destruction, use @p flags for different + * behavior. + * @see @ref release() + */ + static Shader wrap(Type type, GLuint id, ObjectFlags flags = {}) { + return Shader{type, id, flags}; + } + /** * @brief Constructor * @param version Target version @@ -535,7 +552,8 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { * corresponding to @p version parameter at the beginning. If * @ref Version::None is specified, (not) adding the @glsl #version @ce * directive is left to the user. - * @see @fn_gl_keyword{CreateShader} + * @see @ref Shader(NoCreateT), @ref wrap(), + * @fn_gl_keyword{CreateShader} */ explicit Shader(Version version, Type type); @@ -564,7 +582,7 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { * @brief Destructor * * Deletes associated OpenGL shader. - * @see @fn_gl_keyword{DeleteShader} + * @see @ref wrap(), @ref release(), @fn_gl_keyword{DeleteShader} */ ~Shader(); @@ -577,6 +595,18 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { /** @brief OpenGL shader ID */ GLuint id() const { return _id; } + /** + * @brief Release the underlying OpenGL object + * @m_since_latest + * + * Releases ownership of the OpenGL shader object and returns its ID so + * it's not deleted on destruction. The internal state is then + * equivalent to a moved-from state. + * @see @ref wrap() + */ + /* MinGW complains loudly if the declaration doesn't also have inline */ + inline GLuint release(); + #ifndef MAGNUM_TARGET_WEBGL /** * @brief Shader label @@ -711,6 +741,8 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { bool isCompileFinished(); private: + explicit Shader(Type type, GLuint id, ObjectFlags flags) noexcept; + void MAGNUM_GL_LOCAL addSourceImplementationDefault(std::string source); #if defined(CORRADE_TARGET_EMSCRIPTEN) && defined(__EMSCRIPTEN_PTHREADS__) void MAGNUM_GL_LOCAL addSourceImplementationEmscriptenPthread(std::string source); @@ -725,6 +757,7 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { Type _type; GLuint _id; + ObjectFlags _flags; std::vector _sources; }; @@ -732,7 +765,7 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { /** @debugoperatorclassenum{Shader,Shader::Type} */ MAGNUM_GL_EXPORT Debug& operator<<(Debug& debug, Shader::Type value); -inline Shader::Shader(Shader&& other) noexcept: _type(other._type), _id(other._id), _sources(std::move(other._sources)) { +inline Shader::Shader(Shader&& other) noexcept: _type{other._type}, _id{other._id}, _flags{other._flags}, _sources{std::move(other._sources)} { other._id = 0; } @@ -740,10 +773,17 @@ inline Shader& Shader::operator=(Shader&& other) noexcept { using std::swap; swap(_type, other._type); swap(_id, other._id); + swap(_flags, other._flags); swap(_sources, other._sources); return *this; } +inline GLuint Shader::release() { + const GLuint id = _id; + _id = 0; + return id; +} + }} #endif diff --git a/src/Magnum/GL/Test/ShaderGLTest.cpp b/src/Magnum/GL/Test/ShaderGLTest.cpp index bd74c93e9..ba8194c05 100644 --- a/src/Magnum/GL/Test/ShaderGLTest.cpp +++ b/src/Magnum/GL/Test/ShaderGLTest.cpp @@ -49,6 +49,7 @@ struct ShaderGLTest: OpenGLTester { void construct(); void constructNoVersion(); void constructMove(); + void wrap(); #ifndef MAGNUM_TARGET_WEBGL void label(); @@ -69,6 +70,7 @@ ShaderGLTest::ShaderGLTest() { addTests({&ShaderGLTest::construct, &ShaderGLTest::constructNoVersion, &ShaderGLTest::constructMove, + &ShaderGLTest::wrap, #ifndef MAGNUM_TARGET_WEBGL &ShaderGLTest::label, @@ -160,6 +162,20 @@ void ShaderGLTest::constructMove() { CORRADE_VERIFY(std::is_nothrow_move_assignable::value); } +void ShaderGLTest::wrap() { + GLuint id = glCreateShader(GL_FRAGMENT_SHADER); + + /* Releasing won't delete anything */ + { + auto shader = Shader::wrap(Shader::Type::Fragment, id, ObjectFlag::DeleteOnDestruction); + CORRADE_COMPARE(shader.release(), id); + } + + /* ...so we can wrap it again */ + Shader::wrap(Shader::Type::Fragment, id); + glDeleteShader(id); +} + #ifndef MAGNUM_TARGET_WEBGL void ShaderGLTest::label() { /* No-Op version is tested in AbstractObjectGLTest */