-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Enable transform chaining by making scaled/rotated/translated consistent #1336
Comments
|
As discussed here, both of these are available. You can achieve that behavior with
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
|
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 Other reasons for having all 6:
|
I think you are making a mistake. Now, I think that your mistake is interpreting
But it is not true! If you think about it, So, I think This means that for each different value of
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 |
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. |
Edit: I just saw your youtube video!! That is a very interesting app!!! :-) 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 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:
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 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.: 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:
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. |
No, if you have a general transformation matrix
There is no right or wrong. It all depends on what you want to achieve.
Yes, and there are use valid cases for that.
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. |
For the record: @tagcup2 made a good point in godotengine/godot#48769 (comment) regarding the naming of Also I've finally an implementation ready for it godotengine/godot#55923 |
I did some testing with the two PRs: godotengine/godot#48769 godotengine/godot#55923 In math, when thinking of transforms visually, we read things right-to-left. Thus, 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 Note that @bluenote10's PR needs to be rebased. |
@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 |
@bluenote10 We might want to go the other way, and first make a PR that adds |
@aaronfranke I guess this would mean that we need 3 PRs for full safety:
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. |
@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: godotengine/godot#48769 (comment)).
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. |
It is, no backwards thinking required! Imagine asking a random developer what
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):
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. |
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. |
Specifically this... is not about math notation. It is about how the programing language works. Godot's object orientation works like this:
When you chain stuff, you make
If you don't have any arguments, just the object (it is an argument, but the grammar is different for the object):
This has nothing to do with any math library. This is how the programming language grammar is defined!!! |
(1) is exactly how godotengine/godot#48769 is designed to work. |
Nope. It is exactly how gdscript currently works. If you write
this is the same as applying 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:
|
For those who are confused, please, check this post: All this confusion has nothing to do with left or right. Imagine this coordinate system about the origin:
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:
That is, the person is rotated, but s/he gets fat, not tall!!! If you apply a rotation "on the left", you get:
That is, the person is rotated and is still tall!!! Making things "on the right" means controlling the coordinate system from inside. The originWhat 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 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. |
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
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. |
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. |
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. |
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:
MultiMeshInstance
I can't rely less on node hierarchies, but have to compute complex transformation chains manually.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 godotengine/godot#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 forTransform2D
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
first transforms
x
byA
, then byB
, then byC
, opposite to how the mathematical equation is typically written. Put another way:Transform left multiplication
means that
A
is applied afterM
is applied.Transform right multiplication
means that
A
is applied beforeM
is applied.Current behavior
Currently the behavior is a mix of left and right multiplication.
Scaled:
has left multiplication semantics (i.e., happens after in transformation order):
Rotated:
has left multiplication semantics (i.e., happens after in transformation order):
Translated:
has right multiplication semantics (i.e., happens before in transformation order):
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:
is equivalent to (if I didn't get it wrong again)
Note how
R
has moved from last to first,S
has moved to the middle, andT
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:
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 theR
in the right position.The only way to write it is in a non-chained way, for instance:
Example 2: Imagine implementing longer transform chains like:
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 assembledin 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):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
Right multiply
Scale
Left multiply
Right multiply
Rotation
Left multiply
Right multiply
Generic transform
Left multiply (rhs only)
Right multiply (rhs only)
Counting the number of floating point operations gives:
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:
.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 multiplicationThis 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
bypre_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.
The text was updated successfully, but these errors were encountered: