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

Overhaul Basis' documentation #87175

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 14, 2024

Continuation of #86664

Bugsquad edited:
Related to #87114, and several other past efforts several other past efforts.

This PR completely overhauls Basis' documentation to the point where it is almost unrecognisable

Why?

The problems with the current documentation are summarised fairly easily:

  • It is written under the assumption that the reader has a PhD in linear algebra, not for a developer.
  • It explains the inner workings of Basis too much.
  • It is just far, far too verbose sometimes, with no actual substance.
    • "Assuming that the matrix is a proper rotation matrix, slerp performs a spherical-linear interpolation with another rotation matrix."
    • "This operator multiplies all components of the Basis, which scales it uniformly."
    • "This can be used to validate if the basis is non-distorted, which is important for physics and other use cases."
    • "Assuming that the matrix is the combination of a rotation and scaling, return the absolute value of scaling factors along each axis." Guys, it's the length of every axis. That's it!

How does this PR fix it?

On the other hand, this PR does so many changes that, explaining the reasoning behind every single one of them is pretty difficult.
As more notes may be edited latter, let's get these out of the way:

  • Avoid advanced math terms at all costs:
    • "Orthonormal", "orthogonal", "transformation". Avoid mentioning them unless it's very appropriate.
    • Exception to terms like "normalized" can stay. They have a clear and common purpose in Godot.
  • Say "basis" consistently, avoid "basis matrix" or "matrix" alone.
    • Most users are reading this class reference for the Basis aspect of the... Basis. Mentioning its matrix a lot can potentially be confusing, unless strictly necessary for a given method.
      Advanced users know fully well it is a 3x3 matrix and will do as they please.
  • Elaborate on oh-so-many methods!
    • How should I structure these parameters? How is the returned value like? What does the operation I'm doing even mean?
    • In some cases, this is just not feasible, as it would not just be too complicated, but it would bloat the documentation with information most users wouldn't care about.
      This may be frowned upon, but sometimes a link to an external article is a must.

More specific notes:

  • Give full details on what Euler angles are:
    • What does each axis represent? What does the default EulerOrder do?
    • At first I was reluctant to link to Wikipedia article, but I was sold by the simple-to-follow visual examples.
  • Explain how the UNIFORM basis is structured;
  • Add a much-needed example for slerp();
  • Expose the formula for tdotx, tdoty, and tdotz;
  • Explain the difference between scripting and C++ source code's [] operator;
  • ...

Feedback is basically welcome.

This PR may contain multiple versions of the same line I was not sure about, denoted with "??". Reviewing of these lines is heavily appreciated.

Sources:

@Mickeon Mickeon requested a review from a team as a code owner January 14, 2024 13:41
@Mickeon Mickeon force-pushed the doc-peeves-basis-based-on-what branch from 1da297d to 31bb583 Compare January 14, 2024 13:42
@AThousandShips AThousandShips added this to the 4.3 milestone Jan 14, 2024
@fire fire requested a review from a team January 15, 2024 18:33
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
@TokageItLab
Copy link
Member

Avoid advanced math terms at all costs:
"Orthonormal", "orthogonal", "transformation". Avoid mentioning them unless it's very appropriate.
Exception to terms like "normalized" can stay. They have a clear and common purpose in Godot.

I suggest that these terms be left as needed. This is because explaining things like "whether the angles of all basis vectors are perpendicular" or "whether the scales are all 1's" every time would be redundant.

Instead, I reccomend you write an explanation of the term in Matrices and transforms and link to it.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 15, 2024

I suggest that these terms be left as needed. This is because explaining things like "whether the angles of all basis vectors are perpendicular" or "whether the scales are all 1's" every time would be redundant.

I am not sure how to feel about this, because the class reference as is in this PR does read just fine even without them.

Perhaps one solution is to add a note that later explains what those terms mean, in relation to the current method?

@Mickeon Mickeon force-pushed the doc-peeves-basis-based-on-what branch from 31bb583 to fb315b4 Compare January 16, 2024 00:37
@Mickeon Mickeon requested a review from fire January 16, 2024 00:37
@Mickeon Mickeon force-pushed the doc-peeves-basis-based-on-what branch from fb315b4 to 2891551 Compare January 17, 2024 12:03
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 17, 2024

I updated the PR with an additional line about the meaning of a few terms, as I wait from more precise feedback from @TokageItLab

When noting the length of the vector of basis matrices, it is necessary to specify that the right-hand orthogonal system is positive. This is because focusing only on length can lead to negative scales.

It is very difficult to understand this. Can you elaborate?

@TokageItLab
Copy link
Member

