Add integer equivalents for `Rect` #7984

LiamGallagher737 · 2023-03-08T21:43:44Z

Objective

Add integer equivalents for the Rect type.

Closes #7967

Solution

Add IRect and URect

Changelog

Added IRect and URect types.

LiamGallagher737 · 2023-03-08T21:47:55Z

I have put this as a draft as I am unsure of how to implement the doc tests for the associated methods. The tests require used of the arguments passed into the macro, however I cannot find an easy way to do this. I can use the #[doc = ...] attribute to do this but it results in very unreadable code, if someone knows a better way to achieve this, please let me know.

Example of #[doc = ...] use.

bevy/crates/bevy_math/src/rect.rs

Lines 139 to 140 in 9439952

    
                       #[doc = concat!(" is empty, this method returns an empty rectangle ([`", stringify!($type_name), "::is_empty()`] returns `true`), but")] 
        
                       #[doc = concat!(" the actual values of [`", stringify!($type_name), "::min`] and [`", stringify!($type_name), "::max`] are implementation-dependent.")]

LiamGallagher737 · 2023-03-11T06:20:26Z

macro_rules! just wasn't the right tool for the job so I've resorted to just manually implementing both types.

crates/bevy_math/src/rects/irect.rs

james7132 · 2023-04-01T19:21:46Z

crates/bevy_math/src/rects/irect.rs

