Reimplement legacy constructor/load/open methods and add throwing constructors

[binary1248]

This change does several things:

• Reimplements default constructors for types whose use in default state was never really problematic (e.g. caused UB)
• Attempts to provide a documented definition of the valid state of a default constructed object
• Renames several create methods to resize since this is technically what they are actually used for from a user perspective
• Renames the factory functions to createFromXYZ since this better conveys that they create new objects instead of operate on existing ones
• Reimplements loadFromXYZ/openFromXYZ methods in order to allow efficient object recycling after construction
• Adds throwing variants of constructors that take the same parameters as the factory or equivalent loadFromXYZ/openFromXYZ methods

I know... this is going to be another involved discussion, but it had to happen.

It has become obvious to me that we underestimated the impact that only providing factory methods would have on existing codebases. The factory methods are in no way a drop-in replacement for the old functionality and would force users to rewrite larger portions of their code than they might be comfortable with. Certain usage patterns that they have established over a long time might not be implementable using only factory methods. Only having access to factory methods also means that the only way they can construct instances of their own types that contain unwrapped SFML objects as members is by calling .value() within the initializer list and ending up having to handle exceptions (std::bad_optional_access) anyway. This basically forces the user to wrap their own objects in optionals and write factory functions for them as well in order to properly handle failure to load a resource. I don't feel good about SFML's API having such a "viral" nature.

struct MyStruct
{
    // How do you handle load failure here without exception handling? You can't...
    MyStruct() : texture(sf::Texture::loadFromFile(...).value())
    {
    }
    sf::Texture texture;
}

struct MyViralStruct
{
    // The only way to support initializer list initialization is by storing an optional in my own types
    // This would mean I would have to check whether it is valid every time I access it
    MyStruct() : texture(sf::Texture::loadFromFile(...))
    {
    }
    std::optional<sf::Texture> texture;
}

struct MyOtherViralStruct
{
    // Another way of solving this problem is by calling the factory outside and moving in
    MyStruct(sf::Texture&& tex) : texture(tex)
    {
    }
    sf::Texture texture;
}

// Now resource loading can only take place outside the struct/class and error handling as well

// MyOtherViralStruct makeStruct() // Nope, would require exceptions
// {
//     auto tex = sf::Texture::loadFromFile(...);
//     return MyOtherViralStruct(std::move(tex.value()));
// }

std::optional<MyOtherViralStruct> maybeMakeStruct()
{
    auto tex = sf::Texture::loadFromFile(...);
    if (tex)
        return std::make_optional<MyOtherViralStruct>(std::move(*tex));
    return std::nullopt;
}

If anyone has been paying attention, the last example is exactly how the Shader example ended up having to be rewritten in order to make use of the new factory methods. This is a very high impact change and as I already said above, nobody is going to be able to sell it as a drop-in replacement. Not everybody is willing to pay for additional safety at any cost. The examples above should also make it obvious that the alternatives forced upon the user will end up making their code noticeably more complex than what it would have been when using the old API. This is not only annoying for experienced users trying to get work done but will also significantly degrade the experience of beginners just trying to get their first object-oriented project through the door.

Also, the lack of non-factory loadFromXYZ/openFromXYZ methods means the only way to reload a resource is by moving a newly created one on top of the older one, which is very inefficient, performance-wise and memory-usage-wise.

