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

Fix local 3D translation editing #81609

Merged
merged 1 commit into from
Sep 16, 2023

Conversation

AThousandShips
Copy link
Member

Transformation was applied incorrectly, transformation is now smoother and doesn't jump incorrectly

@AThousandShips AThousandShips added this to the 4.2 milestone Sep 13, 2023
@AThousandShips AThousandShips requested a review from a team as a code owner September 13, 2023 09:25
@AThousandShips AThousandShips added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 13, 2023
if (p_local) {
p_motion = p_original.basis.xform(p_motion);
t = p_original_local;
t.origin += p_original_local.basis.xform(p_motion);
Copy link
Member Author

Choose a reason for hiding this comment

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

This matches the other cases and makes the movement smooth, using p_original makes the movement jerky with scale, which doesn't make sense to me, for example if it is scaled the motion is scaled up in the original implementation

@capnm
Copy link
Contributor

capnm commented Sep 15, 2023

I have tested my projects, the local 3D transformations/gizmos in the editor seem to work now properly 👍

@TokageItLab
Copy link
Member

I will test it today.

@capnm

This comment was marked as off-topic.

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 16, 2023

Unrelated to this PR, happens without it, please open a bug report for it on the latest version

(also in the future, to avoid giving contributors a fright lol, do test if some bug occurs only with a PR or not, and report it as a bug if it happens without it)

Edit: Also the wireframe error happens regardless of local coordinates, it seems like a bug with the updating of the gizmo

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

Seems to be working fine. 🙃

@kleonc kleonc removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 16, 2023
@kleonc
Copy link
Member

kleonc commented Sep 16, 2023

Removed cherrypick:4.1, it's 4.2.dev4 specific regression.

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 16, 2023

Have you tested that this doesn't happen in 4.1? The code is the same for the compute area (which is incorrect, unless some other tweak elsewhere expanded it)

@kleonc
Copy link
Member

kleonc commented Sep 16, 2023

Have you tested that this doesn't happen in 4.1? The code is the same

Tested v4.1.1.stable.official [bd6af8e], and 4.2.dev1/2/3/4 versions. The issue only happened in 4.2.dev4.

@AThousandShips
Copy link
Member Author

You're right, some other change there interact with it

Will take a look at making a pass over it to see how it interacts with itself and see if the discrepencies can be identified, but the current behavior is so glaring I feel it's best to get it fixed

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

I saw what it broke, but the problem is that translate is not converting correctly, which should be fixed in this PR. I tested this and it works fine.

#81604 (comment)

As for rotate the last argument does not match, but that is not a problem considering it is && true since rotate does not have a plane transform so it will always be true.

@akien-mga akien-mga merged commit 91dee34 into godotengine:master Sep 16, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the gizmo_fix branch September 16, 2023 20:07
@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants