General improvements to sf::Image

[Fytch]

I happen to use sf::Image quite a lot in a current project but I am widely dissatisfied with its somewhat meager functionality. Possible improvements:

• Constructor overload that does the same as sf::Image::create (i.e. creating an image with a certain size).
• sf::Image::create is poorly implemented. It calls .resize on the internal std::vector which either leads to every old pixel being copied (even though they are all overwritten shortly after) or to sf::Image possessing a lot of memory it doesn't use (because std::vector::capacity stays the same regardless). The correct way of implementing this function is to swap the internal vector with a newly created one.
• sf::Image::create grants no exception safety. The instance is indeed corrupt after an erroneous sf::Image::create invocation because the dimensions and the actual data don't match anymore. However, strong exception safety (no changes in case of an exception being thrown) is easily achievable just by changing the order inside the function. Memory allocation must be done first and only then the actual instance should be modified.
• There should be an overload of sf::Image::create that doesn't fill the image with a particular color. This is an optimization for the case that the user intends to overwrite the pixels anyway.
• sf::Image::swap should be implemented. The lack thereof forces users to copy the content from one image to another instead of just swapping the vectors' pointers, which leads to significant worse performance.
• sf::Image::empty would be nice as sf::Image.getPixelsPtr() == nullptr doesn't work and sf::Image.getSize().x && sf::Image.getSize().y is verbose. Multiple functions of sf::Image contain the line if (!m_pixels.empty()), yet this function isn't available outside the class.

I can implement these by myself and create a pull request, if others also appreciate these improvements.

[Mario]

• Constructor overload that does the same as sf::Image::create (i.e. creating an image with a certain size).

I guess the current implementation simply mimics other SFML classes, where you're explicitly calling create() to not have any kind of failed state after running the constructor. Not sure on this one though.

• sf::Image::create is poorly implemented. It calls .resize on the internal std::vector which either leads to every old pixel being copied (even though they are all overwritten shortly after) or to sf::Image possessing a lot of memory it doesn't use (because std::vector::capacity stays the same regardless). The correct way of implementing this function is to swap the internal vector with a newly created one.

This one sounds very reasonable to me.

• sf::Image::create grants no exception safety. The instance is indeed corrupt after an erroneous sf::Image::create invocation because the dimensions and the actual data don't match anymore. However, strong exception safety (no changes in case of an exception being thrown) is easily achievable just by changing the order inside the function. Memory allocation must be done first and only then the actual instance should be modified.

Sounds reasonable as well, especially if the user doesn't check return values.

• There should be an overload of sf::Image::create that doesn't fill the image with a particular color. This is an optimization for the case that the user intends to overwrite the pixels anyway.

Sounds reasonable as well.

• sf::Image::swap should be implemented. The lack thereof forces users to copy the content from one image to another instead of just swapping the vectors' pointers, which leads to significant worse performance.

I'm not sure on this one. I can't think of any specific use case right now. Might also be something for future move semantics?

• sf::Image::empty would be nice as sf::Image.getPixelsPtr() == nullptr doesn't work and sf::Image.getSize().x && sf::Image.getSize().y is verbose. Multiple functions of sf::Image contain the line if (!m_pixels.empty()), yet this function isn't available outside the class.

Well, I guess all SFML resources could need some shorthand way to determine validity/proper initialization.

I can implement these by myself and create a pull request, if others also appreciate these improvements.

Just do so, especially if they're useful for yourself. Even if they're not accepted right away, you can use these changes in your own copy.

[Fytch]

• Constructor overload that does the same as sf::Image::create (i.e. creating an image with a certain size).

I guess the current implementation simply mimics other SFML classes, where you're explicitly calling create() to not have any kind of failed state after running the constructor. Not sure on this one though.

The only way Image::create may fail is if the allocator throws an exception. If there is an exception being thrown by invoking the constructor, the instance is not defined / constructed because of stack unwinding, thus not leaving you with any kind of defective state, nor anything at all for that matter.

• sf::Image::create grants no exception safety. The instance is indeed corrupt after an erroneous sf::Image::create invocation because the dimensions and the actual data don't match anymore. However, strong exception safety (no changes in case of an exception being thrown) is easily achievable just by changing the order inside the function. Memory allocation must be done first and only then the actual instance should be modified.

Sounds reasonable as well, especially if the user doesn't check return values.

sf::Image::create doesn't return anything. In case of insufficient memory, it'll throw std::bad_alloc.

• sf::Image::swap should be implemented. The lack thereof forces users to copy the content from one image to another instead of just swapping the vectors' pointers, which leads to significant worse performance.

I'm not sure on this one. I can't think of any specific use case right now. Might also be something for future move semantics?

It had been rather helpful for me, had sf::Image offered that kind of functionality. Right now, with the lack of both a swap-method and move-semantics, there's no way to efficiently hand over an image from one part of the program to another (or, more importantly, from one thread to another). I eventually solved it by wrapping all sf::Images with std::unique_ptr and then swapping and moving these. Not only is this very inconvenient, this also comes at a cost obviously.

• sf::Image::empty would be nice as sf::Image.getPixelsPtr() == nullptr doesn't work and sf::Image.getSize().x && sf::Image.getSize().y is verbose. Multiple functions of sf::Image contain the line if (!m_pixels.empty()), yet this function isn't available outside the class.

Well, I guess all SFML resources could need some shorthand way to determine validity/proper initialization.

I wholeheartedly agree. Is this a general consensus in the SFML team? I'd be happy to implement that.

How should I go about it? Create a separate pull request for every feature? Or should I ask again on the issue tracker before implementing it? It seem as, apart from you, not many people have cared about this forum post.

Lastly, is there disagreement if I removed the error message from sf::Image::getPixelsPtr() if the image is empty? It seems a bit silly that there is an error message if one wants to access the pixels via a pointer. The error should be on the other end, e.g. the function that reads from the pointer. Returning a nullptr if something is empty is perfectly reasonable and shouldn't come with an error. Neither should one be compelled to check the image's size before requesting a pointer to the data, to avoid the error message.

[korczurekk]

Create single PR with multiple commits, people with write access to repo can select which should be applied.

[eXpl0it3r]

Create single PR with multiple commits, people with write access to repo can select which should be applied.

No.

Every feature/bugfix should get its own PR.

[korczurekk]

Create single PR with multiple commits, people with write access to repo can select which should be applied.

No.

Every feature/bugfix should get its own PR.

Why would you create multiple PR? Such small features easly fit in one commit each, there's a reason why you can apply selected ones.

Or is it some SFML-related tradition?

[eXpl0it3r]

Because it's a lot easier to deal with different changes on a PR level. They can be reviewed, tagged, assigned, discussed, accepted or rejected individually and when you link a PR everyone knows what you're talking about, instead of having to specify individual commits every time.

[Fytch]

Another instance, where sf::Image::swap would come in incredibly handy would be when implementing a filter e.g. the Gaussian filter (or just any operation that doesn't work in-place, including mirroring along the diagonal a.k.a. transposition). If I have an existing image and I would want to apply such a filter to it, then the code may look something like this:

class foo : sf::Drawable
{
        sf::Image m_img;
       
public:
        void f()
        {
                sf::Image newImg;
                newImg.create(m_img.getSize().x, m_img.getSize().y);
               
                // apply Gaussian blur on m_img and write it into newImg
               
                m_img = newImg;
        }
};

In my opinion, there are three things worth criticizing in regards to SFML in the shown snippet:

• As there are no constructors beside the default ones, the programmer is forced to fall back on create. All the major libraries, e.g. STL, boost, eigen to name a few, allow initialization on construction. This principle is a key feature of C++, not least.
• If there is a method getSize that returns a vector, the corresponding functions to create an image should also allow to be invoked with such a vector. There is absolutely no reason not to have an overload that does this tedious boilerplate work every time. If the expression in front of .getSize() is more verbose, this even forces one to create a temporary. The same goes for sf::Texture, sf::VideoMode and other classes along these lines. Funnily enough, there are methods, e.g. setSize and setPosition in sf::Window and thus also sf::RenderWindow, that take a vector but not two scalars. This inconsistency makes it even worse, in my opinion.
• There is absolutely no way how I may swap m_img and newImg efficiently. Both swap methods as well as support for move semantics would solve this problem, though swap methods are the more conservative as they already existed in C++03 (where they were in widespread use).

Let me know what you think about these.

[Hapax]

[1.]
As there are no constructors beside the default ones, the programmer is forced to fall back on create. All the major libraries, e.g. STL, boost, eigen to name a few, allow initialization on construction. This principle is a key feature of C++, not least.

The requirement of using create for most classes in SFML is apparently based on the fact that they return a "successful" boolean value so cannot be automatically included in the constructor as there would be no way to return this value. With sf::Image, however, there is no return value so create could automatically be called on construction. You mentioned some of this already.
This has been mention before (for example, in this thread).
Other libraries are able to use exceptions to convey failure information from constructors, which, I'm sure, will be utilised by SFML in version 3.

[2.]
If there is a method getSize that returns a vector, the corresponding functions to create an image should also allow to be invoked with such a vector. There is absolutely no reason not to have an overload that does this tedious boilerplate work every time. If the expression in front of .getSize() is more verbose, this even forces one to create a temporary. The same goes for sf::Texture, sf::VideoMode and other classes along these lines. Funnily enough, there are methods, e.g. setSize and setPosition in sf::Window and thus also sf::RenderWindow, that take a vector but not two scalars. This inconsistency makes it even worse, in my opinion.

I agree. All parameters that take x and y should take a vector2. Providing x and y as separate values should be considered the 'convenience overload'. I wouldn't be surprised if the separate version was completely removed for SFML 3.

[3.]
There is absolutely no way how I may swap m_img and newImg efficiently. Both swap methods as well as support for move semantics would solve this problem, though swap methods are the more conservative as they already existed in C++03 (where they were in widespread use).

sf::Image is a container and keeps control of its storage. Maybe the best solution isn't to swap an sf::Image with another but allow sf::Image to perform its functions on external memory. Probably not, though.
Your pointer wrapper might be the 'best' solution for now.