In addition to providing throwing constructor variants of the factory methods (equivalent to #3134), this change also reimplements the default constructors and loadFromXYZ/openFromXYZ methods. The factory methods were renamed to createFromXYZ to avoid name clashes and because create better reflects that they create new objects instead of operate on existing ones.

The initial motivation of converting the loadFromXYZ/openFromXYZ methods to factory methods was as a follow-up to the error handling discussions. In retrospect, I think that too little was discussed about the actual implementation and consequences of applying the mechanical transformation to all of the affected APIs.

In my opinion, a discussion should have been had about what constitutes an "invalid object state" before the factory conversion was performed. As far as I can tell, using most of the default constructed objects affected by this change never really resulted in any undefined behaviour if the user didn't load anything into them before using them. What was lacking was a documented definition of the state an object would be in after it had been default constructed. Many users might have already known this and taken it into account over the years. For me, an object that is fully usable after default construction without causing any undefined behaviour also has to be considered fully initialized, thus providing default constructors and loadFromXYZ/openFromXYZ methods was never really two-phase initialization in a formal sense.

One might question what sense it makes to operate on "empty" objects, and you can make the argument that the library has to prevent this from happening, but if we are to follow the example of the C++ standard library, operations on "empty ranges" (i.e. where both first and last iterators are equal) are perfectly legal and don't trigger any asserts or other diagnostic warnings. Empty containers make just as much sense as "empty" SFML objects. If C++ doesn't force us to create new containers just to assign values to an existing one then neither should SFML. It just doesn't feel natural and might come across as overreaching our domain of responsibility. Sure... performing anything on empty objects will often times be the result of a programming error somewhere, but these are logic errors and that's why we still rely on humans to program and use debuggers to find these kinds of mistakes.

The 3 forms of object construction that this change provides are targeted at 3 different user groups:

• Default construction + loadFromXYZ/openFromXYZ for legacy projects and users who have their own established frameworks set up around them
• Factory methods for new projects that prefer them over throwing constructors (might not want exceptions)
• Throwing constructors for new projects that prefer unwrapped objects and don't mind exceptions

Regardless of whether the default constructors still exist or not, providing loadFromXYZ/openFromXYZ methods still provides reload functionality even for newer projects that use throwing constructors or factory methods. Because their implementation is used as the common base for both the throwing constructors and factory methods it wouldn't make sense not to offer them since the code is already there.

The examples and documentation text have not been changed and still reference the factory methods although it might need to be discussed whether throwing constructors should be mentioned as well. Default construction is deliberately not mentioned because it is not recommended for newer projects.

As with #3134 the same applies here, if we add support for our own differentiated exceptions, the user will have finer grained control over how they might want to handle them. The std::runtime_errors can be seen as place holders until we establish if and what kind of exceptions we want to support.

Closes #3134
Resolves #3120
@danieljpetersen might be interested in this.

[trustytrojan]

Great work, Binary! You're helping fix hundreds of codebases that intend on updating to SFML 3.

[ChrisThrasher]

This PR constitutes a major reversal of what we decided in #2139 so I want to recap some points of discussion from that issue thread to provide a basis for what I'll be talking about later. Pardon the wall of text. I'd like to be terse but there is a lot to say about this.

The main reason for opening #2139 was to lift the requirement of using two-phase initialization to load resources. SFML should not force users to write multiple expressions to load a resource. The options laid out in the description of #2139 provide a few alternatives that let you load a resource in one line of code. This is a good and uncontroversial motivation. Everyone in #2139 and elsewhere agrees that it's good to provide the option to load resources in one line of code. How we go about doing that is where the disagreements begin.

Implied in much of the discussion in #2139 was that default constructors would be removed. This implication was not thoroughly questioned. I think there is a lot of value in removing the default constructors for these types. These resource types (sf::Font, sf::Music, etc) have little value in their default-constructed state. The default-constructed state is a temporarily placeholder until loading happens shortly after construction. If you never do anything to these resources beyond default construction then your program likely has a bug. It's pointless to construct an sf::Font if you never load font data into it. In effect all these types were already std::optional-like. Removing that empty, default state removed a potentially buggy state from user programs. If users still want the std::optional-like behavior then they must now explicitly use std::optional to get it. I think this is a good thing but will also admit it adds friction to the API. Good friction, but friction nonetheless.

Also implied in #2139 was that the non-static member functions like loadFromFile would be removed. If you're going to remove the default constructors then it makes sense to remove the non-static loading functions. If you want to reload a resource you can load a new resource as a temporary then use move assignment to update your existing variable. No sense keeping around those non-static loading functions when the language offers us a way to reload resources.

As for the conclusion we reached, one thing I want to mention is how I was largely motivated by the exception-free history of SFML. I was not keen to be the guy to start filling SFML with exceptions since users have spent decades using SFML under the assumption that our interfaces won't throw. This would represent a fairly large subversion of expectation which could make SFML 3 harder to adopt. I'm actually sympathetic to the use of exceptions. What I take issue with is an API that requires users to write catch block for detecting common errors. I don't want users being forced to write catch blocks to detect when an image fails to load for example.

What I like about the current std::optional API is that it lets users pick how they want to handle errors. It supports explicit control flow through the use of std::optional::has_value if you want to write your own conditional to handle the null case. If you like exceptions you can use std::optional::value to either unwrap the optional or throw. This is personally what I prefer. Perhaps you take a monadic approach and use std::optional::value_or or even some fancier C++23 additions to the interface. The std::optional API is rather simple and well documented so users can easily figure out what works best for them. I like how this solution offers a lot of flexibility in a single interface.

However, as Binary has recently pointed out, the removal of these non-static loading functions has performance implications due to them enabling "efficient object recycling after construction". You cannot recycle resources as efficiently if loading a resource requires full construction of that object from scratch. For example, internal vectors of data must be allocated and moved rather than simply repopulating an existing allocation. Moving into a std::vector implies that the existing data is deallocated first. The precise performance implications are yet to be measured but it's reasonable to assume that fewer allocations is a good thing.

---

So what about the old default constructors? I still don't like them, but I understand they offer a convenience to users who aren't yet ready to start using std::optional to solve these problems. If we are going to reintroduce default constructors then there are a few implications that must be acknowledged. You have already acknowledge some of the points but I'll repeat them for emphasis.

1. We're creating types that are implicitly optional. An sf::Font is in effect a sf::MaybeFont. That object might contain actual font data but it might not. Instead of expressing optionality via a std::optional we're moving that empty state into the class.
2. Calling any member function on a default constructed resource must not result in UB in Debug builds. I can't compromise on this. This shouldn't be a difficult thing to achieve. Bonus points if all behavior is defined in Release builds as well.
3. We must explicitly document the optionality of these types. Users need to know that default constructing these types leaves them in a defined but somewhat useless state.
4. We must provide an interface to query whether a given resource is in an empty state. In a large enough program it's possible to end up in a state where you're working with a resource class and need to verify it's not-empty before using it. This brings our optional-ish resource classes more in line with std::optional, std::expected, and smart pointers which all provide an API for querying whether or not they're empty. operator bool() is a good candidate for an interface since all the standard library classes I mentioned define this. Implementing this should be straightforward.

---

Now I can talk about the current state of the PR. You have kept the existing std::optional factory function then added throwing constructors, default constructors, and non-static loading functions. You've done so in a way that avoids code duplication and have well tested everything which I appreciate! The lack of code duplication and the existence of tests makes these interfaces very reasonable to maintain. A major gripe I had with #3134 was how it required a lot of code to be duplicated in a way I found objectionable given the maintenance burden of maintaining so many duplicated code paths.

The key reason you can implement this PR without duplication is that you reintroduced the empty default state. For what it's worth that empty state doesn't have to be accessible to users for this code reuse to be possible. It merely needs to exist internally to the class. In theory we could have both throwing constructors and factory functions that call a private default constructor and private non-static loading functions. However, if we already do all the work to make these types privately default constructible and privately contain non-static loading functions then it seems reasonable to make those things public.

Let's recap

1. The current API has benefits I like
2. It's reasonable to add throwing constructors. They pair well with emplace functions in standard containers and have UX benefits over using std::optional::value.
3. Implementing throwing constructors and factory functions alongside each other without duplicating code and without catching exceptions requires the class contain an internal empty state alongside non-static initialization functions.
4. If we're adding an internal empty state and non-static loading/initialization functions then I see the value in exposing such interfaces to users for the sake of convenience and better compatibility with SFML 2 code.

The extrapolation goes farther though.
5. If we now have a default constructor with a non-static loading function, it no longer makes sense to keep the std::optional factory functions.

These two snippets are functionality identical and it doesn't make sense to have both interfaces exist alongside each other. It's too repetitious to have two interfaces that are so similar.

sf::Font font;
if (!font.openFromFile(...))
    // Handle error

auto font = sf::Font::openFromFile(...);
if (!font)
    // Handle error

---

If we are to move forward with this PR then we can't merely add to the existing API. Doing so would result in something lacking cohesion. To consider this PR is to entirely revisit and potentially revert all the decisions made in #2139. All discussion of this PR must center around whether not we got it right in #2139.

[binary1248]

I think the "missing link" between #2139 and the implementations in the respective PRs was a thorough discussion about what constitutes an "error". I am always for preventing errors using any technical means we have available to us. But to do this I have to be absolutely sure what I am trying to prevent is always going to be a provable error.

If you never do anything to these resources beyond default construction then your program likely has a bug.

As you said, it is likely the program has a bug, but that is not a high enough bar to start enforcing something 100% of the time. If there is at least 1 scenario where this kind of usage is part of a legitimate design choice then we will not be able to enforce anything. This is also the reason why it is a very hard task to implement static analysis tools to detect potential errors. As long as they keep spitting out false positives, nobody is going to use them. Saying "it looks like your code might have a bug" isn't enough, they have to be able to prove it.

Also, as long as the halting problem is still undecidable, there will never be any technical means of determining at the site of construction whether the object will actually be loaded later on in the program. I see it as the responsibility of the programmer to determine whether control flow can actually reach the point of loading and not the API.

There are times when it is uncontroversial whether a function should return an optional or not, and the conclusions reached in #2139 can be implemented in those scenarios to their fullest extent. Functions like sf::IpAddress::resolve() which by nature have a chance of failing and need a way to convey failure back to the user are natural fits for an optional. Because every value of a 32-bit IP address is a valid address, we need a mechanism to represent "not-an-address" and this is where optional comes in, to save us from having to declare sentinel values.

Carrying over the sf::IpAddress thought, if we need to use optional to represent "not-an-object" then it follows that the object representation itself is always valid as in the case of sf::IpAddress. If we have to conclude that the object representation always has to be valid, then there is no need to prohibit default construction of it. Following the sf::IpAddress example, there is no mutation we can perform on it that would make it invalid. As long as we can guarantee that there is no mutation that we can perform on any of these other objects that would all of a sudden make them "invalid" there is no point trying to define such a state. These objects will always be valid by definition.

In my opinion, the validity of an object shouldn't be defined by the usefulness of operations that can be performed on it. As long as the operations that can be performed on a given object are always defined (i.e. don't result in undefined behaviour), they will be valid from a program-is-well-defined perspective, and that is the only technically enforceable criteria we can use to decide when an object is valid or not. This would mean that "empty" objects whose operations would result in well-defined lack of side effects are also valid objects.

