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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Add Transform::look_to #6692

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Add Transform::look_to #6692

wants to merge 2 commits into from

Conversation

tim-blackbird
Copy link
Contributor

Add a method to rotate a transform to point towards a direction.

Also updated the docs to link to forward and up instead of mentioning local negative Z and local Y.

Unfortunately, links to methods don't work in rust-analyzer :(

@alice-i-cecile
Copy link
Member

Docs could be clearer: it's hard when reading to understand how this compares and contrasts to Transform::look_at.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales C-Docs An addition or correction to our documentation labels Nov 19, 2022
alice-i-cecile
alice-i-cecile reviewed Nov 19, 2022
View reviewed changes
crates/bevy_transform/src/components/transform.rs
/// vector in the local negative `Z` direction is toward `target` and its
/// unit vector in the local `Y` direction is toward `up`.
/// Returns this [`Transform`] with a new rotation so that [`Transform::forward`]
/// points towards the `target` position and [`Transform::up`] points towards `up`.
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should also keep the local z etc explanations, probably in parentheticals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already documented on the linked methods. I'd rather avoid repeating the documentation.
Although, since rust-analyzer doesn't support these links it does kinda suck. hmmmm

@tim-blackbird
Copy link
Contributor Author

look_at docs: "Transform::forward points towards the target position"
look_to docs: "Transform::forward points towards the given direction"
I'm not sure how to make that more clear. Method names match glam.

@alice-i-cecile
Copy link
Member

Ah, I see! Hard without changing the names. look_to feels like look_in_direction to me? Yeah okay, I'm happy enough with this as is. The method itself is quite useful!

@mockersf
Copy link
Member

I feel my suggestion (in to replace towards) makes the difference more obvious, and clearer at least for me. Looking towards a position, looking in a direction

@tim-blackbird @mockersf
Apply suggestions from code review
b576c69 
Co-authored-by: François <mockersf@gmail.com>
@mockersf mockersf 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 Nov 19, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 21, 2022
@tim-blackbird
Add Transform::look_to (#6692)
30070a9 
Add a method to rotate a transform to point towards a direction.

Also updated the docs to link to `forward` and `up` instead of mentioning local negative `Z` and local `Y`.

Unfortunately, links to methods don't work in rust-analyzer :(

Co-authored-by: Devil Ira <justthecooldude@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 21, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add Transform::look_to [Merged by Bors] - Add Transform::look_to Nov 21, 2022
@bors bors bot closed this Nov 21, 2022
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
@tim-blackbird @taiyoungjang
Add Transform::look_to (bevyengine#6692)
e35b3d6 
Add a method to rotate a transform to point towards a direction.

Also updated the docs to link to `forward` and `up` instead of mentioning local negative `Z` and local `Y`.

Unfortunately, links to methods don't work in rust-analyzer :(

Co-authored-by: Devil Ira <justthecooldude@gmail.com>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
@tim-blackbird @alradish
Add Transform::look_to (bevyengine#6692)
6892b2d 
Add a method to rotate a transform to point towards a direction.

Also updated the docs to link to `forward` and `up` instead of mentioning local negative `Z` and local `Y`.

Unfortunately, links to methods don't work in rust-analyzer :(

Co-authored-by: Devil Ira <justthecooldude@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@tim-blackbird @ItsDoot
Add Transform::look_to (bevyengine#6692)
16aaa57 
Add a method to rotate a transform to point towards a direction.

Also updated the docs to link to `forward` and `up` instead of mentioning local negative `Z` and local `Y`.

Unfortunately, links to methods don't work in rust-analyzer :(

Co-authored-by: Devil Ira <justthecooldude@gmail.com>
@nicopap nicopap mentioned this pull request Apr 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Transform Translations, rotations and scales C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tim-blackbird @alice-i-cecile @mockersf