Revert transform back to using Mat4

[lassade]

The article on #596 miss interpreted and there is no compelling reason to use TRS for global transforms, here is what I found:

• Mat4 is need to apply any transformations, this means way more TRS to Mat4 conversions will be need than Mat4 to TRS. So keeping a Mat4 is faster and more convenient
• The article mentioned in the issue Moving transform data out of Mat4 #596 shows that matrices drifts when too many multiplications are made over it. But what the transform system actually do is to calculate the global matrices by multiplying the parent matrix with local TRS which then will be carried down to the next child. So the drift will be only carried away through the hierarchy chain, meaning that, according with the article, the 200 child will have a drift of 0.000001 and having such deep hierarchy isn't realistic in a game context
• Mat4 multiplication is ~1.9 times faster than TRS, this affects directly the transform_propagation_system
• Mat4 point transform is ~5.3 times faster than TRS and that is a very common operation!
• Mat4 vector multiplication is ~2.4 times faster than TRS
• LocalToWorld translation is trivially accessible, it's just field like in TRS so nothing changes!
• LocalToWorld access to right, up, forward vectors is marginally cheaper than TRS so nothing changes!
• LocalToWorld shouldn't be modified directly, because the changes won't propagate
• Why name it LocalToWorld, because GlobalTransfrom confuses people into thinking they should modify it, the ecs approach doesnt allow for transform.position = ... like in traditional engines, because the transformations WONT propagate; Later on we want to add a WorldToLocal it can be useful in some situations.
• We can have a LocalToWorld2D that 2D and 2.5D games will only pay for what they use, a mixed hierarchy using both types is sort of simple to understand (see a WIP in here)
• Mat4 can encode shear and other types of deformations

A few disadvantages

• TRS to Mat4 is ~4.5 times faster than Mat4 to TRS
• Mat4 inverse is not always possible
• TRS inverse is ~1.7 times faster than Mat4 inverse
• TRS any direction is ~1.2 times faster than Mat4 and that is a very common operation!

Sorry for wanting to break everyone else code

[lassade]

Converting a Mat4 to TRS my result in a double cover Quaternion which may mess with some interpolation code that don't account for this, which isn't very common.

Not a really big problem

[CptPotato]

👋 A related issue came up on discord earlier today. I'll try to give some context and my opinion:

