[go: up one dir, main page]

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mickeon
Copy link
Contributor
@Mickeon Mickeon commented Nov 24, 2024

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.

@@ -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]).
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 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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor
@tetrapod00 tetrapod00 Nov 24, 2024

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)

Copy link
Contributor Author

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.

@fire fire requested review from a team November 24, 2024 20:19
@Mickeon Mickeon force-pushed the documentation-transform-basis-quaternion-consistency branch from 296a30d to a3c913b Compare November 25, 2024 16:27
@Mickeon Mickeon marked this pull request as ready for review November 25, 2024 16:27
@Mickeon Mickeon requested a review from a team as a code owner November 25, 2024 16:27
# | 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.
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 believe this is the opposite ("When a variant is transformed by this constant")? This current text was derived from the original, rather vague statement.

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 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#.
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 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:
Copy link
Contributor Author

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.

@Mickeon Mickeon force-pushed the documentation-transform-basis-quaternion-consistency branch from a3c913b to d7ded61 Compare November 25, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-uniform scaled basis returns wrong euler angles
3 participants