SFML community forums

General => Feature requests => Topic started by: shadowmouse on May 16, 2015, 08:07:31 pm

Title: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 16, 2015, 08:07:31 pm
I've just been looking through the API documentation and it seemed fairly strange that for the want of a single if statement, the functions can produce undefined behaviour. Would it not be very simple to check whether the pixel is in range and either put the coordinates into range, or return black? I know it would be easy for the programmer to put these checks in place before using the function, but is not the point of these functions to make it easier/shorter to do certain tasks?
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: eXpl0it3r on May 16, 2015, 08:25:56 pm
Not really.

Trying to get or set a pixel outside of the bounds is a logic error and as such should not be acceptable behavior, thus we most certainly won't just clamp the coordinates or even worse return a randomly defined color.
Additionally it would add a tiny overhead for each function call. Given that the bounds can be ensured by the programmer once, it's really not a recommended way to go.

What one might think about is adding an assert (for debug mode) which will stop the application if the coordinates are out of bound.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 16, 2015, 08:28:48 pm
I only suggested black because the usual return for something that is invalid is 0. I do think the assert would be a good idea, with the separate debug and release libraries allowing scope for that sort of thing.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Hapax on May 17, 2015, 02:56:38 am
...the usual return for something that is invalid is 0.
I don't think this is correct. Zero is a very common valid input. In fact, main() itself returns zero when it executes successfully  ;D
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Nexus on May 17, 2015, 02:03:12 pm
I do think the assert would be a good idea, with the separate debug and release libraries allowing scope for that sort of thing.
Why not? The whole point of assert is to have debug checks that are optimized away in release mode ;)

I really suggest to detect logic errors in the application (=bugs) as early as possible. Trying to hide them by returning arbitrary values (yes, 0 as well as black color is a magic constant) will just make them propagate and pop up somewhere else, where they are much harder to find. Assertions are a powerful tool, use them where appropriate.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 17, 2015, 02:18:55 pm
If I wrote an updated version of each of the functions so that they use assert, where could I submit it? Also, is there anything anywhere that says what all of the macros are that are used in things like #ifndef? I've just been looking through the git repository and didn't find anything that had anything like #ifdef SFML_DEBUG. Is SFML_DEBUG what is used to check if it is release of debug or is it something else?

EDIT: Never mind, I found it. Github's search function is remarkably useful.
Title: AW: sf::Image::getPixel()/setPixel() validity
Post by: eXpl0it3r on May 17, 2015, 03:29:31 pm
Asserts are optimized away in release mode by default.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 17, 2015, 03:53:11 pm
I added a pull request. I'm hoping that I did it right seeing as I'm new to Github. I put #ifdef SFML_DEBUG partly because it's best to be safe, and partly because I didn't see your post in time eXpl0it3r.
Title: Re: AW: sf::Image::getPixel()/setPixel() validity
Post by: Nexus on May 17, 2015, 04:38:17 pm
Strictly speaking, "release mode" is not a term in standard C++, but rather a configuration option of compilers. It does not directly affect the language.

What determines whether assertions are enabled is the NDEBUG macro. And the problem is that on several compilers, this macro is not defined in release mode by default. But that's a different issue that we might need to address in the CMake files.

shadowmouse, I don't know where you added your pull request, but not to the SFML repository ;)
And because SFML_DEBUG is just dependent on NDEBUG, adding it around assertions won't help.

By the way, this particular issue here is more of academic than actual interest: the sf::Image class stores an STL container, and every useful STL implementation already provides index checks in debug mode. So we're not really changing anything, apart from a slightly higher-level assertion.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 17, 2015, 04:41:22 pm
Ah, okay, I'll give it up then, I really should be revising rather than doing this (7 exams next week). For the record, I added it here https://github.com/SFML/SFML/pull/890
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: eXpl0it3r on May 17, 2015, 08:37:56 pm
So should it be added?
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 17, 2015, 08:52:50 pm
Personally, can I say I'm confused as to whether what I did will have actually made a difference (thanks Nexus :P), but if it will, I'd be honoured if you'd put something I wrote (even something that pitifully simple) in.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Nexus on May 17, 2015, 09:13:06 pm
What I'm not sure is: SFML currently doesn't use a lot of assertions... maybe we should add them in other places, too. Maybe Laurent knows more about that.

In my opinion we can add it; a simple assert() with 2 checks (width/height) and conforming SFML's code style should be enough ;)

Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: binary1248 on May 18, 2015, 01:49:27 am
I really don't see how adding assertions here will help a "proper" C++ programmer. It feels to me like we are doing more and more to cater to people who don't know how to develop in C++ to begin with.