I have no issue with us adding additional methods that allow the user to query whether object usage would result in visible side effects or not, I just have an issue with us introducing the notion of "validity". As long as these querying methods exclude any notion of "validity" and instead refer to e.g. something being "empty" we can name them whatever makes the most sense.

For a concrete example, default constructing an sf::Image should result in the image being "empty". If loading something into it fails, the state remains unchanged i.e. "empty" if it was previously "empty" and loaded with data if it was previously loaded with data. We could also decide that failure to load results in the image becoming "empty" once more, and in this matter I have no preference as long as the behaviour is well documented.

Choosing to delay loading of resources shouldn't be categorized as a likely error. There are many legitimate scenarios where this might make sense. In order to load a resource you will have to have enough information at the site of loading to make the call to the respective loading function. If this information is only available at a later point of program execution then it should still be possible to set up your object hierarchies up front and only load in the necessary data later on.

struct CatSprites : sf::Drawable
{
    CatSprites() : sprite1(texture), sprite2(texture), sprite3(texture)
    {
        // Set up everything we can set up at this point
        // positions... colors... etc.
        // However we still don't know which cat picture the user likes
    }

    void setCatPicture(const std::filesystem::path& path)
    {
        if (!texture.loadFromFile(path))
            // handle loading failure
    }

    void draw(RenderTarget& target, RenderStates states) const override
    {
        // This is where something like .empty() would come in
        if (texture.getSize().x * texture.getSize().y == 0)
            return;

        target.draw(sprite1, states);
        target.draw(sprite2, states);
        target.draw(sprite3, states);
    }

