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

Remove From implementations from the direction types #10857

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

tim-blackbird
Copy link
Contributor

This removes the From<Vec2/3> implementations for the direction types.
It doesn't seem right to have when it only works if the vector is nonzero and finite and produces NaN otherwise.

Added Direction2d/3d::new which uses Vec2/3::try_normalize to guarantee it returns either a valid direction or None.

This should make it impossible to create an invalid direction, which I think was the intention with these types.

@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 Dec 3, 2023
@Jondolf
Copy link
Contributor

Jondolf commented Dec 3, 2023

My main concern with this is that it potentially makes a lot of APIs needlessly awkward. For example, creating a Segment2d:

// Before
let segment = Segment2d::new(Vec2::X.into(), 2.0); // .into() could even be removed with impl Into<Direction2d>

// After
let segment = Segment2d::new(Direction2d::new(Vec2::X).unwrap(), 2.0);

For rays, I was considering an API where Ray2d::new takes an impl Into<Direction2d> so that you could either just pass a vector that would automatically be normalized, or directly pass a Direction2d to avoid the extra normalization. This isn't possible if Direction2d doesn't implement From<Vec2>, so it would either have to always normalize the given vector or take a Direction2d which is bad for DX.

I do see value in requiring the direction to always be valid, but NaN or non-finite values for directions should be rare enough that I'm hesitant to take the DX hit for it. I believe the main goal of Direction2d/Direction3d is to make sure normalize is always called (but as few times as possible) for all direction vectors. It doesn't necessarily give guarantees about degenerate cases like NaN.

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.

I think that the correctness is more important here. There's no guaranteed one-to-one mapping between a Vec2 and a direction unfortunately.

I'd be fine with a TryFrom impl instead or in addition to this though.

@alice-i-cecile alice-i-cecile 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 Dec 4, 2023
@alice-i-cecile
Copy link
Member

@Jondolf, I definitely see your point, but IMO we should instead add explicitly infallible (panicking) constructors.

Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

After thinking about this more, I think I'm also leaning in favor of this change now. It does make some things a bit more verbose, but the correctness and explicitness is perhaps more important, and if desired, primitives could still have panicking constructors that handle the vector to direction conversion in the background.

I think having an additional TryFrom impl would make sense because that's basically what new does, but try_from/try_into might be less common and accessible for users so I'd still preserve new.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 5, 2023
Merged via the queue into bevyengine:main with commit 83ee6de Dec 5, 2023
25 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2023
…10884)

# Objective

Implement `TryFrom<Vec2>`/`TryFrom<Vec3>` for direction primitives as
considered in #10857.

## Solution

Implement `TryFrom` for the direction primitives.

These are all equivalent:

```rust
let dir2d = Direction2d::try_from(Vec2::new(0.5, 0.5)).unwrap();
let dir2d = Vec2::new(0.5, 0.5).try_into().unwrap(); // (assumes that the type is inferred)
let dir2d = Direction2d::new(Vec2::new(0.5, 0.5)).unwrap();
```

For error cases, an `Err(InvalidDirectionError)` is returned. It
contains the type of failure:

```rust
/// An error indicating that a direction is invalid.
#[derive(Debug, PartialEq)]
pub enum InvalidDirectionError {
    /// The length of the direction vector is zero or very close to zero.
    Zero,
    /// The length of the direction vector is `std::f32::INFINITY`.
    Infinite,
    /// The length of the direction vector is `NaN`.
    NaN,
}
```
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 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.

4 participants