From daec63072dfdebc838ab879bc02b529a20cd8bb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 12 Apr 2015 20:09:17 +0200 Subject: [PATCH] Simplified *Framebuffer::bind() functionality. This was a leftover from some not-well-thought-out design decision. The function is now used exclusively for binding for draw, as all framebuffer reading functions (blit(), read()) are doing the read binding internally. Moreover it required the user to be extra careful on ES2, because in many cases there are no separate binding points for reading and drawing. The function is now parameter-less and always bind the framebuffer for drawing. The logic for internal binding was also simplified and on ES2 there are separate implementations for single/separate binding points. For *Framebuffer::checkStatus() the documentation was updated to explain the meaning of the parameter on ES2 implementation. Also removed the need for FramebufferTarget::ReadDraw binding, as it was rather confusing. Old *Framebuffer::bind(FramebufferTarget) is now just an alias to the parameter-less function, ignoring the parameter. Along with FramebufferTarget::ReadDraw it is marked as deprecated and will be removed in some future release. --- src/Magnum/AbstractFramebuffer.cpp | 102 +++++++++++------- src/Magnum/AbstractFramebuffer.h | 61 +++++++---- src/Magnum/DefaultFramebuffer.h | 4 + src/Magnum/Framebuffer.cpp | 2 +- src/Magnum/Framebuffer.h | 4 + .../Implementation/FramebufferState.cpp | 15 ++- src/Magnum/Implementation/FramebufferState.h | 6 +- src/Magnum/Test/MeshGLTest.cpp | 4 +- src/Magnum/Test/SampleQueryGLTest.cpp | 4 +- src/Magnum/TextureTools/DistanceField.cpp | 2 +- 10 files changed, 133 insertions(+), 71 deletions(-) diff --git a/src/Magnum/AbstractFramebuffer.cpp b/src/Magnum/AbstractFramebuffer.cpp index 260c6d605..a55f34fee 100644 --- a/src/Magnum/AbstractFramebuffer.cpp +++ b/src/Magnum/AbstractFramebuffer.cpp @@ -93,27 +93,45 @@ void AbstractFramebuffer::createIfNotAlready() { CORRADE_INTERNAL_ASSERT(_created); } -void AbstractFramebuffer::bind(const FramebufferTarget target) { - bindInternal(target); - - /* Ensure the viewport is set if the user is going to draw */ - if(target == FramebufferTarget::Draw || target == FramebufferTarget::ReadDraw) - setViewportInternal(); +void AbstractFramebuffer::bind() { + bindInternal(FramebufferTarget::Draw); + setViewportInternal(); } void AbstractFramebuffer::bindInternal(FramebufferTarget target) { + #ifndef MAGNUM_TARGET_GLES2 + bindImplementationDefault(target); + #else + (this->*Context::current()->state().framebuffer->bindImplementation)(target); + #endif +} + +#ifdef MAGNUM_TARGET_GLES2 +void AbstractFramebuffer::bindImplementationSingle(FramebufferTarget) { + Implementation::FramebufferState& state = *Context::current()->state().framebuffer; + CORRADE_INTERNAL_ASSERT(state.readBinding == state.drawBinding); + if(state.readBinding == _id) return; + + state.readBinding = state.drawBinding = _id; + + /* Binding the framebuffer finally creates it */ + _created = true; + glBindFramebuffer(GL_FRAMEBUFFER, _id); +} +#endif + +#ifndef MAGNUM_TARGET_GLES2 +inline +#endif +void AbstractFramebuffer::bindImplementationDefault(FramebufferTarget target) { Implementation::FramebufferState& state = *Context::current()->state().framebuffer; - /* If already bound, done, otherwise update tracked state */ if(target == FramebufferTarget::Read) { if(state.readBinding == _id) return; state.readBinding = _id; } else if(target == FramebufferTarget::Draw) { if(state.drawBinding == _id) return; state.drawBinding = _id; - } else if(target == FramebufferTarget::ReadDraw) { - if(state.readBinding == _id && state.drawBinding == _id) return; - state.readBinding = state.drawBinding = _id; } else CORRADE_ASSERT_UNREACHABLE(); /* Binding the framebuffer finally creates it */ @@ -122,11 +140,38 @@ void AbstractFramebuffer::bindInternal(FramebufferTarget target) { } FramebufferTarget AbstractFramebuffer::bindInternal() { + #ifndef MAGNUM_TARGET_GLES2 + return bindImplementationDefault(); + #else + return (this->*Context::current()->state().framebuffer->bindInternalImplementation)(); + #endif +} + +#ifdef MAGNUM_TARGET_GLES2 +FramebufferTarget AbstractFramebuffer::bindImplementationSingle() { + Implementation::FramebufferState& state = *Context::current()->state().framebuffer; + CORRADE_INTERNAL_ASSERT(state.readBinding == state.drawBinding); + + /* Bind the framebuffer, if not already */ + if(state.readBinding != _id) { + state.readBinding = state.drawBinding = _id; + + /* Binding the framebuffer finally creates it */ + _created = true; + glBindFramebuffer(GL_FRAMEBUFFER, _id); + } + + return FramebufferTarget{}; +} +#endif + +#ifndef MAGNUM_TARGET_GLES2 +inline +#endif +FramebufferTarget AbstractFramebuffer::bindImplementationDefault() { Implementation::FramebufferState& state = *Context::current()->state().framebuffer; /* Return target to which the framebuffer is already bound */ - if(state.readBinding == _id && state.drawBinding == _id) - return FramebufferTarget::ReadDraw; if(state.readBinding == _id) return FramebufferTarget::Read; if(state.drawBinding == _id) @@ -137,14 +182,8 @@ FramebufferTarget AbstractFramebuffer::bindInternal() { /* Binding the framebuffer finally creates it */ _created = true; - #ifndef MAGNUM_TARGET_GLES2 glBindFramebuffer(GLenum(FramebufferTarget::Read), _id); return FramebufferTarget::Read; - #else - if(state.readTarget == FramebufferTarget::ReadDraw) state.drawBinding = _id; - glBindFramebuffer(GLenum(state.readTarget), _id); - return state.readTarget; - #endif } void AbstractFramebuffer::blit(AbstractFramebuffer& source, AbstractFramebuffer& destination, const Range2Di& sourceRectangle, const Range2Di& destinationRectangle, const FramebufferBlitMask mask, const FramebufferBlitFilter filter) { @@ -225,22 +264,14 @@ void AbstractFramebuffer::setViewportInternal() { } AbstractFramebuffer& AbstractFramebuffer::clear(const FramebufferClearMask mask) { - #ifndef MAGNUM_TARGET_GLES2 bindInternal(FramebufferTarget::Draw); - #else - bindInternal(Context::current()->state().framebuffer->drawTarget); - #endif glClear(GLbitfield(mask)); return *this; } void AbstractFramebuffer::read(const Range2Di& rectangle, Image2D& image) { - #ifndef MAGNUM_TARGET_GLES2 bindInternal(FramebufferTarget::Read); - #else - bindInternal(Context::current()->state().framebuffer->readTarget); - #endif const std::size_t dataSize = image.dataSize(rectangle.size()); char* const data = new char[dataSize]; (Context::current()->state().framebuffer->readImplementation)(rectangle, image.format(), image.type(), dataSize, data); @@ -309,6 +340,13 @@ GLenum AbstractFramebuffer::checkStatusImplementationDefault(const FramebufferTa return glCheckFramebufferStatus(GLenum(target)); } +#ifdef MAGNUM_TARGET_GLES2 +GLenum AbstractFramebuffer::checkStatusImplementationSingle(FramebufferTarget) { + bindInternal(FramebufferTarget{}); + return glCheckFramebufferStatus(GL_FRAMEBUFFER); +} +#endif + #ifndef MAGNUM_TARGET_GLES GLenum AbstractFramebuffer::checkStatusImplementationDSA(const FramebufferTarget target) { return glCheckNamedFramebufferStatus(_id, GLenum(target)); @@ -321,11 +359,7 @@ GLenum AbstractFramebuffer::checkStatusImplementationDSAEXT(const FramebufferTar #endif void AbstractFramebuffer::drawBuffersImplementationDefault(GLsizei count, const GLenum* buffers) { - #ifndef MAGNUM_TARGET_GLES2 bindInternal(FramebufferTarget::Draw); - #else - bindInternal(Context::current()->state().framebuffer->drawTarget); - #endif #ifndef MAGNUM_TARGET_GLES2 glDrawBuffers(count, buffers); @@ -350,11 +384,7 @@ void AbstractFramebuffer::drawBuffersImplementationDSAEXT(GLsizei count, const G #endif void AbstractFramebuffer::drawBufferImplementationDefault(GLenum buffer) { - #ifndef MAGNUM_TARGET_GLES2 bindInternal(FramebufferTarget::Draw); - #else - bindInternal(Context::current()->state().framebuffer->drawTarget); - #endif #ifndef MAGNUM_TARGET_GLES glDrawBuffer(buffer); @@ -380,11 +410,7 @@ void AbstractFramebuffer::drawBufferImplementationDSAEXT(GLenum buffer) { #endif void AbstractFramebuffer::readBufferImplementationDefault(GLenum buffer) { - #ifndef MAGNUM_TARGET_GLES2 bindInternal(FramebufferTarget::Read); - #else - bindInternal(Context::current()->state().framebuffer->readTarget); - #endif #ifndef MAGNUM_TARGET_GLES2 glReadBuffer(buffer); diff --git a/src/Magnum/AbstractFramebuffer.h b/src/Magnum/AbstractFramebuffer.h index 39471424e..b3a16d372 100644 --- a/src/Magnum/AbstractFramebuffer.h +++ b/src/Magnum/AbstractFramebuffer.h @@ -30,6 +30,7 @@ */ #include +#include #include "Magnum/Magnum.h" #include "Magnum/OpenGL.h" @@ -120,37 +121,38 @@ enum class FramebufferBlitFilter: GLenum { }; /** -@brief Target for binding framebuffer +@brief Framebuffer target -@see @ref DefaultFramebuffer::bind(), @ref Framebuffer::bind() +@see @ref DefaultFramebuffer::checkStatus(), @ref Framebuffer::checkStatus() @requires_gl30 Extension @extension{ARB,framebuffer_object} */ enum class FramebufferTarget: GLenum { - /** - * For reading only. - * @requires_gles30 Extension @es_extension{APPLE,framebuffer_multisample}, - * @es_extension{ANGLE,framebuffer_blit} or @es_extension{NV,framebuffer_blit} - * in OpenGL ES 2.0 - */ + /** Frambebuffer reading target */ #ifndef MAGNUM_TARGET_GLES2 Read = GL_READ_FRAMEBUFFER, #else Read = GL_READ_FRAMEBUFFER_APPLE, #endif - /** - * For drawing only. - * @requires_gles30 Extension @es_extension{APPLE,framebuffer_multisample}, - * @es_extension{ANGLE,framebuffer_blit} or @es_extension{NV,framebuffer_blit} - * in OpenGL ES 2.0 - */ + /** Framebuffer drawing target */ #ifndef MAGNUM_TARGET_GLES2 Draw = GL_DRAW_FRAMEBUFFER, #else Draw = GL_DRAW_FRAMEBUFFER_APPLE, #endif - ReadDraw = GL_FRAMEBUFFER /**< For both reading and drawing. */ + #ifdef MAGNUM_BUILD_DEPRECATED + /** + * Framebuffer drawing target + * @deprecated Use @ref FramebufferTarget::Draw instead. + */ + ReadDraw CORRADE_DEPRECATED_ENUM("use FramebufferTarget::Draw instead") = + #ifndef MAGNUM_TARGET_GLES2 + GL_DRAW_FRAMEBUFFER + #else + GL_DRAW_FRAMEBUFFER_APPLE + #endif + #endif }; namespace Implementation { struct FramebufferState; } @@ -255,17 +257,26 @@ class MAGNUM_EXPORT AbstractFramebuffer { } /** - * @brief Bind framebuffer for rendering + * @brief Bind framebuffer for drawing * - * Binds the framebuffer and updates viewport to saved dimensions. + * Binds the framebuffer for drawing and updates viewport to saved + * dimensions. * @see @ref setViewport(), @ref DefaultFramebuffer::mapForRead(), * @ref Framebuffer::mapForRead(), @ref DefaultFramebuffer::mapForDraw(), * @ref Framebuffer::mapForDraw(), @fn_gl{BindFramebuffer}, * @fn_gl{Viewport} - * @todo Bind internally to ReadDraw if separate binding points are not - * supported */ - void bind(FramebufferTarget target); + void bind(); + + #ifdef MAGNUM_BUILD_DEPRECATED + /** + * @copybrief bind() + * @deprecated Use parameter-less @ref bind() instead. + */ + CORRADE_DEPRECATED("use parameter-less bind() instead") void bind(FramebufferTarget) { + bind(); + } + #endif /** @brief Viewport rectangle */ Range2Di viewport() const { return _viewport; } @@ -397,7 +408,17 @@ class MAGNUM_EXPORT AbstractFramebuffer { static void MAGNUM_LOCAL blitImplementationNV(AbstractFramebuffer& source, AbstractFramebuffer& destination, const Range2Di& sourceRectangle, const Range2Di& destinationRectangle, FramebufferBlitMask mask, FramebufferBlitFilter filter); #endif + void MAGNUM_LOCAL bindImplementationDefault(FramebufferTarget target); + FramebufferTarget MAGNUM_LOCAL bindImplementationDefault(); + #ifdef MAGNUM_TARGET_GLES2 + void MAGNUM_LOCAL bindImplementationSingle(FramebufferTarget); + FramebufferTarget MAGNUM_LOCAL bindImplementationSingle(); + #endif + GLenum MAGNUM_LOCAL checkStatusImplementationDefault(FramebufferTarget target); + #ifdef MAGNUM_TARGET_GLES2 + GLenum MAGNUM_LOCAL checkStatusImplementationSingle(FramebufferTarget); + #endif #ifndef MAGNUM_TARGET_GLES GLenum MAGNUM_LOCAL checkStatusImplementationDSA(FramebufferTarget target); GLenum MAGNUM_LOCAL checkStatusImplementationDSAEXT(FramebufferTarget target); diff --git a/src/Magnum/DefaultFramebuffer.h b/src/Magnum/DefaultFramebuffer.h index 122244c4a..f35983f79 100644 --- a/src/Magnum/DefaultFramebuffer.h +++ b/src/Magnum/DefaultFramebuffer.h @@ -318,6 +318,10 @@ class MAGNUM_EXPORT DefaultFramebuffer: public AbstractFramebuffer { * If on OpenGL ES or neither @extension{ARB,direct_state_access} (part * of OpenGL 4.5) nor @extension{EXT,direct_state_access} is available, * the framebuffer is bound before the operation (if not already). + * + * On OpenGL ES 2.0, if none of @es_extension{APPLE,framebuffer_multisample}, + * @es_extension{ANGLE,framebuffer_blit} or @es_extension{NV,framebuffer_blit} + * is available, the @p target parameter is ignored. * @see @fn_gl2{CheckNamedFramebufferStatus,CheckFramebufferStatus}, * @fn_gl_extension{CheckNamedFramebufferStatus,EXT,direct_state_access}, * eventually @fn_gl{BindFramebuffer} and @fn_gl{CheckFramebufferStatus} diff --git a/src/Magnum/Framebuffer.cpp b/src/Magnum/Framebuffer.cpp index 8cdaa8d91..635106c3c 100644 --- a/src/Magnum/Framebuffer.cpp +++ b/src/Magnum/Framebuffer.cpp @@ -114,7 +114,7 @@ Framebuffer::~Framebuffer() { * @todo Less ugly solution (need to call setViewportInternal() to * reset the viewport to size of default framebuffer) */ - defaultFramebuffer.bind(FramebufferTarget::Draw); + defaultFramebuffer.bind(); } glDeleteFramebuffers(1, &_id); diff --git a/src/Magnum/Framebuffer.h b/src/Magnum/Framebuffer.h index b94fe76df..d73c2973f 100644 --- a/src/Magnum/Framebuffer.h +++ b/src/Magnum/Framebuffer.h @@ -375,6 +375,10 @@ class MAGNUM_EXPORT Framebuffer: public AbstractFramebuffer, public AbstractObje * If on OpenGL ES or neither @extension{ARB,direct_state_access} (part * of OpenGL 4.5) nor @extension{EXT,direct_state_access} is available, * the framebuffer is bound before the operation (if not already). + * + * On OpenGL ES 2.0, if none of @es_extension{APPLE,framebuffer_multisample}, + * @es_extension{ANGLE,framebuffer_blit} or @es_extension{NV,framebuffer_blit} + * is available, the @p target parameter is ignored. * @see @fn_gl2{CheckNamedFramebufferStatus,CheckFramebufferStatus}, * @fn_gl_extension{CheckNamedFramebufferStatus,EXT,direct_state_access}, * eventually @fn_gl{BindFramebuffer} and @fn_gl{CheckFramebufferStatus} diff --git a/src/Magnum/Implementation/FramebufferState.cpp b/src/Magnum/Implementation/FramebufferState.cpp index 16eb19d8b..b10210775 100644 --- a/src/Magnum/Implementation/FramebufferState.cpp +++ b/src/Magnum/Implementation/FramebufferState.cpp @@ -104,12 +104,13 @@ FramebufferState::FramebufferState(Context& context, std::vector& e renderbufferStorageImplementation = &Renderbuffer::storageImplementationDefault; } - /* Framebuffer binding on ES2 */ #ifdef MAGNUM_TARGET_GLES2 + /* Framebuffer binding and checking on ES2 */ /* Optimistically set separate binding targets and check if one of the extensions providing them is available */ - readTarget = FramebufferTarget::Read; - drawTarget = FramebufferTarget::Draw; + bindImplementation = &Framebuffer::bindImplementationDefault; + bindInternalImplementation = &Framebuffer::bindImplementationDefault; + checkStatusImplementation = &Framebuffer::checkStatusImplementationDefault; if(context.isExtensionSupported()) { extensions.push_back(Extensions::GL::ANGLE::framebuffer_blit::string()); @@ -127,8 +128,12 @@ FramebufferState::FramebufferState(Context& context, std::vector& e } else if(context.isExtensionSupported()) { extensions.push_back(Extensions::GL::NV::framebuffer_multisample::string()); - /* If no such extension is available, reset back to unified target */ - } else readTarget = drawTarget = FramebufferTarget::ReadDraw; + /* If no such extension is available, reset back to single target */ + } else { + bindImplementation = &Framebuffer::bindImplementationSingle; + bindInternalImplementation = &Framebuffer::bindImplementationSingle; + checkStatusImplementation = &Framebuffer::checkStatusImplementationSingle; + } #endif /* Framebuffer reading implementation */ diff --git a/src/Magnum/Implementation/FramebufferState.h b/src/Magnum/Implementation/FramebufferState.h index 4bd383c53..e2e44c470 100644 --- a/src/Magnum/Implementation/FramebufferState.h +++ b/src/Magnum/Implementation/FramebufferState.h @@ -48,6 +48,10 @@ struct FramebufferState { #ifndef MAGNUM_TARGET_GLES2 void(AbstractFramebuffer::*invalidateSubImplementation)(GLsizei, const GLenum*, const Range2Di&); #endif + #ifdef MAGNUM_TARGET_GLES2 + void(AbstractFramebuffer::*bindImplementation)(FramebufferTarget); + FramebufferTarget(AbstractFramebuffer::*bindInternalImplementation)(); + #endif void(Framebuffer::*createImplementation)(); void(Framebuffer::*renderbufferImplementation)(Framebuffer::BufferAttachment, Renderbuffer&); @@ -63,8 +67,6 @@ struct FramebufferState { void(*readImplementation)(const Range2Di&, ColorFormat, ColorType, std::size_t, GLvoid*); - FramebufferTarget readTarget, drawTarget; - GLuint readBinding, drawBinding, renderbufferBinding; GLint maxDrawBuffers, maxColorAttachments, maxRenderbufferSize, maxSamples; #ifndef MAGNUM_TARGET_GLES diff --git a/src/Magnum/Test/MeshGLTest.cpp b/src/Magnum/Test/MeshGLTest.cpp index a9d88d762..c785f6090 100644 --- a/src/Magnum/Test/MeshGLTest.cpp +++ b/src/Magnum/Test/MeshGLTest.cpp @@ -438,7 +438,7 @@ Checker::Checker(AbstractShaderProgram&& shader, RenderbufferFormat format, Mesh renderbuffer.setStorage(format, Vector2i(1)); framebuffer.attachRenderbuffer(Framebuffer::ColorAttachment(0), renderbuffer); - framebuffer.bind(FramebufferTarget::ReadDraw); + framebuffer.bind(); mesh.setPrimitive(MeshPrimitive::Points) .setCount(2); @@ -1726,7 +1726,7 @@ MultiChecker::MultiChecker(AbstractShaderProgram&& shader, Mesh& mesh): framebuf Vector2i(1)); framebuffer.attachRenderbuffer(Framebuffer::ColorAttachment(0), renderbuffer); - framebuffer.bind(FramebufferTarget::ReadDraw); + framebuffer.bind(); mesh.setPrimitive(MeshPrimitive::Points) .setCount(2); diff --git a/src/Magnum/Test/SampleQueryGLTest.cpp b/src/Magnum/Test/SampleQueryGLTest.cpp index ab0fbf203..3a1e153f5 100644 --- a/src/Magnum/Test/SampleQueryGLTest.cpp +++ b/src/Magnum/Test/SampleQueryGLTest.cpp @@ -128,7 +128,7 @@ void SampleQueryGLTest::querySamplesPassed() { #endif q.begin(); - framebuffer.bind(FramebufferTarget::ReadDraw); + framebuffer.bind(); mesh.draw(shader); q.end(); @@ -167,7 +167,7 @@ void SampleQueryGLTest::conditionalRender() { .addVertexBuffer(buffer, 0, Attribute<0, Vector2>{}); MyShader shader; - framebuffer.bind(FramebufferTarget::ReadDraw); + framebuffer.bind(); MAGNUM_VERIFY_NO_ERROR(); diff --git a/src/Magnum/TextureTools/DistanceField.cpp b/src/Magnum/TextureTools/DistanceField.cpp index d7422c730..44e3a26f9 100644 --- a/src/Magnum/TextureTools/DistanceField.cpp +++ b/src/Magnum/TextureTools/DistanceField.cpp @@ -152,7 +152,7 @@ void distanceField(Texture2D& input, Texture2D& output, const Range2Di& rectangl Framebuffer framebuffer(rectangle); framebuffer.attachTexture(Framebuffer::ColorAttachment(0), output, 0); - framebuffer.bind(FramebufferTarget::Draw); + framebuffer.bind(); framebuffer.clear(FramebufferClear::Color); const Framebuffer::Status status = framebuffer.checkStatus(FramebufferTarget::Draw);