PDA

View Full Version : CL_Rect problems



rombust
11-29-2012, 07:47 AM
This post is for both ClanLib 2.3 and ClanLib 3.0 (Devel), but we cannot change CL_Rect on ClanLib 2.3 without breaking existing code.

CL_Rect (or clan::Rect ) is currently defined as follows:

Using the following rectangle containing these pixels:


XXXXXXXXXX
XX####XXX
XX####XXX
XX####XXX
XXXXXXXXXX


The CL_Rect will contain:
left (X1) = 2, top (Y1) = 1, right (X2) = 6, bottom (Y2) = 4

So X2 and Y2 are exclusive points (outside the rectangle).

CL_Rect calculates:
Type get_width() const { return right - left; }
Type get_height() const { return bottom - top; }

So, width = 4 and height = 3, as expected

However, these following functions are treating X2 and Y2 as inclusive points (inside the rectange)
"contains, is_overlapped, is_inside"

These functions do not make it clear that the right and bottom are exclusive points:
"(set/get)_top_right, (set/get)_bottom_right, (set/get)_bottom_left)"

An additional comment about "contains"


bool contains(const Vec2<Type> &p) const
{
return ((p.x >= left && p.x <= right) || (p.x <= left && p.x >= right))
&& ((p.y >= top && p.y <= bottom) || (p.y <= top && p.y >= bottom));
}

Note "p.x <= right" should be "p.x < right" etc.
Also that function is also handling the situation where X2 is less than X1 and treating it as a valid rectangle (swapping left and right). I don't think that it should.
The "normalize" function can reverse backward rectangles. (Although, I don't think that function should be there either)

Comments?

Judas
11-30-2012, 02:26 AM
Yes, this is clearly a bug. Bottom/right should be exclusive always.

Rectangles where right < left are invalid. I am not sure what is the best way to deal with such rectangles. Some possible choices:


Throw an exception if a rectangle is invalid.
Treat invalid rectangles as null rectangles.
Do not handle invalid rectangles. They are simply considered to have negative sizes.


Personally I prefer the third option since the other two will hurt performance.

I think a normalize function should convert invalid rectangles into null rectangles.

rombust
11-30-2012, 09:29 AM
Fixed in ClanLib 3.0 svn.

I used the third option