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] - Rename Transform::mul_vec3 to transform_point and improve docs #6132

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Rename Transform::mul_vec3 to transform_point and improve docs #6132

wants to merge 1 commit into from

Conversation

irate-devil
Copy link
Contributor

@irate-devil irate-devil commented Sep 30, 2022 •
edited

The docs ended up quite verbose :v

Also added a missing #[inline] to GlobalTransform::mul_transform.

I'd say this resolves #5500

Migration Guide

Transform::mul_vec3 has been renamed to transform_point.

@irate-devil
Copy link
Contributor Author

CI failure is probably because of panicking threadpools.

@IceSentry
Copy link
Contributor

I'm really not sure I understand how the new name is better. mul_vec3 is really clear to me since it multiplies the transform with the given vec3. transform_point doesn't tell me anything because transform isn't an operation. Or more precisely, there are a lot of transform operations, but transform, on it's own isn't one as far as I know.

@IceSentry
Copy link
Contributor

Ok, reading the code again, I guess mul_vec3 is indeed a bad name. I just don't get the new name.

@irate-devil
Copy link
Contributor Author

irate-devil commented Sep 30, 2022 •
edited

The name transform_point was taken from glam, but it seems to be the accepted term generally and is used in Unity/Unreal.
I suppose "Apply a transformation matrix to a point" == "transform a point".

edit: mul_vec3 can be ambiguous betweem transform_point and transform_vector, which is why glam chose not to impl Mul for Vec3 and Mat4 for example.

IceSentry
IceSentry approved these changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Not sure about the CI failures, but now that I see it's a terminology used by other engines LGTM

@NathanSWard NathanSWard added A-Transform Translations, rotations and scales C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 30, 2022
tguichaoua
tguichaoua approved these changes Oct 1, 2022
View reviewed changes
Copy link
Contributor

@tguichaoua tguichaoua left a comment

Choose a reason for hiding this comment

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

LGTM

@rparrett rparrett 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 Oct 6, 2022
alice-i-cecile
alice-i-cecile approved these changes Oct 10, 2022
View reviewed changes
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.

Excellent docs, and I agree with this change.

bors r+

bors bot pushed a commit that referenced this pull request Oct 10, 2022
@irate-devil
Rename Transform::mul_vec3 to transform_point and improve docs (#…
9423cb6 
…6132)

The docs ended up quite verbose :v

Also added a missing `#[inline]` to `GlobalTransform::mul_transform`.

I'd say this resolves #5500

# Migration Guide
`Transform::mul_vec3` has been renamed to `transform_point`.

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

bors bot commented Oct 10, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Rename Transform::mul_vec3 to transform_point and improve docs [Merged by Bors] - Rename Transform::mul_vec3 to transform_point and improve docs Oct 10, 2022
@bors bors bot closed this Oct 10, 2022
@irate-devil irate-devil deleted the transform_point branch October 13, 2022 13:12
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
@irate-devil @james7132
Rename Transform::mul_vec3 to transform_point and improve docs (b…
ea36035 
…evyengine#6132)

The docs ended up quite verbose :v

Also added a missing `#[inline]` to `GlobalTransform::mul_transform`.

I'd say this resolves bevyengine#5500

# Migration Guide
`Transform::mul_vec3` has been renamed to `transform_point`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@irate-devil @james7132
Rename Transform::mul_vec3 to transform_point and improve docs (b…
deb17f0 
…evyengine#6132)

The docs ended up quite verbose :v

Also added a missing `#[inline]` to `GlobalTransform::mul_transform`.

I'd say this resolves bevyengine#5500

# Migration Guide
`Transform::mul_vec3` has been renamed to `transform_point`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
@irate-devil @Pietrek14
Rename Transform::mul_vec3 to transform_point and improve docs (b…
efd2ee0 
…evyengine#6132)

The docs ended up quite verbose :v

Also added a missing `#[inline]` to `GlobalTransform::mul_transform`.

I'd say this resolves bevyengine#5500

# Migration Guide
`Transform::mul_vec3` has been renamed to `transform_point`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@irate-devil @ItsDoot
Rename Transform::mul_vec3 to transform_point and improve docs (b…
85fd2a7 
…evyengine#6132)

The docs ended up quite verbose :v

Also added a missing `#[inline]` to `GlobalTransform::mul_transform`.

I'd say this resolves bevyengine#5500

# Migration Guide
`Transform::mul_vec3` has been renamed to `transform_point`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@IceSentry IceSentry IceSentry approved these changes

@tguichaoua tguichaoua tguichaoua approved these changes

Assignees
No one assigned
Labels
A-Transform Translations, rotations and scales C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

Global to Local methods for transforms
6 participants
@irate-devil @IceSentry @alice-i-cecile @tguichaoua @rparrett @NathanSWard