I don't think that it is unreasonable to ask of people to learn how to use a debugger properly when developing in C++. By using a debugger, out-of-bounds errors such as this can be resolved in a matter of seconds, even with a complete stack trace. If you have 100 places in your code where you call getPixel()/setPixel() an assert on its own won't help you at all. You would still end up running the debugger to find out which call resulted in the assert, or, if you are a complete beginner and never heard of a debugger, just resort to going through the 100 call sites trying to figure out what caused the problem. Not only that, but any decent debugger will also show you the passed coordinates, helping you to solve the problem much earlier in the call tree and even faster than a simple assert. Debugging with a debugger would even work when linking to a release library (if the toolchain supports it, unlike MSVC although it does work for C APIs), the call stack all the way up to the call leading into the library would be shown, which is the only information that the developer needs to fix the problem.

This is all disregarding the fact that the STL containers might even have bounds checking implemented already when built in debug.

Really, people should just learn to use the debugger or stick to Java where everything is done for you without requiring any effort or knowledge. Asserts are for the developers of the library, not the users. As their name implies, they assert that certain conditions are true and protect against possible regressions that are the result of complex interactions between different code segments. They were never meant to replace proper bounds checking code. If you ask me, any triggered assert should be reported to the maintainers of the code that triggered it, since it should not have been triggered in well-behaved code in the first place and definitely should not "leak out" beyond the scope of the library/application triggering it.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Tank on May 19, 2015, 10:18:39 am
That escalated quickly. Maybe a nicer tone would be appropriate.

Whatever, to the topic: Why should assertions only be for library developers and not for users? First time I hear this. asserts are useful in Java as well; actually they are in every language.

The out-of-bounds error is not a very good example, because -- as Nexus said -- the underlying implementation will most likely(!) spit an error in debug config. However you will have a lot of cases where functions can be misused without going into an obvious error state. These are called "logic errors" -- those kind of errors you have to hunt in the debugger, often for many hours.

Assertions help with this. The developer of a library should evaluate (all) possible input values and give hints how NOT to use functions. Generally one should make it really hard to use your library in a wrong way, but if it's possible, tell the user. This has nothing to do with "C++ noobs", albeit I think that "noobs" also benefit from this.

Addings assertions to get/setPixel() still makes sense as a high-level way of catching errors in my opinion. It makes clear what the function expects as input. A note like "(valid values: 0 to width-1)" will further help, and the assertion triggers when this contract is broken by the user.

How else would you raise an error, especially if no STL container is used underneath that catches the error you missed? Exceptions are no option, because exceptions are only raised when unavoidable errors happen. Error return values? Those are completely unrelated and dangerous (even more dangerous is returning black for invalid getPixel() inputs, as there's no way to distinguish from error or success).

Last, but not least: Assertions are also useful apart from the debugger. Adding a neat little message to the assertion makes the error case expressive, as well:

assert(x < width && y < height && "Coordinate out of bounds.");

For me this is all about added code safety. Whether a "noob" or a "pro" is using a library.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 19, 2015, 10:50:44 am
Thank you Tank. I do think that even if asserts aren't going to be put into these functions, the documentation for them should say that the STL containers (usually) won't allow incorrect values, as at the moment it makes it seem like an unprotected function, and an assert with a message like that would be very useful in any function that has the possibility of taking incorrect values.

Oh, and for the record binary1248, thank you for taking the time to voice your opinions, but making people use a debugger where it is not necessary, purely because it is there, seems pointless. And also while I know I have nowhere near the experience of you guys, I do object to being (implicitly) called a c++ noob. I taught myself GCSE standard c++ when I was 10 (when c++ is regarded as too difficult to teach the higher achieving GCSE students), and I am currently doing A level to University level programming in year 11, while my class mates struggle with checking user inputs from a shell. And no, people should not be encouraged into languages like Java, because if people aren't encouraged away from c++, then we'll have more future c++ devs and so more programs written in good fast languages, rather than ending up with situations where brilliant programs (such as blender to give a single example), are unable to be used for large projects, because the developers feel more comfortable programming in slower 'easier' languages.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Tank on May 19, 2015, 11:04:43 am
Blender's written in C. ;)
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 19, 2015, 11:05:34 am
No, it uses python. Just checked on the website and it uses c, c++ and python, with the backbone being written in c and c++ and most of the functions written in python, holding your mouse over functions shows that they are all called by python functions.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Nexus on May 19, 2015, 11:22:22 am
I fully agree with Tank here.

Asserts are for the developers of the library, not the users.
Assertions are regularly used to check preconditions, both internal or external. The STL is the best example: they do those checks for the users, not for themselves.

They were never meant to replace proper bounds checking code.
How would such code look? Other error-checking mechanisms such as exceptions and error codes are associated with a runtime cost. You don't want to have an overhead every time you access an index, because as you say, this is not Java. Assertions are the ideal compromise between safety in debug and speed in release mode.

By using a debugger, out-of-bounds errors such as this can be resolved in a matter of seconds, even with a complete stack trace.
This is simply not true. Out-of-bound errors cause undefined behavior, there's no guarantee that debuggers detect them. For example, it's very well possible that there's other data after the end of the array, and by accessing an index beyond the array, you just write to that data.
struct MyStruct
{
        int a[3];
        int b[3];
};

int main()
{
        MyStruct s = {};
        s.a[3] = 12;

        // Values after assignment:
        // s.a = {0, 0, 0}
        // s.b = {12, 0, 0}
}

That's exactly the reason why high-level containers such as std::vector and std::array provide additional index checks.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Tank on May 19, 2015, 11:56:16 am
No, it uses python. Just checked on the website and it uses c, c++ and python, with the backbone being written in c and c++ and most of the functions written in python, holding your mouse over functions shows that they are all called by python functions.
Yes, it uses Python. For extensions and the game engine. The Python info in tooltips are for extension writers as some kind of documentation, so you know what to call in Python to reach the same functionality. The code behind those calls is implemented in C though.

Blender is everything, but not slow. :)
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 19, 2015, 12:00:23 pm
No, it uses python. Just checked on the website and it uses c, c++ and python, with the backbone being written in c and c++ and most of the functions written in python, holding your mouse over functions shows that they are all called by python functions.
Yes, it uses Python. For extensions and the game engine. The Python info in tooltips are for extension writers as some kind of documentation, so you know what to call in Python to reach the same functionality. The code behind those calls is implemented in C though.

Blender is everything, but not slow. :)
Ignoring the fact that this is off topic, I was part of the first ever project (that we could find out about), where we made the school trophy by scanning people in and (me) doing the modelling in blender. It crashed every computer we put it on, including an alienware aurora, that has an i7 and 16gb of ram. It was also taking about a minute per vertex move or quadrilateral creation.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: therocode on May 19, 2015, 12:14:49 pm
Ignoring the fact that this is off topic, I was part of the first ever project (that we could find out about), where we made the school trophy by scanning people in and (me) doing the modelling in blender. It crashed every computer we put it on, including an alienware aurora, that has an i7 and 16gb of ram. It was also taking about a minute per vertex move or quadrilateral creation.

I bet you that's due to implementation, not the language selection. If it was years ago, it is likely that blender back then was more unstable.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 19, 2015, 12:15:46 pm
Actually, it was a couple of months ago.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: binary1248 on May 20, 2015, 03:00:17 am
Tank was right... this did escalate quickly, but only after he said it escalated quickly. ;)

I guess it's my turn to remind people that this is the "Feature requests" sub-forum. In past feature requests it was always Laurent who ended up asking the typical "How will this benefit SFML users in general?". As soon as I do it and in the process suggest what I think is a better alternative that everybody has access to and IMHO should learn to use, everybody labels me an elitist. Just to be clear, I have no hidden agenda, I just want the best for the library, and I thought it was a valid concern considering whether adding asserts would actually have any real impact on the daily lives of SFML users. It was never my intention to point fingers at anyone in this thread. Playing devil's advocate can have such unpredictable results sometimes. ::)

You can go on and on about how assertions are great when you use them, but remember, it's not just about you. The people who have bothered posting in this thread are probably the ones who would benefit from them the least. I myself use assertions all over my code and find them great, and yet I would never consider adding them as part of the public API of any library I write. Now I ask again: Would assertions benefit those who don't bother posting on the forum when they don't have a problem with SFML? Since we can only estimate using previous experience, I would say no. You might have another opinion, but I think yours would be harder to prove than mine taking into consideration they are both based on estimates. If there is one thing that I have learnt browsing through the forum, it is that one must never underestimate the helplessness of some of its users. Now you might say: Well, those users are better off learning to avoid common mistakes and develop good habits in the first place. This kind of disproves your own point. If assertions aren't targeted at them, and more experienced users don't make the kind of mistakes that are caught by assertions that often, then who are they targeted at?

Now for my pragmatic side.