    sf::Texture texture;
    sf::Sprite sprite1;
    sf::Sprite sprite2;
    sf::Sprite sprite3;
};

// ... somewhere in event handling ...
switch (keyPressed->code)
{
    case sf::Keyboard::Key::A:
        catSprites.setCatPicture("CatA.jpg");
        break;
    case sf::Keyboard::Key::B:
        catSprites.setCatPicture("CatB.jpg");
        break;
    default:
        break;
}

Prohibiting the user from creating an "empty" texture up front to be able to perform all the necessary set up for their drawable would force them to either store optionals or just delay construction of the object all together until the necessary information (in this case the file path) is available. It is common knowledge that in interactive applications, as much initialization should be performed as early as possible in order to avoid any kind of performance spikes during runtime.

Another nice side-effect of being able to set up RAII hierarchies even before all information is available is address stability. It is a known fact that certain objects rely on internal references to other objects and these other objects must outlive the objects that hold references to them. The easiest way to guarantee this is by dumping them into the same composite object in the proper order and initialize their relationship in the composite object constructor as shown in the example above. As long as the address of the composite object stays stable as well (either dynamically allocate it or make sure it is never moved) it is guaranteed that there will never be any lifetime issues between the texture and the sprites. This is in fact a pattern I often use in my own code when I know that lifetime issues can arise from lack of address stability. It is also the reason I don't have to worry much about this kind of a problem. Not being able to set something like this up would end up increasing my mental load because I would have to additionally track the lifetimes of my objects.

