godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Enable transform chaining by making scaled/rotated/translated consistent #1336

Closed bluenote10 closed 2 years ago

bluenote10 commented 4 years ago

Describe the project you are working on:

A guitar practicing application (here is an old version but in VR).

I suspect the reasons why I'm facing this issue more than others are:

However the issue I'm facing is a fairly fundamental one, and can affect almost any project.

Describe the problem or limitation you are having in your project:

Basically every time I try to chain .scaled/.rotated/.translated I end up with a bug. I had raised this issue before https://github.com/godotengine/godot/issues/34329, which has been closed by adding a line to the documentation. This line didn't help much, I still run into the same problem a lot. I finally had the time to analyze why that is, and since 4.0 is the chance to get things right, here is my proposal.

Note: I'm discussing the topic based on Transform, but both the problem and solution would also apply for Transform2D as far as I can see.

Also feel free to read the much shorter proposal (down below) first, and only dive into the lengthy reasoning behind it (above) if need be ;).

Terminology

Chaining transformation always requires to be aware of left-to-right or right-to-left thinking, because the "mathematical reading order" is typically opposite to the "transformation order". For instance

x' = C · B · A · x

first transforms x by A, then by B, then by C, opposite to how the mathematical equation is typically written. Put another way:

Current behavior

Currently the behavior is a mix of left and right multiplication.

Issue 1: Hard to read

Because of mixing left and right multiplication, I find it fairly hard to look at chained expressions and come up with the underlying mathematical order. Going from the code to the mathematical expression cannot be done by just reading in one direction, but rather requires to switch between left-to-right and right-to-left thinking. For instance:

var M = Transform.IDENTITY\
    .scaled(... S ...)\
    .translated(... T ...)\
    .rotated(... R ...)

