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

Add update_skew to RemoteTransform2D #103445

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

daniel-szarkowicz
Copy link

Fixes #103368 by adding an update_skew property to the RemoteTransform2D.

@daniel-szarkowicz daniel-szarkowicz requested review from a team as code owners March 1, 2025 22:46
@Calinou Calinou added this to the 4.x milestone Mar 1, 2025
@daniel-szarkowicz daniel-szarkowicz force-pushed the RemoteTransform2D-update_skew branch from 2c2d7a1 to f231a11 Compare March 2, 2025 21:26
Copy link
Member

@KoBeWi KoBeWi 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 work correctly.

Looks like it will supersede #103377
CC @Rindbee

@daniel-szarkowicz daniel-szarkowicz force-pushed the RemoteTransform2D-update_skew branch from f231a11 to e51ea74 Compare March 3, 2025 22:02
@Rindbee
Copy link
Contributor

Rindbee commented Mar 4, 2025

set_rotation_scale_and_skew() is quite universal, but in some cases, it may not be the fastest. In cases where only scale or skew needs to be set, it may be faster to call the corresponding setter. Benchmarking may be required.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 4, 2025

I just noticed that code for updating local and global transform looks the same. It could be moved to a method.

@Rindbee
Copy link
Contributor

Rindbee commented Mar 4, 2025

[b]Note:[/b] Negative X scales in 2D are not decomposable from the transformation matrix. Due to the way scale is represented with transformation matrices in Godot, negative scales on the X axis will be changed to negative scales on the Y axis and a rotation of 180 degrees when decomposed.

The effect of negative x scale on rotation should be eliminated when update remote scale is disabled.

@daniel-szarkowicz daniel-szarkowicz force-pushed the RemoteTransform2D-update_skew branch from e51ea74 to 5dba27e Compare March 6, 2025 00:34
@daniel-szarkowicz daniel-szarkowicz requested a review from a team as a code owner March 6, 2025 00:34
@daniel-szarkowicz
Copy link
Author

[b]Note:[/b] Negative X scales in 2D are not decomposable from the transformation matrix. Due to the way scale is represented with transformation matrices in Godot, negative scales on the X axis will be changed to negative scales on the Y axis and a rotation of 180 degrees when decomposed.

The effect of negative x scale on rotation should be eliminated when update remote scale is disabled.

I don't think effect on the rotation can be negated reliably, so I added these notes to update_scale too. The negative y scale getting updated when update_scale is disabled was caused by a bug in Transform2D's set_scale, which now should be fixed.

Comment on lines +50 to +55
if (!update_remote_scale) {
our_trans.set_scale(n_trans.get_scale());
}
if (!update_remote_skew) {
our_trans.set_skew(n_trans.get_skew());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the elapsed time of these methods: set_scale() << set_rotation_scale_and_skew() < set_skew()

That is, if you don't just call set_scale(), it is better to call set_rotation_scale_and_skew() directly. You can single out the case where only set_scale() is called.

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.

RemoteTransform2D works inconsistently with skew
5 participants