[vittorioromeo]

@binary1248: The problem with your code is that sf::Sprite and sf::Texture are incorrectly tied together at construction time rather draw time, which I attempted to fix with #3072 and #3080.

With my SFML fork that implements #3072, it's a non-issue.

struct CatSprites
{
    CatSprites()
    {
        // ...whatever you want...
    }

    void setCatPicture(const std::filesystem::path& path)
    {
        if (!(texture = sf::Texture::loadFromFile(path)))
            // handle loading failure

        sprite1.setTextureRect(texture->getRect());
        sprite2.setTextureRect(texture->getRect());
        sprite3.setTextureRect(texture->getRect());
    }

    void draw(RenderTarget& target, RenderStates states) const
    {
        // This is where something like .empty() would come in
        if (!texture.has_value())
            return;

        target.draw(sprite1, *texture, states);
        target.draw(sprite2, *texture, states);
        target.draw(sprite3, *texture, states);
    }

    std::optional<sf::Texture> texture;

    sf::Sprite sprite1;
    sf::Sprite sprite2;
    sf::Sprite sprite3;
};

Everything considered, I largely agree with @ChrisThrasher, and I do think this PR is a huge step backwards for SFML 3.x.

[ChrisThrasher]

Everything considered, I largely agree with @ChrisThrasher, and I do think this PR is a huge step backwards for SFML 3.x.

To be clear my previous comment should not be summarized as a disapproval of this PR. I'm keeping an open mind. If I am to approve this PR, I have a few requests that I have outlined. I haven't yet made up my mind.

[vittorioromeo]

Everything considered, I largely agree with @ChrisThrasher, and I do think this PR is a huge step backwards for SFML 3.x.

To be clear my previous comment should not be summarized as a disapproval of this PR. I'm keeping an open mind. If I am to approve this PR, I have a few requests that I have outlined. I haven't yet made up my mind.

To be clear myself, I did not mean to imply you disapprove of the PR. I wanted to say that I largely agree with you and that I personally strongly disapprove of this PR.

[binary1248]

The problem with your code is that sf::Sprite and sf::Texture are incorrectly tied together at construction time rather draw time

This is just a matter of opinion and not based on any currently available definition of when sf::Sprite and sf::Texture must be linked to each other. Indeed, we currently don't provide any definition of when it has to be performed, but that does not imply that preferring to do it one way over another is definitely incorrect. We also don't expect users to know how the implementation works under the hood. All a beginner user sees are 2 objects, a texture and a sprite. The documentation makes it clear that the texture has to outlive the sprite and that is just about it. The point at which the user links them together is completely up to them. Some might do it sooner and some might do it later, both are valid approaches to this problem.

The fact that the old API supported this usage for well over a decade and this wasn't even a significant issue for many users suggests that not many share your opinion that this usage is definitely incorrect and must be prohibited at any cost.

I can understand that you are an advocate for data driven design and that is fine. Everybody is entitled to their own opinions and preferences. But when one opinion declares another opinion as "invalid" or "incorrect" that is a bit too much for me.

[vittorioromeo]