is equivalent to (if I didn't get it wrong again)

M = R · S · T

Note how R has moved from last to first, S has moved to the middle, and T ended up at the end. The result feels almost like a random shuffle of the order written in the code. Doing such transformations on longer expressions is a challenging (and unnecessary) mental exercise.

Issue 2: Hard to write

The problem is even more tricky the other way around, when trying to convert a mathematical expression into code.

Example 1: Imagine your goal is to write the following purely with chaining:

M = S · T · R

As far as I can see this actually cannot be written purely with chaining, because having the .translated in the middle breaks the right-to-left flow, and there is no way get the R in the right position.

The only way to write it is in a non-chained way, for instance:

var M = Transform.IDENTITY\
    .scaled(... S ...)
    .translated(... T ...)
M *= Transform.IDENTITY.rotated(... R ...)

Example 2: Imagine implementing longer transform chains like:

M = R_2 · T_2 · R_1 · S_2 · T_1 · S_1

Trying to work out the code becomes more and more awkward, because it is necessary to split the expression into subgroups at each T_*, which break the right-to-left flow. The individual groups can be assembled right-to-left, but need to be assembled in an outer multiplication left-to-right. An alternative is to manually implement translation left multiplication with the trick to use temporary.offset += T_*. In any case the resulting code is much less clear than a full chaining expression (if .translated would do left-multiplication as well):

var M = Transform.IDENTITY\
    .scaled(... S_1 ...)\
    .translated(... T_1 ...)\
    .scaled(... S_2 ...)\
    .rotated(... R_1 ...)\
    .translated(... T_2 ...)\
    .rotated(... R_2 ...)

Issue 3: Performance aspects

In general writing transforms as chains is faster than using full transform1 * transform2 expressions, because the implementation can exploit the particular matrix properties of .scaled/.rotated/.translated. However, because of the error prone chaining semantics, I have basically replaced many transform chains by transform product expressions, which has performance drawbacks.

I had to refresh my memory about the differences, in case you are interested in the details:

All possible transform operations ### Translation **Left multiply** ![Translation_lm](https://user-images.githubusercontent.com/3620703/89706741-bc779080-d968-11ea-870c-80641dedacd9.png) **Right multiply** ![Translation_rm](https://user-images.githubusercontent.com/3620703/89706744-c4373500-d968-11ea-87f0-3d11c7536392.png) ### Scale **Left multiply** ![Scale_lm](https://user-images.githubusercontent.com/3620703/89706748-cd280680-d968-11ea-9055-f1dc5f9eaaa0.png) **Right multiply** ![Scale_rm](https://user-images.githubusercontent.com/3620703/89706750-d1ecba80-d968-11ea-84e5-d56d720cacdb.png) ### Rotation **Left multiply** ![Rotation_lm](https://user-images.githubusercontent.com/3620703/89706735-abc71a80-d968-11ea-8a01-95ebb6469efe.png) **Right multiply** ![Rotation_rm](https://user-images.githubusercontent.com/3620703/89706736-b08bce80-d968-11ea-9adb-6e5f8b2d7056.png) ### Generic transform **Left multiply (rhs only)** ![Transform_lm](https://user-images.githubusercontent.com/3620703/89706714-85a17a80-d968-11ea-80fa-ff5a22fc840f.png) **Right multiply (rhs only)** ![Transform_rm](https://user-images.githubusercontent.com/3620703/89706733-a2d64900-d968-11ea-8c1c-b922a66d1215.png)

Counting the number of floating point operations gives:

Operation # Floating point operations available
Translation (left multiply) 3
Translation (right multiply) 18 *
Scale (left multiply) 12 *
Scale (right multiply) 9
Rotation (left multiply) 60 *
Rotation (right multiply) 45
Full transform multiplication 60 *

It is interesting to see how much more costly a full transform1 * transform2 (60 ops) is compared to a simple translation left multiply (3 ops). The other aspect I vaguely remembered: For scale/rotate the faster operation is right multiplication, whereas for translation it is left multiplication. If performance is critical, it can be helpful to build a transform exactly in the way that minimizes floating point operations. Unfortunately, the interface in Godot is not only inconsistent, but also offers only the less efficient variants.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:

The solution I'm proposing is to make the interface consistent and offer all possible operations:

This is also the solution chosen by Eigen (possibly the most famous library in that area) just the other way around because of using participles instead of infinitives, and to be less of a breaking change. What "pre" means is a matter of convention anyway, and up to the documentation to communicate.

Thus, in terms of documentation it is key to clearly describe what these functions do mathematically. Currently the docs do not even clearly say whether they are performing left or right multiplication, I only figured it out after reading the C++ sources. I could contribute some of the stuff above to the docs.

In terms of breaking changes this would add one item to the Godot 4.0 migration guide: Replace translated by pre_translated.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

Should be covered by the section above.

If this enhancement will not be used often, can it be worked around with a few lines of script?:

It affects Transform / Transform2D so the enhancement is probably used often.

Is there a reason why this should be core and not an add-on in the asset library?:

Mainly to avoid headaches for others.

Calinou commented 4 years ago

Thus, in terms of documentation it is key to clearly describe what these functions do mathematically. Currently the docs do not even clearly say whether they are performing left or right multiplication, I only figured it out after reading the C++ sources. I could contribute some of the stuff above to the docs.

Please open a pull request for that :slightly_smiling_face:

aaronfranke commented 3 years ago
Operation # Floating point operations available
Translation (left multiply) 3
Translation (right multiply) 18 *

As discussed here, both of these are available. You can achieve that behavior with t.origin += whatever so there's not much of a reason to have a method for it.

var M = Transform.IDENTITY\
    .scaled(... S_1 ...)\
    .translated(... T_1 ...)\
    .scaled(... S_2 ...)\
    .rotated(... R_1 ...)\
    .translated(... T_2 ...)\
    .rotated(... R_2 ...)

You listed this, and a similar example, under sections called "Hard to read" and "Hard to write". This seems like a pretty weak argument to me, if you are chaining 6 methods after each other then that's not very readable anyway. It would make more sense to calculate each of the transforms that you need, and give them names using variables (ex: for an FPS game, for calculating the camera's transform, one could be called body and one could be head).

