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

Expose some useful method from rotation class Quaternion & Basis & Transform2D to GDScript #69202

Conversation

TokageItLab
Copy link
Member

Expose useful methods to GDScript for calculating rotations and scales.

@TokageItLab TokageItLab added this to the 4.0 milestone Nov 26, 2022
@TokageItLab TokageItLab requested review from a team as code owners November 26, 2022 08:56
@TokageItLab TokageItLab force-pushed the expose-useful-method-in-rotation-class branch from a2404ef to 88c9dc9 Compare November 26, 2022 08:59
@TokageItLab TokageItLab changed the title Expose some useful method from rotation classQuaternion & Basis in GDScript Expose some useful method from rotation classQuaternion & Basis to GDScript Nov 26, 2022
@TokageItLab TokageItLab force-pushed the expose-useful-method-in-rotation-class branch from 88c9dc9 to edc3468 Compare November 26, 2022 09:19
@TokageItLab TokageItLab force-pushed the expose-useful-method-in-rotation-class branch from edc3468 to 234f8e9 Compare November 26, 2022 10:35
core/variant/variant_call.cpp Outdated Show resolved Hide resolved
core/variant/variant_call.cpp Outdated Show resolved Hide resolved
core/variant/variant_call.cpp Show resolved Hide resolved
core/variant/variant_call.cpp Show resolved Hide resolved
core/variant/variant_call.cpp Show resolved Hide resolved
core/variant/variant_call.cpp Outdated Show resolved Hide resolved
core/variant/variant_call.cpp Show resolved Hide resolved
@TokageItLab TokageItLab marked this pull request as draft November 26, 2022 12:34
@TokageItLab TokageItLab force-pushed the expose-useful-method-in-rotation-class branch from 234f8e9 to 6267281 Compare November 26, 2022 14:06
@TokageItLab
Copy link
Member Author

TokageItLab commented Nov 26, 2022

Added some functions to Transform2D for scale, but I am not sure if the behavior is correct, so I will test it later.

@TokageItLab TokageItLab changed the title Expose some useful method from rotation classQuaternion & Basis to GDScript Expose some useful method from rotation classQuaternion & Basis & Transform2D to GDScript Nov 26, 2022
@TokageItLab TokageItLab force-pushed the expose-useful-method-in-rotation-class branch from 6267281 to 12241ab Compare November 29, 2022 16:33
@TokageItLab TokageItLab marked this pull request as ready for review November 29, 2022 16:34
@TokageItLab TokageItLab requested a review from a team as a code owner November 29, 2022 16:34
@TokageItLab
Copy link
Member Author

Finished testing the methods added in Transform2D. The calculation seems to be correct.

@TokageItLab TokageItLab force-pushed the expose-useful-method-in-rotation-class branch from 12241ab to 8d6690d Compare November 29, 2022 16:38
@@ -144,6 +156,18 @@ void Transform2D::translate_local(const Vector2 &p_translation) {
columns[2] += basis_xform(p_translation);
}

void Transform2D::orthogonalize() {
Copy link
Member

Choose a reason for hiding this comment

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

The problem with a method like this is that its very misleading. You cant call multiple times without incurring into precision loss. The size will mutate over time and deform the object.

Copy link
Member Author

@TokageItLab TokageItLab Dec 4, 2022

Choose a reason for hiding this comment

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

The same thing has already been done in Basis, but I can't think of any other way. Definitely there is an accuracy issue, but I assume that calling it multiple times (consectively) seems to be a corner case. Should I describe the possible loss of accuracy in the documentation?

Copy link
Member Author

@TokageItLab TokageItLab Dec 4, 2022

Choose a reason for hiding this comment

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

Added notes:

Note: When a Basis/Transform2D that is already orthogonalized uses this method, the equality of the values before and after is not guaranteed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of adding notes, I added a check is_orthogonal() to Basis and added a check Math::is_zero_approx(get_skew()) to Transform2D for making guarantee the equality.

Copy link
Member

Choose a reason for hiding this comment

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

One possibility for the Transform2D version is to always make the Y axis orthogonal to the X axis and leave the X axis untouched, this should more or less avoid precision errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented what you said, but the behavior is no different from setting skew = 0, so I don't think there is much point in having it. Well, I think it is fine to have the function for consistency with 3D.

Copy link
Member Author

Choose a reason for hiding this comment

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

for now

Vector2 y = Vector2(-(columns[0].y), columns[0].x).normalized(); // Make a vector orthogonal to x.
if (y.dot(columns[1].normalized()) < 0) {
	y = -y; // Flip to be closer to current y.
}
columns[1] = y * columns[1].length();

@TokageItLab TokageItLab force-pushed the expose-useful-method-in-rotation-class branch 2 times, most recently from 427e8f9 to 1cc4474 Compare December 4, 2022 21:01
@TokageItLab TokageItLab requested a review from reduz December 6, 2022 10:51
@YuriSizov YuriSizov changed the title Expose some useful method from rotation classQuaternion & Basis & Transform2D to GDScript Expose some useful method from rotation class Quaternion & Basis & Transform2D to GDScript Dec 7, 2022
@TokageItLab TokageItLab force-pushed the expose-useful-method-in-rotation-class branch from 1cc4474 to 5125ed2 Compare December 9, 2022 13:55
@AThousandShips
Copy link
Member

Note that adding mutating methods to the binds is not possible and doesn't work, see #62706

@TokageItLab TokageItLab deleted the expose-useful-method-in-rotation-class branch February 14, 2024 05:28
@AThousandShips AThousandShips removed this from the 4.x milestone Feb 14, 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