Skip to content

Add Transform Helper methods #15200

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Bayslak
Copy link

@Bayslak Bayslak commented Sep 14, 2024

Objective

  • The objective of this PR reflects the idea of adding some helper methods to Transform to avoid cumbersome way of providing direction and moving an object. The issue taken in consideration is the following: Helper methods on Transform

Solution

  • I decided to add a basic translate method in the Transform class translate(&mut self, direction: Vec3, speed: f32, delta_time_seconds: f32). This function simply provides a faster way of adding a translation to the Transform by providing a direction, a speed and a delta_seconds (or delta in general referring to time). This doesn't take in account the rotation of the Transform.
  • I also added a second function translate_with_local_rotation(&mut self, direction: Vec3, speed: f32, delta_time_seconds: f32). This function differs from the one above because it also utilizes the rotation of the Transform to provide a way of moving the object following a specific direction and taking in account also it's rotation.

Testing

  • I didn't test the new function, but I changed the example translation.rs using the newly implemented function translate to see if the result was the same.
  • I also tried the second method translate_with_local_rotation by modifing the example 3d_rotation and adding a new line just after the rotation. It moved as expected, but the addition of the line felt like out of scope of the example, so I removed it.

Copy link
Contributor

Welcome, new contributor!

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

@IQuick143 IQuick143 added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 14, 2024
Refactored the two methods to accept a single Vec3 as the result of the direction Vec3 multiplied by the speed of the entity we are trying to move.

This should remove the redundant assegnation of the speed.
@Bayslak Bayslak requested a review from IQuick143 September 14, 2024 14:51
@IQuick143
Copy link
Contributor

Overall I think the code is good, I'm not sure on the naming though.
I feel like translate should take an offset vector, and translate the Transform by that value.
This is more related to velocities and kinematics (moving objects over time).

Not sure what would be a better name.

@Bayslak
Copy link
Author

Bayslak commented Sep 15, 2024

Overall I think the code is good, I'm not sure on the naming though.
I feel like translate should take an offset vector, and translate the Transform by that value.
This is more related to velocities and kinematics (moving objects over time).

Not sure what would be a better name.

I see what you mean.

Maybe to make it more coherent might be better to only have a Vec3 as the signature of the method, calling it translation and let the user create the translation vector by multiplying it by the speed AND the delta time?

@Bayslak
Copy link
Author

Bayslak commented Sep 15, 2024

Going to change it later to only receive a Vec3

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

I think translate_to for a new absolute position and translate_by for an offset would be a good naming convention. It might also be worth wrapping the Vec3 in a newtype Offset and Position to prevent accidentally providing the wrong kind of value. Mojang actually just had this exact mixup earlier this month when reimplementing ender pearl teleportation!

@Bayslak
Copy link
Author

Bayslak commented Sep 25, 2024

I think translate_to for a new absolute position and translate_by for an offset would be a good naming convention. It might also be worth wrapping the Vec3 in a newtype Offset and Position to prevent accidentally providing the wrong kind of value. Mojang actually just had this exact mixup earlier this month when reimplementing ender pearl teleportation!

I see your points, but isn't just easier to read and understand a single Vec3 as a parameter? Then it's the user's job to provide the correct calculation over that Vec3 I feel 🤔

@BenjaminBrienen
Copy link
Contributor

I think translate_to for a new absolute position and translate_by for an offset would be a good naming convention. It might also be worth wrapping the Vec3 in a newtype Offset and Position to prevent accidentally providing the wrong kind of value. Mojang actually just had this exact mixup earlier this month when reimplementing ender pearl teleportation!

I see your points, but isn't just easier to read and understand a single Vec3 as a parameter? Then it's the user's job to provide the correct calculation over that Vec3 I feel 🤔

It's not wrong either way. I feel like it's the same difference between having a raw boolean parameter vs an enum with 2 values. There is a clippy lint against boolean parameters because it's easy to accidentally mix up which parameter or which value you meant to use. Also, a raw true doesn't mean much to a first time reader unless they also look at the name of the parameter. However, there's also a balance to be had, of course. We don't need to newtype everything. My suggestion can be decided against. It's just a little piece of input in case everyone likes the idea. :)

@IQuick143
Copy link
Contributor

RE: Point & Offset newtype.

While mathematically I 100% agree that the semantics of vectors (as offsets) vs points are very different (and the reason we use Vec3 for both could be regarded as a "hack" based on arbitrarily fixing the origin) the codebase is generally not made for this and it would be a very controversial change.
(Feel free to make an issue to discuss this, but we would likely need to convince glam to cooperate and we would make a very big breaking change.)
(Note: There's also some API design issues with affine combinations (specific subset of linear combinations) being valid operations on both Points and Vectors, while general linear combinations being well-defined only on vectors, however there is no type-based way to decide this, so you either need to allow points to behave like vectors, or force users to convert a bunch, introducing a lot of friction.)

Relevant discussion @ glam: bitshifter/glam-rs#133

Copy link
Member

@chompaa chompaa left a comment

Choose a reason for hiding this comment

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

Doc suggestions :)

@Bayslak Bayslak requested a review from chompaa September 27, 2024 07:58
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 22, 2025
@kristoff3r kristoff3r added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants