Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Box::from_min_max method #3322

Closed
wants to merge 2 commits into from
Closed

Conversation

rezural
Copy link
Contributor

@rezural rezural commented Dec 14, 2021

Objective

  • This adds from_min_max to the Box shape type, to make non-centred Boxes easier.
  • This is helpful for creating a visual representation of an axis aligned bounding box.

Solution

  • Add the method, which recieves min: Vec3, max: Vec3

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 14, 2021
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Thanks, looks useful.

One concern I have is e.g. if you pass (1, 0, 1), (0, 1, 0), then the given box would potentially be weird. I don't know if there are any troubles with this case (which can also be reached manually or when using Box::new with negative numbers).

I wonder if it would make more sense to either rename it, or to automatically work out the minimum for each axis. It might be worth renaming new to from_side_lengths?

To be honest, I think Box could easily just be defined as min: Vec3, max: Vec3. I'm not sure why it wasn't defined as such in #883.

@DJMcNab DJMcNab added A-Utils Utility functions and types C-Feature A new feature, making something new possible S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Dec 14, 2021
@rezural
Copy link
Contributor Author

rezural commented Dec 14, 2021

Yeah, I can code to work out the minimum, or asserts perhaps. It perhaps violates principal of least surprise to swap out if the minimums. Perhaps returning an Option is the best option.

However, this kind of stuff is already in the shape primitives PR, and perhaps effort is best concentrated there..?

Cheers

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.

@rezural I like this, but I'd like to see:

  1. Basic doc strings.
  2. Assertions that min < max in all cases.

@james7132
Copy link
Member

I'd rather just use Vec3::min and Vec3::max instead of asserts. Both are likely faster than asserts, being branchless on all platforms.

@james7132 james7132 added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Oct 20, 2022
@james7132
Copy link
Member

Closing in favor of #6672.

@james7132 james7132 closed this Nov 18, 2022
bors bot pushed a commit that referenced this pull request Nov 21, 2022
# Objective

This add a ctor to `Box` to aid the creation of non-centred boxes. The PR adopts @rezural's work on PR #3322, taking into account the feedback on that PR from @james7132.

## Solution

`Box::from_corners()` creates a `Box` from two opposing corners and automatically determines the min and max extents to ensure that the `Box` is well-formed.

Co-authored-by: rezural <rezural@protonmail.com>
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
# Objective

This add a ctor to `Box` to aid the creation of non-centred boxes. The PR adopts @rezural's work on PR bevyengine#3322, taking into account the feedback on that PR from @james7132.

## Solution

`Box::from_corners()` creates a `Box` from two opposing corners and automatically determines the min and max extents to ensure that the `Box` is well-formed.

Co-authored-by: rezural <rezural@protonmail.com>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

This add a ctor to `Box` to aid the creation of non-centred boxes. The PR adopts @rezural's work on PR bevyengine#3322, taking into account the feedback on that PR from @james7132.

## Solution

`Box::from_corners()` creates a `Box` from two opposing corners and automatically determines the min and max extents to ensure that the `Box` is well-formed.

Co-authored-by: rezural <rezural@protonmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

This add a ctor to `Box` to aid the creation of non-centred boxes. The PR adopts @rezural's work on PR bevyengine#3322, taking into account the feedback on that PR from @james7132.

## Solution

`Box::from_corners()` creates a `Box` from two opposing corners and automatically determines the min and max extents to ensure that the `Box` is well-formed.

Co-authored-by: rezural <rezural@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Utils Utility functions and types C-Feature A new feature, making something new possible S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants