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

Impl TryFrom vector for directions and add InvalidDirectionError #10884

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Dec 5, 2023

Objective

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

Solution

Implement TryFrom for the direction primitives.

These are all equivalent:

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:

/// 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,
}

@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 5, 2023
@@ -13,6 +13,19 @@ pub trait Primitive2d {}
/// A marker trait for 3D primitives
pub trait Primitive3d {}

/// An error indicating that a direction is invalid because it is zero or not finite.
#[derive(Debug)]
pub struct InvalidDirectionError;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably more useful as an enum: the different failure modes are useful to bubble up.

Copy link
Contributor Author

@Jondolf Jondolf Dec 5, 2023

Choose a reason for hiding this comment

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

I think we need to do a custom try_normalize for this, but it should be easy since it's just an .is_finite() and a comparison against zero

Copy link
Contributor Author

@Jondolf Jondolf Dec 5, 2023

Choose a reason for hiding this comment

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

Or alternatively, do the checks again, but only in error cases. I think this could be better since we can also check for NaN separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to an enum with Zero, Infinite and NaN variants and added tests to verify that it gives the correct errors.

@Jondolf
Copy link
Contributor Author

Jondolf commented Dec 5, 2023

Is there a reason why the new constructor returns an Option, or could it also be changed to return a Result with this error? It'd be more descriptive.

Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

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

Looks good to me if the new constructor is changed to use this error as well

@Jondolf
Copy link
Contributor Author

Jondolf commented Dec 5, 2023

I changed the new constructors to use this error and updated docs accordingly. I also made the docs of the error variants a bit more consistent with each other

@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 6, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 6, 2023
Merged via the queue into bevyengine:main with commit f683b80 Dec 6, 2023
23 checks passed
NaN,
}

impl std::fmt::Display for InvalidDirectionError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why thiserror is not used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bevy_math doesn't have thiserror as a dependency currently. Could add it though, especially if we add more error types

@Jondolf Jondolf deleted the direction-try-from branch December 6, 2023 10:26
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