[Merged by Bors] - Use Affine3A for GlobalTransform to allow any affine transformation

[HackerFoo]

Objective

• Add capability to use Affine3As for some GlobalTransforms. This allows affine transformations that are not possible using a single Transform such as shear and non-uniform scaling along an arbitrary axis.
• Related to Revert transform back to using Mat4 #1755 and Reduce code duplication between Transform and GlobalTransform #2026

Solution

• GlobalTransform becomes an enum wrapping either a Transform or an Affine3A.
• The API of GlobalTransform is minimized to avoid inefficiency, and to make it clear that operations should be performed using the underlying data types.
• using GlobalTransform::Affine3A disables transform propagation, because the main use is for cases that Transforms cannot support.

---

Changelog

• GlobalTransforms can optionally support any affine transformation using an Affine3A.

[HackerFoo]

To be clear, the goal isn't to allow mixing Transform and Affine3A, but rather to allow selectively using Affine3A for entities where Transform won't suffice.

[HackerFoo]

This change would also allow for changing Transform::scale to a scalar f32, because composition cannot be implemented correctly for Transforms with non-uniform scale, so non-uniform scale in a parent Transform will give incorrect and surprising results in most cases.

[CptPotato]

Sorry for being a bit negative here, but I personally feel like this makes GlobalTransform quite convoluted and even harder to understand since it now may or may not propagate depending on which variant you choose. Users could also end up having to abstract their code over both types when they access a global transform.

Besides that, I'm not really seeing a use case where I would need to selectively use one transform type over the other. I would rather think about the requirements of my app and then decide which type of global transform to use across the board (while having propagation work in either case).

Do you have a specific example for how this might be used?

[james7132]

It should probably be noted that this would also make GlobalTransform 80 bytes over its current 48, which may have a stronger negative effect on cache perf for the type.

[HackerFoo]

@CptPotato @james7132 The Transform variant isn't really needed - it just seemed like a good idea to avoid affecting current code since the conversion to Affine3A is lossy, but GlobalTransform will get converted into a Mat4 before rendering.

I've removed the Transform case, just using an Affine3A. This makes the size ofGlobalTransform 64 bytes. It could be reduced to 48 bytes using a Mat3 + Vec3 if needed, but might reduce performance. Accumulating hierarchical transforms into a matrix representation has the benefit of handling non-uniform scale in Transforms correctly, which is not possible with SRT transforms, so this will benefit existing code.

My use case is a 3d modeler to support interactively stretching shapes along arbitrary axes. SRT can only represent scaling along the model's cardinal axes. Once the shapes are stretched, the change can be baked into the vertices, but it hurts performance to have to update meshes every frame while the user is performing the operation, and it's entirely unnecessary, because transforms are converted to matrices before rendering, which can represent this operation.

EDIT: With the latest change, transforms propagate as they did before, since all GlobalTransforms are Affine3A now.

[superdump]

@cart it seems like @HackerFoo wants to be able to do shears, if I understood correctly. I was also fiddling in Blender and noticed that the exported matrix transforms are not exact. I’m not certain it uses matrix representations throughout the hierarchy but it could well be an indication of that.

[HackerFoo]

Another option is to use a Mat3 for scale:

pub struct MyTransform {
    pub translation: Vec3,
    pub rotation: Quat,
    pub scale: Mat3,
}

This is what I've settled on for my app. This representation allows shear/arbitrary scaling while keeping rotations easy to work with. It requires 3 (translation) + 4 (rotation) + 9 (scale) = 16 floats, the same as Mat4/Affine3A, but may not be as performant.

[HackerFoo]

Here's a quick demo to show how non-axis-aligned scaling is useful in Noumenal (click for video):

With only axis-aligned scaling, you can't create diamonds from cubes. Worse, if a shape's local axes become misaligned with what the user thinks they should be, non-uniform scaling gives unexpected results. There are algorithms to automatically align axes to a model, but they are complicated, and not guaranteed to be correct.

[superdump]

