return Direction3d from Transform::up and friends #11604

rosefromthedead · 2024-01-29T17:21:59Z

Objective

Drawing a Gizmos::circle whose normal is derived from a Transform's local axes now requires converting a Vec3 to a Direction3d and unwrapping the result, and I think we shold move the conversion into Bevy.

Solution

We can make Transform::{left,right,up,down,forward,back,local_x,local_y,local_z} return a Direction3d, because they know that their results will be of finite non-zero length (roughly 1.0).

Changelog

Transform::up() and similar functions now return Direction3d instead of Vec3.

Migration Guide

Callers of Transform::up() and similar functions may have to dereference the returned Direction3d to get to the inner Vec3.

github-actions · 2024-01-29T17:22:10Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

rosefromthedead · 2024-01-29T17:33:57Z

Maybe this would be better with a new method on Direction3d to convert to Vec3, so we're not making people use the magic star?

alice-i-cecile · 2024-01-29T17:41:06Z

I like this, but I'd prefer to use a From impl between Direction3D and Vec3.

Jondolf · 2024-01-29T17:41:30Z

crates/bevy_transform/src/components/transform.rs

-        self.rotation * Vec3::X
+    pub fn local_x(&self) -> Direction3d {
+        // Direction3d::new(x) panics if x is of invalid length, but quat * unit vector is length 1
+        Direction3d::new(self.rotation * Vec3::X).unwrap()


These should probably use new_unchecked to avoid the normalization. It'd be useful to implement Mul<Quat> for the direction types though; with that, this would just be:

self.rotation * Direction3d::X

Of course, it would have to be guaranteed that rotation doesn't change the length. I think it should work, but I haven't tried with degenerate quaternions.

This (rotate direction with multiplication) would also only work for 3D directions until we have a Rotation2d

I don't know enough maths to confidently use new_unchecked (or to say that the unwrap will never be hit), so I'd appreciate if someone smarter than me could help here

I think we should merge this with the checked version for now, and look into accelerating this (with tests) in a follow-up.

NiseVoid · 2024-01-29T18:13:10Z

@rosefromthedead

Maybe this would be better with a new method on Direction3d to convert to Vec3, so we're not making people use the magic star?

You mean to stop people from having to do *transform.forward() * length? I think a Mul<f32> for Direction3d that returns Vec3 could be nice too 🤔

@alice-i-cecile

I like this, but I'd prefer to use a From impl between Direction3D and Vec3.

What From are we talking about? From<Vec3> for Direction3d?

alice-i-cecile · 2024-01-29T18:54:41Z

What From are we talking about? From for Direction3d?

Both directions of this, and then in the code changes use direction.into() or take an impl Into<Vec3> rather than using deferencing.

also impl From<Direction3d> for Vec3 and Mul<f32> for Direction3d to make Direction3ds a bit easier to use

rosefromthedead · 2024-01-30T16:27:27Z

New rev has Mul<f32> for Direction3d, From<Direction3d> for Vec3, and we already had TryFrom<Vec3> for Direction3d. This doesn't eliminate dereferencing in examples, because e.g. examples/3d/tonemapping.rs has:

    transform.translation += *transform.forward();

and this can't be a .into() call because then type inference fails. I wonder whether alongside the From impl, we should have a .to_vec3()? Or just stick with the dereferencing?

alice-i-cecile · 2024-01-30T19:59:16Z

I'm fine with the dereference-ing there for now; no need to bikeshed endlessly.

Jondolf

I think we could get rid of all of the derefs shown here with just some simple impls. Mul<f32> already allows us to get rid of several of them.

I'm fine leaving more impls to follow-ups too though

Jondolf · 2024-01-31T17:32:30Z

crates/bevy_math/src/primitives/dim3.rs

+impl std::ops::Mul<f32> for Direction3d {
+    type Output = Vec3;
+    fn mul(self, rhs: f32) -> Self::Output {
+        self.0 * rhs
+    }
+}
+


This should be implemented for Direction2d as well for consistency. It should probably also be implemented the other way around, i.e. impl Mul<Direction3d> for f32.

Jondolf · 2024-01-31T17:33:48Z

crates/bevy_math/src/primitives/dim3.rs

+impl From<Direction3d> for Vec3 {
+    fn from(value: Direction3d) -> Self {
+        value.0
+    }
+}
+


This should also be implemented for the Direction2d & Vec2 pair.

crates/bevy_audio/src/sinks.rs

Jondolf · 2024-01-31T17:36:32Z

examples/helpers/camera_controller.rs

+        let forward = *transform.forward();
+        let right = *transform.right();
        transform.translation += controller.velocity.x * dt * right
            + controller.velocity.y * dt * Vec3::Y
            + controller.velocity.z * dt * forward;


If we impl Mul<Direction3d> for f32 or switch around the operand order, these derefs shouldn't be needed either.

examples/transforms/transform.rs

examples/transforms/translation.rs

Jondolf · 2024-01-31T17:41:01Z

examples/transforms/transform.rs

@@ -127,7 +127,7 @@ fn rotate_cube(
    // Update the rotation of the cube(s).
    for (mut transform, cube) in &mut cubes {
        // Calculate the rotation of the cube if it would be looking at the sphere in the center.
-        let look_at_sphere = transform.looking_at(center, transform.local_y());
+        let look_at_sphere = transform.looking_at(center, *transform.local_y());


We could technically make looking_at take an impl Into<Vec3> to remove the need for deref here, but it's probably follow-up material (and also pretty niche probably)

Jondolf · 2024-01-31T17:45:19Z

examples/3d/tonemapping.rs

@@ -224,7 +224,7 @@ fn setup_color_gradient_scene(
    camera_transform: Res<CameraTransform>,
 ) {
    let mut transform = camera_transform.0;
-    transform.translation += transform.forward();
+    transform.translation += *transform.forward();


One way to avoid the deref here would be to simply implement Add<Direction3d> for Vec3. That can be done in a follow-up though

I would find this a little odd, from an API perspective. If we include direction in the vector algebra we might as well just call it UnitVector. Multiplying a direction by a scalar to get a vector makes sense. Adding a direction to a vector... that's just weird to me.

NthTensor

While I do think the remaining components are worth fixing, I don't think they are worth blocking on.

Let's get this merged.

Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>

alice-i-cecile · 2024-02-02T14:53:36Z

Committed the simple suggestions, resolved merge conflicts. Going to try and merge: CI will validate that I didn't break any of those trivial changes.

rosefromthedead · 2024-02-03T17:00:17Z

Thanks for pushing this forward, I got busy and it's nice to see it land :)

# Objective Drawing a `Gizmos::circle` whose normal is derived from a Transform's local axes now requires converting a Vec3 to a Direction3d and unwrapping the result, and I think we shold move the conversion into Bevy. ## Solution We can make `Transform::{left,right,up,down,forward,back,local_x,local_y,local_z}` return a Direction3d, because they know that their results will be of finite non-zero length (roughly 1.0). --- ## Changelog `Transform::up()` and similar functions now return `Direction3d` instead of `Vec3`. ## Migration Guide Callers of `Transform::up()` and similar functions may have to dereference the returned `Direction3d` to get to the inner `Vec3`. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>

rosefromthedead force-pushed the use-direction3d branch from 7291423 to 0b26a5a Compare January 29, 2024 17:32

alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales A-Math Fundamental domain-agnostic mathematical operations labels Jan 29, 2024

Jondolf reviewed Jan 29, 2024

View reviewed changes

rosefromthedead force-pushed the use-direction3d branch from 0b26a5a to bb37d15 Compare January 30, 2024 16:14

return Direction3d from Transform::up and friends

4c9ac33

also impl From<Direction3d> for Vec3 and Mul<f32> for Direction3d to make Direction3ds a bit easier to use

rosefromthedead force-pushed the use-direction3d branch from bb37d15 to 4c9ac33 Compare January 30, 2024 16:15

alice-i-cecile approved these changes Jan 30, 2024

View reviewed changes

Jondolf reviewed Jan 31, 2024

View reviewed changes

NthTensor approved these changes Feb 2, 2024

View reviewed changes

Jondolf 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 Feb 2, 2024

alice-i-cecile and others added 3 commits February 2, 2024 09:50

Example nit

baa4f46

Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>

Example nit

a6a6bb8

Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>

Code quality nit

f23ebd7

Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>

alice-i-cecile approved these changes Feb 2, 2024

View reviewed changes

Merge branch 'main' into use-direction3d

e63c897

alice-i-cecile enabled auto-merge February 2, 2024 14:53

alice-i-cecile added this pull request to the merge queue Feb 2, 2024

Merged via the queue into bevyengine:main with commit d6f1649 Feb 2, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return Direction3d from Transform::up and friends #11604

return Direction3d from Transform::up and friends #11604

rosefromthedead commented Jan 29, 2024

github-actions bot commented Jan 29, 2024

rosefromthedead commented Jan 29, 2024

alice-i-cecile commented Jan 29, 2024

Jondolf Jan 29, 2024

Jondolf Jan 29, 2024 •

edited

Loading

rosefromthedead Jan 30, 2024

alice-i-cecile Jan 30, 2024

NiseVoid commented Jan 29, 2024 •

edited

Loading

alice-i-cecile commented Jan 29, 2024

rosefromthedead commented Jan 30, 2024

alice-i-cecile commented Jan 30, 2024

Jondolf left a comment

Jondolf Jan 31, 2024

Jondolf Jan 31, 2024

Jondolf Jan 31, 2024

Jondolf Jan 31, 2024

Jondolf Jan 31, 2024

NthTensor Feb 2, 2024 •

edited

Loading

NthTensor left a comment

alice-i-cecile commented Feb 2, 2024

rosefromthedead commented Feb 3, 2024

return Direction3d from Transform::up and friends #11604

return Direction3d from Transform::up and friends #11604

Conversation

rosefromthedead commented Jan 29, 2024

Objective

Solution

Changelog

Migration Guide

github-actions bot commented Jan 29, 2024

rosefromthedead commented Jan 29, 2024

alice-i-cecile commented Jan 29, 2024

Choose a reason for hiding this comment

Jondolf Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiseVoid commented Jan 29, 2024 • edited Loading

alice-i-cecile commented Jan 29, 2024

rosefromthedead commented Jan 30, 2024

alice-i-cecile commented Jan 30, 2024

Jondolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthTensor Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

NthTensor left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Feb 2, 2024

rosefromthedead commented Feb 3, 2024

Jondolf Jan 29, 2024 •

edited

Loading

NiseVoid commented Jan 29, 2024 •

edited

Loading

NthTensor Feb 2, 2024 •

edited

Loading