From 944b068f6da11ef326b5a5e85308334c2506911c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 15 Aug 2017 22:04:55 +0200 Subject: [PATCH] Buffer: deprecate templated overloads of data(), subData() and map(). They were utterly confusing, as it was completely unclear what the units of offset/size parameters are, whether byte sizes or element counts (and moreover, some of these APIs had offset in bytes and size in count and some not). All of those are deprecated now, with hinting the user to convert to non-templated APIs in combination with Containers::arrayCast(). Moreover, the non-templated range map() function doesn't return just void* anymore, but a properly sized ArrayView. The old map() (which doesn't take range) still returns just a pointer (but also a char* instead of void* for consistency), as getting size there is non-trivial (and impossible on old ES/WebGL). The switch to ArrayView might be a source breaking change, but I silently hope that everyone was just using the templated functions anyway (that are deprecated now). So, in short, this was before: T* a = buf.map(0, size_in_what_i_have_no_idea); And this is now, with proper size safety and clear API: ArrayView a = Containers::arrayCast(buf.map(0, size_in_bytes); The deprecated APIs will be removed at some point in the future, as usual. --- doc/changelog.dox | 7 +++- src/Magnum/Buffer.cpp | 23 +++++++++-- src/Magnum/Buffer.h | 92 ++++++++++++++++++++++++++++++------------- 3 files changed, 88 insertions(+), 34 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index b8d966720..eb8104dae 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -301,8 +301,6 @@ namespace Magnum { ## Changes and improvements -### Core library - - Functionality that is not available on WebGL (like debug output etc.) is not present in WebGL builds to reduce compiled code size - The @ref Mesh class now stores a copy of @ref Buffer instance instead of @@ -322,6 +320,8 @@ namespace Magnum { - It's no longer possible to call @ref Image::data() "Image*::data()" on r-value instances as that would cause accessing freed data. Use @ref Image::release() instead. +- @ref Buffer::map() now returns @ref Corrade::Containers::ArrayView instead + of a plain pointer for better security - Graceful handling of broken GL contexts - Behavior of @ref Version::GLES200 and upwards on desktop OpenGL is changed to request an ES dialect of GLSL when used in @ref Shader (instead of a @@ -478,6 +478,9 @@ namespace Magnum { and @ref ImageView::setData() taking `void*` are deprecated, use constructors and functions taking @ref Corrade::Containers::Array / @ref Corrade::Containers::ArrayView instead +- Templated @ref Buffer::data(), @ref Buffer::subData() and + @ref Buffer::map() are deprecated, use the non-templated versions in + combination with @ref Corrade::Containers::arrayCast() instead - @ref Context::current() and @ref Audio::Context::current() returning a pointer is deprecated, it's returning a reference now and asserts that a context exists. Use @ref Context::hasCurrent() and @ref Audio::Context::hasCurrent() diff --git a/src/Magnum/Buffer.cpp b/src/Magnum/Buffer.cpp index 4da901a6d..3579c6c95 100644 --- a/src/Magnum/Buffer.cpp +++ b/src/Magnum/Buffer.cpp @@ -341,6 +341,12 @@ Int Buffer::size() { return size; } +#ifndef MAGNUM_TARGET_GLES +Containers::Array Buffer::data() { + return subData(0, size()); +} +#endif + Buffer& Buffer::setData(const Containers::ArrayView data, const BufferUsage usage) { (this->*Context::current().state().buffer->dataImplementation)(data.size(), data, usage); return *this; @@ -362,8 +368,8 @@ Buffer& Buffer::invalidateSubData(const GLintptr offset, const GLsizeiptr length } #ifndef MAGNUM_TARGET_WEBGL -void* Buffer::map(const MapAccess access) { - return (this->*Context::current().state().buffer->mapImplementation)(access); +char* Buffer::map(const MapAccess access) { + return static_cast((this->*Context::current().state().buffer->mapImplementation)(access)); } #if defined(DOXYGEN_GENERATING_OUTPUT) || defined(CORRADE_TARGET_NACL) @@ -373,8 +379,8 @@ void* Buffer::mapSub(const GLintptr offset, const GLsizeiptr length, const MapAc } #endif -void* Buffer::map(const GLintptr offset, const GLsizeiptr length, const MapFlags flags) { - return (this->*Context::current().state().buffer->mapRangeImplementation)(offset, length, flags); +Containers::ArrayView Buffer::map(const GLintptr offset, const GLsizeiptr length, const MapFlags flags) { + return {static_cast((this->*Context::current().state().buffer->mapRangeImplementation)(offset, length, flags)), std::size_t(length)}; } Buffer& Buffer::flushMappedRange(const GLintptr offset, const GLsizeiptr length) { @@ -394,10 +400,19 @@ void Buffer::unmapSub() { #endif #ifndef MAGNUM_TARGET_GLES +Containers::Array Buffer::subData(const GLintptr offset, const GLsizeiptr size) { + Containers::Array data(size); + if(size) (this->*Context::current().state().buffer->getSubDataImplementation)(offset, size, data); + return data; +} + +/** @todo remove when this is not used anymore */ +#ifdef MAGNUM_BUILD_DEPRECATED void Buffer::subDataInternal(GLintptr offset, GLsizeiptr size, GLvoid* data) { (this->*Context::current().state().buffer->getSubDataImplementation)(offset, size, data); } #endif +#endif #ifndef MAGNUM_TARGET_GLES2 void Buffer::bindImplementationFallback(const Target target, const GLuint firstIndex, Containers::ArrayView buffers) { diff --git a/src/Magnum/Buffer.h b/src/Magnum/Buffer.h index 2a898e233..fe59ed763 100644 --- a/src/Magnum/Buffer.h +++ b/src/Magnum/Buffer.h @@ -32,7 +32,7 @@ #include #include #include -#include +#include #include #include @@ -41,6 +41,7 @@ #include "Magnum/Tags.h" #ifdef MAGNUM_BUILD_DEPRECATED +#include #include #endif @@ -165,18 +166,21 @@ buffer.setData({nullptr, 200*sizeof(Vector3)}, BufferUsage::StaticDraw); @endcode Then you can map the buffer to client memory and operate with the memory directly. After you are done with the operation, call @ref unmap() to unmap the -buffer again. +buffer again. The @ref map() functions return a view on `char` array and you +may want to cast it to some useful type first using @ref Containers::arrayCast(): @code -Vector3* data = buffer.map(0, 200*sizeof(Vector3), Buffer::MapFlag::Write|Buffer::MapFlag::InvalidateBuffer); -for(std::size_t i = 0; i != 200; ++i) - data[i] = ...; +Containers::ArrayView data = Containers::arrayCast(buffer.map(0, 200*sizeof(Vector3), Buffer::MapFlag::Write|Buffer::MapFlag::InvalidateBuffer)); +CORRADE_INTERNAL_ASSERT(data); +for(Vector3& d: data) + d = ...; CORRADE_INTERNAL_ASSERT_OUTPUT(buffer.unmap()); @endcode If you are updating only a few discrete portions of the buffer, you can use @ref MapFlag::FlushExplicit and @ref flushMappedRange() to reduce number of memory operations performed by OpenGL on unmapping. Example: @code -Vector3* data = buffer.map(0, 200*sizeof(Vector3), Buffer::MapFlag::Write|Buffer::MapFlag::FlushExplicit); +Containers::ArrayView data = Containers::arrayCast(buffer.map(0, 200*sizeof(Vector3), Buffer::MapFlag::Write|Buffer::MapFlag::FlushExplicit)); +CORRADE_INTERNAL_ASSERT(data); for(std::size_t i: {7, 27, 56, 128}) { data[i] = ...; buffer.flushMappedRange(i*sizeof(Vector3), sizeof(Vector3)); @@ -1072,13 +1076,21 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { * WebGL. Use @ref map() or @ref DebugTools::bufferData() in * OpenGL ES instead. */ + Containers::Array data(); + + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief data() + * @deprecated Use non-templated @ref subData() and @ref Containers::arrayCast() + * instead. + */ /* MinGW complains loudly if the declaration doesn't also have inline */ - template inline Containers::Array data(); + template CORRADE_DEPRECATED("use non-templated data() and Containers::arrayCast() instead") inline Containers::Array data(); + #endif /** * @brief Buffer subdata * @param offset Byte offset in the buffer - * @param size Data size (count of @p T values) + * @param size Data size in bytes * * Returns data of given buffer portion. If neither * @extension{ARB,direct_state_access} (part of OpenGL 4.5) nor @@ -1092,8 +1104,15 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { * WebGL. Use @ref map() or @ref DebugTools::bufferData() in * OpenGL ES instead. */ + Containers::Array subData(GLintptr offset, GLsizeiptr size); + + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief subData() + * @deprecated Use non-templated @ref subData() and @ref Containers::arrayCast() instead + */ /* MinGW complains loudly if the declaration doesn't also have inline */ - template inline Containers::Array subData(GLintptr offset, GLsizeiptr size); + template CORRADE_DEPRECATED("use non-templated subData() and Containers::arrayCast() instead") inline Containers::Array subData(GLintptr offset, GLsizeiptr size); + #endif #endif /** @@ -1126,7 +1145,7 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { /** * @brief Set buffer subdata - * @param offset Offset in the buffer + * @param offset Byte offset in the buffer * @param data Data * @return Reference to self (for method chaining) * @@ -1164,7 +1183,7 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { /** * @brief Invalidate buffer subdata - * @param offset Offset into the buffer + * @param offset Byte offset into the buffer * @param length Length of the invalidated range * @return Reference to self (for method chaining) * @@ -1178,7 +1197,7 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { /** * @brief Map buffer to client memory * @param access Access - * @return Pointer to buffer data + * @return Pointer to mapped buffer data or `nullptr` on error * * If neither @extension{ARB,direct_state_access} (part of OpenGL 4.5) * nor @extension{EXT,direct_state_access} desktop extension is @@ -1195,12 +1214,17 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { * @deprecated_gl Prefer to use @ref map(GLintptr, GLsizeiptr, MapFlags) * instead, as it has more complete set of features. */ - void* map(MapAccess access); + char* map(MapAccess access); - /** @overload */ - template T* map(MapAccess access) { - return static_cast(map(access)); + #ifdef MAGNUM_BUILD_DEPRECATED + /** @overload + * @deprecated Use non-templated @ref map() and cast the result + * manually instead. + */ + template CORRADE_DEPRECATED("use non-templated map() and cast the result manually instead") T* map(MapAccess access) { + return reinterpret_cast(map(access)); } + #endif #if defined(DOXYGEN_GENERATING_OUTPUT) || defined(CORRADE_TARGET_NACL) /** @@ -1231,11 +1255,11 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { /** * @brief Map buffer to client memory - * @param offset Offset into the buffer - * @param length Length of the mapped memory + * @param offset Byte offset into the buffer + * @param length Length of the mapped memory in bytes * @param flags Flags. At least @ref MapFlag::Read or * @ref MapFlag::Write must be specified. - * @return Pointer to buffer data + * @return Sized view to buffer data or `nullptr` on error * * If neither @extension{ARB,direct_state_access} (part of OpenGL 4.5) * nor @extension{EXT,direct_state_access} desktop extension is @@ -1251,17 +1275,22 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { * OpenGL ES 2.0. * @requires_gles Buffer mapping is not available in WebGL. */ - void* map(GLintptr offset, GLsizeiptr length, MapFlags flags); + Containers::ArrayView map(GLintptr offset, GLsizeiptr length, MapFlags flags); - /** @overload */ - template T* map(GLintptr offset, GLsizeiptr length, MapFlags flags) { - return static_cast(map(offset, length, flags)); + #ifdef MAGNUM_BUILD_DEPRECATED + /** @overload + * @deprecated Use non-templated @ref map() and @ref Containers::arrayCast() + * instead. + */ + template CORRADE_DEPRECATED("use non-templated map() and Containers::arrayCast() instead") T* map(GLintptr offset, GLsizeiptr length, MapFlags flags) { + return Containers::arrayCast(map(offset, length, flags)); } + #endif /** * @brief Flush mapped range - * @param offset Offset relative to start of mapped range - * @param length Length of the flushed memory + * @param offset Byte offset relative to start of mapped range + * @param length Length of the flushed memory in bytes * @return Reference to self (for method chaining) * * Flushes specified subsection of mapped range. Use only if you called @@ -1364,8 +1393,8 @@ class MAGNUM_EXPORT Buffer: public AbstractObject { Buffer& setLabelInternal(Containers::ArrayView label); #endif - #ifndef MAGNUM_TARGET_GLES - void subDataInternal(GLintptr offset, GLsizeiptr size, GLvoid* data); + #if !defined(MAGNUM_TARGET_GLES) && defined(MAGNUM_BUILD_DEPRECATED) + CORRADE_DEPRECATED("used only by deprecated subData()") void subDataInternal(GLintptr offset, GLsizeiptr size, GLvoid* data); #endif void MAGNUM_LOCAL getParameterImplementationDefault(GLenum value, GLint* data); @@ -1483,7 +1512,11 @@ inline GLuint Buffer::release() { return id; } -#ifndef MAGNUM_TARGET_GLES +#if !defined(MAGNUM_TARGET_GLES) && defined(MAGNUM_BUILD_DEPRECATED) +#ifdef __GNUC__ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#endif template Containers::Array inline Buffer::data() { const Int bufferSize = size(); CORRADE_ASSERT(bufferSize%sizeof(T) == 0, "Buffer::data(): the buffer size is" << bufferSize << "bytes, which can't be expressed as array of types with size" << sizeof(T), nullptr); @@ -1495,6 +1528,9 @@ template Containers::Array inline Buffer::subData(const GLintptr off if(size) subDataInternal(offset, size*sizeof(T), data); return data; } +#ifdef __GNUC__ +#pragma GCC diagnostic pop +#endif #endif }