+    /// ```
+    #[inline]
+    pub fn from_center_size(origin: IVec2, size: IVec2) -> Self {
+        assert!(size.cmpge(IVec2::ZERO).all());


Not sure if we only want this to be a debug_assert or not. I prefer keeping asserts out of potentially hot math code. Especially since we assert in from_center_half_size as well.

LiamGallagher737 · 2023-04-02T08:11:17Z

Removed the assert for checking if the size is positive in URect's from size and half size methods as UVec2 is always positive.

bevy/crates/bevy_math/src/rects/urect.rs

Line 77 in a19968e

assert!(size.cmpge(UVec2::ZERO).all());

nicoburns · 2023-05-09T18:18:56Z

#8540 suggests creating a generic Rect<T> instead. These types could then become aliases. Might that be a better path here to avoid code duplication? We have such types in Taffy https://github.com/DioxusLabs/taffy/blob/main/src/geometry.rs and they work pretty well.

nicopap · 2023-05-09T18:41:14Z

Note that glam "solves" the issue of code duplication (think of Vec2, IVec2 and UVec2) by using the tera templating engine to generate code.

For bevy, I'd be in favor of what nicoburns describes. Having a Rect<T> and then re-exporting a pub type IRect = Rect<i32> allows having one implementation of Rect, one doc string per method, one set of methods. So when changing something Rect-related, only one place needs to be changed and checked by reviewers later.

The disadvantage is that I always found type aliases a bit confusing in rustdoc (which is what makes glam better than nalgebra imo) but I think a Rect<T> would be the better solution long term

james7132 · 2023-05-09T23:20:31Z

I think the problem with Rect<T> is the need to return glam types (i.e. Vec2, IVec2, UVec2), as well as the associated type level assertions needed for each one potentially being different. This is potentially resolvable by using a trait with associated types, but even then it's a bit rough around the edges in terms of UX when working with the types.

crates/bevy_math/src/rects/irect.rs

crates/bevy_math/src/rects/urect.rs

djeedai

Just a few minor things, but I think it's almost ready for merge. Really good contribution, thanks!

crates/bevy_math/src/rects/irect.rs

LiamGallagher737 · 2023-06-09T22:26:18Z

For URect I've created another method outset as the inset method takes a positive only integer, however to me it makes more sense for inset to create a smaller rect but the orginal f32 rect implementation has it creating a larger rect and only when a negative number was passed did it create a smaller rect.

For URect I have it so inset creates a smaller rect and outset creates a larger one but this makes it out of sync with IRect and Rect. I'm happy to change IRect to create a smaller rect with a positive inset but as Rect is already in use anyone using the inset method would now have to invert what they are passing in, I think changing this is still the correct way forward but would appreciate some input on this.

LiamGallagher737 · 2023-06-09T22:42:52Z

Did a quick poll on Discord and from this I think the best solution is to have URect's inset method take an i32

LiamGallagher737 · 2023-06-09T22:49:00Z

Someone else also pointed out than inset could return an option instead of an empty rect when a negative insets absolute value is larger than the half size. Personally I think this should be done in a separate PR as it would be a breaking change for Rect and so far, this PR isn't breaking.

Infact all panicking methods should really return an option or result, I can do this in a follow up PR

…egative insets`

djeedai · 2023-06-10T06:40:37Z

it makes more sense for inset to create a smaller rect but the orginal f32 rect implementation has it creating a larger rect and only when a negative number was passed did it create a smaller rect.

The API is inspired by Druid/Kurbo. It takes the current rectangle and adds (positive) an inset, creating a new larger rectangle.
https://docs.rs/druid/latest/druid/struct.Rect.html#method.inset

I'm happy to change IRect to create a smaller rect with a positive inset but as Rect is already in use anyone using the inset method would now have to invert what they are passing in, I think changing this is still the correct way forward but would appreciate some input on this.

I'm not. I think the current API is correct. And as you pointed it will break everyone in possibly subtle ways.

Someone else also pointed out than inset could return an option instead of an empty rect when a negative insets absolute value is larger than the half size.

This is redundant. If the user wants to know if the resulting rectangle is empty, they can call is_empty() on the result. There's no need to make things more annoying with an Option in the common case. In general for UI you want to apply all styles and options, then at the very end possibly test if empty as an optimization. Having to check the result of Option at each step makes the API really "heavy" to deal with.

crates/bevy_math/src/rects/irect.rs

crates/bevy_math/src/rects/rect.rs

crates/bevy_math/src/rects/urect.rs

djeedai · 2023-06-10T06:56:27Z

crates/bevy_math/src/rects/urect.rs

        let mut r = Self {
-            min: self.min - inset,
-            max: self.max + inset,
+            min: (self.min.as_ivec2() - inset).as_uvec2(),


I'd rather avoid the conversion to signed, although it's a bit unlikely it still loses some precision and will return wrong results for many u32 values which can't fit into an i32.

Do you think expand and shrink methods for URect is the correct solution?

Maybe, although that makes the API different from IRect and Rect which is probably confusing. But you can just calculate the new size here without conversion, just need to fall back to per-component operation because there's no good way to do it directly with the UVec2 interface I think.

Also about naming I'd expect expand to expand the current object in-place, vs. expanded to return a new expanded object.

crates/bevy_math/src/rects/urect.rs

djeedai · 2023-06-10T09:17:31Z

crates/bevy_math/src/rects/irect.rs

+
+    /// Returns self as [`Rect`] (f32)
+    #[inline]
+    pub fn as_rect(&self) -> Rect {


Very good idea 👍

crates/bevy_math/src/rects/urect.rs

djeedai

Thanks for following through, looks good for me!

alice-i-cecile

Well-made! I'm happy enough to have this in the engine even without an internal use for now: it's a natural extension and a useful primitive for game dev.

# Objective Some of the conversion methods on the new rect types introduced in #7984 have misleading names. ## Solution Rename all methods returning an `IRect` to `as_irect` and all methods returning a `URect` to `as_urect`. ## Migration Guide Replace uses of the old method names with the new method names.

…ect (#9085) # Objective Continue #7867 now that we have URect #7984 - Return `URect` instead of `(UVec2, UVec2)` in `Camera::physical_viewport_rect` - Add `URect` and `IRect` to prelude ## Changelog - Changed `Camera::physical_viewport_rect` return type from `(UVec2, UVec2)` to `URect` - `URect` and `IRect` were added to prelude ## Migration Guide Before: ```rust fn view_physical_camera_rect(camera_query: Query<&Camera>) { let camera = camera_query.single(); let Some((min, max)) = camera.physical_viewport_rect() else { return }; dbg!(min, max); } ``` After: ```rust fn view_physical_camera_rect(camera_query: Query<&Camera>) { let camera = camera_query.single(); let Some(URect { min, max }) = camera.physical_viewport_rect() else { return }; dbg!(min, max); } ```

LiamGallagher737 added 2 commits March 8, 2023 22:50

create_rect_type macro, no docs

a2c4731

Method docs

9439952

alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations labels Mar 8, 2023

LiamGallagher737 mentioned this pull request Mar 8, 2023

Add integer equivalents for Rect #7967

Closed

james7132 self-requested a review March 9, 2023 03:27

LiamGallagher737 added 2 commits March 11, 2023 19:13

Add IRect and URect

7899852

pass ci

af87270

LiamGallagher737 marked this pull request as ready for review March 11, 2023 06:20

Custom assert message for URect overflow

a19968e

james7132 requested changes Apr 1, 2023

View reviewed changes

Better asset messages

b001f5b

cargo fmt

6d2abd3

james7132 approved these changes Apr 2, 2023

View reviewed changes

djeedai suggested changes May 10, 2023

View reviewed changes

rounding comment + better assert messages

850c9f6

djeedai suggested changes May 27, 2023

View reviewed changes

crates/bevy_math/src/rects/irect.rs Show resolved Hide resolved

crates/bevy_math/src/rects/irect.rs Show resolved Hide resolved

crates/bevy_math/src/rects/irect.rs Show resolved Hide resolved

Merge branch 'bevyengine:main' into integer-rects

e34428d

LiamGallagher737 added 2 commits June 10, 2023 10:29

More rounding comments and outset for URect

d61d00c

fmt

9a2c392

Make URect inset take i32, remove outset, `add examples for n…

007cd93

…egative insets`

fmt, again

ba87c38

djeedai suggested changes Jun 10, 2023

View reviewed changes

LiamGallagher737 added 6 commits June 10, 2023 20:16

code review

063b384

more rounding notes

747cafd

URect inset without conversion

363263c

forgot fmt again

8f05a6f

Methods for convert between rect types

f3c3aed

Use glams as_* methods for rect conversions

fb56d15

djeedai reviewed Jun 10, 2023

View reviewed changes

crates/bevy_math/src/rects/urect.rs Outdated Show resolved Hide resolved

djeedai reviewed Jun 10, 2023

View reviewed changes

Saturte insted of panic for URect inset

f9fb051

djeedai reviewed Jun 10, 2023

View reviewed changes

crates/bevy_math/src/rects/urect.rs Show resolved Hide resolved

remove incorrect panic docs

1557557

djeedai approved these changes Jun 11, 2023

View reviewed changes

djeedai added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 11, 2023

alice-i-cecile approved these changes Jun 12, 2023

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jun 12, 2023

Merged via the queue into bevyengine:main with commit 942766c Jun 12, 2023

ameknite mentioned this pull request Jul 9, 2023

Return URect instead of (UVec2, UVec2) in Camera::physical_viewport_rect #9085

Merged

RCoder01 mentioned this pull request Jul 14, 2023

Rename bevy_math::rects conversion methods #9159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integer equivalents for `Rect` #7984

Add integer equivalents for `Rect` #7984

LiamGallagher737 commented Mar 8, 2023 •

edited

Loading

LiamGallagher737 commented Mar 8, 2023

LiamGallagher737 commented Mar 11, 2023

james7132 Apr 1, 2023

LiamGallagher737 commented Apr 2, 2023

nicoburns commented May 9, 2023 •

edited

Loading

nicopap commented May 9, 2023 •

edited

Loading

james7132 commented May 9, 2023

djeedai left a comment

LiamGallagher737 commented Jun 9, 2023

LiamGallagher737 commented Jun 9, 2023

LiamGallagher737 commented Jun 9, 2023 •

edited

Loading

djeedai commented Jun 10, 2023

djeedai Jun 10, 2023

LiamGallagher737 Jun 10, 2023

djeedai Jun 10, 2023

djeedai Jun 10, 2023

djeedai left a comment

alice-i-cecile left a comment

Add integer equivalents for Rect #7984

Add integer equivalents for Rect #7984

Conversation

LiamGallagher737 commented Mar 8, 2023 • edited Loading

Objective

Solution

Changelog

LiamGallagher737 commented Mar 8, 2023

LiamGallagher737 commented Mar 11, 2023

james7132 Apr 1, 2023

Choose a reason for hiding this comment

LiamGallagher737 commented Apr 2, 2023

nicoburns commented May 9, 2023 • edited Loading

nicopap commented May 9, 2023 • edited Loading

james7132 commented May 9, 2023

djeedai left a comment

Choose a reason for hiding this comment

LiamGallagher737 commented Jun 9, 2023

LiamGallagher737 commented Jun 9, 2023

LiamGallagher737 commented Jun 9, 2023 • edited Loading

djeedai commented Jun 10, 2023

djeedai Jun 10, 2023

Choose a reason for hiding this comment

LiamGallagher737 Jun 10, 2023

Choose a reason for hiding this comment

djeedai Jun 10, 2023

Choose a reason for hiding this comment

djeedai Jun 10, 2023

Choose a reason for hiding this comment

djeedai left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

Add integer equivalents for `Rect` #7984

Add integer equivalents for `Rect` #7984

LiamGallagher737 commented Mar 8, 2023 •

edited

Loading

nicoburns commented May 9, 2023 •

edited

Loading

nicopap commented May 9, 2023 •

edited

Loading

LiamGallagher737 commented Jun 9, 2023 •

edited

Loading