If we try to look for cases where people have sought help on the forum because of exactly this or similar "problems", then you won't find that many. Maybe because it is so trivial that even beginners are able to spot them without external help. If you are going to add assertions every time at least one person posts with a corresponding problem, then it is no different to adding a new feature every time someone needs it in their own project. If you look at assertions like genuine "feature requests" then the same conditions for their rejection would apply here. I am just trying to treat them the same.

Also, if we went ahead and added an assertion here, we would have to add them everywhere else as well to stay consistent. I'm just estimating at this point, but this is at least a few hundred lines of new code, maybe even up to or over 1000. This adds to the total maintenance cost of the library, considering that the assertions would have to be adjusted, possibly at multiple remote locations of the library any time we decide to change any pre-/post-conditions. Is this extra maintenance cost worth that little peace of mind that assertions might give us?

Like I said, there are tools ("good" debugging environments, clang with its various sanitizers, which would have already caught Nexus' example during static analysis) that are more effective at catching programmer errors than manually maintained asserts. We shouldn't have to re-invent the debugging wheel just because we find a specific concept awesome. Leave the developer aids/tools to the ones who know what and how to implement them for the end user. Having a tool that goes hand in hand with the toolchain you are using is much more powerful than anything a library writer could ever add to the library, that is of course if people use those tools in the first place.

Lastly, we mustn't forget that like everybody has already stated, assertions only have an effect in debug configurations. Think of the times when people have come to the forum for help with an obscure screenshot that proves they didn't bother building/running in debug. Also, think of the people who make use of the SFML packages on Linux systems, they won't benefit from the assertions at all since the packages rarely if ever come with a debugging configuration. Same story if people get SFML via NuGet. If we really expect people in these environments/situations to build the library themselves or download the official release archives just to benefit from assertions, we already lost the battle.

All I am asking for at this point is tangible proof that the change we are going to make is actually going to be effective (and more effective than its alternatives) by benefiting more than 1% of the typical SFML user base. Everything posted up to now has failed to meet that requirement, talking around the actual point. Provide proof and I will happily concede.

TL;DR: Unintended self-fulfilling statement, concepts are awesome if they benefit you, concepts don't have to benefit others just because they benefit you, still no real proof this change is worth adding.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Hiura on May 20, 2015, 09:49:47 am
I agree with the devil's advocate here.  :P

A crucial argument for me (it was partially covered by Binary but I'd like to put more emphasis on it): Such defense programming is good in debug mode hence why assertions are (or should be) disabled in this configuration. And when developing an application, it is my responsibility to fulfill the contracts (mostly preconditions here) imposed by the libraries I use. Using those libraries in debug mode to benefit from its assertions is not a good strategy.

From the user POV, this strategy only works properly when those assertions are properly documented in the libraries APIs. And from the implementation POV, Adding in SFML documentation that such and such functions have assertions for this and that would, as Binary explained, remove a lot of flexibility/add a lot of maintenance burden.

But beyond this technicality, using libraries' assertions is not a good strategy simply because this is no longer defensive programming: without looking at the implementation, you have no insurance that it will actually check whether preconditions hold or not.

Now, if we shift the perspective to SFML implementation, some assertions might be useful in some places such as checking private functions' contracts or communication with third-party libraries. And it might be interesting to keep that in mind, especially when working on V3.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Nexus on May 20, 2015, 10:23:24 am
Those are good points indeed, binary1248. In your earlier post, you focused on the technical and not library maintainer side, and I responded because I disagreed with statements like "assertions are not for library users" or "with debuggers/tools you can achieve the same" ;)

What I don't exactly understand is Hiura's argument: the assertions are an implementation detail, an additional safety check -- they're neither something that must be documented nor something the user can rely on. They merely catch some common mistakes in the usage.

But I agree that SFML has to be consistent here, hence my earlier "maybe we should add them in other places, too"... But I see now that I underestimated this a bit -- and in cases like this particular one and possibly more, we don't even add safety (as stated in my 2nd post).
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Hiura on May 20, 2015, 10:48:47 am
What I don't exactly understand is Hiura's argument: the assertions are an implementation detail, an additional safety check -- they're neither something that must be documented nor something the user can rely on. They merely catch some common mistakes in the usage.
My point was more about defensive programming strategy in itself: since they are not documented the user cannot rely on them (and it would be a poor decision to rely on them if they were documented -- okay, debatable opinion but not essential here) and thus has no reasons to believe there are any assertions. Hence, he doesn't have any actual benefits in using debug binaries for SFML in the (debug) configuration for his application. (*) Therefore we don't catch more common mistakes.

(*) the fact that there is a bug in SFML and using debug binaries to catch it is a different story; same goes for the "feature" in MS compilers forcing users to not mix debug/release binaries -- but I guess we all agree on that.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: shadowmouse on May 20, 2015, 11:02:37 am
I'm going to leave this feature now, as has been said, this particular case isn't particularly beneficial, my point was that I thought it was pointless to purposefully leave easily solvable chances for the function to take invalid inputs, after all, the documentation makes it sound worse that it actually is. Oh, and binary, sorry for re-escalating, I probably over-reacted to what I thought was a "get in your place, programming noob" style post. But as I said, I'll leave this before anything else I say causes a full-scale war of the programming gods.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Tank on May 21, 2015, 09:21:58 am
I myself use assertions all over my code and find them great, and yet I would never consider adding them as part of the public API of any library I write.
But why? Why are you using them yourself (out of reasons, I suppose, which bring advantages?), but not in SFML? That's something I really don't get. I mean, assertions *do a thing*, it's not like saying "I like chocolate ice cream, you like vanilla.".

Quote
Now I ask again: Would assertions benefit those who don't bother posting on the forum when they don't have a problem with SFML?
Assertions only help when you *have* a problem.

Quote
If assertions aren't targeted at them, and more experienced users don't make the kind of mistakes that are caught by assertions that often, then who are they targeted at?
They are targeted at everyone. Assertions increase code safety, so if things go wrong, anyone can benefit from it. Beginners and experts make mistakes.

Quote
Also, if we went ahead and added an assertion here, we would have to add them everywhere else as well to stay consistent.
Sure thing. It would be part of a consistent error reporting strategy.

Quote
I'm just estimating at this point, but this is at least a few hundred lines of new code, maybe even up to or over 1000. This adds to the total maintenance cost of the library, considering that the assertions would have to be adjusted, possibly at multiple remote locations of the library any time we decide to change any pre-/post-conditions. Is this extra maintenance cost worth that little peace of mind that assertions might give us?
Yep. :) For me code safety is very high up on my list of important requirements for code in general. Even if it costs effort, I'm happy to have it. It has already saved my life multiple times, which made me value it a lot more.

