Rect::contains should be exclusive for right and bottom edges

[hoqhuuep]

This may require some discussion as it is a breaking change.

The pixels with coordinates equal to the right and bottom edges are not included for drawing operations such as fill_rect. Therefore they should not be included for the contains method either. This caught me out when using Rect::contains to check if the mouse cursor was inside a drawn rectangle, it was giving a false positive when the mouse was 1 pixel outside the rectangle to the right or bottom.

Current implementation:
https://github.com/AngryLawyer/rust-sdl2/blob/master/src/sdl2/rect.rs#L301-L306

    /// Checks whether this rect contains a given point.
    pub fn contains<P>(&self, point: P) -> bool where P: Into<(i32, i32)> {
        let (x, y) = point.into();
        let inside_x = x >= self.left() && x <= self.right();
        inside_x && (y >= self.top() && y <= self.bottom())
    }

Proposed fix (only 2 characters different, "<" rather than "<=" for right and bottom):

    /// Checks whether this rect contains a given point.
    pub fn contains<P>(&self, point: P) -> bool where P: Into<(i32, i32)> {
        let (x, y) = point.into();
        let inside_x = x >= self.left() && x < self.right();
        inside_x && (y >= self.top() && y < self.bottom())
    }

Additionally, I would expect this method to be consistent with SDL_PointInRect which is inclusive for the lower bound and exclusive for the upper bound:
https://hg.libsdl.org/SDL/file/bfddf6a4bcad/include/SDL_rect.h#l70

/**
 *  \brief Returns true if point resides inside a rectangle.
 */
SDL_FORCE_INLINE SDL_bool SDL_PointInRect(const SDL_Point *p, const SDL_Rect *r)
{
    return ( (p->x >= r->x) && (p->x < (r->x + r->w)) &&
             (p->y >= r->y) && (p->y < (r->y + r->h)) ) ? SDL_TRUE : SDL_FALSE;
}

[Cobrand]

Breaking the APi is fine, but breaking it "sneakily", meaning it doesn't change the signature but changes the behavior, is not fine at all.

Either you replace this " contains" by a contains_inclusive and add another contains_exclusive as well, or you simply add another function "contains_exclusive" which does exactly what you said, without changing the current "contains".

[hoqhuuep]

I agree 100% with your comments regarding changing the name to not sneakily break code.

Just out of curiosity, does anyone have an example of a use-case for the current behavior? In what case would you want this to be inclusive?

[hoqhuuep]

Just an image to help visualize the problem:

• The white area is the rectangle {x: 2, y: 2, width: 4, height: 4}.
• The green pixel is the point {x: 2, y: 2} this is clearly "contained" by the rectangle.
• The red pixel is the point {x: 6, y: 6} this is clearly not "contained" by the rectangle.

I feel that the name "contains_exclusive" could be misleading. What do you think about something more neutral like "includes" or "contains_point" and removing the current behavior?

[Cobrand]

That makes sense, we can delete this function and "rename" it, and add an explicit warning in the description of the new one.
That or we can just call it a bugfix, hoping it won't break too much someone's code.

I'm leaning for the first option, but I guess the second is fine as well ? It's clearly invalid behavior right there.

[AngryLawyer]

I think this counts as a bugfix, but I'm happy for a rename too.

[Cobrand]

Maybe we can think about .bottom and .right as well. Here is bottom :

/// Returns the y-position of the bottom side of this rectangle.
    pub fn bottom(&self) -> i32 {
        self.raw.y + self.raw.h
    }

In your example this function would return 6 (2+4 == 6), but as it stands I'm led to believe it would be "the y-position of the bottom side of the rectangle" and in other words "the lowest y-position of the rectangle", meaning it should return 5 instead of 6. Now which one is correct ? Depends on your use-case I'd say.

[Cobrand]

