New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify documentation on Rect::contains function bounds #1151

Closed
wants to merge 2 commits into
base: master
from

Conversation

@Mischa-Alff
Contributor

Mischa-Alff commented Sep 17, 2016

Add documentation clarifying that points on a Rect's "border" will not be considered as if they were contained by the Rect.

Perhaps this behaviour could be changed? Or another function could be added? For now this should suffice.

Thanks

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 18, 2016

Member

I guess this doesn't hurt, even though inside means in it, so excluding the border (at least according to my understanding of the word).

Perhaps this behaviour could be changed?

Probably not, otherwise we can't properly handle zero-sized rectangles.

Member

mantognini commented Sep 18, 2016

I guess this doesn't hurt, even though inside means in it, so excluding the border (at least according to my understanding of the word).

Perhaps this behaviour could be changed?

Probably not, otherwise we can't properly handle zero-sized rectangles.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 18, 2016

Member

Big question for me would be whether "on the border" is even that accurate to answer with floating point inaccuracies. Same for intersections: Don't have time to check right now, but do both exclude the edges? IMO it would be a lot more natural to simply include the edges in both cases (as far as possible).

Member

MarioLiebisch commented Sep 18, 2016

Big question for me would be whether "on the border" is even that accurate to answer with floating point inaccuracies. Same for intersections: Don't have time to check right now, but do both exclude the edges? IMO it would be a lot more natural to simply include the edges in both cases (as far as possible).

@Mischa-Alff

This comment has been minimized.

Show comment
Hide comment
@Mischa-Alff

Mischa-Alff Sep 18, 2016

Contributor

Context: Someone complained about this not being more explicit and refused to do a PR, so I filmed myself making this one in 3 minutes to prove how easy it was.

However, I do agree that such functions should be more explicit in explaining whether they are inclusive or exclusive to their bounds.

As for floating point inaccuracies: that's a valid point, but I don't think it's too important either.

Not having it include the border prevents the classic case of calculating a point that should be on the border but due to floating point inaccuracies is slightly off. Including the border just makes floating point inaccuracies a "fact of life". Most of these algorithms that I have seen don't care for floating point issues and leave it up to the user to make sure their input is as they want it.

Plus, the classic solution of using an epsilon could lead to unwanted behavior for smaller numbers. Perhaps we could add an epsilon that is passed as an optional argument to each function?

We could also just simply change the code to use >= or <= for comparisons, or add functions that are inclusive to the border, and not care about floating point inaccuracies or any related problems.

Contributor

Mischa-Alff commented Sep 18, 2016

Context: Someone complained about this not being more explicit and refused to do a PR, so I filmed myself making this one in 3 minutes to prove how easy it was.

However, I do agree that such functions should be more explicit in explaining whether they are inclusive or exclusive to their bounds.

As for floating point inaccuracies: that's a valid point, but I don't think it's too important either.

Not having it include the border prevents the classic case of calculating a point that should be on the border but due to floating point inaccuracies is slightly off. Including the border just makes floating point inaccuracies a "fact of life". Most of these algorithms that I have seen don't care for floating point issues and leave it up to the user to make sure their input is as they want it.

Plus, the classic solution of using an epsilon could lead to unwanted behavior for smaller numbers. Perhaps we could add an epsilon that is passed as an optional argument to each function?

We could also just simply change the code to use >= or <= for comparisons, or add functions that are inclusive to the border, and not care about floating point inaccuracies or any related problems.

@therocode

All good. Clarifies the current behaviour of the function.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 18, 2016

Member

I'd vote for changing the behavior to include the edges, simply due to the fact noone uses a rectangle to see whether some point is on the outline. The typical use case would be inside. If one really wanted to check the outline, they'd most likely use a different approach or a second, rectangle to actually get an area rather than a direct line.

Member

MarioLiebisch commented Sep 18, 2016

I'd vote for changing the behavior to include the edges, simply due to the fact noone uses a rectangle to see whether some point is on the outline. The typical use case would be inside. If one really wanted to check the outline, they'd most likely use a different approach or a second, rectangle to actually get an area rather than a direct line.

