Skip to content

Impl TryFrom vector for directions and add InvalidDirectionError#10884

Merged
alice-i-cecile merged 4 commits intobevyengine:mainfrom
Jondolf:direction-try-from
Dec 6, 2023
Merged

Impl TryFrom vector for directions and add InvalidDirectionError#10884
alice-i-cecile merged 4 commits intobevyengine:mainfrom
Jondolf:direction-try-from

Conversation

@Jondolf
Copy link
Copy Markdown
Contributor

@Jondolf Jondolf commented Dec 5, 2023 •
edited
Loading

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
alice-i-cecile
alice-i-cecile reviewed Dec 5, 2023
View reviewed changes
crates/bevy_math/src/primitives/mod.rs Outdated

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

@alice-i-cecile alice-i-cecile 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 this is probably more useful as an enum: the different failure modes are useful to bubble up.

Copy link
Copy Markdown
Contributor Author

@Jondolf Jondolf Dec 5, 2023 •
edited
Loading

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
Copy Markdown
Contributor Author

@Jondolf Jondolf Dec 5, 2023 •
edited
Loading

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
Copy Markdown
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 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
Copy Markdown
Contributor Author

Jondolf commented Dec 5, 2023 •
edited
Loading

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.

tim-blackbird
tim-blackbird approved these changes Dec 5, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
rlidwka
rlidwka reviewed Dec 6, 2023
View reviewed changes
crates/bevy_math/src/primitives/mod.rs
NaN,
}

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

@rlidwka rlidwka Dec 6, 2023

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
Copy Markdown
Contributor Author

@Jondolf Jondolf Dec 6, 2023

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
@nicopap nicopap mentioned this pull request Jan 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+2 more reviewers

@rlidwka rlidwka rlidwka left review comments

@tim-blackbird tim-blackbird tim-blackbird approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Jondolf @rlidwka @alice-i-cecile @tim-blackbird