We should bring back default constructors

[danieljpetersen]

Prerequisite Checklist

• I searched for existing issues to prevent duplicates
• I searched for existing discussions on the forum to prevent duplicates
• I understand SFML's scope and believe the feature fits it

Describe your feature request here

I wanted to share my thoughts on using some of the recent changes with SFML 3. There are positives to the changes and I know a lot of work has gone into it, but I'm going to focus on why I think these changes make the experience of using SFML worse.

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

I believe this may backfire. A large amount of people are going to remedy the (confusing, potentially hundreds of lines of dense repetitive compile errors) by changing the type to a raw pointer. I believe the bug class we're attempting to solve is something that people who understand the lifetime model behind things like textures and sprites are unlikely to make. I think it's possible that, in our effort to reduce bugs among folks who are new, we're going to push them down a path which is likely to cause even more bugs than the amount that currently exist due to misuse. I know that when I was new I would have changed it to use a raw pointer. The way I resolved this today was by using smart pointers, but this actually caused a handful of runtime crashes that I had to track down due to not having called make_unique as I overlooked a few.

I think we're attempting to solve something which is a problem on paper and not one in practice. I think we get a more miserable API in exchange. In order to get Binary's stencil buffer fix I updated my SFML branch from prior to the event change to the latest master. Here's an example of the type of thing which I think is going to trip people up.

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

asyncLoadedImages[workOrder.path] = std::move(img.value());

But the solution is using insert_or_assign, so we apparently can't use bracket operators here anymore. A small trap here is to make sure that you're not using insert and not using emplace because that's different behavior than what we originally had.

asyncLoadedImages.insert_or_assign(workOrder.path, std::move(img.value()));

This type of thing permeats the API. It's miserable and unergonomic. It's also safer, but I explicitly choose to use C++ for gamedev rather than Rust because I don't think safety is worth the cost in ergonomics and developer speed. If one wants to make a different trade off, respectfully, they should be using a different language. You cannot fix C++. It's like trying to grasp falling water.

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

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

Things become more verbose. It should actually be worse than this in terms of boilerplate because I'm of course not handling whether the optional has a value. When I say you cannot fix C++, you cannot stop users from doing things that are dumb. Many people are going to assume the item was succesfully constructed in place, look at Rust game dev code with implicit assumptions that unwrap() will work everywhere. And that's Rust where there is a much stronger culture of caring about this.

I don't think either of the two things above are deal breakers, but they are annoying. I think the worst thing about this may be that the compiler messages are awful. I'm getting 500+ lines of compiler errors where I need to hunt for context on where it's gone wrong. I sometimes don't get the actual line, I get the general gist of where it is and then need to manually find the sfml object that I need to update away from using a default constructor.

I worry that the changes increase the cognitive load and the level of knowledge you need of C++ in order to be succesful.

Discoverability is also an issue. Both for the static initialization functions, but more re the events API. I think it might be improved slightly if we renamed the template parameter (currently named T) in .is and .getIf with something more descriptive. When your cursor is in the <> brackets, IDEs will give a pop up with the template parameter name -- I think it'd be good to use this to communicate something potentially helpful to the user to let them know it expects an Event type. There is also something awkward about the way this flows now. We check whether it is a specific event, and then calling something like e = event.getIf<...>(); it's already an event, it's awkward.

I don't mind the event API changes too much because it's centralized to a single location in my code and it's fine. I suspect it may be confusing for new folks, but I think the union was also not great, and I think we may actually prevent bugs with this change.

I think removing the default constructors is a mistake though. I frankly am probably going to revert rather than continue using SFML 3. I think these changes are a net negative to me. I'm not interested in arguing, I am aware that I am unlikely to win over people with this, but I care about SFML and wanted to register my thoughts on what I think is going to be a net negative for the library. I didn't comment on the changes when they were being built because in theory they do sound great, but the experience with using the library is frankly worse than I thought it'd be.

Use Cases

.

API Example

No response

[vittorioromeo]

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.

[ChrisThrasher]

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.

[vittorioromeo]

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.

[binary1248]

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.

[xparq]

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. :) )

[binary1248]

@xparq Would #3134 help to address your concerns?