Skip to content

Commit

Permalink
GL: add Shader::wrap() and release().
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mosra committed Sep 7, 2022
1 parent 966b948 commit a494b47
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 8 deletions.
1 change: 1 addition & 0 deletions doc/changelog.dox
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions doc/opengl-wrapping.dox
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions src/Magnum/GL/Shader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand Down
46 changes: 43 additions & 3 deletions src/Magnum/GL/Shader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<Containers::Reference<Shader>> 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
Expand All @@ -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);

Expand Down Expand Up @@ -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();

Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -725,25 +757,33 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject {

Type _type;
GLuint _id;
ObjectFlags _flags;

std::vector<std::string> _sources;
};

/** @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;
}

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
16 changes: 16 additions & 0 deletions src/Magnum/GL/Test/ShaderGLTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ struct ShaderGLTest: OpenGLTester {
void construct();
void constructNoVersion();
void constructMove();
void wrap();

#ifndef MAGNUM_TARGET_WEBGL
void label();
Expand All @@ -69,6 +70,7 @@ ShaderGLTest::ShaderGLTest() {
addTests({&ShaderGLTest::construct,
&ShaderGLTest::constructNoVersion,
&ShaderGLTest::constructMove,
&ShaderGLTest::wrap,

#ifndef MAGNUM_TARGET_WEBGL
&ShaderGLTest::label,
Expand Down Expand Up @@ -160,6 +162,20 @@ void ShaderGLTest::constructMove() {
CORRADE_VERIFY(std::is_nothrow_move_assignable<Shader>::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 */
Expand Down

0 comments on commit a494b47

Please sign in to comment.