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/Direction3d to Dir2/Dir3 and add Dir3A #12017

Closed
wants to merge 2 commits into from

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Feb 20, 2024

Objective

Currently, Bevy has the Direction2d and Direction3d types, which are used for Ray2d and Ray3d, several primitive shapes, some gizmo and transform APIs, and more.

Direction3d stores a Vec3. However, the SIMD version Vec3A can be meaningfully faster for many tasks like ray casting, so we want types like Ray2d and Ray3d to use it. We could just change Direction3d to use a Vec3A, but the unaligned version is also valuable for determinism and APIs that use Vec3. As a result, we need both Vec3 and Vec3A versions of Direction3d.

The name Direction3dA doesn't feel very natural, and SimdDirection3d is a bit long. Thus, I propose renaming the direction types to shortened versions that follow Glam's naming conventions: Dir2, Dir3, and Dir3A. This was also proposed in the original primitive PR, but back then we were slightly in favor of the longer, more explicit names.

I would argue that Dir2 and Dir3 are also a bit nicer to write than Direction2d and Direction3d, and in some cases they can produce nicer formatting. The name length also matches Vec2 and Vec3, and the API is intended to be very similar (but with normalization guarantees). In my opinion, the direction types are to be treated like core math types, and it makes sense to match Glam's naming convention.

Some examples of what it'd look like in code:

// 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);

The diff of this PR has more examples.

Personally, I find Dir2 and Dir3 to look better aesthetically in addition to being more consistent with Glam's naming conventions. They are often next to Vec2 or Vec3, and having the type names be the same length looks nice.

Solution

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

There's now quite a bit of direction code (including tests and the InvalidDirectionError), and we will add more implementations and helpers in the future. The direction types also feel a bit out-of-place in the primitives module in my opinion.

I have discussed this with others on the Discord, and have decided to move the direction types out of primitives into a dedicated direction module. This is something we were intending to do much earlier, but never did.


Changelog

  • Renamed Direction2d to Dir2 and Direction3d to Dir3
  • Added Dir3A
  • Moved direction types out of primitives and into a new direction module

Migration Guide

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

@Jondolf Jondolf added C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Math Fundamental domain-agnostic mathematical operations M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 20, 2024
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible and removed C-Feature A new feature, making something new possible labels Feb 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.

Can you add deprecated type aliases for the old names to ease migration?

@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 20, 2024

I already did, they're in the primitive modules where the old Direction2d and Direction3d used to be.

@JMS55
Copy link
Contributor

JMS55 commented Feb 20, 2024

I don't like the shortened names personally.

@alice-i-cecile alice-i-cecile self-requested a review February 20, 2024 23:35
@alice-i-cecile
Copy link
Member

Can we split out the rename from the code org + new aligned types? I'd like to avoid getting derailed by bikeshedding.

@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 20, 2024

What should the new aligned type be called though? That was one of the motivating factors behind the rename. Direction3dA feels a bit weird, SimdDirection3d isn't ideal imo, and I'm not sure what other viable options there would be

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 20, 2024

Let's do Direction3dA for now: it's at least consistent.

@alice-i-cecile
Copy link
Member

Closing in favor of #12018: we can do the rename in another PR after :)

@Jondolf Jondolf closed this Feb 21, 2024
github-merge-queue bot pushed a commit that referenced this pull request Feb 26, 2024
# Objective

Split up from #12017, add an aligned version of `Direction3d` for SIMD,
and move direction types out of `primitives`.

## Solution

Add `Direction3dA` and move direction types into a new `direction`
module.

---

## Migration Guide

The `Direction2d`, `Direction3d`, and `InvalidDirectionError` types have
been moved out of `bevy::math::primitives`.

Before:

```rust
use bevy::math::primitives::Direction3d;
```

After:

```rust
use bevy::math::Direction3d;
```

---------

Co-authored-by: Alice Cecile <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
…engine#12018)

# Objective

Split up from bevyengine#12017, add an aligned version of `Direction3d` for SIMD,
and move direction types out of `primitives`.

## Solution

Add `Direction3dA` and move direction types into a new `direction`
module.

---

## Migration Guide

The `Direction2d`, `Direction3d`, and `InvalidDirectionError` types have
been moved out of `bevy::math::primitives`.

Before:

```rust
use bevy::math::primitives::Direction3d;
```

After:

```rust
use bevy::math::Direction3d;
```

---------

Co-authored-by: Alice Cecile <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
…engine#12018)

# Objective

Split up from bevyengine#12017, add an aligned version of `Direction3d` for SIMD,
and move direction types out of `primitives`.

## Solution

Add `Direction3dA` and move direction types into a new `direction`
module.

---

## Migration Guide

The `Direction2d`, `Direction3d`, and `InvalidDirectionError` types have
been moved out of `bevy::math::primitives`.

Before:

```rust
use bevy::math::primitives::Direction3d;
```

After:

```rust
use bevy::math::Direction3d;
```

---------

Co-authored-by: Alice Cecile <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request 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)

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

// After
let ray_cast = RayCast2d::new(Vec2::ZERO, Dir2::X, 5.0);
```

```rust
// 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(),
);
```

```rust
// 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](https://discord.com/channels/691052431525675048/1203087353850364004/1212465038711984158)
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.

---------

Co-authored-by: IceSentry <[email protected]>
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-Feature A new feature, making something new possible 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants