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

return Direction3d from Transform::up and friends #11604

Merged
merged 5 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/bevy_audio/src/sinks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ impl SpatialAudioSink {
/// Set the listener position, with an ear on each side separated by `gap`.
pub fn set_listener_position(&self, position: Transform, gap: f32) {
self.set_ears_position(
position.translation + position.left() * gap / 2.0,
position.translation + position.right() * gap / 2.0,
position.translation + *position.left() * gap / 2.0,
position.translation + *position.right() * gap / 2.0,
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
);
}

Expand Down
13 changes: 13 additions & 0 deletions crates/bevy_math/src/primitives/dim3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ impl TryFrom<Vec3> for Direction3d {
}
}

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

Comment on lines +74 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

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

impl std::ops::Deref for Direction3d {
type Target = Vec3;
fn deref(&self) -> &Self::Target {
Expand All @@ -85,6 +91,13 @@ impl std::ops::Neg for Direction3d {
}
}

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

Comment on lines +94 to +100
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

/// A sphere primitive
#[derive(Clone, Copy, Debug, PartialEq)]
#[cfg_attr(feature = "serialize", derive(serde::Serialize, serde::Deserialize))]
Expand Down
28 changes: 16 additions & 12 deletions crates/bevy_transform/src/components/transform.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::GlobalTransform;
use bevy_ecs::{component::Component, reflect::ReflectComponent};
use bevy_math::primitives::Direction3d;
use bevy_math::{Affine3A, Mat3, Mat4, Quat, Vec3};
use bevy_reflect::prelude::*;
use bevy_reflect::Reflect;
Expand Down Expand Up @@ -185,55 +186,58 @@ impl Transform {

/// Get the unit vector in the local `X` direction.
#[inline]
pub fn local_x(&self) -> Vec3 {
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Jondolf Jondolf Jan 29, 2024

Choose a reason for hiding this comment

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

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

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 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

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 we should merge this with the checked version for now, and look into accelerating this (with tests) in a follow-up.

}

/// Equivalent to [`-local_x()`][Transform::local_x()]
#[inline]
pub fn left(&self) -> Vec3 {
pub fn left(&self) -> Direction3d {
-self.local_x()
}

/// Equivalent to [`local_x()`][Transform::local_x()]
#[inline]
pub fn right(&self) -> Vec3 {
pub fn right(&self) -> Direction3d {
self.local_x()
}

/// Get the unit vector in the local `Y` direction.
#[inline]
pub fn local_y(&self) -> Vec3 {
self.rotation * Vec3::Y
pub fn local_y(&self) -> Direction3d {
// Direction3d::new(x) panics if x is of invalid length, but quat * unit vector is length 1
Direction3d::new(self.rotation * Vec3::Y).unwrap()
}

/// Equivalent to [`local_y()`][Transform::local_y]
#[inline]
pub fn up(&self) -> Vec3 {
pub fn up(&self) -> Direction3d {
self.local_y()
}

/// Equivalent to [`-local_y()`][Transform::local_y]
#[inline]
pub fn down(&self) -> Vec3 {
pub fn down(&self) -> Direction3d {
-self.local_y()
}

/// Get the unit vector in the local `Z` direction.
#[inline]
pub fn local_z(&self) -> Vec3 {
self.rotation * Vec3::Z
pub fn local_z(&self) -> Direction3d {
// Direction3d::new(x) panics if x is of invalid length, but quat * unit vector is length 1
Direction3d::new(self.rotation * Vec3::Z).unwrap()
}

/// Equivalent to [`-local_z()`][Transform::local_z]
#[inline]
pub fn forward(&self) -> Vec3 {
pub fn forward(&self) -> Direction3d {
-self.local_z()
}

/// Equivalent to [`local_z()`][Transform::local_z]
#[inline]
pub fn back(&self) -> Vec3 {
pub fn back(&self) -> Direction3d {
self.local_z()
}

Expand Down
4 changes: 2 additions & 2 deletions examples/3d/tonemapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@NthTensor NthTensor Feb 2, 2024

Choose a reason for hiding this comment

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

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.


commands.spawn((
MaterialMeshBundle {
Expand All @@ -248,7 +248,7 @@ fn setup_image_viewer_scene(
camera_transform: Res<CameraTransform>,
) {
let mut transform = camera_transform.0;
transform.translation += transform.forward();
transform.translation += *transform.forward();

// exr/hdr viewer (exr requires enabling bevy feature)
commands.spawn((
Expand Down
4 changes: 2 additions & 2 deletions examples/helpers/camera_controller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ fn run_camera_controller(
controller.velocity = Vec3::ZERO;
}
}
let forward = transform.forward();
let right = transform.right();
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;
Comment on lines +174 to 178
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Expand Down
4 changes: 2 additions & 2 deletions examples/transforms/transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ fn setup(
fn move_cube(mut cubes: Query<(&mut Transform, &mut CubeState)>, timer: Res<Time>) {
for (mut transform, cube) in &mut cubes {
// Move the cube forward smoothly at a given move_speed.
let forward = transform.forward();
let forward = *transform.forward();
transform.translation += forward * cube.move_speed * timer.delta_seconds();
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
}
}
Expand All @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

// Interpolate between the current rotation and the fully turned rotation
// when looking a the sphere, with a given turn speed to get a smooth motion.
// With higher speed the curvature of the orbit would be smaller.
Expand Down
2 changes: 1 addition & 1 deletion examples/transforms/translation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ fn move_cube(mut cubes: Query<(&mut Transform, &mut Movable)>, timer: Res<Time>)
if (cube.spawn - transform.translation).length() > cube.max_distance {
cube.speed *= -1.0;
}
let direction = transform.local_x();
let direction = *transform.local_x();
transform.translation += direction * cube.speed * timer.delta_seconds();
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
}
}