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.
- Constructor overload that does the same as sf::Image::create (i.e. creating an image with a certain size).
This one sounds very reasonable to me.
- 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.
Sounds reasonable as well, especially if the user doesn't check return values.
- 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.
- 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.
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::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.
Well, I guess all SFML resources could need some shorthand way to determine validity/proper initialization.
- 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.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.
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.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.
- Constructor overload that does the same as sf::Image::create (i.e. creating an image with a certain size).
sf::Image::create doesn't return anything. In case of insufficient memory, it'll throw std::bad_alloc.Sounds reasonable as well, especially if the user doesn't check return values.
- 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.
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.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::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 wholeheartedly agree. Is this a general consensus in the SFML team? I'd be happy to implement that.Well, I guess all SFML resources could need some shorthand way to determine validity/proper initialization.
- 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.
Create single PR with multiple commits, people with write access to repo can select which should be applied.No.
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.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.
[1.]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.
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.
[2.]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.
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.
[3.]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.
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).