Quote
Like I said, there are tools that are more effective at catching programmer errors than manually maintained asserts.
Yes, they are good at detecting programming errors, but they fail miserably for logic errors. Like I said before, those are the errors that are really tough to debug. If one single assert() triggers due to malicious user input, then this already paid off.

Quote
Having a tool that goes hand in hand with the toolchain you are using is much more powerful than anything a library writer could ever add to the library, that is of course if people use those tools in the first place.
I think it ADDS to safety, not replaces it. You can use the tools on top.

Quote
Lastly, we mustn't forget that like everybody has already stated, assertions only have an effect in debug configurations.
This is indeed a problem I can see as well. The best we could do is spending a tutorial or something that explains this.

Quote
Also, think of the people who make use of the SFML packages on Linux systems, they won't benefit from the assertions at all since the packages rarely if ever come with a debugging configuration.
At least Debian-based systems all provide *-dbg packages. But in general it's an issue, yes.

Quote
All I am asking for at this point is tangible proof that the change we are going to make is actually going to be effective (and more effective than its alternatives) by benefiting more than 1% of the typical SFML user base. Everything posted up to now has failed to meet that requirement, talking around the actual point. Provide proof and I will happily concede.
I'm not sure what kind of proof you are looking for. I don't think I have to prove that assertions increase code safety. And increased code safety benefits everyone. I tend to declare this as a fact. The problem of using a debug build is a big one, and the effort of adding all assertions initially is huge. But other than that, I'm not sure what I should prove.

I'd say that this is at least not a high priority at all, and as error reporting in SFML is going to overhauled in the future at some time anyway, this is for sure a topic that we can bring up again.

@shadowmouse:
"We are not struggling, we are discussing!" -- <insert names of any parents here> ;)
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: eXpl0it3r on June 27, 2015, 04:25:13 pm
So what's the verdict here?

For now, I'm going to close the related PR (https://github.com/SFML/SFML/pull/890).
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Nexus on June 27, 2015, 04:49:20 pm
I agree with closing the PR; I don't think there's any meaningful thing to change right now.

Hiding user errors by returning arbitrary colors is clearly out of question. The assertions can be discussed again at a later stage, but then library-wide, not only for this specific example.
Title: Re: sf::Image::getPixel()/setPixel() validity
Post by: Hiura on June 27, 2015, 07:29:31 pm
+1 to that