As part of a PR (#2049) I double checked if child-entities inherit the scale of their parents, which could cause some issues with my algorithm through shearing. At first I thought it didn't because GlobalTransform can't encode that properly (as @lassade also points out above). We then found that the scale is inherited anyway but simply multiplied (src) without taking rotation into account (possibly by accident in #596).

This can lead to unexpected behaviour if parent-child hierarchies, rotation and non-uniform scaling are mixed together at the same time. More specifically, scaling the parent along its x-axis would scale the children each along their own (local) x-axis. The change @lassade proposed here would potentially fix that (which is how all that ties into this specific issue).

In general I see different ways to approach this, each with its own quirks and limitations, and I feel like all of them have their own values. Note that this is just my personal view, opinions of more experienced users are welcome:

1. inherit scaling correctly (=> this issue, also my preference)
   • requires to change the type of GlobalTransform into something that can propagate scales (Mat4)
   • usually this is the expected behaviour
   • things like physics can get tricky if users rely on shearing
2. don't inherit scaling at all
   • will not cause shearing (can allow for simpler & more optimized code)
   • works with the current GlobalTransform representation
   • probably not what users expect
   • gltf import requires hacks (applying transforms on import)
3. keep the current approach
   • it works for most common cases
   • ..but causes unexpected results otherwise

---

Apparently there were discussions similar to this before, so sorry if I brought up a problem that has already been solved / dismissed.

I guess all in all I'd like to bring some awareness to this issue, weighing the possible options - even if it's only confirming that the current approach is preferred - and documenting the limitations of that at some point.

[alice-i-cecile]

Adding the high-impact and help-wanted tags: the Transform bugs are reasonably serious and this seems to be the place where that discussion around fixing that should occur, at least to start.

[Benjamin-L]

The article mentioned in the issue #596 shows that matrices drifts when too many multiplications are made over it. But what the transform system actually do is to calculate the global matrices by multiplying the parent matrix with local TRS which then will be carried down to the next child. So the drift will be only carried away through the hierarchy chain, meaning that, according with the article, the 200 child will have a drift of 0.000001 and having such deep hierarchy isn't realistic in a game context

I don't think the issue is really with propagating transforms from parents through the hierarchy, it's with incremental updates between frames. If we want to use only the Mat4 representation for transformations, that means that every time the rotation of the local transformation is changed (via a set_rotation method or such), it will accumulate a small error, which will compound over multiple frames. Because of this, I don't think using Mat4s for the "source of truth" representation is an option.

There's a problem with our current TRS representation, which I don't think has been discussed outside of #2055, which is the order of application for different components. Our current implementation applies rotation, then scale, then translation (it also generates matrices by incorrectly applying SRT instead of RST, but that's a solvable problem). The RST ordering is pretty uninituitive, and is uncommon in graphics and game development. Unfortunately, this is actually the only representation we can use if we want to support nonuniform scale. The reason is that two affine transforms without a shear component can compose to generate a transform with a shear component. Shear cannot be represented with SRT ordering, but it can be represented with RST (because 'S' here is really more a combination of scale and shear than it is pure scale). Note that with uniform scale, shear is not an issue, and SRT and RST are equivalent.

There is no real clean solution to this problem, and I think the right choice is to either keep the RST ordering and just fix all the bugs, or to split into separate components for uniform and nonuniform scaling with different semantics.

Either way I think it makes sense to have a TransformationMatrix and GlobalTransformationMatrix component and a system to generate these every frame, since recalculating the matrix representation on the fly every time you need it is not efficient.

[cart]

@Benjamin-L

Because of this, I don't think using Mat4s for the "source of truth" representation is an option.

Yup this is one of main reasons we opted for "similarities" over mat4 as the source of truth.

There is no real clean solution to this problem, and I think the right choice is to either keep the RST ordering and just fix all the bugs, or to split into separate components for uniform and nonuniform scaling with different semantics.

My personal preference is to keep the current Transform representation(*) and to fix the transform op bugs. Separate components introduces extra complexity to entity construction and makes it way harder to do operations that require the values of multiple "source of truth" components (ex: transforming a point using Translation, Rotation, and Scale components requires querying for all three, then using some abstraction to group them and do the transform op).

I don't yet have an opinion on SRT vs RST. I don't fully grok the implications of each yet.

(*) including non-uniform scale ... it creates number of problems (physics, more expensive to calculate, etc) but in practice people want to squash and stretch stuff. I want that to work by default. Later we can internally optimize for uniform scales if it becomes a major concern (ex: consider making scale an enum Scale { Uniform(f32), NonUniform(Vec3) }, although that has ergonomics implications and maybe perf implications due to more branching) .

Either way I think it makes sense to have a TransformationMatrix and GlobalTransformationMatrix component and a system to generate these every frame, since recalculating the matrix representation on the fly every time you need it is not efficient.

My main problem with making these separate components is that they can't have "up to date" values during arbitrary points in the UPDATE stage, so they won't be useful for game logic unless the user manually inserts hard TransformMatrix sync points between GAME_LOGIC_A and GAME_LOGIC_B (which currently means adding multiple stages). They might save one or two conversions by "internal systems" that run after the UPDATE stage, but generally the only thing that needs the final Mat4 is shader uniforms. And we generally only generate those uniforms once per entity, so caching the Mat4 wouldn't save us much. Most game-related requirements for Transforms (editing translation/rotation/scale, transforming a point or other transform, etc) can be done without recalculating the Mat4. Things that do require Mat4s could create their own caches if perf becomes a problem.

We could consider embedding Mat4s into the Transform component and incrementally keeping them up to date, but that has a number of negative implications:

1. Naively, each transform op gets more expensive: rotation now involves updating the Mat4 and the rotation Quat (and the drift problem is re-introduced). Instead we'd probably need a "dirty" bit that gets flipped and we would lazily recalculate the Mat4 when it is requested.
2. Adding a Mat4 to Transform makes it less cache friendly. We want Transform to be as small as possible.

@lassade

Mat4 vector multiplication is ~2.4 times faster than TRS

Can you link to sources / benchmarks for these numbers?

[lassade]

@cart the benches are here

[Benjamin-L]

Note that this benchmark is with the current (broken) composition implementation, which is going to be faster than the correct implementation.

[Benjamin-L]

My main problem with making these separate components is that they can't have "up to date" values during arbitrary points in the UPDATE stage, so they won't be useful for game logic unless the user manually inserts hard TransformMatrix sync points between GAME_LOGIC_A and GAME_LOGIC_B (which currently means adding multiple stages).

yeah, this is a good point. I don't think using the raw matrices is going to be a particularly common requirement anyway, especially if we flesh out the Transformation API to add more useful operations. I also agree that storing a Mat4 alongside the TRS values would be bad, for the same reasons you stated.

For SRT vs RST, if we do want to support nonuniform scale by default, we effectively have the choice between the relatively unintuitive RST representation, which is closed under composition, and the intuitive SRT, which is not. I think it makes sense here to go with RST to avoid introducing complexity to users that only care about uniform scaling, since it is much more common. No matter what, we need to document this carefully.

[lassade]

Local transform should be kept the same, I only want to change the GlobalTransform to Mat4 witch shouldn't be changed directly

[Benjamin-L]

Oh, I see. Using different representations for the local vs global representations does make me a bit uncomfortable. For example, wanting to convert the global transformation of one entity into the local frame of reference of a different entity is a useful operation. As long as we have a robust way to convert back from the matrix representation to the TRS representation, I think this is okay.

[mockersf]

GlobalTransform [...] witch shouldn't be changed directly

about that, there was a lengthy discussion here: #1460

[cart]

Would *global_transform_mat4 = parent_mat4 * local_trs.compute_matrix() be faster than *global_transform_trs = parent_trs * local_trs?

The benchmarks above only compare mat4 * mat4 and trs * trs, which isn't what we'd be comparing in this case.

Does anyone have good links to resources covering non-uniform Similarity operations? Finding that material is harder than I expected. We should also try to find examples of other game engines / math libs that are operating under similar constraints / design choices as us.

[Benjamin-L]

This isn't super important, but will probably be useful when researching this: similarities always have uniform scale and preserve angles. I'm not sure there's a word for an affine transformation without shear, and I'm not sure it's a particularly common or useful object because it isn't closed under composition.

For the performance of matrix vs trs composition, we're comparing trs * mat4 to trs * trs. I don't have any benchmarks for this on hand, but my hunch is that the former will be faster because the trs * trs operation is rather complex.

[lassade]

@cart

Would *global_transform_mat4 = parent_mat4 * local_trs.compute_matrix() be faster than *global_transform_trs = parent_trs * local_trs?

The benchmarks above only compare mat4 * mat4 and trs * trs, which isn't what we'd be comparing in this case.

Does anyone have good links to resources covering non-uniform Similarity operations? Finding that material is harder than I expected. We should also try to find examples of other game engines / math libs that are operating under similar constraints / design choices as us.

Ops sorry,

Edit:
Sorry again Mat4 is faster by 1ns (abour 1,13 times), forgot to turn on SIMD compilation on my windows machine

[Benjamin-L]

So after banging my head against a wall trying to figure out why my implementation for Transform::mul_transform didn't work, I realized I made a stupid mistake previously. Neither of the orderings (SRT or RST) can represent shear, and neither are closed under composition. A simple counterexample would be trying to compose the following transformations:

• Rotate 45 deg
• Nonuniform scale (2,1)
• Rotate 45 deg

Now the scale is off-axis for both SRT and RST representations.

This is kinda nice because it opens back up the opportunity to switch back to SRT representation for Transform. It's not so nice because I now think the best option is to just not support composing Transforms at all. The only other option would be to add a shear component of some kind to Transform, or to just give up and not handle nonuniform scale correctly.

[cart]

I think we need to start deferring to the wider gamedev ecosystem at this point. SRT with non-uniform scale appears to dominate:

• Unity: SRT
  • Internal representation: https://forum.unity.com/threads/transform-matrix-elements.18836/#post-128496
  • Public api: https://docs.unity3d.com/ScriptReference/Transform.html (note the "lossy global scale" read-only property)
  • CTO's Transform Rewrite Proposal (also SRT): https://gist.github.com/joeante/79d25ec3a0e86436e53eb74f3ac82c0c
• Defold: SRT
  • Source: https://github.com/defold/defold/blob/1ae302ec33d4514408c04ad3ae5d3c1efe2057bd/engine/dlib/src/dlib/transform.h
  • Note that it uses the cheap / simple transform multiplication algorithm we currently use (SRT)
• Unreal: SRT
  • Public api: https://docs.unrealengine.com/en-US/API/Runtime/Core/Math/FTransform/index.html
  • Note that they explicitly call out SRT application order
• Godot: Vec3 origin + Mat3 basis
  • Source: https://github.com/godotengine/godot/blob/master/core/math/transform.cpp

In short: I don't think we are in crisis here (other than some superficial AxB vs BxA ordering problems). It looks like everyone is doing exactly what we are currently doing, with the exception of godot.

[Benjamin-L]

The question for all of these implementations is how (and whether) they handle composition with nonuniform scale. Defold is the only one using SRT with public source code and the answer is that they handle it incorrectly by just multiplying the scales together: https://github.com/defold/defold/blob/1ae302ec33d4514408c04ad3ae5d3c1efe2057bd/engine/dlib/src/dlib/transform.h#L341. For unity and unreal we could theoretically check what they are doing by passing in values and observing the result. It's possible that this is the solution we have to go with, but it definitely bothers me that the results of transform_a * transform_b will be different from Transform::from_matrix(transform_a.compute_matrix() * transform_b.compute_matrix()) (and that (transform_a * transform_b) * point != transform_a * (transform_b * point)). IMO it's better to just not have a function for composing transforms directly than to have a function that isn't actually composing them.

[alice-i-cecile]

IMO it's better to just not have a function for composing transforms directly than to have a function that isn't actually composing them.

I agree.

[Benjamin-L]

Another realization: SRT (or RST) representation can't even be inverted. We don't currently have a Transform::inverse method, but I wanted to add one. Maybe instead what we want is methods like Transform::inverse_transform_point, although that's definitely ugly.

[aevyrie]

In the past I've had the need to update only translations, or only querying rotations using change detection. Is it out of the question that we split out Translation Rotation and Scale as their own components? Considering it's not uncommon to want to iterate over and update only one property of the transform, this seems like the "ECS way" of handling this. Composition could then even be adjustable.

[alice-i-cecile]

Is it out of the question that we split out Translation Rotation and Scale as their own components?

As an end user, I would love this solution. It would be quite a bit clearer, and, as discussed, I'm frequently updating only one property at a time in my own code.

[Benjamin-L]

I would also prefer having separate components. It would also nicely resolve our questions about whether to include (impossible) functionality for Transform if Transform no longer existed.

I don't know how common wanting to switch spaces between the local transform and the parent is. Personally, I almost never want to do this, but frequentlywant to switch between local and global, which would still be fine if GlobalTransform was a single component represented by a matrix.

[cart]

@Benjamin-L, @alice-i-cecile, @aevyrie

I've already given my current opinion on this above (and in other threads). It works fine for the simple stuff. But it creates problems for anything "interesting". How do you propose enabling operations like "transforming a point using the current rotation / translation/ scale of the entity", "rotating an entity around a point", or "scaling an entity using a non-zero origin"?

A centralized Transform makes it easy to perform these operations.

[aevyrie]

We could still split out TRS from Transform without preventing those operations. TRS would be individual components instead of fields.

We would still need to settle on SRT/RST/whatever representation for transform propagation. Either the user needs to provide a system that turns Transform Rotation Scale into a Mat4 Transform, or there will need to a be a system that does this automatically, defaulting to SRT.

stage start ---> user modifies TRS. If they access Transform it will be stale (from last stage sync) ---> TRS is converted to Mat4 Transform --> Transform propagation ---> stage end

[cart]

The question for all of these implementations is how (and whether) they handle composition with nonuniform scale. Defold is the only one using SRT with public source code and the answer is that they handle it incorrectly by just multiplying the scales together: https://github.com/defold/defold/blob/1ae302ec33d4514408c04ad3ae5d3c1efe2057bd/engine/dlib/src/dlib/transform.h#L341. For unity and unreal we could theoretically check what they are doing by passing in values and observing the result. It's possible that this is the solution we have to go with, but it definitely bothers me that the results of transform_a * transform_b will be different from Transform::from_matrix(transform_a.compute_matrix() * transform_b.compute_matrix()) (and that (transform_a * transform_b) * point != transform_a * (transform_b * point)). IMO it's better to just not have a function for composing transforms directly than to have a function that isn't actually composing them.

I'm pretty sure this is a tradeoff each engine has accepted. Clearly Unity has with its description of the lossy global scale property. If it works for Unity, Defold, and (presumably) Unreal, its can work for us. Seems like a pragmatic tradeoff to me.

user modifies TRS. If they access Transform it will be stale (from last stage sync) -

This is the problem. Users wanting to do the more complicated operations mentioned above in the update stage can't rely on a Transform that lags behind by a frame. Therefore they need to query for them individually, then use some abstraction (construct a Transform in the system, use a Transform WorldQuery, etc) to do the op using "live data".

[alice-i-cecile]

Therefore they need to query for them individually, then use some abstraction (construct a Transform in the system, use a Transform WorldQuery, etc) to do the op using "live data".

I think we can get around this using bundle queries, enabled by #1843. Regardless, I think this split is RFC-worthy on its own, and out-of-scope for now.

[Benjamin-L]

Hmm, if we go forward with having a single Transform and implementing the operations that don't work correctly with nonuniform scale, I'd be in favor of renaming the scale field to lossy_scale or something like that, similar to unity.

I think we need to be careful to avoid exposing an interface that behaves unexpectedly in a very subtle way. One component of this is documentation, but another is that the names for the functions/fields involved have to signify the actual behavior clearly.

[aevyrie]

This is the problem. Users wanting to do the more complicated operations mentioned above in the update stage can't rely on a Transform that lags behind by a frame.

Isn't this exactly the problem we have now with GlobalTransform? You have to be extremely careful with your system ordering and stage placement or you risk having multiple-frame delays that can aggregate across systems. If these hypothetical users are trying to do these operations, they will already experience this problem because their GlobalTransform will be out of date. Plus, I presume we would have a TRS equivalent of Transform::compute_mat4 which would allow exactly the same use cases as we currently have with the "monolithic" Transform.

If anything, having the Transform and GlobalTransfom both sync at the same time between stages is preferable because it avoids the current situation where your Transform and GlobalTransfom are always out of sync with each other as soon as you touch the Transform. This can lead to some very difficult-to-troubleshoot problems because the "scope" of the transforms aren't clear. It feels analogous to immutable access while mutably borrowed

Before I veer too far off the topic of this issue, I should reframe to highlight what this proposed change solves:

1. Updates to TRS won't suffer the same drift as repeated Mat4 transforms (see first post for more pro/con)
2. By updating Mat4 versions of Transform and GlobalTransform at stage sync points, they are kept in lockstep and you are ensured that when you access them, they are in sync.
3. Instead of having to compute the Mat4 of a local Transform every time you need it, the result has been memoized in the component. Yes, it is out of sync since the last stage update, but that is already the case with GlobalTransform, and at some level is always going to be necessary unless we want to compute the Mat4 every time the Transform is touched. Maybe in the distant future we could have a update_transform() function that syncs the TRS/Transform/GlobalTransform so the user can specify sync points.
4. This removes some of the confusion around the Transform and GlobalTransform API - they are now both just Mat4s, and the TRS that construct them are completely separate.

[cart]

I think we can get around this using bundle queries, enabled by #1843. Regardless, I think this split is RFC-worthy on its own, and out-of-scope for now.

Agreed. Moving back to a "split component" model will require an RFC and it should take into account the large amount of history we have on the topic of transforms.

I personally think using a WorldQuery derive (or a theoretical bundle query) should be avoided when possible. They add complexity and feel "too special" for core things like transforms. We'd need to start explaining to users why they need to do Query<Transform> and Query<TransformMut> instead of Query<&Transform> and Query<&mut Transform>, which is what they've previously learned. It also creates a new "&Transform is still a valid query" class of error.

Hmm, if we go forward with having a single Transform and implementing the operations that don't work correctly with nonuniform scale, I'd be in favor of renaming the scale field to lossy_scale or something like that, similar to unity.

Local scale is not lossy, only global scale (and the results of Transform * Transform ops). We could consider representing that with new types.

If anything, having the Transform and GlobalTransfom both sync at the same time between stages is preferable because it avoids the current situation where your Transform and GlobalTransfom are always out of sync with each other as soon as you touch the Transform. This can lead to some very difficult-to-troubleshoot problems because the "scope" of the transforms aren't clear. It feels analogous to immutable access while mutably borrowed

GlobalTransform cannot be trusted and (unlike local transforms) is O(HIERACHY_SIZE) to update on the fly. Due to that time complexity, it should always be out of sync by default (imo). This doesn't need to be true for local transforms. Making Transform out of sync with individual SRT components just creates a new class of error and increases the complexity of the system as a whole / makes it harder for newbies to learn "the rules".

[HackerFoo]

I'd like non-uniform scaling of arbitrary axes in my app, and this cannot be supported in general with RST or SRT, so I'm currently restricted to scaling X/Y/Z in object space using Bevy's Transform. I will eventually need to switch to glam::Affine3A/Mat4 to achieve this, which means bypassing Transform/GlobalTransform. My internal respresentation uses Affine3A.

I'm about half-way there, because I'm already using my own custom transform component for all 3D objects drawn, so I may have my own workaround soon. Still, it'd be cleaner if there was direct support for it.

[alice-i-cecile]

How do you propose enabling operations like "transforming a point using the current rotation / translation/ scale of the entity", "rotating an entity around a point", or "scaling an entity using a non-zero origin"?

#2713 (or even a manual impl) would enable us to easily impl WorldQuery for Transform: allowing us to split out translation / rotation / scale into separate components. Then, users could request &mut Transform when they need to perform these rare but important compound operations, while reaping the benefits of improved performance and ergonomics by querying for &mut Translation the other 80% of the time.

[cart]

#2713 (or even a manual impl) would enable us to easily impl WorldQuery for Transform:

It was never about "ease of impl". Its about keeping abstractions to a minimum, especially for core / common / high traffic components. When a new bevy user asks "what is a Transform?", I don't want to say "oh thats a derived WorldQuery that combines Position, Rotation, and Scale components into a single Query". It immediately throws people into the deep end when they see Query<Transform>. They will ask "why not Query<&mut Transform>?", "is Transform a Component?", etc, etc.

Then, users could request &mut Transform

They couldn't, because &mut Transform is reserved for Component queries. We would either need Transform to pull in mutable components (which does enable iter() and iter_mut() queries), or have Transform and TransformMut variants.

[alice-i-cecile]

Its about keeping abstractions to a minimum, especially for core / common / high traffic components. When a new bevy user asks "what is a Transform?", I don't want to say "oh thats a derived WorldQuery that combines Position, Rotation, and Scale components into a single Query"

This is reasonable. My position is that new Bevy users should not be working with Transform, unless they have very unusual needs. For users without a game development background, the very concept of a "transform" (oh, it's a 4x4 matrix that can be decomposed using linear algebra to get the translation / rotation / scale / skew of an object in 3-dimensional space) is much more intimidating than translation / rotation / scale, which they learned about in 3rd grade.

In my experience helping Bevy users, I have never seen beginner gameplay code that needed to work with translation, rotation and scale in a compound fashion other than to convert screen space into world space and vice versa (which should not have to be done manually by beginners in the first place).

They couldn't, because &mut Transform is reserved for Component queries. We would either need Transform to pull in mutable components (which does enable iter() and iter_mut() queries), or have Transform and TransformMut variants.

Right. No specialization, so we can't even magic it to convert WorldQuery types into their mutable forms for non-components. Pulling in mutable components seems quite reasonable: it's a good default and in the 1% of cases where it matters there's a trivial escape-hatch.

[aloucks]

Having Transform implemented as scale, rotation, and translation is definitely more intuitive than having it represented as a raw matrix. However, I don't see a lot value (from a user perspective) in splitting it into different components other than perhaps to implement change detection on the individual attributes (which is already possible with some workarounds).

Optimizing around the "hello world" use case would add extra complexity for any non-trivial operation in 3d space (i.e. when you need to recompose the transform).