I’ve only skimmed this so far. Don’t yet act on this feedback as I think @cart will need to decide what to do. We had discussed elsewhere that Affine3A is better for performance in propagation if just multiplying transforms, and that Affine3A represents shears which the separate representation does not, and that shears are desirable for art. Plus it seemed that other engines support shears. As such my personal conclusion was that we should use the separate representation for Transform and Affine3A for GlobalTransform, and that Transform’s compute_matrix would return an Affine3A.

Then I hadn’t figured out how to support any API breakage due to that. I feel like GlobalTransform should probably use all Vec3A and Mat3A for performance but then we’d have disparity in Vec3A vs Vec3. We could use Vec3A everywhere but there was an argument made that people may be confused by the A or confused that it’s 16 bytes instead of 12 bytes. I didn’t like the proposals to type alias Vec3A to Vec3 because that kind of implicit mapping is the kind of stuff that catches you out that I really don’t like wasting time to have to discover and subsequently remember. Things should be what they say they are in my opinion, and not add cognitive overhead as that is unergonomic.

I’m not sure how to expose the rotation and scale parts from GlobalTransform. It does feel like one has to accept that if you are interested in the scale then you must also take the rotation into account. But you should be able to extract rotation to get directional information. I think your macros covered the forward/back/up/down/left/right cases for Affine3A.

But yeah, this is a very pervasive change and so must be decided upon by @cart .

[CptPotato]

I agree with @superdump, that a matrix representation like Affine3A is definitely desirable for GlobalTransform in regards to transform propagation. So I like having the option to use it, I just felt like a generic approach could be cleaner and more flexible than using an enum.

Having only Affine3A as GlobalTransform (which is the current impl of this PR iiuc) would be my preference, unless transform types get configurable through generics in the future.

[superdump]

I don’t think we should have any enum and I don’t see a reason for generics currently. Just Affine3A. :)

[aevyrie]

As much as I don't look forward to all my stuff this will break, I agree this is the right direction. @HackerFoo it would be nice to mention in the changelog/migration section that this is a breaking change for anyone accessing GlobalTransform's public fields.

It seems odd that we would now support shear in the GlobalTransform but not the Transform. Would it make sense if Transform also became an Affine3A? What are the benefits of using TRS in the Transform? We could still expose TRS methods on Transform even if its underlying representation is an Affine3A. I imagine updating translation is the most common operation, yet it can be added directly to the contained translation vector in the Affine3A without any overhead.

[HackerFoo]

The benefits of TRS over Affine3A are that scale and rotation can be altered separately, and that shear is not introduced inadvertently. For my application, I use a different representatiobn:

pub struct MyTransform {
    /// translational movement
    pub translation: Vec3,
    /// rotation around the center
    pub rotation: Quat,
    /// used for scale, but can also contain rotation
    pub matrix: Mat3,
}

The Vec3 scale has been replaced with a Mat3. This allows scale and reflection along any axis. I could use polar decomposition to remove any rotation in the Mat3, but if only scaling and reflection are applied to it, there shouldn't be any. It's useful to allow rotation in the matrix for fast conversion from an Affine3A without polar decomposition, though.

So, yeah, deciding what to use for Transform is more difficult, and it doesn't have to be the same as GlobalTransform, since they serve different purposes. I think a better approach would be to make it easy to replace Transform with custom types, and read from GlobalTransform wherever possible to avoid dependence on Transform (I think Bevy already does this.)

[ruifengx]

If this is eventually merged into Bevy, would it be possible to have some generic Into<Affine3A> for local transformations instead of hard-coded Transform (or at least replace the current Transform with some enum)? Because eventually in transform_propagate_system it has to be converted into an Affine3A, but the current SRT encoding completely eliminated the possibility of e.g. shears. It seems to be against the principle of abstraction: users are paying (because some otherwise-perfectly-fine transformations are prohibited) for features they might not be using (i.e. separate SRT).

I imagine it could work like this (a very rough idea):

• bevy_transform only use Transform and GlobalTransform (both a thin wrapper for Affine3A) in transform_propagate_system;
• To use SRT, introduce another component TransformSRT, and have an additional system responsible for converting TransformSRT to Transform (by editing the Transform component on the entity);
• To properly detect errors, this system should avoid overwriting Transforms which are meant to be manipulated directly in user code. This can be done by introducing a tag component PersistentTransform;

I don't know the cost of the data duplication if both Transform and TransformSRT is present on the entity (if it is not affordable, an alternative is to remove generated Transforms after propagation), but otherwise I think this approach is flexible and extensible. The most general encoding (Transform, or Affine3A under the hood) is always available to users as a fallback. Besides, it is always possible to have different high-level encodings (e.g., different orderings of SRT, or perhaps Translation only): one can just add a new system for the conversion .before(TransformSystem::TransformPropagate).

There is however one downside I can think of (other than possible efficiency concerns): we will have to decide which encoding for local transformation should be used in SpriteBundle etc.

[james7132]

@ruifengx probably not the best place to be bikeshedding the design of the Transforms in the engine. This has been discussed at length in #4213. There are quite a few alternative ideas on how to approach that kind of use case in that discussion.

[cart]

I haven't actually reviewed this yet, but I am already pretty convinced that this is the right direction for Bevy. I'll try to get this reviewed (and ideally merged) before Bevy 0.8.

[james7132]

Adding the Needs-Benchmarking tag since this does impact a core system via transform propagation. I'm pretty sure this isn't going to be a potential issue given that Affine3A tends to be faster to multiply through and transform propagation is already random read/write heavy to begin with, but the increased size of the struct does raise concerns over increased cache misses.

[james7132]

I did a quick comparison for a few transform-intensive systems between this PR and main via the many_foxes stress test (yellow is this PR, red is main).

extract_skinned_meshes (717.23us -> 609.56us, ~15% faster)

transform propagation (1.69ms -> 1.89ms, ~10.5% slower)

It should be noted that transform propagation can be easily parallelized, but skinned mesh extraction cannot, so take these results with a grain of salt. I'm pretty sure the degradation in perf is coming from two sources:

• the increased size of the type: at 64-bytes it's basically forcing a cache miss every time we propagate down one level.
• The transform -> affine computation has been moved there.

[cart]

(just resolved merge conflicts)

Personally I don't like proxying a GlobalTransform via Transform in order to gain access to API that was removed from GlobalTransform. I think API should be available on GlobalTransform directly, even if it feels boilerplatey to implement.

I agree with @superdump. We should make it easier to interact with GlobalTransform / retain as many Transform apis as we can.

[HackerFoo]

@superdump @cart The difference in GlobalTransform and Transform's API is because I see Transform as more of a read/write component while GlobalTransform is an accumulator for Transform (or any other component that can be converted into GlobalTransform), and is mostly read. Furthermore, converting from GlobalTransform to Transform, or any RST representation, isn't always possible, and when it is possible, is expensive to do correctly with polar decomposition.

So I intentionally tried to restrict GlobalTransform to avoid expensive or incorrect computations, which in the worst case will allow the same errors back into Bevy that this PR aims to fix.

[HackerFoo]

That said, all the lossless from_whatever methods should be okay. It's conversion from Affine3A to RST (specifically polar decomposition of the 3x3 matrix into rotation and scaling) that I'm worried about.

[cart]

I do agree that Affine3A to RST should be avoided, yet we do still have places that rely on that behavior (at least, for now). And I think there will always be valid cases that want that (and are wiling to make assumptions). Might as well embrace that / make it easier. My last commit added docs calling out these constraints (snagged from glam) to the relevant methods.

On the topic of using Transform as "the" way to extract RST from GlobalTransform, I'm a bit torn. Clearly, its tied up in "inherited transform component" semantics. But I can see it also filling the role as a nice "core" RST math type. I think for now, lets avoid it where possible (in favor of to_rotation_scale_translation).

I also agree that GlobalTransform should largely be a read-only component. That being said, it is the "source of truth" transform component and there are valid reasons to interact with it. Might as well give those cases the best UX we can. There may be future scenarios that eschew Transform entirely (or implement an alternative propagation system that doesn't use Transform).

[HackerFoo]

I have eliminated all conversions from Affine3A to TRS in Bevy. Using any of these can give invalid results, but they are currently left for user convenience.

[cart]

bors r+

[bors]

Merge conflict.

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12