Rectangle.Contains must be exclusive on Width and Height

[jinek]

Describe the bug
Current implementation of Rectangle.Contains includes additional row and column also, which means, for instance, that zero height rectangle contains 1 horizontal line of height 1, which means that height=0 is not less than height=1, which is mathematically wrong.

        public bool Contains(Point p)
        {
            return p.X >= _x && p.X <= _x + _width &&
                   p.Y >= _y && p.Y <= _y + _height;
        }

Right and down boundaries must be exclusive, like p.Y < _y + _height rather than p.Y <= _y + _height.
One more example is rectangle with height=1. It's drawn like a line, 1 pixel height (not like two lines of 1 pixel height). If we click on it, avalonia by mistake will count next line (where no rectangle is drawn) also as a part of rectangle:

To Reproduce
Rectangle with height=1:

-------------------------
    *

---- - represents a rectangle with height=1 (0,0,20,1)
*-represents point of mouse click (4,1)
As you see, we have clicked below the rectangle, but Avalonia counts it as a click on the rectangle.

Additional context
Standard .NET class rectangle also contains wrong implementation and includes one more horizontal and one more vertical lines with width=1 in to considiration.

[jinek]

As a small addition, currently I observe that wrong element receives the mouse click. Lets say we have two buttons:
(0,0,10,10)
and next one
(0,10,10,10)
If we click on second button right at the top (5,10), first button is clicked by mistake. So, visually it looks like we are clicking second button, but first one is clicked.

[grokys]

This is the same as WPF/UWP: in these frameworks Rect.Contains is also inclusive of the right/bottom edges. For a discussion of this and why it's the way it is with regards to continuous coordinate spaces, see (in particular the reply from "Hans"):

https://social.msdn.microsoft.com/Forums/vstudio/en-US/a4f6500c-561b-4bc7-9a4f-6829cc11329f/quotcontainsquot-method-in-rect-wpf-and-rectangle-winforms?forum=wpf

Also if you look at QT, their Rect.contains is also inclusive:

https://doc.qt.io/qt-6/qrect.html#contains

In Avalonia we have recently added the ContainsExclusive method which does as you are expecting:

https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Base/Rect.cs#L262

This method should be used for hit-testing, though we've not yet updated our code to use it in all cases. Interestingly WPF also has the erroneous behavior you mention above when hit-testing controls.

Now I'd be the first to admit that the ContainsExclusive behavior makes more sense to me, but we don't want to deviate from the behavior of other platforms (or annoy the mathematicians ;) )

[jinek]

@grokys Thank you for the answer. It is a bug, and currently for me it is a game stopper.
What could be possible workarounds? May be a flag could be introduced or may be Func can be set to replace current Rect.Contains?

I also checked Hans answer. In the case which I'm describing it is nothing to do with the drawing system. I'm talking more about HitTest geometry, which uses transformed coordinates which eventually are exactly pixels, like GDI.

[robloo]

I certainly agree that if a pixel does not contain a rectangle when drawn on the screen, it should not register a hit for said rectangle at that pixel. At least that much certainly must be a bug.

[grokys]

Yeah, the bug is that Avalonia doesn't use ContainsExclusive when hit-testing in some situations. That definitely should be fixed, but this issue is about Rect.Contains being wrong, which it isn't in the sense that WPF/UWP/QT all have this behavior. There was an issue already open for the hit-testing problem that I wanted to link to, but I'm unable to find it... I'll open a new issue if I fail to find it.

[grokys]

Ah here we go: #3079

[jinek]

@grokys If we use ContainsExclusive everywhere, why to keep original? Just to keep things similar? I just don't see any applications for inclusive Contains.
To be honest, I don't completely understand Hans argument. While doing a drawing system, we have to carry about anti-aliasing and similar stuff, we don't do just simple overlaps of random shapes. And even if we did that, we don't need to use Contains for that.
My overall point is just that inclusive Contains is wrong in all senses and does not have any possible applications neither in computer science nor in math. I still can't find anything useful for that. Even Koshi math does not work with it correctly.

Relative to the topic: Rust-SDL2/rust-sdl2#569

[grokys]

Yeah, I mean I agree with you: exclusive makes a lot more sense to me, but WPF and UWP (and by extension Avalonia) use bottom/right inclusive and our general rule is to follow WPF and UWP behavior. My only argument for inclusive Contains comes from the commit which changed to use inclusive in the first place:

        public bool Contains(Rect r)
        {
            return Contains(r.TopLeft) && Contains(r.BottomRight);
        }

Which make sense as well.

Anyway, as much sense as I think bottom/right excusive makes I'd be hesitant to change this behavior now though because:

• Each time we deviate from WPF/UWP, people complain
• As a comment in the Rust-SDL issue says: "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". I realise they did change it in the end, but I'm not sure how many users they had at the time, or whether they'll have users who will be porting code from other frameworks which use a different scheme (i.e. porting from WPF/UWP as many of our users do)

[robloo]

Just a few comments, none actionable.

1. I think we all agree that registering a hit where nothing is drawn doesn't make much sense. The whole point in keeping this functionality is compatibility though which may be a good reason. I also feel like we must be missing something, it seems many other frameworks have done this... why?
2. Avalonia has deviated from other XAML frameworks when it makes sense. This might be one of those places where it is so obviously wrong it might make sense to change for 11.0 breaking changes. Not my call though and it seems like @grokys doesn't think this is warranted.
3. From a UI-framework standpoint I don't think this is a big issue. Pressing with a mouse or touch is already so imprecise it won't be noticeable for end-users. However, if the app is a game or using a custom control with heavy zoom factor this may become apparent. In those cases, I've almost always seen custom renderers though (with their own hit testing).
4. I agree breaking things silently is always very dangerous. The only question is if this is so low-profile it may slip through unnoticed by the vast majority of apps. This is where Microsoft generally checks usage analytics in GitHub.
5. I'm curious how this was noticed and why it's a problem in specific apps. Yes, I know I've taken the opposite position on such a question before.

[grokys]

Yeah, I'm really not sure why Contains is like it is though it would appear there's a mathematical reason. As you can see from the commit history, it was exclusive at one point and then I changed it to inclusive, but didn't write my reasoning in a commit message (bad Steven). The only clue was Contains(r.TopLeft) && Contains(r.BottomRight). I'm guessing I wrote that, saw it was wrong, checked WPF and noticed it was inclusive, so changed it to be like WPF.

If I'd not done that, then at this point I'd be saying "Avalonia is correct, WPF is wrong" but since I did do that, I think as you say, changing it silently at this point would be causing more trouble than its worth because it's not functionality that will obviously break for anyone already using it - it's an insidious change that will probably break things in minor ways. Of course it may also fix just as many things in minor ways, but hard to know.

So given that, I think we should fix our hit-testing to use ContainsExclusive and leave Rect.Contains to match WPF/UWP/QT. So yeah I think we're in agreement here ;)

[jinek]

5. I'm curious how this was noticed and why it's a problem in specific apps. Yes, I know I've taken the opposite position on such a question before.

In my case it was very special I think, I've been introducing mouse support here https://github.com/jinek/ToolUI
If we click buttons at the bottom then ScrollBar receives click instead.
1 pixel there is bigger than the cursor itself.