But when one opinion declares another opinion as "invalid" or "incorrect" that is a bit too much for me.

Perhaps the word "incorrect" is too strong, but it's definitely "unnecessary" for the sprite to hold the texture for any longer than its draw function, because that's the only place where the texture data is accessed. This claim can be factually verified by looking at the uses of m_texture inside Sprite.cpp, so it is not a matter of opinion.

BTW, I am only bringing this up again because such unnecessary relationship does cause friction for users as seen in your code snippet example.

[binary1248]

You are basing your definition of usage on the current private implementation of sf::Sprite. This implementation is subject to change at any time in the future. When designing an API with the user in mind, I tend not to think of how the implementation will look like because that is something they won't care about anyway. Having the implementation "leak out" through the API is normally a smell for me and should be avoided when possible. Sometimes it is not completely possible, but in many cases it is.

If we were to consider a hypothetical game console, the "PlayBox64", and we wanted to support its proprietary rendering API, and this API required us to create "textures" as 2D images that are stored in the console memory and for some reason this API also only allowed us to draw textures to the screen through a "sprite" object that we also have to create and assign the texture to, then the relation between drawing to the screen and the texture completely disappears.

This hypothetical example demonstrates that your idea of delaying linking of the texture and sprite to the draw call relies too much on the way our current graphics API of choice (OpenGL) operates. If we wanted to expose the entire OpenGL mode of operation to the user through our own API then there wouldn't be much value in it except maybe as a thin C++ wrapper. Because SFML aims to be a cross-platform library that attempts to provide an API agnostic of the platform implementation we can and have to define our own concepts independent of the concrete way we will end up having to implementing them.

I remember there was a more in depth discussion about what a "sprite" even is somewhere else, so those who are interested can refer to that for further reading.

[vittorioromeo]

If we were to consider a hypothetical game console, the "PlayBox64", and we wanted to support its proprietary rendering API, and this API required us to create "textures" as 2D images that are stored in the console memory and for some reason this API also only allowed us to draw textures to the screen through a "sprite" object that we also have to create and assign the texture to

Is there any such rendering API and/or platform that (1) exists, (2) is widely used, and (3) SFML is planning to support in the future?

[Bambo-Borris]

Just wanted to chime in as a poweruser, I don't aim to speak for all but just here's my stance:

I can live without the old form of load functions, I can live with the optional loading factory functions. But this is all in aid of understanding that the end goal is std::expected as the error handling in future, as I would prefer that to exceptions. I think @binary1248 hit on an interesting idea with the address stability, and the notion that the default constructors weren't the route to bugs in the old resource class forms. I think we can provide users the address stability they desire, and move towards the std::expected error handling by bringing back the default constructors here, and retaining the optional factory functions. I think this mix is the best way to provide flexibility for the end user in the lazy loading, and stepping towards the agreed upon error handling method.

I may turn out to be in the minority here, but I would prefer not to walk towards exceptions. It's easier to opt into using exceptions when a library doesn't (merely throw when the return is false or whatever) than to opt out of the exceptions since you are forced to use a try/catch wherever the exception throwing code is present or be forced to have an unhandled exception close your application.

I agree on the intended path for error handling, I think the less frictional way to get there is to bring back default constructors here.

[binary1248]

There doesn't have to be such a platform. As I said it was purely hypothetical and meant to demonstrate we have to design an API detached from the operation of any concrete underlying implementation.

If you absolutely insist on a real-world example of an API, you need not look any further than our own Vulkan example. I invite everyone who has the time to study the entire example, but for those without much time, the relevant functions are setupDraw() and draw(). You will find there are no references to the texture in those functions because it is no longer needed when they are called.

Anyone who has used multiple graphics APIs will know that OpenGL is probably the most unique among them (in a bad way). It has its roots in the early 90s and started out only providing immediate mode rendering. OpenGL has for lack of a better word the most anti-object-oriented API of probably all of them. In order to link different aspects together you need to bind resources to different global binding points. Because the relationship between different resources is not explicitly stored by OpenGL you have this side-effect where you will have to rebind resources a lot every frame. Modern OpenGL fixed this issue a bit but the problem was always the underlying reliance on these binding points. If you look at D3D or Vulkan you will see a completely different model. One that is based on objects with relationships to each other. Khronos learned from the mistakes of OpenGL and made sure that the main bottleneck of OpenGL, changing the state of the rendering pipeline, didn't get carried over into the design of Vulkan. This is also the reason why Vulkan supports multi-threading so much better than OpenGL.