But the way, this is also the case for

    /// Returns the bottom-right corner of this rectangle.
    ///
    /// # Example
    ///
    /// ```
    /// use sdl2::rect::{Rect, Point};
    /// let rect = Rect::new(1, 0, 2, 3);
    /// assert_eq!(Point::new(3, 3), rect.bottom_right());
    /// ```
    pub fn bottom_right(&self) -> Point {
        Point::new(self.right(), self.bottom())
    }

Is the corner included in the rectangle or excluded ? Here it's told as if it would be included, but a t first sight you might want to think it's the included one.

[AngryLawyer]

Does the basic SDL2 library have any similar function? I feel this should be the same as the library we're wrapping, if at all possible :)

[Cobrand]

There is nothing of the like in SDL2, so it's up to us.

[hoqhuuep]

I am not aware of any similar function in SDL2 either.

Personally I am a fan of the lower-bound-inclusive, upper-bound-exclusive style. As such, I like the current implementation of bottom/right/bottom_right. Here are some use-cases where this behavior is nice, but I'm sure you can find others where the converse is true.

Iterating though all pixels in a Rect:

let rect = Rect::new(2, 2, 4, 4);
for y in rect.top()..rect.bottom() {
    for x in rect.left()..rect.right() {
        // ...
    }
}

Defining some non-overlapping Rects:

let rect1 = Rect::new(2, 2, 4, 4);
let rect2 = Rect::new(2, rect1.bottom(), 4, 4);
let rect3 = Rect::new(2, rect2.bottom(), 4, 4);

The last element in a Vec is "size() - 1", the last pixel in a Rect is "right() - 1".

"right = left + width" feels more natural than "right = left + width - 1".

[Cobrand]

I don't mind, but then additional documentation is required. Ideally a diagram with every method represented explicitly would be perfect.

[AngryLawyer]

Sure. In which case, lets fix Contains and add some doc comments in the meantime

[Cobrand]

@hoqhuuep what did you use to create that little diagram of yours ?

[hoqhuuep]

My WIP tile/pixel art editor (the project for which I am using rust-sdl2). Not currently available ;-)

Here are a few I made just now using MS Excel:

let rect = Rect::new(2, 1, 6, 4);

rect.left()

rect.right()

rect.top()

rect.bottom()

rect.top_left()

rect.top_right()

rect.bottom_left()

rect.bottom_right()

left and right

top and bottom

four corners

four sides

[hoqhuuep]

I think the key to it making sense is that the origin of each pixel is its top-left corner.

[Cobrand]

I think we should still add inclusive_bottom, inclusive_right, inclusive_bottom_left, inclusive_bottom_right and inclusive_top_right as well (if we take your diagram would be 4, 7, (2,4), (7,4), (7,1))

[hoqhuuep]

I can see value in that because they are the coordinates of the pixels which are stroked in a draw_rect call. Just needs to be explained well.

[mdsteele]

This issue is also affecting me. (I've been using Rect::contains() all this time assuming it was exclusive, and am now finding bugs in my game due to the fact that it isn't. Oops!)

Is anyone already working on a PR for this? If not, I can try to put one together.

[Cobrand]

This issue was made months ago, I doubt anyone is working on it right now, otherwise it would have been implemented already and I'd have received a PR about that by now. Feel free to do a PR, they are always welcome !

[mdsteele]

Looks like base SDL2 actually does have an equivalent function now, SDL_PointInRect. The docs don't specify whether it's inclusive or exclusive on the right/bottom edges, but looking at the source, it is in fact exclusive:

SDL_FORCE_INLINE SDL_bool SDL_PointInRect(const SDL_Point *p, const SDL_Rect *r)
{
    return ( (p->x >= r->x) && (p->x < (r->x + r->w)) &&
             (p->y >= r->y) && (p->y < (r->y + r->h)) ) ? SDL_TRUE : SDL_FALSE;
}

As an additional data point, the Pygame library (which also wraps SDL, but for Python) has an equivalent method (Rect.collidepoint), and that method's docs explicitly specify that it is exclusive.

So I think exclusive is correct behavior here.