PixelRect Contains can be dangerous as implementation is not what I am expecting

[dbriard]

Just found the cause of my Access Violation exception: the implementation of PixelRect Contains is not what I was expecting...
In my program I am displaying the color of the pixel at the mouse position,
and so as I always did, I get the position of the mouse, convert it to int (PixelPoint), and checked if this point is in the PixelRect of the image with image.Bounds.Contains(point) and BAM, random crashes...

After I added more checks, I found the cause:

public bool Contains(PixelPoint p)
{
    return p.X >= X && p.X <= Right && p.Y >= Y && p.Y <= Bottom;
}

public bool ContainsExclusive(PixelPoint p)
{
    return p.X >= X && p.X < X + Width &&
           p.Y >= Y && p.Y < Y + Height;
}

And the existence of ContainsExclusive.

To my mind, the implementation of Contains shoud be the one of ContainsExclusive, as it is what I generally see in other frameworks.
For example in System.Drawing.Rectangle:

public readonly bool Contains(int x, int y) => X <= x && x < X + Width && Y <= y && y < Y + Height;
public readonly bool Contains(Point pt) => Contains(pt.X, pt.Y);

There is only 6 and 1 references to those methods in Avalonia so that could be a small change

Something like:
ContainsExclusive become Contains
Contains become ContainsInclusive
that would be more intuitive and safe for new users.

[dbriard]

In addition, I am not sure that the use of Contains (inclusive) is correct in Avalonia,
for example in WindowImplBase:

var monitor = Screen.AllScreens.OrderBy(x => x.Scaling)
        .FirstOrDefault(m => m.Bounds.Contains(Position));

should probably be ContainsExclusive?

in fact I do not see any use case for Contains (inclusive)???

[MrJul]

Whatever is done here should also be applied to Rect.Contains for consistency.
I believe this came from the WPF implementation, which includes the bottom right point inside the rectangle.

All other frameworks I checked (Winforms, Skia, Godot, android.graphics, java.awt, and probably 95% of graphics frameworks in existence) all use an exclusive contains, which I agree is more intuitive.

While it's a breaking change to change Contains as proposed - since people might be using it for its inclusive behavior - I believe that most usages (even in the Avalonia solution) are probably like yours, assuming it's inclusive, and actually buggy.

My personal opinion is that this should be changed in v12.

[dbriard]

@MrJul Sound good for V12 as it is a breaking change.

About WPF, there is no Contains method in Int32Rect. There is a Contains method for Rect (double) but it is ok to include right/bottom point as coordinates are double. The correct implementation for PixelRect can be found in System.Drawing's Rectangle.

[jp2masa]

This was probably copied from Rect and not adapted correctly. For discrete coordinates, there's only one definition of Contains that makes sense: the exclusive one.

[MrJul]

I'd be very wary to have different implementations between Rect.Contains and PixelRect.Contains.
From a very quick look, most Rect.Contains usage in Avalonia should probably be exclusive too.

[Meloman19]

In my opinion, the Rect.Contains works as expected. If I create Rect1 = (0, 0, 10, 10) and Rect2 = (9, 9, 10, 10), then I expect that Rect1 will include Rect2.

The problem here is the definition of PixelPoint. PixelPoint is not really a point, but a rectangle with a size of 1 by 1! If you replace PixelPoint with PixelRect with a size (1x1), then the PixelRect.Contains(PixelRect realPixel) will work exactly as expected (for PixelPoint).

I think it's enough to add the "size" of the pixel to the PixelRect.Contains(PixelPoint) and PixelRect.ContainsExclusive(PixelPoint) methods.

[kekekeks]

