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

Warn when setting a scale that has a negative X component in 2D #37376

Closed
wants to merge 1 commit into from

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Mar 28, 2020

Due to the way scale is represented with Transform2D, negative scales on the X axis will be changed to negative scales on the Y axis plus a rotation of 180 degrees when decomposed.

Because of this, we should show a warning. Fixes #17405 and its duplicates #21020, #21849, and #42219.

@aaronfranke aaronfranke added this to the 4.0 milestone Mar 28, 2020
@aaronfranke aaronfranke added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:core labels Mar 28, 2020
@realkotob
Copy link
Contributor

I don't disagree with adding a warning, but it should be clear why this is important for the user to know, or else it just seems like a technical comment you'd find in engine code.

It should explain the end result on the user side, even briefly, or else it's just noise.

@groud
Copy link
Member

groud commented Mar 28, 2020

I don't think this warning is a good idea, you might use an x negative scale on purpose.
Such warning would be really annoying.

@aaronfranke
Copy link
Member Author

@groud We still need a way to inform users that negative X scales aren't treated as such internally, since many users ask for help about this and many issues have been opened about this. Do you have another idea on how to warn users? Perhaps the warning should be able to be disabled in the editor settings?

Also, there are still ways to use a negative X scale on purpose without a warning. Maybe I should have been more explicit with the description. This doesn't show any warning when doing t.x *= -1, since setting t.x is a reliable way to change the X axis (and if you choose to decompose that, it will silently decompose into a rotation of 180 degrees and scale of (1, -1)).

@groud
Copy link
Member

groud commented Mar 28, 2020

No, even if it's possible to disable it I don't think it is a good idea.

I believe stating that in the documentation should be enough.

@aaronfranke
Copy link
Member Author

@groud Where in the documentation should this be stated? I'm not against a solution that only involves documentation changes.

@groud
Copy link
Member

groud commented Jul 2, 2020

I'd say in the reference, for the scale property of Node2D ? And maybe in the Transform2D reference too.

@ghost
Copy link

ghost commented Jul 31, 2020

There was this suggestion:

Maybe it would just be better to have .global_scale and .global_rotation calculated directly from the stored values instead of using the transform2D?

Or maybe we could add separate functions to retrieve the true scale and rotation.

I never saw any discussion in this direction, wondering if or why this isn't a viable solution?

@aaronfranke
Copy link
Member Author

I still think a warning is a good idea, but I just updated this PR so that there's a setting to disable the warning.

@aaronfranke aaronfranke force-pushed the scale-warning branch 2 times, most recently from de1caf7 to 3bb5502 Compare February 20, 2022 03:51
@reduz
Copy link
Member

reduz commented Jul 28, 2022

I dont think this PR makes much sense, Godot 2D nodes dont store tranforms but loc/rot/scale, so negative scale is fine. The only situation where this may work is whether for some reason in the editor the global transform is modified but I am not sure in which case this may happen.

@aaronfranke aaronfranke deleted the scale-warning branch July 28, 2022 15:27
@Zireael07
Copy link
Contributor

The OP lists four reports of what seems to be the same issue this fixes.

@aaronfranke
Copy link
Member Author

@Zireael07 Thanks, I went through and ensured they were closed appropriately. For reference, they are #17405, #21020, #21849, #31980, #42219, #62544, and #62663.

@Zireael07
Copy link
Contributor

Whoa, that's even more multiplicates than I thought. So it's not something once in a blue moon - I think the PR should be reconsidered.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 28, 2022

Yeah I myself closed a few of such reports. But if we go with a warning, there should be a way to disable it. It's only meant to inform the user about the unexpected behavior; if you know it, the warning is useless and annoying.

@aaronfranke
Copy link
Member Author

@KoBeWi That's what this PR does.

@YuriSizov YuriSizov removed this from the 4.0 milestone Sep 8, 2022
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.

global_scale property wrong in Node2D when scale is negative in x
7 participants