We should bring back default constructors about sfml HOT 6 CLOSED

Keep in mind, SFML 2 uses C++03 and thus has relatively fewer options for good, expressive factory function APIs. While I was not around at the time SFML 2's API was being written, I can imagine it made sense to resort to two-phase initialization to allow for initialization to fail. Perhaps SFML 2 could have used boost::optional or a custom optional wrapper class but both of those options come with their own downsides. As Hyrum's Law describes, users inevitably came to depend on this two-phase initialization even though that wasn't necessary the intension of its designers.

I largely agree with Vittorio. Anywhere you want to do two-phase initialization you can wrap the type in std::optional. That two-phase initialization must be explicitly spelled out in your code rather than it being baked into the SFML type itself. I understand this pattern may look ugly but I think it's ugly for good reason. Two-phase initialization is bug prone and worth removing. If you find yourself needing to use optionals a lot to defer initialization then perhaps revisit why you want some much deferred initialization in the first place.

As library authors we have to cater to a large number of users of different backgrounds with different desires. We have little control over the way in which users write code. One of our most effective tools to root out bugs in SFML programs is to make invalid states unrepresentable by the type system. This what we have done by removing these particular default constructors. We now have more confidence that certain types of bugs related to loading resources will either be fixed when upgrading to the v3 API or will be made much more clear when reading code.

I appreciate you taking the time to detail your struggles and hope some of these recommendations ease the pain with using the new API. I'm quite confident the SFML team is going to maintain our current stance and will not add default constructors back and as such will be closing this issue.

from sfml.

BTW, I would like to encourage discussion on the matter. Sharing problematic code snippets, or anecdotes regarding pain/friction, is very useful.

There's very often a different way to write the code to avoid the pain/friction, and I believe we should collect these examples so that we can then integrate them in our documentation/tutorials.

The associative container pain point mentioned here was a great one.

from sfml.

Thanks for sharing your perspective, I think it's important to consider your experience and whether we've been going down the right path.

I believe this is always going to be a subjective discussion, but anecdotally I can tell you that the added "friction" I've felt when migrating Open Hexagon to SFML 3.x has actually helped me discover holes in my original design and led to a more robust architecture.

I think that for people like me, the tradeoff between ergonomics/safety is a good thing. But of course I cannot speak for everyone.

I'd also like to point out that the model used in SFML 3.x needs a bit of a mindset change to get used to, and that all our tutorials/documentation do not provide tips and examples on how to effectively consume our new APIs.

Am I correct that if we want to store sfml objects on classes, our options are

• wrap it in a smart pointer
• store it as an optional
• manually manage memory via new and delete

Not really, you can just store it as usual. You just need to initialize it in the constructor of your own class. See our Shader.cpp example:

class Pixelate : public Effect
{
public:
    explicit Pixelate(sf::Texture&& texture, sf::Shader&& shader) : // <- pass in ctor
    m_texture(std::move(texture)), // <- move into class member
    m_shader(std::move(shader))    // <- move into class member
    {
        m_shader.setUniform("texture", sf::Shader::CurrentTexture);
    }

    // ...

private:
    sf::Texture m_texture; // <- stored as usual
    sf::Shader  m_shader;  // <- stored as usual 
};

To create Pixelate itself from fallible resource loading, you follow SFML's pattern with optionals:

std::optional<Pixelate> tryLoadPixelate()
{
    auto texture = sf::Texture::loadFromFile("resources/background.jpg");
    if (!texture.has_value())
       return std::nullopt;

    auto shader = sf::Shader::loadFromFile("resources/pixelate.frag", sf::Shader::Type::Fragment);
    if (!shader.has_value())
        return std::nullopt;

    return std::make_optional<Pixelate>(std::move(*texture), std::move(*shader));
}

Or you can just "wing it" if you don't care about propagating the errors:

auto pixelate = Pixelate{
    sf::Texture::loadFromFile("resources/background.jpg").value(),
    sf::Shader::loadFromFile("resources/pixelate.frag", sf::Shader::Type::Fragment).value()
};

This gives the user the freedom to choose whether they want to handle the errors or not, but always makes them aware that they can happen.

I have an unordered_map<string, sf::Image>. I thought for a moment this would work

This is mostly the fault of associative containers APIs in the standard library, and I do feel your pain here. Consider using std::unordered_map<string, std::optional<sf::Image>>.

In general, any time you feel like "we should have a default constructor here", a valid answer is "wrap it in an optional yourself". This always allows you to get default constructibility when you want, but also ensures that places where default constructibility is not required do not unnecessarily introduce an extra "empty" state that can cause run-time issues.

Another example from a breathing app I made a while ago. This is now a line in my code

What's the reason that sf::SoundBuffer is wrapped in a std::unique_ptr? If you share more code, I can likely help you figure out how to avoid the friction you're experiencing.

I think it might be improved slightly if we renamed the template parameter (currently named T) in .is and .getIf with something more descriptive.

This is a great suggestion that takes low effort to implement. I also think event visitation would be greatly improved with #3014.

I think removing the default constructors is a mistake though.

Your pain is real and the friction you've experienced should not be understated. However, I strongly disagree with your claim, as you can always reintroduce the empty state of any SFML type when (and only where) you need it by wrapping it in a std::optional yourself.

In most cases, you will not need to do that, and you'll benefit from the static compile-time knowledge that if you have a sf::Image, that image is guaranteed to be valid.

In some cases, either due to outdated standard library APIs, or due to pragmaticity/prototyping speed, it's trivial to wrap whatever you want in an optional.

I hope that these tips help you ease your pain and reduce your friction when working with SFML 3.x in the future. I hope you give our new model another chance in light of this new knowledge, but I totally understand if you still end up preferring the older one.

I think our new APIs are quite opinionated and they will never please everyone, however I think they're a step in the right direction.

When our tutorials/documentation are updated, all of the things I've mentioned above should be included in some form to aid new users and teach the idiomatic and proper use of std::optional-based APIs.

BTW, if you have any further pain point or code sample, please share -- I'm curious to see if the new APIs create friction for no good reason, or if they expose some possible areas of improvement.

from sfml.

Just my 2 cents...

I am somewhere between both camps on this issue.

I generally agree that preventing creation of objects in an unusable state is a good thing.

Because the original code wasn't posted, I assume it would have looked something like this:

asyncLoadedImages[workOrder.path].loadFromFile(...);

This worked because sf::Image had a default constructor, however what would happen if loading fails? In correct code, the result of loadFromFile would have to be checked and if the assumption that asyncLoadedImages only contains loaded images was to be kept, the empty image would have to be removed from the map:

if (!asyncLoadedImages[workOrder.path].loadFromFile(...))
    asyncLoadedImages.erase(workOrder.path);

While this is a correct solution, it is inefficient (inserting and directly erasing an element) and can lead to errors if something prevents the erasure from taking place.

The new API forces the user to check for validity before inserting the (valid) object into the map thus guaranteeing everything in the map is actually valid.

Now... the reason I said I was somewhere between both camps is because I am on @danieljpetersen's side when it comes to how "ugly" the new API makes code look.

The main goal of the change was to prevent useless objects from being created, not to force std::optional usage down user's throats. The current implementation just happens to be implemented using std::optional but I think there is something we can do that satisfies the goal of the change while not making user's code too "ugly".

If we take asio's API as an example, they often provide both non-throwing and throwing variants of many functions. Obviously it is important to know if a network operation is successful or not so there is no way around getting back the result of the operation, either through an error_code reference variable or via exceptions.

What we have now is the non-throwing variant of our loading functions.

In my own projects I tend to use std::optional in cases where there is an equal probability of it having a value and not having a value. I personally would never use std::optional to signal success or failure, especially when failure rarely ever happens. In these cases I use exceptions, since you know... failing to load a file is an exceptional case that should never really happen if everything works out...

I think we can defuse some of the perceived "opinionation" by providing throwing alternatives to our loading functions for those who would rather deal with exceptions than std::optionals. I would be one of those people. The throwing constructors would have the same signature as the loading functions and throw on failure. Since the object is never fully constructed in this case, it would satisfy the same "disallow useless objects" goal as the current API without forcing users to deal with std::optionals if they don't like them.

The example code that @danieljpetersen provided would then look like:

asyncLoadedImages.try_emplace(workOrder.path, "somepath");

and

soundBufferExhale = std::make_unique<sf::SoundBuffer>(fi::os::getPathToAudioDirectory() + "exhaleTone.wav");

I've omitted the exception handling, but I think everyone gets the picture. If anything goes wrong no useless object is created, same as with the std::optional functions.

I think we should keep an open mind and take into consideration user's coding preferences. Some people might prefer exceptions, some might disable them completely, some people might love std::optional and others might unwrap them at the first opportunity possible.

I assume that both camps agree that preventing the creation of useless objects is a good thing, currently there is just disagreement on the way to get to the goal. The current implementation was written taking the subjective coding preference of a subset of users into mind. We should also provide those who prefer exceptions an alternative as well. As they say... all roads lead to Rome.

from sfml.

Not sure if closing this (which is fine, esp. regarding the v3 API freeze) also means the discussion itself is closed. (BTW: how about a Discussions section here, on GitHub? The forum may not quite be the optimal way for this; not sure about the GH Discussions feature either, though.)

Anyway, for another 2 cents (from a C++ veteran no longer professionally active, but still tinkering), just wanted to say I wholeheartedly agree with the sentiment of the OP.

Well, and with the sentiment of the devs, too... -- which hopefully indicates I'm not dogmatic either way. However, I kinda can't help but feel the recent API changes have shifted toward that direction a bit.

The new design, with all of its good intentions, by committing to C++isms, it has also (inevitably) committed to giving up part of SFML's main appeal: simplicity (that letter "S" in the name).

Also, excluding a set of pitfalls entails the cost of removing some flexibility of design choices, too.

For some toy projects (altogether <100k lines) for which I've chosen SFML-master (acknowledging breaking changes), now I'm finding myself in the need to grow a horribly kludgy -- but most importantly: with the more lenient old API design entirely unnecessary -- wrapper layer around SFML, overriding my previous commitment to just riding it raw.

Unfortunately I don't have the time to go into details, but please understand that I'm not "emotionally invested", I'm simply sharing my observation of myself as a user of SFML3.

Just one random note about a RAII-related point above, because it's still fresh in my mind:

You just need to initialize it in the constructor of your own class.

Unfortunately, as member/ancestor ctor params tend to propagate all the way down a class hierarchy, this also means that, unlike before, you can longer just create a complex app instance with an idle, empty state (with a statically designed/instantiated class structure), and then populate its components on-demand, as you go, but are now kinda forced to design everything around all-or-nothing RAII.

(Also, ctor init lists are one of the worst features of C++ (in terms of ergonomics), even compared to the feature group it belongs to: initialization, which itself is already the worst part of the language. :) )

from sfml.

@xparq Would #3134 help to address your concerns?

from sfml.