PixelPoint/PixelSize/PixelRect structs were intended to be used as device coordinates (i. e. as something that's not affected by transformations or DPI and maps directly to the pixel grid) with the same semantics as our normal rects.

We should probably rename Contains to ContainsInclusive and keep Contains as [Obsolete] for compatibility, as it's currently rather confusing.

Rect itself will stay the same, but we might add WPF semantics for emptiness at some point (or just use Rect? instead).

[MrJul]

In my opinion, the Rect.Contains works as expected. If I create Rect1 = (0, 0, 10, 10) and Rect2 = (9, 9, 10, 10), then I expect that Rect1 will include Rect2.

Yes, and there's no ambiguity there, Rect2 is contained into Rect1 whichever Contains definition you use.

Note that we're not talking at all about Rect.Contains(Rect) in this issue which is exclusive of the bottom and right outside edges, and works intuitively.

However Rect.Contains(Point) is inclusive of these edges.

That's why I argue that both PixelRect and Rect should be changed, as it doesn't make much sense to have the Rect overload works differently from the Point one. No other graphics framework in existence I could find other than WPF and Avalonia use the exclusive definition for their floating point Rect.Contains(Point).

Here's a visual sample:

var point = new Point(5, 1);
var blueRect = new Rect(0, 0, 5, 3);
var redRect = new Rect(point, new Size(1, 1));

Console.WriteLine("Blue contains Point? " + blueRect.Contains(point));
Console.WriteLine("Red contains Point?  " + redRect.Contains(point));
Console.WriteLine("Blue contains Red?  " + blueRect.Contains(redRect));

Output:

Blue contains Point? True
Red contains Point?  True
Blue contains Red?  False

This result doesn't make much sense if the grid represents pixels (PixelRect), and everybody seems to agree with that.
If you now see these two rectangles as 2 control bounds (Rect), the behavior doesn't suddenly make more sense to me.

Rect itself will stay the same

Therefore, I strongly believe we should adjust both in the same way, either changing Contains directly, or making ContainsInclusive instead as proposed, but for both types of rectangles.

[Meloman19]

This result doesn't make much sense if the grid represents pixels (PixelRect), and everybody seems to agree with that. If you now see these two rectangles as 2 control bounds (Rect), the behavior doesn't suddenly make more sense to me.

But this result is correct. Default Contains must work as inclusive.
Both rectangles intersect only with edges - that's true. One rectangle not include to other - that's true. The point is included in both rectangles - this is also true.

In geometry, Point determined by the intersection of two curves. In this case, it is the intersection of a straight line (5, 0)-(5, 1) and (5, 1)-(5, 2). I.e. it is the intersection of the edges of these two rectangles. At the same time, the edges of the rectangles are part of them. I.e. the rectangles do not include each other, but both include point.

The main problem is in the structure name!
@dbriard expected that PixelPoint is behave like a pixel (i.e. describe a pixel on the screen). However, in Avalonia code PixelPoint is behave just like IntPoint (integer Point). But a pixel is not a point. Geometrically, it is a square (or rectangle).

Here's a visual sample:

You've just drawn a perfect example of a pixel grid. In your case, the red square is PixelPoint. And blue is a some of PixelRect.

And I see only two ways of fix here:

• If everyone agrees with the author that PixelPoint describes a screen pixel. Then it is enough to fix Contains method as follows according to the real geometric definition, then the method will work as needed:

public bool Contains(PixelPoint p)
{
    return p.X >= X && p.X + 1 <= Right && p.Y >= Y && p.Y + 1 <= Bottom;
}

That is, replace it with the code from ContainsExclusive:

public bool Contains(PixelPoint p)
{
    return p.X >= X && p.X < Right && p.Y >= Y && p.Y < Bottom;
}

But ContainsExclusive must be deleted.
Also needed fix Contains(PixelRect r):

public bool Contains(PixelRect r)
{
    return r.TopLeft.X >= X && r.TopLeft.X <= Right && r.TopLeft.Y >= Y && r.TopLeft.Y <= Bottom
        && r.BottomRight.X >= X && r.BottomRight.X <= Right && r.BottomRight.Y >= Y && r.BottomRight.Y <= Bottom;
}

And most importantly, there is no need to change the behavior for Rect and Point.

• If the main meaning of PixelPoint and PixelRect is just an integer Point and Rect. Then you need to rename these types so that no one will have any misunderstandings in the future. For example, PixelPoint to IntPoint, PixelRect to IntRect and PixelSize to IntSize.
  And in this case, you don't need to change anything except the name. Because Contains will have to work inclusively as well. And @dbriard will need to rewrite the code taking into account the actual definition of these structures.

P.S. Moreover, Rect.ContainsExclusive(Point p) should exclude all edges, not just the right and bottom ones. This method is currently used in HitTest. And it is not clear why the left edge passes hit test, but right edge - not.

[dbriard]

I confirm that there is no problem with Point (float) and Rect (float).
When using float, I expect the point is a coordinate with a size of zero.
And when using PixelPoint, I expect that it is coordinate in a matrix of pixels, so for a rect of size 10, indices goes from 0 to 9, and PixelPoint 10,10 is not contained by a rect of 10x10.

[grokys]

Related, for context: #8594