[IMPROVEMENT] Move support for the Image class about magnum HOT 3 CLOSED

I changed my implementation of image loading to use a std::unique_ptr instead of a std::vector. This way I can release() ownership of the std::unique_ptr so that it does not get deleted.

I guess I was unaware that I could do such a thing. Anyway, I still personally thing you shouldn't be dealing with raw pointers in your implementation (perhaps you should consider a std::unique_ptr, as they do have a custom deleter for arrays [using std::unique_ptr<T[]>] and offers 0 overhead).

EDIT:

Also a pro about using a unique_ptr is the ability to release it, so that I can transfer ownership. As other libraries may want to manage memory for me. An example is with libRocket when it loads a font, as it will delete the data for the font (image data) when it has generated a texture, which causes a run-time error.

from magnum.

At the current moment, the only way to upload texture information is via the Image class.

Nope. Image is only one of three four interchargeable classes which you can use for storing or referencing of image data, along with BufferImage, Trade::ImageData and ImageReference. In fact, all image uploading functions take the ImageReference class and all the other image classes are implicitly convertible to it. Thanks to that you can just pass Image into Texture::setData() without any explicit conversions. The ImageReference class is probably what you want, as it doesn't take ownership of the data.

use a std::vector instead of a raw pointer in the implementation of the class. This would allow moving.

The Image class is movable (and also majority of all non-copyable classes in Magnum is):

Image2D a(...);
Image2D b(std::move(a));

I don't want to expose the data pointer as any std::unique_ptr<T[]> or std::vector<T>, because then the data will look like an array, which is wrong -- the data shouldn't be accessed through array subscript operator for at least these two reasons:

• There is no universal pixel format, sometimes the data are 8-bit-per-channel RGBA, sometimes four-bit grayscale, sometimes RGB565. Exposing std::vector<char> (or similar type) to the user thus has no point, as the data are unusable anyway. On the other hand having some "safe" pixel() access function and doing conversion on-the-fly in software would be slow. Also, the std::vector would need to have information about the data size, which is not needed in this case (it depends on pixel size and particular color format/type combination and GL functions don't need it, so there's no point in storing that information).
• The GPU should do all operations on pixels, not the CPU ;-) This effectively prevents the user from doing things that shouldn't be done in HW-accelerated graphics engine.

I still personally thing you shouldn't be dealing with raw pointers in your implementation (perhaps you should consider a std::unique_ptr, as they do have a custom deleter for arrays [using std::unique_ptr<T[]>] and offers 0 overhead).

In my opinion using raw pointers in low-level internals is okay as long as the explicit memory management isn't exposed to the end user. Using std::unique_ptr for Image internals isn't needed, as Image is actually doing the exact thing as std::unique_ptr, with some frosting on top. It would be similar to using std::vector in my own Vector implementation ;-)

I'm not aware of your opinions of using the STL in your library, but it would help a lot.

I'm not against using STL in the library (where it makes sense), the only problem are the horribly bloated headers (<memory> is one of them), which have huge impact on compilation times. But the std::unique_ptr is already spreaded through the codebase anyway, so one more #include won't worsen it too much.

Anyway, apart from the ImageReference solution, as you described the issue, it might be good to have some release() function on Image. It would reset the internal values to defaults (0x0 size, some default color format and type and nullptr data) and return the current data as std::unique_ptr. Accessing the raw data pointer would then look like this, which might be somewhat uninutitive:

Image2D a;
unsigned char* data = a.release().release();
// ...
delete[] a;

Just in case, have you tried the Trade::PngImporter etc. plugins? These might solve a bunch of the "not-well-integrated" problems you described.

from magnum.

I added Image::release() and Trade::ImageData::release() functions in 5ae723c. They return raw pointer and deletion of the array is then up to the user, just like in std::unique_ptr<T[]>:

Image2D a;
unsigned char* data = a.release();
// ...
delete[] a;

I guess I can close this now.

from magnum.