Assuming that texturing is a property of the draw call itself just because that is how OpenGL currently forces us to implement it is unwise at best and foolish at worst. As I said above, implementing our public API based on this concept is just carrying over the very mistake that Khronos managed to correct in Vulkan. If we are serious on supporting more than one rendering API in the future we have to be smarter than narrowing our perspective to what we have in front of us right now.

[ChrisThrasher]

For the sake of making this PR simpler and more internally consistent, I removed the factory functions from all classes except sf::Cursor which lacks a default constructor. It's important that users can detect loading failure without being forced to write a catch block. The ~10 commits I added will get careful squashed into the correct commit should we approve this PR.

[coveralls]

Pull Request Test Coverage Report for Build 10295468899

Details

• 458 of 522 (87.74%) changed or added relevant lines in 21 files are covered.
• 6 unchanged lines in 6 files lost coverage.
• Overall coverage increased (+0.8%) to 56.618%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/SFML/Audio/Music.cpp	45	46	97.83%
src/SFML/System/FileInputStream.cpp	17	18	94.44%
src/SFML/Graphics/Image.cpp	42	44	95.45%
src/SFML/Window/Cursor.cpp	13	15	86.67%
src/SFML/Audio/SoundBufferRecorder.cpp	0	3	0.0%
src/SFML/Graphics/RenderTexture.cpp	18	21	85.71%
src/SFML/Audio/OutputSoundFile.cpp	7	12	58.33%
src/SFML/Audio/InputSoundFile.cpp	46	53	86.79%
src/SFML/Audio/SoundBuffer.cpp	37	45	82.22%
src/SFML/Graphics/Texture.cpp	70	85	82.35%

Files with Coverage Reduction	New Missed Lines	%
include/SFML/Graphics/RenderTarget.hpp	1	83.33%
src/SFML/Graphics/RenderTexture.cpp	1	87.96%
include/SFML/Graphics/RenderTexture.hpp	1	0.0%
src/SFML/Audio/SoundBufferRecorder.cpp	1	0.0%
src/SFML/Audio/SoundStream.cpp	1	65.63%
src/SFML/Audio/Music.cpp	1	69.9%

Totals
Change from base Build 10293229079:	0.8%
Covered Lines:	11886
Relevant Lines:	19909

---

💛 - Coveralls

[vittorioromeo]

I'm still heavily against this change. There are a few misconceptions that I'd like to clear out and points that I'd like to make:

• Reimplements default constructors for types whose use in default state was never really problematic (e.g. caused UB)

  • UB is not the only concern. The concern is that an object such as sf::Music might or not be a valid music object depending on run-time control flow. Factory-based construction ensures that any existing sf::Music object is a valid object containing an usable music.

• Reimplements loadFromXYZ/openFromXYZ methods in order to allow efficient object recycling after construction

  • This is a separate change that doesn't have to affect factory-based creation. I don't think it's worth it at all (extremely unconvinced that the performance of recycling heavyweight resources such as fonts, music, soundbuffers is a problem in practice, as those should not be frequently loaded during the game loop itself), but I'm not entirely against it. It would be entirely reasonable to have:

  class Texture
  {
      [[nodiscard]] std::optional<Texture> createFromFile(/* ... */);
      [[nodiscard]] bool reloadFromFile(/* ... */);
  };

  The design above ensures that an existing sf::Texture object is always a valid texture, and also allows its internal buffers to be recycled via reloadFromFile.

• Adds throwing variants of constructors

  • This makes error checking and propagation implicit and invisible to the type system. It will never be clear whether an error is expected or not by looking at someone's SFML code. Factory-based construction makes it obvious that something can fail and forces the user to think about that failure case. If they don't care much, they can just .value(). But once they've thought about it, maybe they'd want to .value_or(defaultTexture) to keep the game running if a user-provided texture is missing. Or maybe they want to branch to some other part of the code.

  Constructors that silently throw make all of these considerations implicit and less obvious.

