Remove From implementations from the direction types#10857
Remove From implementations from the direction types#10857alice-i-cecile merged 4 commits intobevyengine:mainfrom
From implementations from the direction types#10857Conversation
Jondolf
added
C-Usability
|
My main concern with this is that it potentially makes a lot of APIs needlessly awkward. For example, creating a // 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 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 |
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
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
added
the
S-Ready-For-Final-Review
|
@Jondolf, I definitely see your point, but IMO we should instead add explicitly infallible (panicking) constructors. |
Jondolf
left a comment
Jondolf
left a comment
There was a problem hiding this comment.
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.
…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, } ```
4 participants
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::newwhich usesVec2/3::try_normalizeto guarantee it returns either a valid direction orNone.This should make it impossible to create an invalid direction, which I think was the intention with these types.