Skip to content

Conversation

@mnmaita
Copy link
Member

@mnmaita mnmaita commented May 21, 2024

Objective

Solution

  • Renamed the inset() method in Rect, IRect and URect to inflate().
  • Added EMPTY constants to all Rect variants, represented by corners with the maximum numerical values for each kind.

Migration Guide

  • Replace Rect::inset(), IRect::inset() and URect::inset() calls with inflate().

@alice-i-cecile alice-i-cecile requested a review from viridia May 21, 2024 12:31
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels May 21, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much clearer. Thanks for tackling this.

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 21, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, not quite ready. The docs on inflate (just above the changes) for both Rect and IRect need to be revised.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it D-Trivial Nice and easy! A great choice to get started with Bevy labels May 21, 2024
@mnmaita
Copy link
Member Author

mnmaita commented May 21, 2024

Ah I see, not quite ready. The docs on inflate (just above the changes) for both Rect and IRect need to be revised.

Thanks for taking a look at this @alice-i-cecile. Since the behavior is the same, I wonder how I could make the docs clearer. Would it be better to refer simply to a "value" that makes the Rect bigger when positive, instead of using the word "inset"? If so, maybe I could rename the parameter in the methods too. Any other suggestions?

@alice-i-cecile
Copy link
Member

Existing docs:

    /// Create a new rectangle with a constant inset.
    ///
    /// The inset is the extra border on all sides. A positive inset produces a larger rectangle,
    /// while a negative inset is allowed and produces a smaller rectangle. If the inset is negative
    /// and its absolute value is larger than the rectangle half-size, the created rectangle is empty.

Proposed replacement:

    /// Create a new rectangle by expanding it evenly on all sides.
    ///
    /// A positive expansion value produces a larger rectangle,
    /// while a negative expansion value produces a smaller rectangle.
    /// If this would result in zero or negative width or height, [`Rect::EMPTY`] is returned instead.

And then change the param name to expansion :) Does that sound reasonable to you?

@mnmaita
Copy link
Member Author

mnmaita commented May 21, 2024

@alice-i-cecile applied the suggested changes and also realized that the test could be renamed too.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 21, 2024
Comment on lines +22 to +27
/// An empty `IRect`, represented by maximum and minimum corner points
/// with all `i32::MAX` values.
pub const EMPTY: Self = Self {
max: IVec2::MAX,
min: IVec2::MAX,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use the maximum value? The docs seem to indicate that negative or zero sizes will return this empty rect, but a user might assume that in those cases its size would be limited to zero, not jump to the max value.

Some users may be storing positional data and stuff in these rects, meaning that jumping to the max value could be an issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the behavior of the method was not changed, so it's not that we're explicitly returning these constants. It's a good point though, I just followed the requirements from the original issue. Might be good to open a new one if this is something we should address later, maybe?

Copy link
Contributor

@viridia viridia May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EMPTY rect should be one in which max is set to MIN and min is set to MAX. In other words, it's a rectangle of infinite negative size.

This is a common practice in many other geometry libraries. The reasoning is simple: an empty rectangle is often used when calculating the union of a series of rectangles (that is, the outer bounds of all of them.). The union of a finite rect A with a negative infinite rect B is just A. So we can start with an empty rect and successively union with each rectangle in turn, using a classic "reduce" or "fold" method.

Note that the EMPTY rect is not the same as a zero-size rect. The rect that is the result of too much negative inflation is not the same as EMPTY because it is still centered at the point of the original rect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I like that convention. We should change to that and clearly document it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mnmaita this might be better saved for a followup PR. Not sure we want to change these kinds of semantics in what's supposed to be a rename PR.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 21, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 21, 2024
Merged via the queue into bevyengine:main with commit f9da5ee May 21, 2024
@mnmaita mnmaita deleted the mnmaita/rename-rect-inset branch May 23, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename Rect.inset() to Rect.inflate()

5 participants