The solution I'm proposing is to make the interface consistent and offer all possible operations:

  • .scaled performs left multiplication
  • .rotated performs left multiplication
  • .translated performs left multiplication
  • .pre_scaled performs right multiplication
  • .pre_rotated performs right multiplication
  • .pre_translated performs right multiplication

We only need one translated method, since the other is trivial using t.origin += whatever. The rest make sense. EDIT: Actually, having both is fine, it's consistent to have both versions of each.

bluenote10 commented 3 years ago

As discussed here, both of these are available. You can achieve that behavior with t.origin += whatever so there's not much of a reason to have a method for it.

As I tried to explain (An alternative is to manually implement translation left multiplication ...), this is not a chainable, which is what the proposal is about. The table was mainly summarizing available chainable operations.

By not chainable I mean that if you want a chain of say

This was basically the primary motivation to open the proposal: scale and rotate are already available as left-multiplies, but translate isn't, so you really can't chain the three with same semantics at all -- or rather -- you're lured in to squeeze the available translate into a chain, but you're not getting what you'd expect because it switches the operation associativity.

Other reasons for having all 6:

andre-caldas commented 3 years ago

I think you are making a mistake. Transform2D is about affine transformations, as you already know, since you are using what is called an augmented matrix.

Now, I think that your mistake is interpreting scaled(Vector(a,b)) or rotated(theta) as being equivalent to multiplication on the left by the matrices

[a 0 0] [0 b 0] [0 0 1] and [cos(theta) -sin(theta) 0] [sin(theta) cos(theta) 0] [................................. 1].

But it is not true! If you think about it, scaled() and rotated() are ill defined. There is no "universal rotation" in a plane. What there is, is rotation "about some point"!!! The same is true for scaling. There is no "universal scaling" in the plane. There is scaling "about some point"! When you scale, only one point is kept fixed. Which point are you trying to keep fixed when you calculate t.rotated(theta)? If you do with matrices and multiply those matrices above "by the left", you are rotating about the point (0,0) in your global system of coordinates. When you do t.scaled(Vector2d(2,2)), it is just the same: you are scaling and keeping the global origin fixed. What point do you want to scale about? The origin of the global system of coordinates? Or the origin of the local system?

So, I think rotated() and scaled() mean rotation and scaling about the "origin" of the local system of coordinates. This means that, from the left you should not multiply by the matrices you are multiplying.

This means that for each different value of Transform2D.origin, you have a different matrix you should use to "multiply by the left". For "scaled(Vector2(a,b))", it would be:

[a 0 p] [0 b q] [0 0 1] where p = origin.x * (1 - a), and q = origin.y * (1 - b).

If you want to scale by the global origin, all you have to do is scale the Transform2D.origin as well! And the same goes for rotation. There is no need to have pre_scaled or pre_rotated. This has nothing to do with "left" and "right". This is just because "scaled()" and "rotated()" mean scaled and rotated about the local origin.

bluenote10 commented 3 years ago

I think you are making a mistake.

How come all other libraries out there do it that way?

When multiplying matrices there are simply two ways, from the left and from the right. And when it comes to "chaining", i.e., putting multiple of them together in a chain, associativity is crucial. Having all 6 operations available in consistent way is strictly superior in functionality than having only 3 -- even more so when these three mix left and right multiplication semantics.

Look at code like this. The most intuitive interpretation of this code is that the order of these three lines corresponds to the order the matrices are multiplied, which is not the case (so either you likely see a bug here, or you have it look like a bug while being intentional).

Our brains as developers have hard enough tasks to solve, there is no need to make it any more harder with inconsistencies in associativity and non-intuitive order flipping.