The PR as is it's pretty much a "over my dead body" veto for me, as I strongly believe that removing optional-based factory construction loses many practical benefits in terms of SFML program safety/stability/robustness/architecture in order to get a bit of extra convenience when writing code.

Let's think about the principles that matter in software engineering:

1. "Code is read more than it is written"
2. "Make invalid states unrepresentable"
3. "Local reasoning is the idea that the reader can make sense of the code directly in front of them, without going on a journey discovering how the code works" (from here)

Factory-based construction is in line with all the principles listed above. Removing is would be a step backwards.

Now, I am sympathetic to the issue of "resource recycling". I would consider an alternative version of this PR where:

• Construction is left untouched via the factory-based APIs we have
• "Reloading" APIs are introduced where appropriate
• Objects will always be in a non-empty, valid state after construction (where possible, of course C++ move semantics make this impossible in general)

---

Addendum: the example shown in the OP is exactly why I think that factory-based construction promotes good software engineering practices. I will mark my own comments with (VR):

struct MyStruct
{
    // How do you handle load failure here without exception handling? You can't...
    // (VR): That's a good indication that you shouldn't handle loading failure here,
    //       as loading a texture is not the responsibility of who is holding the texture.
    //       As SFML really wants to be an OOP library, let's not forget about SOLID... :)
    MyStruct() : texture(sf::Texture::loadFromFile(...).value())
    {
    }
    sf::Texture texture;
}

struct MyViralStruct
{
    // The only way to support initializer list initialization is by storing an optional in my own types
    // This would mean I would have to check whether it is valid every time I access it
    // (VR): This is a workaround that doesn't address the actual problem. Friction is
    //       present, and rightfully so, because responsiblities are being mixed together.
    MyStruct() : texture(sf::Texture::loadFromFile(...))
    {
    }
    std::optional<sf::Texture> texture;
}

struct MyOtherViralStruct
{
    // Another way of solving this problem is by calling the factory outside and moving in
    // (VR): This is the correct solution. The struct shouldn't care about how the texture
    //       was loaded/created, the struct only needs to hold/use a texture. Responsiblities
    //       are now evenly divided. Factory-based construction is promoting good software
    //       engineering practices!
    MyStruct(sf::Texture&& tex) : texture(tex)
    {
    }
    sf::Texture texture;
}

// Now resource loading can only take place outside the struct/class and error handling as well
// (VR): This is again, a great thing! Users are forced to think about error handling cases,
//       and they can now choose how to compose loading of resources and handling strategies.

// MyOtherViralStruct makeStruct() // Nope, would require exceptions
// {
//     auto tex = sf::Texture::loadFromFile(...);
//     return MyOtherViralStruct(std::move(tex.value()));
// }

// (VR): This is one example of how the loading can be done, but it's not the only one.
//       It can be much simpler in a prototype or if the user doesn't care much about error
//       handling. See below.
std::optional<MyOtherViralStruct> maybeMakeStruct()
{
    auto tex = sf::Texture::loadFromFile(...);
    if (tex)
        return std::make_optional<MyOtherViralStruct>(std::move(*tex));
    return std::nullopt;
}

// (VR): I added this to the example to show a simpler solution if the user wants to
//       create a fast prototype.
int main()
{
    MyOtherViralStruct s{sf::Texture::loadFromFile(...).value()};
    // (VR): Note that the struct here is not wrapped in an optional!
    //       Also note that there's no need to use `std::move` at all.
}

[vittorioromeo]

Heavily against removing factory-based construction and reintroducing an "empty" or "invalid" default state for SFML objects.

Would not block this PR or a separate one if a "recycling API" was added to SFML objects without impacting factory-based construction.

[ChrisThrasher]

While there are some things I really appreciate about a std::optional-based interface for object creation, it seems like the right move to continue to support these default, empty states as they 1. are not particularly harmful and 2. convenient to users.

Moving forward with this design

[eXpl0it3r]

Bunch of documentation improvements, but also a 2-3 leftover things to consider.

One question to discuss in a more general sense is, what we're going to promote by default, throwing constructors or default construction + loadFrom* functions?

[eXpl0it3r]

As discussed, we'll move forward with this.

Bool operator and potential other adjustments can come with a separate PR.