Print warnings when using a Rect2 or AABB with a negative size

[aaronfranke]

Fixes #23711, fixes #25829, fixes #37617 (or at the very least, mitigates them by showing a warning).

One idea mentioned in #37617 is to make the code in Rect2 automatically correct this, but that would be bad for a number of reasons, including performance and the fact that abs() might not actually be the desired solution. I think the best idea is to print a warning, letting the user know something's up.

Note 1: I didn't actually test if the warnings show up specifically for each of the above bugs, but I would be surprised if they didn't (and if so, they can just be re-opened).

Note 2: I noticed that there were a few typos of Point2 being used in place of Point2i, I fixed them.

Part of this is mutually exclusive with #38310, so if this is merged then #38310 should be closed.

[groud]

I don't think this is a good idea. If I am not mistaking, the negative scale for Rect2 is purposefully used in sprite regions to allow for textures flips, which does make sense.

Generally, adding warning is not a good thing. They add noise for use cases where people might do something on purpose. Either the API is not good, and then we forbid negative scale (which I don't think is a good idea), either we throw an error. There should not be an "in-between" behavior where you do things almost right.

Also, if a behavior is "undefined", we should define it in the documentation. We don't fix things by just throwing a warning.

[aaronfranke]

I don't get what you mean by defining it in the documentation. For some methods, like "has_point", it fails with negative size. How is documenting "It doesn't work" better than having a warning?

We don't fix things by just throwing a warning.

Well, this is a common approach in many pieces of software. Giving up and throwing errors was one of the unique things that Unix did to stay lean and performant.

Maybe the warnings need to only show up for some methods, to allow some uses without yelling at the user? I'll add "needs testing" then.

[groud]

I don't get what you mean by defining it in the documentation. For some methods, like "has_point", it fails with negative size. How is documenting "It doesn't work" better than having a warning?

If it does not work consistently with negative scales, then it is a bug, which should be fixed. And if it has an undefined behavior, then we should instead document the behavior in the documentation. Adding a warning stating "this behavior is undefined" instead of fixing the problem itself, when the problem is easy to solve, is probably the worst solution to the problem.

Well, this is a common approach in many pieces of software. Giving up and throwing errors was one of the unique things that Unix did to stay lean and performant.

When the program crashes, it's not a problem to throw logs to explain why. When it does not crashes, like Godot does, we should avoid warning as much as possible, especially when there is a valid use case to the feature.

Maybe the warnings need to only show up for some methods, to allow some uses without yelling at the user? I'll add "needs testing" then.

No, as I said it is definitely better to implement a correct version of all functions there, and document the behavior in such case. If it needs to automatically transform the rect to a positive scale, then it should be stated in the documentation.

[mhilbrunner]

PR meeting:

1. This is good and we want to merge.
2. This should be an error, not a warning
3. It needs an unlikely added to the if checks
4. We need to make sure docs state negative sizes aren't supported
5. It should say unsupported, not undefined

Thank you for contributing!

[aaronfranke]

2. Done
3. Done
4. I wasn't sure if I should add this for each method or just on the struct description. I added it to the struct description.
5. Done

[mhilbrunner]

Merged as discussed in the PR meeting.
Thanks for the doing the changes!

[akien-mga]

Cherry-picked for 3.5.

Edit: Reverted due to regressions / false positive warnings.

[akien-mga]

There are various issues caused by this change (linked above) which warrant more discussion on whether this was a good idea.

For now I'm reverting it in 3.x (thus fixing #56509) and we can decide what to do for master (and eventually do the same for 3.x).