andre-caldas commented 3 years ago

Edit: I just saw your youtube video!! That is a very interesting app!!! :-) It is a shame I do not have VR equipment. :-(


This has nothing to do with the order of matrix multiplication. You are multiplying by the wrong matrix. Associativity continues working.

When you scale an object, it should NOT go far away from the origin. It should stay in the same position and scale the object.

Associativity continues working because associativity has to do with "function composition". This continues working, as transformations are being applied when they are invoked. The examples you are using:

  1. Rotation about the local origin.
  2. Scaling by the same factor in both directions about the local origin.
  3. Translation of the local origin in relation to the global origin. Those operations ALWAYS commute. Associativity WORKS.

But rotation and scaling about the local origin are represented by different matrices if you ate talking about different origins. This is giving you the impression that multiplication is being done by "the right". It is not being done "by the right". Those operations listed above commute. From the right, the correct matrix is the matrix with (0, 0, 1) in the last column, because from the right, the origin of the coordinate system is the local origin. But from the left, the origin of the coordinate system is the global origin. Since you are not rotating about the global (parent's) origin, you are rotating about the local origin, matrix is different. You are multiplying by the wrong matrix. I found this stackoverflow question about scaling about a particular point. I insist that scaling about the parent's origin is not the expected behaviour.

Any operation done "by the right" can be done "by the left" by a suitable, possibly different, matrix. The problem here is that the matrix you are using only makes sense "from the right", because in this context the point of rotation is the origin of the coordinate system. If you do it from the left, then the point of rotation is no longer the origin of the coordinate system anymore. In this context, the origin of the coordinate system is the parent's origin. Your examples SEEM to show that the order of operations are being messed with, because those operations in fact commute. This is giving you the impression that multiplication is being done "by the right". If you do your experiments with a scaling in only one direction (i.e.: scaled(Vector2(2,1)), you will realize that those transformations are NOT being done "by the right" in the order you think they are. Because, in this case, rotation and scaling do not commute.

The bahaviour you seem to want is not intuitive, as you seem to want that scaling and rotation of an object changes its position relative to its parent's coordinate system. This is not what is supposed to happen. The origin of the local coordinate system is supposed to stay in place, and the changes are supposed to be applied only on the axis (i.e.: variables x and y of Transform2D).

If you want the scaled/rotated object to move away from the parent's origin, you can:

  1. Apply the scaling/rotation to both: origin and "linear part" (that is, origin, x and y variables).
  2. Change the parent's scale/rotation. (this would affect all siblings, though)

Maybe, if you show a simplified piece of your (real) code and what you are trying to achieve (graphically), it would be easier to talk.

bluenote10 commented 3 years ago

Your examples SEEM to show that the order of operations are being messed with, because those operations in fact commute. Translation of the local origin in relation to the global origin. Those operations ALWAYS commute. Associativity WORKS.

No, if you have a general transformation matrix M than no operation commutes. Even for translations it is different if you do M · T (translate first by T, followed by transforming with M) or T · M (apply M first, followed translating with M).

You are multiplying by the wrong matrix.

There is no right or wrong. It all depends on what you want to achieve.

If you do it from the left, then the point of rotation is no longer the origin of the coordinate system anymore. In this context, the origin of the coordinate system is the parent's origin.

Yes, and there are use valid cases for that.

Maybe, if you show a simplified piece of your (real) code and what you are trying to achieve (graphically), it would be easier to talk.

This is not about my code. It is just about that fact that the current behavior is highly confusing, and in contrast to all other libraries out there. Just try to infer the a mathematical equation from the three lines of the linked example.


I don't think this is leading anywhere. All I can say is:

Take the Eigen library as an example, which is a de-facto standard for linear transformation e.g. in robotics, where complex transformation chains arise frequently e.g. when traversing from one robot component/sensor to another (my daytime job). Do you really want to argue that a library that is praised for its design got it all wrong? I can tell you that any programmer with a background in Eigen will be massively confused by the current behavior, and from what I recall from my Java/Scala/GLM/Python days it was the same there.

bluenote10 commented 2 years ago

For the record: @tagcup2 made a good point in https://github.com/godotengine/godot/pull/48769#issuecomment-844585736 regarding the naming of pre_translated, pre_rotated, pre_scaled: The implementation of Basis is already calling these operations ..._local which has a more intuitive interpretation compared to "pre", so I'd revise the proposal to use the _local suffix for consistency and readability.

Also I've finally an implementation ready for it https://github.com/godotengine/godot/pull/55923

aaronfranke commented 2 years ago

I did some testing with the two PRs: https://github.com/godotengine/godot/pull/48769 https://github.com/godotengine/godot/pull/55923

In math, when thinking of transforms visually, we read things right-to-left. Thus, T * S means scale first then translate, and S * T means translate first then scale.

func _ready():
    # These 5 prints are equivalent in madmiraal's PR:
    var T = Transform3D(Basis(), Vector3(1, 0, 0))
    var S = Transform3D(Basis() * 2, Vector3())
    print(T * S)
    var TR1 = Transform3D.IDENTITY
    TR1 = TR1.translated(Vector3(1.0, 0.0, 0.0))
    TR1 = TR1.scaled(Vector3(2.0, 2.0, 2.0))
    print(TR1)
    var TR2 = Transform3D.IDENTITY
    TR2 = TR2.pre_scaled(Vector3(2.0, 2.0, 2.0))
    TR2 = TR2.pre_translated(Vector3(1.0, 0.0, 0.0))
    print(TR2)
    print(Transform3D.IDENTITY.translated(Vector3(1.0, 0.0, 0.0)).scaled(Vector3(2.0, 2.0, 2.0))) 
    print(Transform3D.IDENTITY.pre_scaled(Vector3(2.0, 2.0, 2.0)).pre_translated(Vector3(1.0, 0.0, 0.0)))
    print("===")

    # These other 5 prints are equivalent in madmiraal's PR:
    #var T = Transform3D(Basis(), Vector3(1, 0, 0))
    #var S = Transform3D(Basis() * 2, Vector3())
    print(S * T)
    var TR3 = Transform3D.IDENTITY
    TR3 = TR3.scaled(Vector3(2.0, 2.0, 2.0))
    TR3 = TR3.translated(Vector3(1.0, 0.0, 0.0))
    print(TR3)
    var TR4 = Transform3D.IDENTITY
    TR4 = TR4.pre_translated(Vector3(1.0, 0.0, 0.0))
    TR4 = TR4.pre_scaled(Vector3(2.0, 2.0, 2.0))
    print(TR4)
    print(Transform3D.IDENTITY.pre_translated(Vector3(1.0, 0.0, 0.0)).pre_scaled(Vector3(2.0, 2.0, 2.0))) 
    print(Transform3D.IDENTITY.scaled(Vector3(2.0, 2.0, 2.0)).translated(Vector3(1.0, 0.0, 0.0)))
    print("===")

    # These 5 prints are equivalent in bluenote10's PR:
    #var T = Transform3D(Basis(), Vector3(1, 0, 0))
    #var S = Transform3D(Basis() * 2, Vector3())
    print(T * S)
    var TR5 = Transform3D.IDENTITY
    TR5 = TR5.scaled(Vector3(2.0, 2.0, 2.0))
    TR5 = TR5.translated(Vector3(1.0, 0.0, 0.0))
    print(TR5)
    var TR6 = Transform3D.IDENTITY
    TR6 = TR6.translated_local(Vector3(1.0, 0.0, 0.0))
    TR6 = TR6.scaled_local(Vector3(2.0, 2.0, 2.0))
    print(TR6)
    print(Transform3D.IDENTITY.translated_local(Vector3(1.0, 0.0, 0.0)).scaled_local(Vector3(2.0, 2.0, 2.0))) 
    print(Transform3D.IDENTITY.scaled(Vector3(2.0, 2.0, 2.0)).translated(Vector3(1.0, 0.0, 0.0)))
    print("===")

    # These other 5 prints are equivalent in bluenote10's PR:
    #var T = Transform3D(Basis(), Vector3(1, 0, 0))
    #var S = Transform3D(Basis() * 2, Vector3())
    print(S * T)
    var TR7 = Transform3D.IDENTITY
    TR7 = TR7.translated(Vector3(1.0, 0.0, 0.0))
    TR7 = TR7.scaled(Vector3(2.0, 2.0, 2.0))
    print(TR7)
    var TR8 = Transform3D.IDENTITY
    TR8 = TR8.scaled_local(Vector3(2.0, 2.0, 2.0))
    TR8 = TR8.translated_local(Vector3(1.0, 0.0, 0.0))
    print(TR8)
    print(Transform3D.IDENTITY.translated(Vector3(1.0, 0.0, 0.0)).scaled(Vector3(2.0, 2.0, 2.0))) 
    print(Transform3D.IDENTITY.scaled_local(Vector3(2.0, 2.0, 2.0)).translated_local(Vector3(1.0, 0.0, 0.0)))
    print("===")

I think we have a winner... @bluenote10's API makes loads more sense, @madmiraal's API is backwards.

As for the concept of having transform chaining available in both directions: I don't plan on using it personally, but it seems that there is consensus that there should be both available, so I guess we should have them. Additionally, the naming of _local was discussed as being better than having pre_ suffixes.

Note that @bluenote10's PR needs to be rebased.

bluenote10 commented 2 years ago

@aaronfranke Great, thanks for testing, I can work on rebasing the PR tomorrow!

If desired, I can also pull out the part that renames the existing translate into translate_local into a separate PR, which we can merge first. This would make sure that I have renamed 100% of all existing occurrences of translate if it passes all CI pipelines. Doing it all in one step and re-introducing the translate with the "new" semantics in the same PR has a certain danger that I miss an occurrence, because it would compile, possibly resulting wrong runtime behavior (as noted, in many cases it also doesn't make a difference like when they are applied to an identity anyway). Technically this is what I did locally back when I prepared the PR, but since some occurrences are behind platform dependent compiler switches (#ifdefs that are disabled on my local platform), this approach is only ~99% "safe". I would be feel better doing it in two separate steps for 100% correctness. What do you think?

aaronfranke commented 2 years ago

@bluenote10 We might want to go the other way, and first make a PR that adds rotated_local and scaled_local, since I think it's widely agreed that these are useful functions so this part could be merged without too much delay. Theoretically, these could also be backported to 3.x since they don't break compatibility.

bluenote10 commented 2 years ago

@aaronfranke I guess this would mean that we need 3 PRs for full safety:

  1. PR that adds rotated_local and scaled_local
  2. PR that renames translated to translated_local
  3. PR that re-adds translated

We don't want to include (3) in (2) for safety reasons. My gut feeling is that it is easier to review 1+3 is together, because the PR would "fill all the gaps" and makes the motivation of the code structure I chose more obvious (like grouping the functions in the headers, or adding unit tests for all 6 variants) compared to the incomplete variant (1). But I'll experiment a bit what is most convenient.

madmiraal commented 2 years ago

In math, when thinking of transforms visually, we read things right-to-left. Thus, T S means scale first then translate, and S T means translate first then scale.

@aaronfranke What you are literally describing is the need to think backwards. This is not intuitive. The unintuitive, backward nature of the mathematics aside, the actual difference is whether the transform happens in the global reference frame or the local reference frame. However, since we can't do things in the global reference frame, any pre-multiplication would only be in the parent reference frame, which is not what the user expects (see the examples I presented here: https://github.com/godotengine/godot/pull/48769#issuecomment-845567486).

Additionally, the naming of local was discussed as being better than having pre suffixes.

It may have been discussed, but I don't believe it was agreed. The more common method should be the method that is not prefixed or suffixed. As elaborated on at length in both godotengine/godot#48769 and https://github.com/godotengine/godot#55923 I believe the more common methods are those that take place in the local reference frame not the parent reference frame.

bluenote10 commented 2 years ago

What you are literally describing is the need to think backwards. This is not intuitive.

It is, no backwards thinking required!

Imagine asking a random developer what t.scaled(S).rotated(R).translated(T) should mean. Consider giving them the options:

  1. It means that a vector v first gets transformed by S then by R then by T.
  2. It means that a vector v first gets transformed by T then by R then by S.

In this case most likely they will pick (1) because that's exactly how the code is written. Based on this, one would argue that left-multiplication / right-to-left semantics / global thinking is more intuitive.

If you instead give them the options (fully equivalent):

  1. It means that a vector v first gets transformed by T * R * S * v.
  2. It means that a vector v first gets transformed by S * R * T * v.

In this case they would suddenly opt for (2) instead, because the original code is closer to the expression written as a formula. All of a sudden we can conclude that right-multiplication / left-to-right semantics / local thinking is more intuitive.

The dilemma is: Both views have an intuition. From my experience developers tend to think in "first this, than that" rather than visualizing the formula expression when doing this kind of method chaining, perhaps because the imperative API is closer to "first this, than that" thinking than a formula.

aaronfranke commented 2 years ago

In math, when thinking of transforms visually, we read things right-to-left. Thus, T S means scale first then translate, and S T means translate first then scale.

@aaronfranke What you are literally describing is the need to think backwards. This is not intuitive.

The math is not up for debate. The multiplication order of transforms is literally how the conventions of math and matrix-vector multiplication insist it must be done. All we can do is make the methods intuitive, which is exactly what @bluenote10's PR does.

andre-caldas commented 2 years ago

Specifically this... is not about math notation. It is about how the programing language works. Godot's object orientation works like this:

object.function(argument)

When you chain stuff, you make function return the object again, so you can call another function:

object.first_function(argument).second_function(argument)

If you don't have any arguments, just the object (it is an argument, but the grammar is different for the object):

object.first_function().second_function()

This has nothing to do with any math library. This is how the programming language grammar is defined!!!

madmiraal commented 2 years ago

Imagine asking a random developer what t.scaled(S).rotated(R).translated(T) should mean. Consider giving them the options:

  1. It means that a vector v first gets transformed by S then by R then by T.
  2. It means that a vector v first gets transformed by T then by R then by S.

In this case most likely they will pick (1) because that's exactly how the code is written. Based on this, one would argue that left-multiplication / right-to-left semantics / global thinking is more intuitive.

(1) is exactly how godotengine/godot#48769 is designed to work.

andre-caldas commented 2 years ago

Imagine asking a random developer what t.scaled(S).rotated(R).translated(T) should mean. Consider giving them the options:

  1. It means that a vector v first gets transformed by S then by R then by T.
  2. It means that a vector v first gets transformed by T then by R then by S.

In this case most likely they will pick (1) because that's exactly how the code is written. Based on this, one would argue that left-multiplication / right-to-left semantics / global thinking is more intuitive.

(1) is exactly how godotengine/godot#48769 is designed to work.

Nope. It is exactly how gdscript currently works. If you write

a.method_one().method_two()

this is the same as applying method_one() to a. Then whatever the resulting object is, it is passed to method_two(). It has nothing to do with multiplying on the right or on the left. As far as I am aware, you proposal was made based on misconceptions and wrong assumptions.

Your proposal has nothing to do with left or right multiplication. Your post is related to the fact that you think that the meaning of rotating or scaling a "transform" is "rotating/scaling about the local origin" and not "about the caller's origin".

It is very confusing that people like so much to talk about left or right matrix multiplication, when all you have is a sequence of operations that should be very well specified. This is the problem with Godot's transforms:

andre-caldas commented 2 years ago

For those who are confused, please, check this post: https://github.com/godotengine/godot/pull/48769#issuecomment-1031398135

All this confusion has nothing to do with left or right. Imagine this coordinate system about the origin:

x = (1, 0) y = (0, 2)

Applying this to a person standing up would make this person tall.

This basically means make everything tall. Now, if you apply a rotation (90 degrees counterclockwise) to this, what shall it mean? If you apply a rotation "on the right", you get:

x = (0, 2) y = (-1, 0)

That is, the person is rotated, but s/he gets fat, not tall!!! If you apply a rotation "on the left", you get:

x = (0, 1) y = (-2, 0)

That is, the person is rotated and is still tall!!!

Making things "on the right" means controlling the coordinate system from inside.

The origin

What is causing most of the confusion is not the discussion if things should be on the left or on the right. What people are discussing is if the rotation should be about the local origin or the global origin. This is a very legitimate discussion. But people are not aware that this has nothing to do with left or right. This has to do with the meaning of rotated(): about which point?

If you do the operation on the right, one SIDE EFFECT is that the origin is kept where it is. The correct thing to do in case of 48769 is simply not applying the rotation to the origin.

Since USUALLY scaling is applying uniformly on every direction (no stretching in only one direction), and since axis are usually not tilted (they are usually orthogonal), multiplying on the right produces no side effects besides the rotation being about the local origin.

For the tilted case, think of the Tower of Pisa. You make a straight tower and tilt it. What should happen if you rotate the tower? Shall it be rotated before being tilted?????? Well, this is what "multiplication on the right means"!!!

You are all talking about left and right, but what you really seem to be discussing is if the rotation or scaling is about the global origin or the local origin.

bluenote10 commented 2 years ago

You are all talking about left and right, but what you really seem to be discussing is if the rotation or scaling is about the global origin or the local origin.

Everyone in the discussion seems to be well aware of that. I'm even trying to write things with a slash (like "multiply by the left" / "transform w.r.t. global origin") as much as possible to address your concern. The "multiply by the left" is the mathematical/technical/implementation view. The "transform w.r.t. global origin" is the conceptual view.

It doesn't help the discussion that you claim it is not about left or right multiplication. It is a fact that t.scaled(S) can mean t * S or S * t. On a technical level this is what it is all about. You're right that on a conceptual level it is about transforming about global vs local origins, but please allow us to refer to it by its mathematical/technical/implementation term as well.

(1) is exactly how https://github.com/godotengine/godot/pull/48769 is designed to work.

As far as I can see it isn't, because it multiplies by the right / transforms around the local origin. You get (1) by multiplying from the left / transform around the global origin.

andre-caldas commented 2 years ago

The "multiply by the left" is the mathematical/technical/implementation view. The "transform w.r.t. global origin" is the conceptual view.

The so called right multiplication is a different thing. It is the same thing only in the special case where there is no uneven stretching.

andre-caldas commented 2 years ago

[ 1 0 ] [ 0 2 ]

Means: stretch on the y coordinate. Make things tall.

Rotate 90 degrees counterclockwise "on the right":

[ 1  0 ] [ 0 -1 ]   [ 0 -1 ]
[      ] [      ] = [      ]
[ 0  2 ] [ 1  0 ]   [ 2  0 ]

This makes things wide, not tall!!! It now stretches the x axis, not the y axis. I don't think this is what you want.

Rotate "on the left":

[ 0 -1 ] [ 1  0 ]   [ 0 -2 ]
[      ] [      ] = [      ]
[ 1  0 ] [ 0  2 ]   [ 1  0 ]

Rotated, but still "tall". The y axis is kept stretched.