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

Direction: Rename from_normalized to new_unchecked #11425

Merged
merged 6 commits into from
Jan 20, 2024

Conversation

doonv
Copy link
Contributor

@doonv doonv commented Jan 19, 2024

Objective

Direction2d::from_normalized & Direction3d::from_normalized don't emphasize that importance of the vector being normalized enough.

Solution

Rename from_normalized to new_unchecked and add more documentation.


Direction2d and Direction3d were added somewhat recently in #10466 (after 0.12), so I don't think documenting the changelog and migration guide is necessary (Since there is no major previous version to migrate from).

But here it is anyway in case it's needed:

Changelog

  • Renamed Direction2d::from_normalized and Direction3d::from_normalized to new_unchecked.

Migration Guide

  • Renamed Direction2d::from_normalized and Direction3d::from_normalized to new_unchecked.

@Jondolf Jondolf added A-Math Fundamental domain-agnostic mathematical operations P-Unsound A bug that results in undefined compiler behavior labels Jan 19, 2024
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
@@ -28,6 +28,18 @@ impl Direction3d {
Self::new_and_length(value).map(|(dir, _)| dir)
}

/// Create a [`Direction3d`] from a [`Vec3`] that is already normalized.
///
/// ## Safety
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// ## Safety
/// # Safety

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
Comment on lines 169 to 170
// SAFETY: Multipling a normalized vector (Vec3::Y) with a rotation returns a normalized vector.
direction: unsafe { Direction3d::new_unchecked(rotation * Vec3::Y) },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can Direction(3d,2d) implements rotation ? (probably in another PR)
to directly use rotation * Direction3d::Y without unsafe.

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.

Conventionally, unsafe in Rust is used strictly for memory safety issues (or other odd corners where you might hit genuine Undefined Behavior in the compiler). While I strongly think that there should be alternatives that work like unsafe, but for other forms of danger, we should stick to the conventions of the ecosystem.

That said, I love the rest of these changes and think we need to warn more aggressively here.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior and removed P-Unsound A bug that results in undefined compiler behavior labels Jan 19, 2024
@doonv
Copy link
Contributor Author

doonv commented Jan 20, 2024

Conventionally, unsafe in Rust is used strictly for memory safety issues.

Yes, but it's also used in cases where there is a promise that a certain type must have some limitation (being normalized in this case).

All the NonZero number types in the standard library of examples of this.

Having a direction vector that is not normalized may not cause UB in the traditional sense, but it could cause unforeseen bugs.

@tguichaoua
Copy link
Contributor

tguichaoua commented Jan 20, 2024

All the NonZero number types in the standard library of examples of this.

NonZero are used for niche optimization; if you create a zeored 'NonZero', then some code becomes unsound.
The question is: is the invariant of Direction (i.e., a vector is normalized) used as a guarantee in unsafe code?

@doonv
Copy link
Contributor Author

doonv commented Jan 20, 2024

The question is: is the invariant of Direction (i.e., a vector is normalized) used as a guarantee in unsafe code?

No, but it almost certainly cause bugs in code that expect Direction to be normalized.

Marking it as unsafe makes the user think twice on if their vector is really normalized.

@tguichaoua
Copy link
Contributor

From the Book:

the intent is that as the programmer, you’ll ensure the code inside an unsafe block will access memory in a valid way.

unsafe in Rust is a matter of accessing valid memory. The worst that could happen with a non-normalized Direction is panic; which is a logic bug like dividing by 0.

@doonv
Copy link
Contributor Author

doonv commented Jan 20, 2024

The worst that could happen with a non-normalized Direction is panic

Panics are arent much of a problem here (they can easily be tracked down), what is a problem is a silent bug (A bug that doesn't provide any debug information when it occurs). For example, if you have a player movement function that takes in a Direction2d and that Direction2d isnt normalized, that cause the player to move slower/faster than expected, and it'll be very hard to track down. (The user will expect Direction2d to be normalized)

The unsafe keyword isn't just for memory issues and UB, it's for places that have a "contract" that the compiler can't check.

The unsafe keyword has two uses:

  • to declare the existence of contracts the compiler can't check (unsafe fn and unsafe trait),
  • and to declare that a programmer has checked that these contracts have been upheld (unsafe {} and unsafe impl, but also unsafe fn -- see below).

- The unsafe keyword's documentation when hovered over (rust-analyzer)

The "contract" here is that the inner vector is normalized.

As I said earlier, making this unsafe makes the user think twice about if their direction is really normalized. And it makes shooting yourself in the foot harder.

@nicopap
Copy link
Contributor

nicopap commented Jan 20, 2024

This has been a longstanding discussion in the rust community. "What should I mark as unsafe?" For the standard library, the answer is "only methods with invariants, which, if not upheld, invoke undefined behaviors".

There is even a chapter on the rust reference about "behaviors not considered unsafe": https://doc.rust-lang.org/stable/reference/behavior-not-considered-unsafe.html. "Logic Errors" is in there.

This doesn't answer the question of whether we should mark the method as unsafe. The thing is, we already expose API that assumes the values passed-in represent a normal vector: The Quat constructors. I think we should be consistant with our API, and it makes sense to not mark the constructor as unsafe, the same way glam doesn't. We could add an optional runtime check though!

@doonv
Copy link
Contributor Author

doonv commented Jan 20, 2024

Well, I guess I'll make it safe.

@doonv doonv changed the title Direction: Rename from_normalized to new_unchecked and declare unsafe Direction: Rename from_normalized to new_unchecked Jan 20, 2024
doonv and others added 2 commits January 20, 2024 16:21
Co-authored-by: Tristan Guichaoua <33934311+tguichaoua@users.noreply.github.com>
Co-authored-by: Tristan Guichaoua <33934311+tguichaoua@users.noreply.github.com>
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.

Some comment nits, but otherwise looks good

crates/bevy_math/src/bounding/bounded3d/primitive_impls.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
@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 Jan 20, 2024
Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 20, 2024
Merged via the queue into bevyengine:main with commit 0387331 Jan 20, 2024
22 checks passed
@rlidwka
Copy link
Contributor

rlidwka commented Jan 20, 2024

The thing is, we already expose API that assumes the values passed-in represent a normal vector: The Quat constructors.

Quaternions do not need to be normalized. Only quaternions that are used for rotation do. I've used quaternions to represent angular velocity before, and those were intentionally not normalized.

In nalgebra, there's UnitQuaternion specifically for rotations, but there's nothing similar in bevy/glam.

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-Bug An unexpected or incorrect behavior 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.

7 participants