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

Rename Direction2d/3d to Dir2/3 #12189

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Feb 28, 2024

Objective

Split up from #12017, rename Bevy's direction types.

Currently, Bevy has the Direction2d, Direction3d, and Direction3dA types, which provide a type-level guarantee that their contained vectors remain normalized. They can be very useful for a lot of APIs for safety, explicitness, and in some cases performance, as they can sometimes avoid unnecessary normalizations.

However, many consider them to be inconvenient to use, and opt for standard vector types like Vec3 because of this. One reason is that the direction type names are a bit long and can be annoying to write (of course you can use autocomplete, but just typing Vec3 is still nicer), and in some intances, the extra characters can make formatting worse. The naming is also inconsistent with Glam's shorter type names, and results in names like Direction3dA, which (in my opinion) are difficult to read and even a bit ugly.

This PR proposes renaming the types to Dir2, Dir3, and Dir3A. These names are nice and easy to write, consistent with Glam, and work well for variants like the SIMD aligned Dir3A. As a bonus, it can also result in nicer formatting in a lot of cases, which can be seen from the diff of this PR.

Some examples of what it looks like: (copied from #12017)

// Before
let ray_cast = RayCast2d::new(Vec2::ZERO, Direction2d::X, 5.0);

// After
let ray_cast = RayCast2d::new(Vec2::ZERO, Dir2::X, 5.0);
// Before (an example using Bevy XPBD)
let hit = spatial_query.cast_ray(
    Vec3::ZERO,
    Direction3d::X,
    f32::MAX,
    true,
    SpatialQueryFilter::default(),
);

// After
let hit = spatial_query.cast_ray(
    Vec3::ZERO,
    Dir3::X,
    f32::MAX,
    true,
    SpatialQueryFilter::default(),
);
// Before
self.circle(
    Vec3::new(0.0, -2.0, 0.0),
    Direction3d::Y,
    5.0,
    Color::TURQUOISE,
);

// After (formatting is collapsed in this case)
self.circle(Vec3::new(0.0, -2.0, 0.0), Dir3::Y, 5.0, Color::TURQUOISE);

Solution

Rename Direction2d, Direction3d, and Direction3dA to Dir2, Dir3, and Dir3A.


Migration Guide

The Direction2d and Direction3d types have been renamed to Dir2 and Dir3.

Additional Context

This has been brought up on the Discord a few times, and we had a small poll on this. Dir2/Dir3/Dir3A was quite unanimously chosen as the best option, but of course it was a very small poll and inconclusive, so other opinions are certainly welcome too.

@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 Feb 28, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@Jondolf Jondolf added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 28, 2024
@tim-blackbird tim-blackbird 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 Feb 28, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 28, 2024
Merged via the queue into bevyengine:main with commit f418de8 Feb 28, 2024
27 checks passed
@Jondolf Jondolf deleted the rename-directions branch February 29, 2024 08:15
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 M-Needs-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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants