Godot: Applying scale to a spatial turns it upside down

[kubecz3k]

Operating system or device - Godot version:
Linux, 53c23b02226968d27e6caadcb801343697ac4fe9

Issue description:
When we apply scale to the model that is looking in z positive direction, in a result it will be turned upside down:

I did bisection and I found that 53c23b02226968d27e6caadcb801343697ac4fe9 introduced this behavior

Steps to reproduce:

1. Download reproduction project.
2. Open res://Issues/YFlipIssue/YFlipIssue.tscn
3. Run it.

Alternatively create new spatial and add this code as it's script:

func _ready():
    look_at(global_transform.origin + Vector3(0,0,1), Vector3(0,1,0));

func _process(delta):
    var lscale = get_scale();
    set_scale(lscale)

Link to minimal example project:
upsidedown_issue.zip

[kubecz3k]

cc @tagcup

[ghost]

Haven't checked what's going on in detail but look_at shouldn't be used with scale (will update the doc on this). You can create an auxiliary parent/child node for it, or use a proper axis-angle rotation.

[ghost]

Based on that script, it doesn't look right though. Let me take a closer look at what's going on here later this week.

[kubecz3k]

@tagcup it's not the subject of this issue but not sure why you are talking me look_at should not be used together with set_scale. I was using those two commands in close neighborhood since 1.0 and never had problems with this... Well this is the first time :)

[ghost]

Because it will just reset your scale, see #10003.

[kubecz3k]

@tagcup that's why I'm using them next to each other in the first place :) I have even shortcut for this in my personal Global singleton :D

var lscale = get_scale();
look_at(point, up_dir);
set_scale(lscale)

[kubecz3k]

@tagcup I wanted at some point to make pr for look_at that would do just that, do you think it's a bad idea to have this as a part of codebase?

[ghost]

Are you sure you got thst behaviour after that commit? Because you're using get/set_scale and look_at which has nothing to do with that commit.

I'm away from my desktop computer this week, can you post the transform matrix of that object and it's determinant before calling any functions on it? It may be a problem with the Euler angle hack (is it introducing a reflection?). Also can you post the scale, before and after?

[kubecz3k]

tagcup, I'm sure it's this commit, I did bisection, if you will have some free time you could download sample project and check if yourself... Off course it's possible bug is somewhere else and that pr just revealed it...
Also I'm not in rush with this, so this don't need to be solved this week but it would be great if you could investigate when you will have time... Also I can check if it's determinant, but I think you would get your answers quicker if you could check sample project, it's super easy, basically an object with provided code. Scale is default (1,1,1) in the editor

[ghost]

Again, I'm away from my computer this week, this is why I asked, anyway, I'll check after I go back then

[kubecz3k]

Sorry in meantime I was experimenting and that was transform/basis of the parent :) (deleted those comments)
actual info below:
basis:

model transform.basis before look_at: 
((1, 0, 0), (0, 1, 0), (0, 0, 1))
model transform.basis after look_at: 
((-1, 0, 0), (0, 1, 0), (0, 0, -1))
model transform.basis before set_scale: 
((-1, 0, 0), (0, 1, 0), (0, 0, -1))
model transform.basis after set_scale: 
((1, 0, 0), (0, -1, -0), (0, 0, -1))

transform:

model transform before look_at: 
1, 0, 0, 0, 1, 0, 0, 0, 1 - 0, 0, 0
model transform after look_at: 
-1, 0, 0, 0, 1, 0, 0, 0, -1 - 0, 0, 0
model transform before set_scale: 
-1, 0, 0, 0, 1, 0, 0, 0, -1 - 0, 0, 0
model transform after set_scale: 
1, 0, 0, 0, -1, -0, 0, 0, -1 - 0, 0, 0

[ghost]

Thankas, could yoy also post lscale too?

Euler angles aren't used at no point in that code, so at this point it doesn't make any sense.

get/set_scale, however, isn't supposed to be used like that. As you can read in its code doc, get_scale uses a certain convention to decompose basis, and it may introduce a rotation. get_scale makes sense only when used together with get_rotation which uses the same convention. As such, please don't make a PR for it.

It was probably a mistake on my side to add and expose get/set scale/rotation as it is very easy to get things wrong. I'm going to un-export them next week because it doesn't do what you think it does.

[kubecz3k]

@tagcup so how we should rotate objects in 3d?

[kubecz3k]

@tagcup lscale is not very interesting: lscale: (1, 1, 1)

[kubecz3k]

also not giving me ability to get/set scale would be quite horrible for my game, where scale of enemies represents their health... It's one of the most basics things I can think of, I'm sorry but I'm surprised by the things you are telling me :o
edit: but still it's quite possible I misunderstood you :)

[ghost]

Those functions weren't there in 2.x, I added them only recently.

You can rotate the objects using 2.x API. You'll need to store the scale separately, or use an auxiliary parent or child for scale.

The problem with those functions is that they require user to understand how they work, and how rotations and scaling work, which is clearly totally unrealistic.

[kubecz3k]

@tagcup those functions were there for sure, I'm using them since 1.0 I think, the project where I have problem is practically pure conversion form 2.x .... We are talking about Spatial.set/get_scale? :P

[ghost]

I've mentioned this time and again, I'll make an issue for this when I return so that I can simply refer to it.

The problem here is that we're not storing rotation and scaling information separately. We only store basis, and it's impossible to decompose basis into scaling and rotation uniquely uaing only that information.

A simple cartoon analogue of that problem is, you can factor 6 as 2 x 3 or -2 x -3, they're both equivalent.

[ghost]

The functions themselves may have been there but there was no polar decomposition, meaning it was broken. It'd give wrong results if you have negative scaling or reflection equivalently.

Anyway there is no bug here. That's just not how it worka.

[ghost]

I think get_scale was there but probably not others.

[ghost]

I has some ideas about how to nicely store rotation and scaling separately and I tried to ask @reduz if he'd be OK with such a change but so far I never got a reply from him.

[kubecz3k]

@tagcup just to be sure, you are telling me that reading scale can change state of my spatial? sorry I think I misread something

[ghost]

I checked and in 2.x, there is indeed no set_scale nor any analog for rotations.

[kubecz3k]

@tagcup There definitelly is: http://i.imgur.com/gsEoID6.png
I'm feeling like we are talking about two different things somehow...

[kubecz3k]

@tagcup I checked even in 1.1 since I still have it on the disk and api is the same...

[ghost]

No it's set_scale that doesn't work as you'd expect.

[ghost]

No I'm talking about set scale not get scale. There is no such function, I introduced it, and it never appeared in a stable release so far.

[kubecz3k]

well apparently I expect from new one to work the old way... but ok, I dont think I'm able to dig into matrix math internals at the moment, would be nice if @reduz could look before I will close this issue.

[kubecz3k]

@tagcup to have some diversity this time screen is from 1.1 http://i.imgur.com/b7s5Xop.png :P

[akien-mga]

@tagcup Check this screenshot again, there is definitely set_scale(Vector3): http://i.imgur.com/gsEoID6.png

http://docs.godotengine.org/en/stable/classes/class_spatial.html?highlight=set_scale#class-spatial-set-scale

You seem to have different conceptions of what the scale is. What @kubecz3k expects (and I believe most users will expect the same) is that the scale should work in 3D like it does in 2D: it scales the width, heigh (and here, depth) of the node. So if you want to shrink all dimensions by 2, you set the scale to (0.5, 0.5, 0.5). It should not be dynamic and related to the rotation or translation, it should be applied on the transform to scale it up or down.

[ghost]

You both are looking at the wrong place, check Matrix3 docs. Spatial set scale does something else

[kubecz3k]