@Mischa-Alff

This comment has been minimized.

Show comment
Hide comment
@Mischa-Alff

Mischa-Alff Sep 18, 2016

Contributor

In any case, the purpose of this PR is not to change the implementation of the .contains functions. It is to improve SFML's documentation.

Perhaps this discussion could be moved to a separate issue so we can focus on adding 6 lines of comments to SFML's source code?

Besides, any changes in SFML's implementation of the .contains functions cannot be merged until SFML 3 because of backwards compatibility. For now, better documentation is surely enough.

Contributor

Mischa-Alff commented Sep 18, 2016

In any case, the purpose of this PR is not to change the implementation of the .contains functions. It is to improve SFML's documentation.

Perhaps this discussion could be moved to a separate issue so we can focus on adding 6 lines of comments to SFML's source code?

Besides, any changes in SFML's implementation of the .contains functions cannot be merged until SFML 3 because of backwards compatibility. For now, better documentation is surely enough.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 18, 2016

Member

Yeah, true, considering the current state isn't ideal, just get this one merged first.

Member

MarioLiebisch commented Sep 18, 2016

Yeah, true, considering the current state isn't ideal, just get this one merged first.

@TankOs

TankOs approved these changes Sep 23, 2016

@TankOs TankOs added s:accepted and removed s:undecided labels Sep 23, 2016

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 23, 2016

Member

@Mischa-Alff Could you squash the commits into one? :-)

Member

mantognini commented Sep 23, 2016

@Mischa-Alff Could you squash the commits into one? :-)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 23, 2016

Member

That can be done when merging, so no longer has to be done manually.

Member

MarioLiebisch commented Sep 23, 2016

That can be done when merging, so no longer has to be done manually.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 23, 2016

Member

That can be done when merging, so no longer has to be done manually.

Except we don't use the merge button and everything is done manually...

Member

eXpl0it3r commented Sep 23, 2016

That can be done when merging, so no longer has to be done manually.

Except we don't use the merge button and everything is done manually...

@eXpl0it3r eXpl0it3r added this to the 2.4.1 milestone Sep 29, 2016

@eXpl0it3r eXpl0it3r self-assigned this Sep 29, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 29, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Sep 29, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r changed the base branch from master to 2.4.x Oct 1, 2016

@eXpl0it3r eXpl0it3r changed the base branch from 2.4.x to master Oct 1, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 1, 2016

Member

Merged in 2fca585 on branch 2.4.x.

Member

eXpl0it3r commented Oct 1, 2016

Merged in 2fca585 on branch 2.4.x.

@eXpl0it3r eXpl0it3r closed this Oct 1, 2016

@Hapaxia

This comment has been minimized.

Show comment
Hide comment
@Hapaxia

Hapaxia Nov 5, 2016

Contributor

I do not think that this new documentation is correct.

Firstly, it states that "[i]f the point lies on the border of the rect, this function will return false", which is incorrect as both minX and minY are also compared with equality.

Secondly, the concept of the rectangle is not defined by its borders/edges and therefore there is no opposite edge. The width and height clearly define the amount of area that is included. The "max" variables used in the implementation of contains are not exposed.

Example:
If a rectangle is at (0, 0) and has a width of 10x10, it clearly does contain a point at (0, 0) and is not large enough to cover a point at (10, 10).
This also shows my first point above that the point can be on the edge of the position of the rectangle (most commonly the top-left).

Contributor

Hapaxia commented Nov 5, 2016

I do not think that this new documentation is correct.

Firstly, it states that "[i]f the point lies on the border of the rect, this function will return false", which is incorrect as both minX and minY are also compared with equality.

Secondly, the concept of the rectangle is not defined by its borders/edges and therefore there is no opposite edge. The width and height clearly define the amount of area that is included. The "max" variables used in the implementation of contains are not exposed.

Example:
If a rectangle is at (0, 0) and has a width of 10x10, it clearly does contain a point at (0, 0) and is not large enough to cover a point at (10, 10).
This also shows my first point above that the point can be on the edge of the position of the rectangle (most commonly the top-left).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment