-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Make Transform2D/3D, Basis, and Quaternion docs more consistent #99650
base: master
Are you sure you want to change the base?
Make Transform2D/3D, Basis, and Quaternion docs more consistent #99650
Conversation
@@ -131,6 +131,7 @@ | |||
- The [member Vector3.y] contains the angle around the [member y] axis (yaw); | |||
- The [member Vector3.z] contains the angle around the [member z] axis (roll). | |||
The order of each consecutive rotation can be changed with [param order] (see [enum EulerOrder] constants). By default, the YXZ convention is used ([constant EULER_ORDER_YXZ]): Z (roll) is calculated first, then X (pitch), and lastly Y (yaw). When using the opposite method [method from_euler], this order is reversed. | |||
[b]Note:[/b] This method assumes the basis is [i]uniform[/i] (all axes share the same length) (see [method get_scale]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This closes #90727
However, the alternative is to move this information on top. I don't think it deserves to be a note (without providing a workaround) because you absolutely need to know this information to use this method properly.
Returns this basis's rotation as a [Vector3] of [url=https://en.wikipedia.org/wiki/Euler_angles]Euler angles[/url], in radians. To prevent unexpected results, the basis must be [i]uniform[/i] (all axes share the same length) (see [method get_scale]).
or, likely too subtle:
Returns this [i]uniform[/i] basis's rotation as a [Vector3] of [url=https://en.wikipedia.org/wiki/Euler_angles]Euler angles[/url], in radians.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is uniform well-defined somewhere?
We have different definitions of uniform, uniform scale, orthogonal vs is only a rotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Uniform" in this context is defined in Basis' leading description among others:
A [Basis] is [b]orthogonal[/b] if its axes are perpendicular to each other. A basis is [b]normalized[/b] if the length of every axis is [code]1[/code]. A basis is [b]uniform[/b] if all axes share the same length (see [method get_scale]). A basis is [b]orthonormal[/b] if it is both orthogonal and normalized, which allows it to only represent rotations. A basis is [b]conformal[/b] if it is both orthogonal and uniform, which ensures it is not distorted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note, but that intro section might be clarified with something like:
A [Basis] is:
- [b]orthogonal[/b] if its axes are perpendicular to each other.
- [b]normalized[/b] if the length of every axis is [code]1[/code].
- [b]uniform[/b] if all axes share the same length (see [method get_scale]).
- [b]orthonormal[/b] if it is both orthogonal and normalized, which allows it to only represent rotations.
- [b]conformal[/b] if it is both orthogonal and uniform, which ensures it is not distorted.
I don't know if a split sentence like that translates well, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lists are awesome but currently they take too much space in the built-in documentation because unordered lists are not supported. Each new line adds a whole other line to simulate a paragraph. When I made the PR originally I was probably accounting for this. But... sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know. Unordered lists are used in a few places for parameters in method descriptions, but I think the vertical space is more acceptable there.
(They do render pretty well in the online class reference, FWIW)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that's accidental because -
is not escaped and it conveniently denotes a list for that markdown format. We should still do "lists" like this for general consistency. Look at #88900 , too.
296a30d
to
a3c913b
Compare
doc/classes/Transform3D.xml
Outdated
# | 0 | 1 | 0 | 0 | ||
# | 0 | 0 | 1 | 0 | ||
[/codeblock] | ||
When this constant is transformed (multiplied) by a [Vector3], an [AABB], or another [Transform3D], no transformation occurs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the opposite ("When a variant is transformed by this constant")? This current text was derived from the original, rather vague statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done the change as I feel confident about this.
# | 0 | 0 | 1 | 0 | ||
[/codeblock] | ||
When this constant is transformed (multiplied) by a [Vector3], an [AABB], or another [Transform3D], no transformation occurs. | ||
[b]Note:[/b] In GDScript, this constant is equivalent to creating a [constructor Transform3D] without any arguments. It can be used to make your code clearer, and for consistency with C#. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am heavily considering removing "It can be used to make your code clearer, and for consistency with C#." it feels... for granted, or perhaps not the right place for recommendations(?).
Instead, I wanted to mention how this is basically the default value for Basis/Transform2D/Transform3D/Quaternion
@@ -181,8 +181,25 @@ | |||
</members> | |||
<constants> | |||
<constant name="IDENTITY" value="Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)"> | |||
A transform with no translation, no rotation, and its scale being [code]1[/code]. Its [member basis] is equal to [constant Basis.IDENTITY]. | |||
When multiplied by another [Variant] such as [AABB] or another [Transform3D], no transformation occurs. | |||
The identity [Transform3D]. This is a transform with no translation, no rotation, and not scaled. Its [member basis] is equal to [constant Basis.IDENTITY]. This also means that: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason why "and its scale being [code]1[/code]" was changed is because, nitpicky, it's technically incorrect. The "scale" is Vector3.
a3c913b
to
d7ded61
Compare
Fixes #90727
This PR aims to be the glue that holds the following together:
It's an assortment of general tweaks and fixes for consistency between all 4 class reference pages, based off of prior reviews.
For people reading this, you may use this opportunity to point out oddities between all 4 so that they can be polished together.