@tagcup but the code and issue is related to Spatial. I'm not and never was messing with Matrix3 scale directly

[ghost]

Err did something else

[kubecz3k]

@tagcup to make me happy I need to:

1. be able to get forward/up/right directions so we have Spatial.get_transform().basis. z / y / x
2. be able to rotate object with vector math (direction vector) so we have Spatial.look_at
3. be able to get and set scale of a spatial, so we have Spatial.set_scale / get_scale

[ghost]

To make you happy I need to stuff? What's that figure of speech? You think you're my boss on anything? I don't care whether you're happy or not.

Anyway I'll get rid of set scale and set rotation of Basis I introduced next week.

[ghost]

Go teach yourself some math before making wild claims

[kubecz3k]

@tagcup I'm very sorry my intention was not to offend you. I think you are doing great job for Godot and whole community. I just wanted to make sure we are at the same level of abstraction, I'm very sorry for such unfortunate wording.

[akien-mga]

To make you happy I need to stuff? What's that figure of speech? You think you're my boss on anything? I don't care whether you're happy or not.

Go teach yourself some math before making wild claims

Mind your attitude. Ever heard of human beings who happen not to be native English speakers?

And read again. "to make me happy I (me, @kubecz3k, not you) need to" does not require anything of you, so don't be an ass. It's pretty obvious that @kubecz3k expresses his view of how the API should be like for his use case. Then the math expert (you) can validate or invalidate those expectations based on their better knowledge of the internal workings, without dissing their "lesser knowing" interlocutor.

[ghost]

I kniw very well what he said back there, OK. Don't tell me to mind my language, it's not my first language either.

I'm away, busy, at a conference, I'll make changes when I'm back.

You know, I explained the same thing many times, it's there in the code, a lengthy one, it's there briefly in classes.xml, it's in other issues, it's in this issue. How many times do I have to explain the same thing such that you'll stop blaming me for dismissing things without giving an explanation?

You're super biased agaianst me, and I'm getting tired of being treated like that.

[akien-mga]

I'm away, busy, at a conference, I'll make changes when I'm back.

Thanks, but no need to rush anything, we're just discussing here, not requiring any change from you. No pressure.

You know, I explained the same thing many times, it's there in the code, a lengthy one, it's there briefly in classes.xml, it's in other issues, it's in this issue. How many times do I have to explain the same thing such that you'll stop blaming me for dismissing things without giving an explanation?

But if questions keep popping up, there are two explanations:

• the whole world is dumb and there is nothing we can do to improve things
• the way things work is not user friendly, and we should consider if it can be made more user friendly - a game engine is all about making complex stuff easy to use for people without the technical background which was needed to implement the complex API.

You're super biased agaianst me, and I'm getting tired of being treated like that.

I am not, but I guess that would be best further discussed offline.

[reduz]

Geez guys, problem is solved using the apply_scale function, set_scale is not supposed to work in this situation

[reduz]

Need more docs on this I guess

[reduz]

An no fixing is needed.

[ghost]

And you can simply use scale/scaled (rather than set_scale) to achieve that particular behaviour as a workaround, as long as you don't have reflections/negative scalings

[kubecz3k]

@reduz unfortunatelly apply_scale is only in 2D at the moment

[kubecz3k]

should I port it to Spatial?

[kubecz3k]

hmm but it's using set_scale basically in the same way how I'm using it in this issue:

void Node2D::apply_scale(const Size2 &p_amount) {
    set_scale(get_scale() * p_amount);
}

edit: mean 'uniformly'

[ghost]

