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

Improve Rectangle and Cuboid consistency #11434

Merged

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Jan 20, 2024

Objective

The Rectangle and Cuboid primitives currently use different representations:

pub struct Rectangle {
    /// The half width of the rectangle
    pub half_width: f32,
    /// The half height of the rectangle
    pub half_height: f32,
}

pub struct Cuboid {
    /// Half of the width, height and depth of the cuboid
    pub half_extents: Vec3,
}

The property names and helpers are also inconsistent. Cuboid has half_extents, but it also has a method called from_size. Most existing code also uses "size" instead of "extents".

Solution

Represent both Rectangle and Cuboid with half_size properties.

@Jondolf Jondolf added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Math Fundamental domain-agnostic mathematical operations labels Jan 20, 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.

Great, thanks.

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Jan 20, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 20, 2024
Merged via the queue into bevyengine:main with commit 6337fb3 Jan 20, 2024
22 checks passed
@Jondolf Jondolf deleted the rectangle-and-cuboid-consistency branch January 20, 2024 18:17
github-merge-queue bot pushed a commit that referenced this pull request Jan 20, 2024
# Objective

Currently, the `Ellipse` primitive is represented by a `half_width` and
`half_height`. To improve consistency (similarly to #11434), it might
make more sense to use a `Vec2` `half_size` instead.

Alternatively, to make the elliptical nature clearer, the properties
could also be called `radius_x` and `radius_y`.

Secondly, `Ellipse::new` currently takes a *full* width and height
instead of two radii. I would expect it to take the half-width and
half-height because ellipses and circles are almost always defined using
radii. I wouldn't expect `Circle::new` to take a diameter (if we had
that method).

## Solution

Change `Ellipse` to store a `half_size` and `new` to take the half-width
and half-height.

I also added a `from_size` method similar to `Rectangle::from_size`, and
added the `semi_minor` and `semi_major` helpers to get the
semi-minor/major radius.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants