godot-core: builtin: reimplement Rect2i to reduce need of inner usage

[yannick-was-taken]

Addresses part of #209 .

Only doing Rect2i for now, so that suggestions only have to be applied once, instead of four times ;)

The implementations are tested (regular case + edge case where applicable) including an itest that checks equivalence between the inner type's fns and the implemented fns.

Documentation is mostly taken from Godot's docs for Rect2i, with little changes here and there.

There are a bunch of FIXMEs that need to be resolved before this is mergable: Godot's rect2i.h prints errors in some cases when the rect size is negative. In accordance with what I believe to be a design principle of gdext, the equivalent fns panic in that case (compare to cast/try_cast: default is to assume and panic?).

There could be _unchecked fns that likely should be marked unsafe, as though they would not introduce memory unsafety, they would result in "Garbage In / Garbage Out", which in Rust usually is considered unsafe, as the safe bet would be to prevent the Garbage In situation via typing, or at least make it return Option<Garbage Out>. The Option approach could be used here as well, but would change the type's API as opposed to what it is in GdScript.

So please provide input on those FIXMEs as to what you think should be done here.

[yannick-was-taken]

Also this line here was a bug, so should be cherry-picked regardless of this PR's status:

End = Position + Size
-> Size = End - Position

[Bromeon]

Thanks a lot! 🙂

Generally, I think quite some code could be simplified by thinking more in terms of vectors than individual coordinates. For example, a function Vector2i::cwise_max/cwise_min() (component-wise max/min) would make a lot of operations easier. We should also try to reuse Self::from_corners() wherever it is simpler than the other constructors.

Regarding duplicating this 4 times (int/float, 2D/3D) -- do you think it makes sense if we find an abstraction? Can also just be a macro, there is some prior art for the vectors already. But I fully agree we should iron out the Rect2i case first.

Also, a lot of these APIs have been previously built in gdnative. While we should not copy-paste code (license), let's at least use the prior art to see how APIs are designed and how special cases are handled. We can gladly deviate from it when we have a reason, but it would avoid starting from scratch and repeating the many discussions we had in previous issues (e.g. godot-rust/gdnative#867 or godot-rust/gdnative#948).

The tests are a great addition, thanks for writing them 👍

[yannick-was-taken]

Regarding duplicating this 4 times (int/float, 2D/3D) -- do you think it makes sense if we find an abstraction? Can also just be a macro, there is some prior art for the vectors already. But I fully agree we should iron out the Rect2i case first.

I agree, especially for Rect2/Rect2i this makes sense. The vector implementation should serve as a good starting point, but I guess this will be in a separate commit, once Rect2 is added to the mix.

[yannick-was-taken]

After implementing the requested changes, quite a few fns are no longer const, this is mostly blocked by const_trait_impl, which is currently experimental, but seems to be needed for using overloaded operators in const contexts. However, this shouldn't pose too much of a problem.

[Bromeon]

Regarding our coord_min/max discussion, it looks like the float Vector2/3 implementations already have min/max. These should probably be renamed -- if not for clarity, then for (future) confusion an implicitly provided Ord::min/max.

[yannick-was-taken]

Regarding our coord_min/max discussion, it looks like the float Vector2/3 implementations already have min/max. These should probably be renamed -- if not for clarity, then for (future) confusion an implicitly provided Ord::min/max.

As min/max on the float vectors did effectively the same as coord_min/coord_max just using glam, I removed the fns in favor of those two.

I agree that we should not imply that there is an (implicit) ordering on vectors, and I think the coord_ variants achieve that.

[Bromeon]

Thanks for the updates, and splitting into separate commits! 🙂

[Bromeon]

Thank you!

bors r+

[bors]

Build succeeded:

• full-ci