Skip to content

Clarify Transform and GlobalTransform documentation (adopted) #15358

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
Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Clarify Transform and GlobalTransform documentation (adopted) #15358

wants to merge 16 commits into from

Conversation

bas-ie
Copy link
Contributor

@bas-ie bas-ie commented Sep 22, 2024 •
edited by alice-i-cecile
Loading

NOTE: this is adopted from Selene-Amanita's #9182. I've:

  • updated the branch
  • tried to apply rparrett's suggestions

Original PR description follows. Because it's from last year, it'd be great to get fresh eyes on it to see if it inadvertently removes things we'd like to keep. I also have one question about documentation links, below.

Objective

The doc about Transform and GlobalTransform mention "the reference frame", which can be confusing for two reasons:

  • a reference frame is y definition relative, Transform is in the reference frame of the parent,
  • not everybody is familiar with this term, and some people might see "frame" and think it is talking about viewport coordinates. (I actually helped someone on discord who was confused because of that, hence this PR)

Also:

  • some links are broken
  • the documentation doesn't detail the meaning of translation, rotation and scale
  • the documentation uses the term "position" which is misleading

This PR tries to fix that.

Solution

  • Specify "local" and "absolute"
  • Specify "in space" (I could have said "in the world" but that could be confusing because of the ECS world, I would also have said "2D/3D space")
  • Specify "main reference frame"
  • fix the links
  • add a section to detail the meaning of translation, rotation and scale
  • use the term "transform", after defining it, instead of "position"

@bas-ie bas-ie mentioned this pull request Sep 22, 2024
Closed
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation 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 22, 2024
pablo-lua
pablo-lua approved these changes Sep 22, 2024
View reviewed changes
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

Looks clear and now populated with useful examples of what to do or how to do. All good.

@pablo-lua pablo-lua added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 22, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 22, 2024
mweatherley
mweatherley suggested changes Sep 22, 2024
View reviewed changes
crates/bevy_transform/src/components/global_transform.rs Outdated
@@ -9,33 +9,62 @@ use bevy_reflect::{std_traits::ReflectDefault, Reflect};
#[cfg(all(feature = "bevy-support", feature = "serialize"))]
use bevy_reflect::{ReflectDeserialize, ReflectSerialize};

/// [`GlobalTransform`] is an affine transformation from entity-local coordinates to worldspace coordinates.
/// Describes the absolute transform (translation, rotation, scale) of an entity in space,
Copy link
Contributor

Choose a reason for hiding this comment

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

The (translation, rotation, scale) part is a bit misleading, since GlobalTransform is not a TRS transformation in general. This also applies to the later part of this doc comment which breaks down translation, rotation, and scale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I might see if I can walk that part back a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mind re-checking, just to see if I've understood you correctly? Cheers!

@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Sep 22, 2024
@alice-i-cecile @mweatherley
Should not mutate GlobalTransform, not cannot
494e9fa 
Co-authored-by: Matty <weatherleymatthew@gmail.com>
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 22, 2024
@rparrett
Copy link
Contributor

Could we remove my tagged username from the description so it doesn't end up in a commit message?

@alice-i-cecile
Copy link
Member

Could we remove my tagged username from the description so it doesn't end up in a commit message?

Removed to prevent endless notification spam for both of you.

@bas-ie
Copy link
Contributor Author

bas-ie commented Sep 22, 2024

Could we remove my tagged username from the description so it doesn't end up in a commit message?

Sorry!

@bas-ie bas-ie requested a review from mweatherley September 22, 2024 22:13
BenjaminBrienen
BenjaminBrienen suggested changes Jan 23, 2025
View reviewed changes
crates/bevy_transform/src/bundles.rs
///
/// [`Transform`] is the local transform (translation, rotation, scale) of an entity in space.
/// If the entity has a parent, the local transform is relative to its parent's
/// [`GlobalTransform`], otherwise, it's relative to the main reference frame.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// [`GlobalTransform`], otherwise, it's relative to the main reference frame.
/// [`GlobalTransform`]; otherwise, it's relative to the main reference frame.

crates/bevy_transform/src/components/global_transform.rs
Comment on lines +28 to +29
/// [`Transform`] is the local transform of an entity in space relative to its parent transform,
/// or the global transform relative to the main reference frame if it doesn't have a [`Parent`].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// [`Transform`] is the local transform of an entity in space relative to its parent transform,
/// or the global transform relative to the main reference frame if it doesn't have a [`Parent`].
/// [`Transform`] is the local transform of an entity in space relative to its parent transform;
/// if it doesn't have a [`Parent`], then it is equivalent to the global transform relative to the main reference frame.

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 think I'd go with a separate sentence here.

crates/bevy_transform/src/components/transform.rs
Comment on lines +45 to +46
/// - The `scale` is relative to the parent's [`GlobalTransform`], it is multiplied
/// by it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// - The `scale` is relative to the parent's [`GlobalTransform`], it is multiplied
/// by it.
/// - The `scale` is relative to the parent's [`GlobalTransform`] (it is multiplied in)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm, I'm not particularly convinced by either one. Maybe simply,

The scale is multiplied by the parent entity's [GlobalTransform].

?

crates/bevy_transform/src/components/transform.rs
Comment on lines +52 to +53
/// [`Transform`] is the local transform of an entity in space relative to its parent transform,
/// or the global transform relative to the main reference frame if it doesn't have a [`Parent`].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// [`Transform`] is the local transform of an entity in space relative to its parent transform,
/// or the global transform relative to the main reference frame if it doesn't have a [`Parent`].
/// [`Transform`] is the local transform of an entity in space relative to its parent transform;
/// if it doesn't have a [`Parent`], then it is the global transform relative to the main reference frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I'll add a sentence here too.

crates/bevy_transform/src/components/transform.rs
/// [`TransformPropagate`].
/// Those systems run during [`PostUpdate`].
/// If you update the [`Transform`] of an entity in this schedule or after,
/// you may notice a 1 frame lag before the [`GlobalTransform`] is updated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// you may notice a 1 frame lag before the [`GlobalTransform`] is updated.
/// you may notice 1 frame of lag before the [`GlobalTransform`] is updated.

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 should update to use "one" 🤔

bas-ie and others added 6 commits January 29, 2025 08:52
@bas-ie @BenjaminBrienen
Update crates/bevy_transform/src/components/global_transform.rs
a7448b1 
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
@bas-ie @BenjaminBrienen
Update crates/bevy_transform/src/components/global_transform.rs
6280762 
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
@bas-ie @BenjaminBrienen
Update crates/bevy_transform/src/components/global_transform.rs
0a3bfb4 
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
@bas-ie @BenjaminBrienen
Update crates/bevy_transform/src/components/transform.rs
bb18c4f 
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
@bas-ie @BenjaminBrienen
Update crates/bevy_transform/src/components/transform.rs
c8d2998 
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
@bas-ie @BenjaminBrienen
Update crates/bevy_transform/src/components/transform.rs
ccf1071 
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
@bas-ie
Copy link
Contributor Author

bas-ie commented Jan 28, 2025

Thanks for the review! I've neglected this PR, will try to get back to it today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenjaminBrienen BenjaminBrienen BenjaminBrienen requested changes

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

@pablo-lua pablo-lua pablo-lua approved these changes

@rparrett rparrett Awaiting requested review from rparrett

@Selene-Amanita Selene-Amanita Awaiting requested review from Selene-Amanita

@mweatherley mweatherley Awaiting requested review from mweatherley

Assignees
No one assigned
Labels
A-Transform Translations, rotations and scales C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bas-ie @rparrett @alice-i-cecile @mweatherley @BenjaminBrienen @pablo-lua @Selene-Amanita