Before making any changes in the API, can you wait until I check what this has to do with Euler angles change (I'm reiterating this but this may be related to a hack in Euler angles)? I need to pin point the issue first

[kubecz3k]

@tagcup sure, I resigned from doing any changes anyway after I saw what apply_scale is doing in 2D. I'm never using negative scale, and my scale is always uniform. Also don't worry about timing, I'm near the computer that's why I'm replying so fast, I'm not in rush in any case.

[ghost]

So but either way, set_scale and set_rotation need to go, because setting only the scale without touching rotation in a non-surprising way, vice versa, requires a unique polar decomposition, which is not possible without storing additional info in Basis.

I agree that there needs to by a scale/scaled in Spatial (if there isn't already), which will apply a scaling on top of the current transformation.

[reduz]

Maybe the solution to these problems is to make TRS mode explicit and
visible to the user

On Aug 16, 2017 6:15 PM, "Ferenc Arn" [e-mail] wrote:

I had a quick look, there appears to be a typo:

elements[2][2] < 0) { // use pure x

Assuming the comment showing the matrix layout doesn't involve any furthee
typos, this needs to be elements[1][1] < 0

I'll try to check more later on

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/10352#issuecomment-322901722,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z244LWDJoh9Pt4MlD77r3kCRpJS7Mks5sY1wHgaJpZM4O3qfT
.

[reduz]

Right now it's transparent

On Aug 16, 2017 6:33 PM, "Juan Linietsky" [e-mail] wrote:

Maybe the solution to these problems is to make TRS mode explicit and
visible to the user

On Aug 16, 2017 6:15 PM, "Ferenc Arn" [e-mail] wrote:

I had a quick look, there appears to be a typo:

elements[2][2] < 0) { // use pure x

Assuming the comment showing the matrix layout doesn't involve any
furthee typos, this needs to be elements[1][1] < 0

I'll try to check more later on

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/10352#issuecomment-322901722,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z244LWDJoh9Pt4MlD77r3kCRpJS7Mks5sY1wHgaJpZM4O3qfT
.

[ghost]

That Euler angle hack doesn't seem to be correct actually, let me work it out again.

@reduz, yeah, it would solve a multiple claas of issues, help a lot with precision if implemented correctly. Would you be OK with changing Basis such that is stores scaling (3 reals), and maybe rotation (further 3 reals) separately? They need to be synchronized with the 3x3 matrix through accessor functions, and we'd need to forbid direct arbitrarily write access to matrix elements since it can results in something beyond rot/scale.

[reduz]

@tagcup No.. if that was the case, I would prefer to store Quaternion like Unity does and do away with the transform alltogether. Problem is that using transform directly is really handy for so many things once you know how to actually use it..

[reduz]

I mean, when I write games, I never really ever touch euler. To me it's completely unnecesary. In fact, when Godot was release this feature was missing. It was added later only because it's more accessible to people without enough linear algebra background

[ghost]

? Unity does use Transform and I'm guessing that is more or less how it works.

Rotation part can be implemented using a quaternion too, and matrix representation can be updated whenever rotation/scaling changes.

How would you split rotation and scaling explicit?

[reduz]

@tagcup I am thinking that, probably, the best solution to this is to:
1) Make the mode explicit, you can select whether you want to use TRS or Transform
2) functions like translate/scale/look_at_etc, might have two versions, that work depending on the selected mode

[reduz]

i mean, set_rotation(euler) internally checks mode and either sets rotation or the transform basis

[ghost]

And how would TRS mode work? What would be the internal data representation? How would the Basis matrix be generated? (On the fly? Generate on demand? Update when rotation/scaling changes?

BTW there are no fundamental issues regarding translations

[ghost]

I'm assuming here you need a matrix representation of transformation for rendering, physics, etc. at sone point, regardless of the mode

[reduz]

did you check the current Spatial implementation, it already has a TRS mode, except it's automatic:
https://github.com/godotengine/godot/blob/master/scene/3d/spatial.h#L72
https://github.com/godotengine/godot/blob/master/scene/3d/spatial.cpp#L344

[ghost]

It uses get_scale and get_rotation of Basis, which is the root of the problem. Another reason why I think this should be handled in Basis is, Basis is used in classes that don't involve Spatial, and users probably want direct access to Transform too.