sf::Sprite does not store sf::Texture*
#3072
Conversation
|
I'm against this change. The whole point of a sprite is to hold the vertex data AND reference the texture. |
|
Just because you can solve a lifetime issue in this way doesn't mean you should. A sprite is a textured quad. If we remove the texture reference from the class it is no longer a sprite, it's an untextured quad. As shown by the act of drawing it the normal way I think a lot of users would find this change nonsensical. I think it would make the sprite tutorial more confusing, "keep the texture alive and also remember to always supply it in the draw call". It doesn't remove the lifetime issues because if you let that texture fall out of scope, you're still going to have the white square or a dangling pointer in a draw call. Don't forget most users who have even a vague notion of how games are made will have some resource holding class, so their textures will be loaded and retained by something external to where the sprite is owned and drawn. This means that decoupling this relationship doesn't solve the lifetime woes for the practical usecase of SFML sprites & textures. |
And I'm challenging that idea. Just because it's the way it has been done in SFML for ages it doesn't mean it's the optimal API. The true purpose of a sprite is to transform and draw textured quads. To achieve that purpose, there's no need for the texture to be referenced by the sprite. |
Please do not cheery pick one part of the rationale and ignore the other reasons and thoughts I clearly listed in the PR description. It is true that solving the lifetime issue prompted this idea, but it's not the only reason why I think this is a change worth considering and it is disingenuous to imply so.
A sprite has never been a textured quad, because there's no such thing. It's a list of vertices plus transform that when drawn with an associated texture ends up producing a textured quad. The PR doesn't change that, it just makes the associated texture explicitly provided as part of the draw call, rather than storing a pointer to it in the sprite which would internally anyway pass it to the draw call.
No, it will not compile if a sprite is being drawn directly. If being drawn though a pointer/reference to a drawable, it would still be possible to inform the user that they forgot to pass the texture at runtime.
But why? It makes much more sense to me. The sprite never needed to hold onto the texture. It just needs it as drawing time. Why do we artificially couple the lifetime of the sprite and the texture when the underlying graphics pipeline does not require that? Maybe we are teaching our users the wrong mental model about graphics, which would also explain why lifetime bugs are more likely.
It doesn't have to be more confusing, I can take a stab at it later. The texture had to be kept alive for an artificially longer time before as well.
It is impossible to remove all lifetime issues in C++ as we don't have a borrow checker. However this PR makes it much less likely to run into those issues, as the lifetime of the texture is not implicitly tied to the sprite anymore. If the texture isn't in scope, you can't even name it in your draw call, preventing the undefined behaviour at compile time.
It solves exactly the problem it's trying to solve in that scenario as well. If the resource holding class is moved, which is far more likely to happen in modern C++, any existing sprites would become dangling. With this PR the resources holding class can be freely moved around in memory, as changing the address of the texture is not an issue since it will be supplied at drawing time. |
Except, that's exactly what it is. The transform is secondary to the role of this, in fact with SFML you could construct your own sprite type without supplying a transform yourself (or inheriting from https://www.sfml-dev.org/tutorials/2.6/graphics-sprite.php#vocabulary
I missed this part, my mistake.
Because the alternative is we pass this responsibility onto the user instead, because it's unavoidable. For a sprite to be drawn textured, the two must be linked together at some point in time. Whether at construction or draw doesn't mitigate all lifetime problems because you can't. So instead of the user considering the notion of "I construct my sprite with the texture that should be applied to it", this PR changes the thinking to "I construct a sprite of some bounds I get from a texture, then I wait until the draw to supply that texture".
Are we? At some stage in all games you must marry your geometry and textures, and many projects have been architected with a model akin to what we see with the current sprite API. What model are you trying to give them, the GL model that textures and geometry are distinctly different concepts? Because SFML also still does give that. The current Sprite API is just indicating to the user the idea that a sprite is a set of geometry that you apply a texture to. Once you separate the texture from its construction, it's a thing you apply a rect from a texture to.
I don't disagree here, it does do that. I just don't think the change in API is worth that, especially in the face of the practical problems I see actually face the
For this to be the case you effectively force a user to always access the texture from their resource holding via a call to get the texture on every draw call. Any user who decides to cache this call and pass the cached pointer will still encounter a lifetime issue. So this resolves an issue but merely passes the likelihood of it happening onto the users code. Or makes us prescribe how the user should interface with their own resource holding classes.
Forgotten in this remark is the fact that a partial indicator of a successful API is its ability to draw in users of various skill levels over many years and to allow those users the freedom to solve the problems they want to solve. I can assure you that, practically speaking, the To me there are more obvious problems that people who are using a lot of sprites have. Lifetime woes are far down the list. Animating sprites, and batching are far more powerful additions. The majority of users we get past the white square problem within a short timespan of working with SFML. If they were to encounter it under these changes it would be a more nefarious and less obvious case of lifetime problems. |
SFML 2.x maintainers came up with that definition. It's not set in stone, and it can be changed. Radical changes can be scary and difficult, but sometimes they're worth it. I'd argue that the value of Bundling Furthermore, it is trivial to create a
The responsibility is already on the users! They're just less aware of it, because it's easy to pass the texture in the sprite's constructor and forget about it. But that leads to misunderstanding of lifetimes and bugs.
That point in time is exactly when the sprite is being drawn. So why force the users to provide a texture before it's needed, and keep it alive for longer than it's needed?
But it does completely prevent a real set of lifetime problems that I ran into just two days ago and was hard to detect and debug for two experienced SFML maintainers and C++ developers. And, again, this is not just about lifetime. I also think it's the better design/API in general.
Yes, and we can teach that to our users. We can teach them that the sprite merely calculates and provides the geometry needed to render a texture or subrectangle of a texture. We can teach them that the point in time where the texture is required is the draw call, and not any time before that. The mental model would be: "I prepare the geometry to subsequently render a texture which will be supplied at draw time.", which is exactly what was happening behind the scenes before this PR.
I don't understand where we disagree here. Yes, geometry and textures eventually need to be married. Textures and geometry are distinctly different concepts in all graphics APIs, not just OpenGL.
It doesn't! The current sprite API is the combination of a set of geometry plus a reference to a texture that needs to be kept stable in memory. If it was "a set of geometry that you apply a texture to", when you'd be describing my PR. In my PR, the sprite is just the geometry, and the texture is applied at draw-time.
Nope, you still have to apply the texture when you actually draw the sprite. Remember, sprite=geometry.
What are the other practical problems?
I don't get it. The point of a "resource holding class" is to cache the texture. The texture gets loaded (expensive) and stored in the "resource holding class". Accessing the texture from the "resource holding class" will be a trivial, lightweight operation. If you wanted to cache the result of that lightweight operation, you only need to do so right before your draw calls, not during your construction of sprites, which massively reduces the scope: // before
class Game
{
private:
sf::Texture* cachedTexture;
sf::Sprite sprite;
public:
Game() : cachedTexture{&Resources::getTexture("t0")}, sprite{cachedTexture} { }
void draw()
{
while (/* ... */)
renderWindow.draw(sprite);
}
};
// after
class Game
{
private:
sf::Sprite sprite;
public:
void draw()
{
sf::Texture& cachedTexture = Resources::getTexture("t0");
while (/* ... */)
renderWindow.draw(sprite, cachedTexture);
}
};In fact, the more I think and write these examples, the more I like this new API.
This is not necessarily a bad thing. Good and safer APIs lead downstream APIs to also be good and safe.
I genuinely doubt that making the changes proposed in this PR would affect the productivity of SFML users and the usability of the library negatively, in fact I think that they'd still be able to solve all the problems they want to solve even more effectively, as they will now be completely shielded from a nasty class of lifetime bugs. The new API would also be more educational, as it would clearly show that the texture and sprite don't need to have the same lifetime, and that the only time where the texture is needed is the draw call, more closely mimicking modern graphics APIs and priming our users for a switch to shader-based rendering or batching via vertex arrays. Just because an API has succeeded it does not mean that it was optimal or that there is no room for improvement.
This PR doesn't aim to solve that problem, but I'd be interested in seeing what can be done in the future.
This is a deflection logic fallacy, this PR has nothing to do with animations or batching. It's not like I sacrificed working on animations or batching because I created this PR.
Can you elaborate on the last sentence? What is the "white square problem", and how would merging this PR make the situation worse...? |
|
Why don't we think out of box for this? So removing texture from sprite isn't the way. The problem you want to solve is lifetime. There don't exist good solution for this. This is simple construction and removing texture isn't helping user understand what is happening. |
I don't understand why bundling the texture pointer and the sprite together seems a good idea. Programming is about data, not trying to emulate real world concepts. How do you get draw a textured quad on the screen? At the point when you're performing the draw call, you need to supply vertices and bind the texture/shader you want to use. Currently
(2) is a recipe for lifetime bugs, complicates This PR simplifies the whole "drawing sprites" ordeal because it gives And if you really want to bundle a sprite and a texture together, you'll be able to trivially do it after this PR.
This is not the only problem I am trying to solve. I am also genuinely convinced that
Using But again, everything is getting more and more complicated because we're needlessly storing the texture pointer as a sprite data member when we only need it during the draw call. All these issues about lifetime management, resource handles, etc... just plainly disappear when you realize that the texture is only needed at the point of drawing the sprite, and not throughout the entirety of the sprite's lifetime. That is a major simplification.
I'd argue it's the opposite. Having It gave the false impression that the sprite needs to hold on to the texture for its entire lifetime, while it's only needed for the draw call. It forced users to ensure address stability of textures when it was not needed at all, complicating texture loading/management code and preventing more value-oriented programming where things are copied/moved around in memory. It's the whole OOP abuse vs DOD programming paradigm debate again... |
|
You‘re free to continue this discussion, however I won‘t be accepting such a change in SFML.
Yes, it seems this is hitting right at the core of the issue and the disconnect in the discussions. It‘s essentially also the reason why I won’t accept the proposed change for SFML. SFML is inherently built object-oriented. If a sprite has-a texture, it should reference it and it‘s not okay to provide it at a later point. You‘ll always run into this issue if you couple two classes by reference (e.g. flyweight pattern), which is a „normal“ thing to do in an OOP design. Just moving the life-time problem somewhere else (i.e. to the user code) and fundamentally changing the design at the same time, isn‘t really a solution to me. Note that SFML didn‘t invent the term sprite:
A sprite has always been about texturing. Also note that the concept/problem here doesn‘t just apply to sprite and texture:
… https://www.sfml-dev.org/tutorials/2.6/graphics-sprite.php#the-white-square-problem |
|
@eXpl0it3r: I'll reply to the rest of your post later, as I disagree with your assessment and philosophy. Just because something is "normal" in a programming paradigm, and because that specific programming paradigm has been used before, it doesn't make it the best thing to do for the quality of the library. Despite all the technical advantages I've shown with this API change, you're choosing to not even consider it because it's unfamiliar or not "normal", which is not in the best interest of our users.
Unless I am missing something, this is completely solved by this PR as well. That entire paragraph would disappear from the tutorial, because: sf::Sprite loadSprite(std::string filename)
{
sf::Texture texture;
texture.loadFromFile(filename);
return sf::Sprite(texture); // <- DOES NOT COMPILE
} And if someone really tries hard to screw up: sf::Sprite loadSprite(std::string filename)
{
sf::Texture texture;
texture.loadFromFile(filename);
return sf::Sprite(texture.getRect()); // Compiles, not UB
}
// ...
sf::Sprite s = loadSprite("...");
renderWindow.draw(s); // <- DOES NOT COMPILE -- oh, I need the texture!Also, the tutorial says:
With this PR, the emphasized part is impossible. And it says:
With this PR, that part is also moot. Another point for this PR :) Don't you see how much simplicity we gain and how much we reduce the surface area for bugs? |
I appreciate your perspective, but I believe there are a few points worth reconsidering.
SFML has been traditionally object-oriented, but the degree of adherence to OOP principles can vary. Defining Even if SFML 2.x was predominantly object-oriented, exploring other paradigms could potentially enhance the library's quality.
A sprite having a texture is one way to look at it. However, as proposed in this PR, defining a sprite as the "geometry for a textured quad" allows for more flexibility, with the texture being supplied at drawing time.
The texture is required only at the drawing point. Extending the sprite's responsibility to manage the texture for its entire lifetime can introduce unnecessary complexity and lifetime management issues.
Indeed, coupling classes by reference can introduce complexity. It's often better to avoid such coupling unless there's a compelling reason to do so.
While it's common in OOP design to couple classes by reference, we should also consider the practical implications:
Despite these considerations, insisting on coupling because it's a common OOP practice might not be the best approach. Our choices should be guided by a careful evaluation of technical pros and cons, rather than adherence to traditional practices alone.
This proposal doesn't merely shift the problem; it addresses it. By ensuring that textures are in scope during the draw call, we simplify lifetime management and reduce potential issues, as demonstrated in #3062. This new API encourages users to manage textures correctly, leading to more robust code.
Two points to consider:
Following Wikipedia's definition strictly would mean our sprite should be a two-dimensional bitmap, which aligns more with
Thank you for considering these points. |
|
You don't understand or don't want to understand my point. You're trying to introduce a new paradigm. That's what I'm against. It's not about not being "normal" or "new", it's a different paradigm and doesn't fit in the current design of SFML's API as a whole. I don't need to discuss any of the technical details, because it's not a paradigm I see for SFML at this point in time, meaning, I don't want to redesign the whole API or have it redesigned to fit this change. |
I addressed this concern in my previous comment -- I'm not introducing a new paradigm,
Given that it's the same paradigm, and as shown by the small diff of this PR it clearly doesn't require redesigning the whole API, instead just a few tweeaks to the existing one, why are you not interested at discussing the technical merits of such a change? |
|
I'm with eXpl0it3r and Bambo-Borris on this one. While it is technically possible to move the texture out of the sprite, the sprite class would lose its meaning. While the definition of "sprite" might not be the same everywhere, it typically means something like an image for drawing. If it contains only coordinates and neither contains an image or texture then the class probably shouldn't be called Sprite anymore. If you move the texture out of the sprite then you are moving one step closer to some lower level draw function like raylib's The current sprite class might not be the best approach, maybe decoupling the texture and vertices could be better, but that it probably something that is better discussed when rewriting the rendering API in SFML 4. |
Perhaps because too much energy and time is spent into changes that turn SFML from a multimedia library into a C++ testing playground. It's been feeling this way for some time now. The API change may be more correct, but it doesn't look palatable. I'd rather have my texture stored with the Sprite, even if it might cause more lifetime issues. |
What changes do you genuinely make you feel that way? Our use of C++11/14/17 features has been very conservative and discussed deeply with all maintainers.
So you admit that the API is more correct and causes fewer lifetime issues, yet you don't think it's a good idea to consider it. Are you sure you're not just giving too much value to familiarity rather than to technical advantages and safety? |
The meaning would slightly change, it would be lost, but this is a mainly subjective point.
Again, just because something is typical it doesn't mean it's optimal. But this is also subjective. You gave me an idea though. Since everyone is married to the idea that This is what I am thinking of: sf:: Texture t;
sf::SpriteGeometry sg(t.getRect());
renderWindow.draw(sf::Sprite(t, sg));Boom. Lifetime issues solved. OOP maintained. Sprite is still a sprite by definition. Thoughts? |
I have come up with an alternative design inspired by your feedback: This new design addresses the primary concerns mentioned here, solves the same lifetime issues, and still simplifies the API and implementation of sprites by a lot. |
So regardless of it being mentioned there's no desire to merge any API changes for sprite, you've opened another PR.. |
I understand that there was a concern about retaining the OOP nature of SFML, expressed by one member of the SFML team.
I have put a lot of effort into solving practical issues, such as the lifetime problems in user applications, while maintaining a safe and convenient API and simplifying our implementation. It would be great to receive some technical feedback on the actual changes proposed. Let's approach this with an open mind and focus on the technical aspects of the proposal to ensure we make the best decision for SFML. |
|
I'm also against this change. It degrades The use cases like reusing a texture rect for multiple textures are not typical in a game, in the majority of cases people use one visual representation for a sprite. For lower-level needs, Regarding lifetime, that's an issue that should be considered separately. There was already a |
|
I think we should be honest with ourselves, SFML doesn't exist in a vacuum and the competition trumps it in basically every aspect where there is functionality overlap. The only things SFML has going for itself are:
To be frank, while I wouldn't have proposed any of the recent This change however is different. This isn't something we are doing hoping that it will become the new norm and expectation from beginners in the future. Remember, we shouldn't really make it an explicit aim of SFML to teach users concepts that don't exist anywhere else just out of principle. If we assume that any new user will have a handful of libraries they are considering when trying to get a project done, they will probably end up picking the one that has the least steep learning curve which implies the least new things they have to learn. One of the cornerstones of the SFML API, that I think is one of the main selling points of the library, is its concept of "drawables". As the name implies, a drawable is something that can be drawn, no surprise there. Maybe it's just me but when someone tells me something is drawable, I expect to be able to pass it as is to the thing that actually draws it, in our case It is important we ask ourselves: who even uses If we assume that We should also try to avoid going down the route that C++ has taken oftentimes, earning it the reputation of becoming "expert friendly" and "hard to teach". While there are many ways of solving any kind of problem that arises in a C++ project, we mustn't forget that like any other programming language or programming library, C++ and SFML are merely tools to get a job done. Everybody who has used a hammer in their lives might have probably accidentally hit themselves once. Does this mean we should always put on protective gear when we want to hammer a nail into the wall? I don't think anybody would even take this into consideration because even though there is a risk of hitting themselves, putting protective gear on every time would just be too annoying to prevent that 1% chance of getting hit. Also, when one gains experience using hammers one will develop techniques that minimize the risk even more, albeit never really eliminating it. Has the white square problem happened to me? Probably, but so long ago that I can't remember where. I would have also considered myself "experienced" when it happened to me. Am I willing to jump through hoops just to minimize the risk of it ever happening again? Not really. The first step on the way to avoiding such mistakes is knowing they exist and understanding them. When you understand where they can happen you can be mindful of their existence every time you write code that involves Lifetime issues are probably one of the iconic "hardships" that anyone who touches C++ reports about, which is why other newer languages made it a priority to solve it in their own special ways. We like to preach modern C++ to our users, but also disregard the fact that there are way more landmines hidden in the standard library than in SFML. If I had to guess, a big portion of the users experiencing the "white square problem" didn't read or fully understand the documentation before starting to write code. It is not uncommon for other C/C++ libraries to have similar APIs that merely store a reference to an object you have to keep alive. If a beginner were to use When designing APIs, sure, it should always be a goal to minimize the risk of wrong usage. Sometimes it is possible without making untenable changes, sometimes sacrifices have to be made that hopefully pay off. Sometimes it is also just not worth an API change because we are making the experience of writing code the other 99% of the time less productive than it could have otherwise been. When talking about "mistakes" I like to differentiate between how they happen. For me there is a difference between "beginner mistakes" and "landmine mistakes". Beginner mistakes are those that happen due to lack of understanding or experience. We all know these kinds of mistakes because we all had to start somewhere. "Landmine mistakes" are those that cause me to fear writing specific kinds of code sections and because of this fear I end up referring to documentation and reviewing the section significantly more than other sections. We all know of these kinds of code sections. For me the "white square problem" doesn't count as a "landmine mistake". I know about it, I know exactly when it happens and thus I can avoid it by following techniques I have developed for myself over time. To the argument that this happened to experienced SFML and C++ users so it must be an issue that happens all over the place: I don't think this is representative of the majority of users and/or the way they write code. Talking about the concrete situation where this occurred, I would urge everyone not to lose sight of the context in which it happened. This happened when rewriting the examples to make (better?) use of the recent API changes. Like I said above, C++ is a very high mental load programming language. My theory is that because there were so many new things to consider when rewriting the code (moving objects around, validity of Examples should primarily be there to teach and familiarize beginners with very specific new concepts that they chose to learn. Nobody is going to force someone to stare at example code, and if they do open it and just think "wow I don't understand anything here, there is way too much stuff going on" it would not have served its purpose at all. It is ironic that this rewrite is probably what lead to this discussion in the first place because I would claim that if the rewrite made it possible for the "white square problem" to even occur, to experienced users nonetheless, then maybe it is a sign that the example has become too complex? Now that we have the philosophical stuff out of the way, we can talk a tiny bit about performance. From profiling data over the last decade, it is kind of obvious that in a typical SFML application, 2D draw performance is going to be the bottleneck almost all the time. Without getting into the technicalities of OpenGL in this discussion, one of the hot paths in from user code is calling the Over the years I have attempted to optimize graphics performance wherever I can. Doing so under the constraints of the current API is challenging to say the least. If we know that we have to cherish any kind of performance we still have, we have to be careful whenever we are making changes to the hot path that might have a direct impact on performance. It won't matter how nice the API is, if SFML applications run so poorly you wouldn't want to present it to someone else, the library would not even be up for initial consideration. I'm not saying that a single change would be enough to bring it past the tipping point, I'm just saying that 0.1% will make a difference if you have 1000 of them. If anyone is now still wondering, why wasn't Coming back to philosophy... As I said above, SFML is just one of many tools in the toolbox of someone wanting to realize the project they have in their head. The more effort we are forcing them to go through (and I don't just mean it from a writing code perspective but also learning etc.) the less attractive the library becomes amongst the competition. We have to be careful not to come over as "those SFML maintainer guys sitting in their ivory tower". The recent API changes are already something we have to sell, we don't need even more radical changes that makes people stop listening before we can even sell them the stuff we think will benefit them. We have to stop thinking we know best how to write users projects for them, that is their responsibility. While I don't want to get the whole "safety by default" debate started, I'm of the opinion that safety should be opt-in with strong recommendations everywhere you look. The problem with forcing something on someone who doesn't learn themselves to appreciate it is that they will never embrace the reason why the better alternative exists in the first place. Speaking of myself, I would never appreciate smart pointers as much as I do now if I didn't start learning C++ in the dark C++98 days where checking for leaks was more important than running tests. The reason why C++ still exists and is still actively used in newer projects is probably because its users need performance and were willing to make all the necessary tradeoffs. C++ is not and will never be used because it is a "safe language". If absolute safety is the top priority then maybe consider using something else. Why is one of C++'s strong points performance? C++ doesn't magically get a bonus from the programming language gods just because it is C++. Performance also generally doesn't come for free. The reason why experienced programmers can get high performance in C++ is because it gives them the freedom to write code in a way specially tailored and optimized for their use case. C++ was never about forcing users to write code in a specific way, that's what other languages are for, and I find it worrying that such a trend even exists in the community right now. We want SFML users to feel good about their projects, not just the result but also the code itself. They might be frustrated enough about their teacher/lecturer giving them some assignments that necessitate them learning C++ and a 2D graphics library. The last thing I want to become is another teacher making their already bad experience worse by forcing them to write code in a very specific (for them maybe even non-intuitive) way. I started out programming because I found it fun (yes even in the C++98 days). I make a living programming now, I still find it fun. I work on open source libraries in my free time because I still find it fun. Why shouldn't we try to make it just as fun for others to learn and program in C++ and SFML as well without coming off oftentimes as patronizing? Giving them the freedom to express themselves in a way they find satisfying and allowing them to make mistakes so they can learn from them and appreciate ready-made solutions is part of this philosophy. We shouldn't have to worry about the next lunar lander crashing due to safety issues in SFML, if it even comes to that I wouldn't know what to say. There is a pretty simple (in my opinion) solution to this problem that doesn't involve any changes to user code. I just never got around to implementing it because I never thought this issue would blow up like it has now. Maybe I should implement it at some point when I'm back home. |
|
@binary1248: First of all, I'd really like to thank you from the bottom of my heart for taking my proposed changes seriously, evaluating their pros/cons, and putting effort into crafting a response that clearly explains your thoughts on the matter and shows kindness and an open-minded nature.
I think our philosophical disagreement is exactly on the last bullet point you listed. As a beginner, having my application segfault inexplicably or only work under certain machines or build settings would be a very frustrating experience that would make me want to stop using SFML and C++ altogether. Having the confidence of my program very likely being correct if it compiles would reduce frustration and increase fun of fearless experimentation and refactoring. There's one great advantage that SFML 3.x has over the competition: we are adopting an API design philosophy that naturally leads programs to have fewer bugs. This is invaluable for beginners, not only because it reduces frustration as mentioned above, but because it teaches good practices that will make them more effective software engineers in any field they decide to end up working in.
The We are making a breaking and controversial change with our API design in SFML 3.x, but I do believe it's a good one. Not everyone will be happy, and not everyone will understand or agree with the underlying philosophy of providing safety by default through clever use of the C++ type system. And that's entirely fine. SFML 2.x still exists. Raylib, SDL, and other libraries still exist. And, as you mentioned, those libraries already trump over SFML. If SFML 3.x were to be a less radical update, you wouldn't convince anyone to start using it -- people happy with the C-like libraries would stick to those, and people happy with SFML 2.x would stick to that. Pushing boundaries and challenging the status quo is painful, but worth it in my opinion, even if not everyone will agree with the value of the aforementioned radical changes. I don't think anyone here has the goal of making SFML the most popular and most widely used library. My goal is to make SFML 3.x a high-quality C++17 library that is suitable as an education tool for beginners, a flexible tool for prototypes / small games, and a valid choice for larger (e.g., commercial) products. Another goal of mine is to be able to point SFML 3.x to anyone who wants to see an example of how Modern C++ abstractions can increase the quality of a library's implementation and its usability. It's all in the best interest of the users.
This change follows the same principles/philosophy as the I agree with you that it is a more radical and controversial change.
Does #3080 alleviate your concern? In that alternative design I propose (which keeps all the advantages and extra safety of the one proposed in this PR),
The changes proposed in this PR or #3080 are beneficial to both beginners and experienced users. First of all, let's get the performance topic out of the discussion -- it isn't relevant here. Any program can benefit from a few True beginners have no idea what they're doing. They do not know what a pointer really is, they have no idea what "ownership" means, no idea what "lifetime management" means. The changes proposed here protect beginners from facing frustration and creating broken programs, from wasting hours of debugging because of subtle and hard-to-detect lifetime issues. More experienced programmers like myself can also run into those same issues, particularly when refactoring existing code, or when failing to realize that RVO/NRVO was keeping the address of a The obvious drawback is that the code will need to be written in such a way that it compiles, and that might not be obvious to beginners, and might not be familiar to experienced programmers. But my argument is that it is much more worthwhile to spend the time to teach those developers a new, safer API that prevents mistakes by design rather than teaching them fickle guidelines that will never eliminate every bug and that need to be kept in one's brain as a form of mental overhead. The beginner is not going to care about how "lifetimes" work or about "ownership". They want their sprite to not be a white square and their program not to randomly crash. Once they get that compiler error that tells them "hey, you're not supposed to use sprites this way" they will have to look at the documentation, understand why The experienced developer doesn't want to keep all those guidelines that help minimize the surface area for bugs in their head -- they want to write high-quality software, minimize time wasted on bug reports and debugging, and maximize profit or fun. The design proposed here helps them achieve that. Rejecting the principle of "safety by default" is, frankly, just misguided rockstar-programmer ego.
Saying that C++ and SFML are "merely tools" is a contradiction of your prior statement regarding the importance of fun and having a pleasant experience when programming. A tool can be designed in such a way to get the job done and also be fun, welcoming, and safe to use. The hammer analogy is interesting, but I think it misses the mark. Everyone knows that they need to be careful around a hammer. They know that they can get hurt easily if they're not careful. This is not at all the case for A more appropriate analogy would be using a hammer with a very special set of nails: every nail has a 1 in 10000 chances of making your entire carpetry project fall apart right when you're showing it to your friends or selling it to a client. Would you ever use that hammer? Perhaps, but you'd take precautions. You'd try to find a way to detect those bad nails, you'd try to minimize the use of the hammer to minimize the chance of ruining your project. Now imagine someone comes along and tells you that there's a brand new hammer that completely eliminates the chance of getting a bad nail, but it has an unfamiliar shape that would take a bit of time getting used to. You could react in different ways:
I feel exactly the same way when reading the arguments against my proposed design. It feels like everyone is jumping through hoops to keep an existing design which is flawed, just because it is subjectively perceived as making more sense. I have presented a design (two, actually) that greatly minimize the possibility of lifetime issues, simplify the implementation of our classes, and educate users on the value of using the type system to catch bugs early. Sprites only need the texture when they're being drawn -- no more, no less. Yet everyone is dying to find reasons to artificially increase the time that texture needs to be associated with the sprite. Rather than embracing the natural data flow of a program and of a rendering pipeline, I've seen suggestions of using shared pointers or reference counting, or I've seen arguments that only appeal to subjective concepts of "familiarity", "meaning of a sprite", or definitions quoted on Wikipedia.
You don't have to experience the pain of the mistake yourself to understand that it can happen. A user getting a compile time error when trying to use
I completely agree -- and a high-quality library should do its best to reduce the mental load on the users. If I know that using SFML 3.x
This is true, but irrelevant. We could have a separate discussion about this, but I consider the standard library to be a very poor example of API design.
This proposal (or #3080) would prevent the white square problem by making such a program unrepresentable. They wouldn't even have a chance to experience it. It's also much more likely that a user would visit the documentation of a library if they cannot figure out how to draw a sprite or how to make their code compile, rather than after having a random crash. They'd blame themselves for the latter, spend hours debugging, and then maybe read the documentation... or just drop the project altogether.
It is true that some APIs need to store a reference to an object you have to keep alive. In those cases, there's really not much we can do to improve their safety. But One of the main reasons why
I agree with your general sentiment here. I completely disagree on the fact that this proposal or #3080 would reduce productivity -- if anything, it would increase it, because you'd know that your code is correct if it compiles, and your mental load would be diminshed. The user code changes are also very small, especially in #3080.
The issue encountered in #3062 due to The "white square problem" might as well be avoided by following specific techniques and guidelines, but again, this feels like jumping through hoops. Are you telling me you really think it's better for the problem to exist in the first place, so that we can have guidelines to avoid it, rather than not have the problem exist in the first place at all...?
I don't agree with your analysis of the situation -- the culprit was not moving objects, or validity of This is an example of how things can easily go wrong when OOP is taken as a dogma and everything has to fit into the notion of a "object", and object must be "related" through pointers and references. The way a computer works doesn't always map well with real-world concepts, and the issue you mentioned was an example of that.
This is exactly what I am trying to do with this PR or #3080, I am trying to improve the architecture of SFML 3.x to prevent this problem from happening in the first place.
The shader example definitely has some inherent complexity: it needs to load resources to initialize various effects, and each effect can fail to be initialized either due to a resource loading failure or because of lack of hardware support (e.g. geometry shaders). At the same time, the example required the effects that failed to load to still be accessible through the interactive UI, only that an unloaded example would display an error message rather than show the actual effect. If you look at the PR that simplifies the example, you'll see that I removed a lot of unnecessary layers of abstractions, and used our The reason why the lifetime issue happened was because an effect was storing both a It was by sheer luck that the address of the effects was stable and that the bug did not present itself. If anything, the refactoring exposed a weakness in the example code and -- most importantly -- in
I would love to see that, and I'd still love to hear your thoughts on #3080. I want to thank you again for the effort you've put in discussing this with me, and even if we are going to still end up disagreeing in the end, I found your perspective very valuable and I hope you can say the same for my rebuttal. I hope did not come across as rude, condescending, or confrontational as that is not my intention. Unfortunately, I am not really good at making sure that my tone is as friendly as I'd like it to be while still making it clear that I deeply believe in my philosophies and software engineering values, so please do not take any offense if my tone felt confrontational. |
|
Firstly I want to reiterate:
After some discussion in private, I just want to add, that I do understand the points made here. I do get the goal of the change. I understand the direction it takes. Yes, I'm familiar with data-driven design. Yes, I know functional programming. I'm taking this proposal and this implementation seriously, and appreciate the effort. Yet, because it's a fundamental change of direction in API design (more towards an data-driven API) and because it couldn't exist like this in a vacuum, but would affect all the other mentioned similar APIs, this is an automatic decline of the PR and change. As such, I don't feel the need to discuss any of the technical arguments, because they won't change my stance on the fundamental API design. No, making a temporary out of geometry data and a texture, which is basically nothing more than a fancy function, isn't going to suddenly change that and it certainly doesn't fulfill an OOP design. If I can't create a concrete instance of a sprite, it's not actually an "object" and doesn't follow the OOP paradigm. Do I have a solution for the life-time issue then? No, but I'm willing to maintain the status quo for SFML 3. I'd potentially even see that it could serve as a good teaching tool about C++ lifetime and scope for new C++ users, which lets face it, are about the only users who will struggle with this. My recommendation: Let's close these PRs and this topic for now and consider the overall design for SFML 4. |
|
I have not had time to weigh in on this but have been keeping up with this thread. To keep my response brief, I agree with Exploiter. Ultimately I do not wish to see this change be merged. |
|
No consensus. |
(Based on #3067.)
Perhaps a radical API change, but I think it's a good one, and possibly a new direction for the
sf::Drawablefamily of classes as a whole.The purpose of
sf::Spriteis to draw an instance of a texture or a sub-texture without requiring to manually create vertices nor mess with transformation matrices.Prior to this PR,
sf::Spriterequired aconst sf::Texture&to be passed in during construction, and the sprite would hold onto a pointer to that texture.I believe that is a suboptimal API and design choice, because:
sf::Spriteonly needs direct access to thesf::Texturewhile it is being drawn, so storing it internally is unnecessary and increases the size ofsf::Sprite.Interally storing a
const sf::Texture*creates a nasty avenue for lifetime bugs, that even experienced developers like myself and @ChrisThrasher can easily miss due to how subtle they can be. See RewriteShader.cppexample #3062 as an example of such an issue caused by totally innocent-looking code and refactoring.The same
sf::Spritecould be used to draw different textures with the same transformations and sub-rectangles without having to tie the sprite itself to a specific texture, thus increasing its flexibility and usefulness.The obvious "drawback" is that now the texture has to be passed explicitly when drawing the sprite, via
sf::RenderStates. This is not really a drawback, because:It makes previous subtle lifetime bugs completely impossible, as the texture needs to be available at the point where the draw call is made.
It removes a confusing layer of abstraction from our users that used to provide a wrong mental model regarding how textures and sprites interact.
There is still a lot of room for improvement to make this PR much better. I am thinking of:
Removing theDone! It is much clearer now.sf::Sprite(const sf::Texture&)convenience constructor altogether, and simply adding asf::Texture::getRect() -> IntRectthat conveniently provides the rectangle required for drawing the texture, reducing confusion and making it more obvious that the sprite does not store/use the texture at all.Asserting or printing a warning message when aDone! It was very easy to catch at compile-time and there is no loss of functionality.sf::Spriteis drawn onto asf::RenderTargetwith an unset texture, making it obvious to the user that they forgot to pass the texture. Maybe this could be caught at compile time...?I am also thinking that this rationale might apply to other SFML drawables, such as the
sf::Text+sf::Fontpair.Thoughts?