TokageItLab commented Jan 17, 2024

I simply point out that there is no definition of what the negative of the basis vector means in visually.

Actually, that definition may not be necessary until rendering, but if you are trying to draw/link a figure, you will need to annotate that the coordinate system of the Godot and the figure are same.

Also, it would be necessary to mention Row-major order and Column-major order. For those moving from other game engines, this can be confusing easily (and I have seen many such people). See also https://www.mindcontrol.org/~hplus/graphics/matrix-layout.html.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 17, 2024

Also, it would be necessary to mention Row-major order and Column-major order. For those moving from other game engines, this can be confusing easily (and I have seen many such people). See also https://www.mindcontrol.org/~hplus/graphics/matrix-layout.html.

I suppose this can be mentioned, yeah... It does sound important.

Actually, that definition may not be necessary until rendering, but if you are trying to draw/link a figure, you will need to annotate that the coordinate system of the Godot and the figure are same.

But I am not sure about. Would it not be for granted that both coordinate systems are the same?

@TokageItLab
Copy link
Member

As far as I know, mathematics basically uses the right-hand coordinate system, but in cases such as when a diagram created by someone working in DirectX is referenced, the left-hand coordinate system may be used and the direction of rotation, etc. may be flipped.

In order to counter such a claim from a DirectX user that the rotation is reversed, it is better to specify that Godot is in the right-hand coordinate system.

@aaronfranke previously wrote about the coordinate system for the asset pipeline https://docs.godotengine.org/en/4.1/tutorials/assets_pipeline/importing_scenes.html#d-asset-direction-conventions, but I am not sure if it is appropriate to link to that. Perhaps you can find a more appropriate link.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 17, 2024

Linking to more advanced "lessons" does sound good to me. The goal of this PR was to make the Basis class reference more accessible, so talking too much about advanced topics is a bit out of scope. Even my head is spinning about this...

As far as I know, mathematics basically uses the right-hand coordinate system, but in cases such as when a diagram created by someone working in DirectX is referenced, the left-hand coordinate system may be used and the direction of rotation, etc. may be flipped.

This is true... Different programs all use different coordinate systems.
As far as I Godot only uses two: one for 2D and another for 3D. In this PR, you may notice I tried to make the Basis' axes easier to imagine (The IDENTITY constant, x, y, and z descriptions). This looked good enough and in my opinion, this kind of information is MUCH more useful to be included in Transform3D's class reference (coming soon).

But if it indeed is not enough, I could pull my favourite reference sheet as a link for users to see: https://bevy-cheatbook.github.io/fundamentals/coords.html

Contains 3 vector fields X, Y and Z as its columns, which are typically interpreted as the local basis vectors of a transformation. For such use, it is composed of a scaling and a rotation matrix, in that order (M = R.S).
Basis can also be accessed as an array of 3D vectors. These vectors are usually orthogonal to each other, but are not necessarily normalized (due to scaling).
The [Basis] built-in [Variant] type represents a 3x3 [url=https://en.wikipedia.org/wiki/Matrix_(mathematics)]matrix[/url] used for 3D rotation, scale, and shearing. It is frequently used within a [Transform3D].
A [Basis] is composed by 3 axis vectors, each representing a column of the matrix: [member x], [member y], and [member z]. The length of each axis ([method Vector3.length]) influences the basis' scale, while the direction of all axes influence the rotation. Usually, these axes are perpendicular to one another. However, when you rotate any axis individually, the basis becomes sheared. Applying a sheared basis to a 3D model will make the model appear distorted.
Copy link
Member

Choose a reason for hiding this comment

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

You could make the argument that "distorted" applies to anything that doesn't pass the is_conformal check, including non-uniform scale. I'm not sure how specifically to rephrase it.

Copy link
Contributor Author

@Mickeon Mickeon Jan 17, 2024

Choose a reason for hiding this comment

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

One draft of the same sentence was in fact pointing is_comformal but I couldn't have it sounding nice or at the very least not overbearing. It needs to be mentioned though, so I need to find a way.

doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-basis-based-on-what branch from 2891551 to 8573670 Compare January 18, 2024 10:56
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 18, 2024

Thank you @TokageItLab and @aaronfranke very much for your time, here comes a second revision.

doc/classes/Basis.xml Outdated Show resolved Hide resolved
For a general introduction, see the [url=$DOCS_URL/tutorials/math/matrices_and_transforms.html]Matrices and transforms[/url] tutorial.
[b]Note:[/b] Godot uses the [b]right-hand coordinate system[/b] (+X is right, +Y is up, and +Z is back). This may be different from the coordinate system you are used to. For example, by convention, 3D assets are rotated towards the camera (+X is [b]left[/b], +Y is up, and +Z is [b]front[/b]). For more information, see the [url=$DOCS_URL/tutorials/assets_pipeline/importing_scenes.html#d-asset-direction-conventions]Importing 3D Scenes[/url] tutorial.
Copy link
Member

Choose a reason for hiding this comment

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

This is misleading, coordinate system handedness has nothing to do with the conventions of how assets are oriented. They are separate things.

To make an analogy, it's like saying "The USA uses US dollars. This may be different from what you are used to, for example, they use feet and inches instead of meters". That statement would imply that US dollars imply feet and inches. No, they are completely separate things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What could be said in this case, then?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, looking at this closer, Basis actually has no inherent handedness. I'm pretty sure that all of the same math methods will work exactly as-is if you used the same Basis code with a left-handed coordinate system. What makes Godot right-handed is its interpretation of the data we store in Basis.

The only places where conventions are encoded into Basis are 1) In the default Euler angle order YXZ which works best for Y-vertical, X-sides, Z-forward/back systems, and 2) The directions used by the looking_at method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the note about the coordinate system would be best suited for Transform3D, instead? Omitting it here may conflict with what @TokageItLab requested to mention above.

Copy link
Member

@TokageItLab TokageItLab Jan 18, 2024

Choose a reason for hiding this comment

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

Actually, that definition may not be necessary until rendering, but if you are trying to draw/link a figure, you will need to annotate that the coordinate system of the Godot and the figure are same.

As I said above, no coordinate system definition is required until rendering. However, since you seem to have links to some figures, so it would be better to indicate whether the figure and the result rendered by Godot match or not.

If we were to write in the document, it would be something like:

"The same matrix can be used for both right- and left-handed coordinate systems, what you can see there is a different drawing result. To match visual result with explanation of the direction of rotation or translation, when you refer to some illustrated documents from external link, we recommend that you refer to those drawn within the right-handed coordinate system, the same as Godot."

Copy link
Member

@aaronfranke aaronfranke Jan 18, 2024

Choose a reason for hiding this comment

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

To clarify, the preamble I wrote in the asset direction conventions section that Tokage linked is prerequisite information to explaining the asset direction conventions in terms of that coordinate system. So it's "Godot is X, and in terms of X, Godot is Y" not "Godot is X, and because of X, Godot is Y". An immediate minor improvement would be to remove the "For example" prefix to that sentence.

In terms of the documentation of the Basis structure, we need to make it clear specifically what documentation applies to Basis itself, what applies to its interpretation, and what applies to a specific method.

It may also be useful to mention specifically what engines don't use right-handed, instead of a vague notion of not being what you're used to. Right-handed is the dominant system, it's just a few outliers like Unity and Unreal that use left-handed.

I recommend that the text be changed to this:

Godot uses a [url=https://en.wikipedia.org/wiki/Right-hand_rule]right-handed coordinate system[/url]. Most software is right-handed, but this is different from the Unity and Unreal engines. Built-in types like [Camera3D] use the convention of +X is right, +Y is up, and +Z is back. Other objects within this coordinate system may use different direction conventions. For more information, see the [url=$DOCS_URL/tutorials/assets_pipeline/importing_scenes.html#d-asset-direction-conventions]Importing 3D Scenes[/url] tutorial.

And yeah it's not perfect, it doesn't explicitly state that Basis is mostly handedness-independent, I don't know if this is worth mentioning, it's probably fine as long as we make the rest of the words clear that they apply to Godot, Godot built-in types, and assets, not Basis itself. Feel free to tweak if needed. Any further elaboration of direction conventions should be localized to the description of the looking_at method IMO.

Oh and also, yes as Tokage mentioned, even if the math is handedness-independent, we must ensure that any diagrams we link are consistent.

@Mickeon Mickeon force-pushed the doc-peeves-basis-based-on-what branch from 8573670 to 383a9c6 Compare January 19, 2024 10:47
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 21, 2024

Also would like to note, I have purposely omitted looking_at from this PR because I believe it would be better to address the description of it and all other similar methods in a separate, focused PR.

doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

This is great! I don't see any more problems with this, it's a great improvement.

@akien-mga akien-mga merged commit c66f87d into godotengine:master Feb 12, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 12, 2024

The late-night merge!!!!! 🌙

@Mickeon Mickeon deleted the doc-peeves-basis-based-on-what branch February 13, 2024 15:54
@akien-mga akien-mga added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 11, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 11, 2024
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.

7 participants