-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 Transform3D documentation #87334
Conversation
doc/classes/Transform3D.xml
Outdated
Returns the inverse of the transform, under the assumption that the transformation basis is orthonormal (i.e. rotation/reflection is fine, scaling/skew is not). Use [method affine_inverse] for non-orthonormal transforms (e.g. with scaling). | ||
Returns the inversed version of this transform. | ||
For this method to return correctly, the transform's [member basis] needs to be orthonormal (see [method Basis.orthonormalized]). In short, the basis should only represent a rotation. If it does not, use [method affine_inverse], instead. |
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.
A bit verbose IMO, will think of a more consise suggestion, but I'd suggest something like "Requires that..."
4ee2d08
to
438b987
Compare
438b987
to
ee7d2e9
Compare
ee7d2e9
to
592781d
Compare
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 the PR is in a good state to be taken a look at, with a few caveats.
If you know more people that would be capable of reviewing and providing additional info feel free to ask them for review, as well. Like Basis's docs, I feel like this is very important.
<description> | ||
Returns a copy of the transform translated by the given [param offset]. | ||
Returns a copy of this transform translated by the given [param offset]. | ||
This method is an optimized version of multiplying the given transform [code]X[/code] with a corresponding translation transform [code]T[/code] from the right, i.e., [code]X * T[/code]. | ||
This can be seen as transforming with respect to the local frame. | ||
</description> |
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.
ALL of these descriptions, yes, all of them need a massive change:
translated
, rotated
, scaled
and their equivalent *_local
methods.
But I have no idea what of this description is meaningful, what NEEDS to be said, and how to phrase it. I need way further feedback.
592781d
to
64ba22a
Compare
This was merged ignoring several review comments |
My bad, I missed those in my latest batch. |
I did not expect this to be merged as is, to be honest, but I'm happy about it... I can make a follow-up PR to address the above comments. |
Cherry-picked for 4.2.2. |
Related to #87181
This PR aims to completely overhaul the Transform3D type's documentation.
The reasoning and changelog are extremely similar to the "Overhaul Basis Documentation" PR, so I'm not sure I should bother repeating them twice.
More specific notes:
orthonormalized
,inverse
, andaffline_inverse
a lot more.lerp
's description as a base forinterpolate_with
.