Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Assertions in sf::Image::getPixel()  (Read 2960 times)

0 Members and 1 Guest are viewing this topic.

Foaly

  • Sr. Member
  • ****
  • Posts: 453
    • View Profile
Assertions in sf::Image::getPixel()
« on: November 16, 2015, 08:34:02 pm »
This is a tiny thing, but a helpful one I think.
Recently more and more assertions have been added to the SFML code (see the new audio reader/writer files) and I think it's an awesome thing from a developers point of view. I have been working with the sf::Image class recently and I think the sf::Image::getPixel(x, y) method could really benefit for a debug assertion that checks if x and y are within range. Speed is what count here, so I totaly see the reason to not do bounds checks in release mode, but I think assertions in debug mode can be really helpful. The documentation only specifies that x and y are the "coordinate of pixel to get" but it doesn't specify in which range. Could be (0, size] or [0, size). (It's the first one by the way ;) ) It took me some trial & error and code reading, that I want to save the next user. Thats way I'm suggesting the assertions and mentioning the expected range in the documentation.

Jesper Juhl

  • Hero Member
  • *****
  • Posts: 1405
    • View Profile
    • Email
Re: Assertions in sf::Image::getPixel()
« Reply #1 on: November 16, 2015, 08:46:31 pm »
I personally wouldn't mind a few asserts here in debug builds - sure, why not.
But I doubt many would find it surprising that the range is 0(inclusive) to size(exclusive). That's extremely common and also matches what you get when indexing standard arrays and STL containers etc.
That C++ uses 0-based idices is something that really shouldn't surprise anyone but the most novice of C++ users and if it does SFML is the least of your problems.
But why not, we all make mistakes and off-by-one errors now and again, so a few asserts could certainly be helpful for debug builds.

I see a pull request in your near future ;-)
« Last Edit: November 16, 2015, 08:49:56 pm by Jesper Juhl »

zsbzsb

  • Hero Member
  • *****
  • Posts: 1409
  • Active Maintainer of CSFML/SFML.NET
    • View Profile
    • My little corner...
    • Email
Re: Assertions in sf::Image::getPixel()
« Reply #2 on: November 16, 2015, 08:51:53 pm »
http://en.sfml-dev.org/forums/index.php?topic=18170.0
Continue there please so we don't have multiple split discussions about the same topic.

And by the way... #890
« Last Edit: November 16, 2015, 08:54:23 pm by zsbzsb »
Motion / MotionNET - Complete video / audio playback for SFML / SFML.NET

NetEXT - An SFML.NET Extension Library based on Thor