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

DirX::new should be more explicit about normalization and have a code example #13429

Closed
Jondolf opened this issue May 19, 2024 · 1 comment · Fixed by #13483
Closed

DirX::new should be more explicit about normalization and have a code example #13429

Jondolf opened this issue May 19, 2024 · 1 comment · Fixed by #13483
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon

Comments

@Jondolf
Copy link
Contributor

Jondolf commented May 19, 2024

How can Bevy's documentation be improved?

The documentation for the new constructors of direction types doesn't explicitly state that the given vector gets automatically normalized by the method.

For example, Dir2:

/// Create a direction from a finite, nonzero [`Vec2`].
///
/// Returns [`Err(InvalidDirectionError)`](InvalidDirectionError) if the length
/// of the given vector is zero (or very close to zero), infinite, or `NaN`.

Arguably, the normalization is implied because it "creates a direction", which based on the type's definition should be normalized. Still, I have seen several users do something like this:

Dir2::new(target.normalize()).unwrap()

resulting in a double normalization.

We should make the comment more explicit about the normalization, and also add a code example in the doc comment to show how the type is intended to be used. This code example could also showcase some error cases, like a zero vector.

@Jondolf Jondolf added C-Docs An addition or correction to our documentation A-Math Fundamental domain-agnostic mathematical operations X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 19, 2024
@bugsweeper
Copy link
Contributor

bugsweeper commented May 20, 2024

Next method in the doc is Dir2::new_unchecked which states that it is for already normalized vectors. This is very idiomatic way to have method with guaranties for any data and method_unchecked for already proved data for double check optimization

github-merge-queue bot pushed a commit that referenced this issue May 23, 2024
# Objective

- Fixes #13429 .

## Solution

- Improved docs for methods `new`, `new_and_length` of `Dir2`, `Dir3`,
`Dir3A`.
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-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants