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

@@ -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)
+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);

src/Magnum/AbstractFramebuffer.h

@@ -30,6 +30,7 @@
  */
 
 #include <Corrade/Containers/EnumSet.h>
+#include <Corrade/Utility/Macros.h>
 
 #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);

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}

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);

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}

src/Magnum/Implementation/FramebufferState.cpp

@@ -104,12 +104,13 @@ FramebufferState::FramebufferState(Context& context, std::vector<std::string>& 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::GL::ANGLE::framebuffer_blit>()) {
         extensions.push_back(Extensions::GL::ANGLE::framebuffer_blit::string());
@@ -127,8 +128,12 @@ FramebufferState::FramebufferState(Context& context, std::vector<std::string>& e
     } else if(context.isExtensionSupported<Extensions::GL::NV::framebuffer_multisample>()) {
         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 */

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

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);
 

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();
 

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);