Skip to content

Add Transform2d component #8268

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

Closed
wants to merge 27 commits into from

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Mar 30, 2023

Objective

Add Transform2d and GlobalTransform2d components and use them to replace Transform and GlobalTransform in 2D systems.

Closes #2548
Closes #4149

Tasks

  • Use the 2D matrix in the shaders. Leaving this as-is for now. See @superdump's comment below.
    It should be investigated while keeping @superdump's thoughts about reducing the split between 2D and 3D in mind.

Changelog

  • Added Transform2d and GlobalTransform2d components.
  • Added Spatial2dBundle and Transform2dBundle.
  • Changed SpriteBundle, MaterialMesh2dBundle, Text2dBundle, Camera2dBundle, and the associated systems to use 2D transform components.
  • Changed sync_simple_transforms and propagate_transforms to be generic over the transform types.

Migration Guide

SpriteBundle, MaterialMesh2dBundle, Text2dBundle, Camera2dBundle, and the associated systems were changed to use the new Transform2d and GlobalTransform2d components.

Keep in mind that queries for Transform and GlobalTransform will no longer match entities spawned with these bundles.

sync_simple_transforms and propagate_transforms are now generic over the transform types.

// Before (0.10)
app.add_systems((
    sync_simple_transforms,
    propagate_transforms,
));

// After (0.11)
app.add_systems(
    Update,
    (
        sync_simple_transforms::<Transform, GlobalTransform>,
        propagate_transforms::<Transform, GlobalTransform>,
        // And the same for the new 2d transform components
        sync_simple_transforms::<Transform2d, GlobalTransform2d>,
        propagate_transforms::<Transform2d, GlobalTransform2d>,
    )
);

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just doing a cursory scan.

I don't like that the transform propagation's unsafe is spreading. IMO we should make that generic or trait based before we merge this.

///
/// Keep in mind that this is relative to the [`Parent`]'s `z_translation`.
/// The other fields on [`Transform2d`] don't affect this because they are strictly 2D.
pub z_translation: f32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this can be inherited. If we're using this as a "2D sort layer", IMO this should always be absolute, otherwise inheritance can result in odd rendering artifacts if you move a parent's Z.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely need better ways to specify the rendering order in 2d, but I'd rather investigate that in a follow-up PR so I kept it as-is.

Your suggestion is fairly straightforward though so if other's deem it uncontroversial I don't mind doing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is fine as is. james concern is legitimate but it can be relegated to a follow-up PR.

For an entity made out of multiple sprites, you should be able to position on z-axis the sprites relative to each other. It probably needs something like "is this on top or below the parent" rather than a f32 but I think it's beyond scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea is that z is used only for sorting, but for rendering, only an Affine2 is needed?

@james7132 james7132 added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-Transform Translations, rotations and scales X-Controversial There is active debate or serious implications around merging this PR labels Mar 30, 2023
@superdump
Copy link
Contributor

In terms of general direction for 2D, I feel like it for sure makes sense to have 2D 'user space' as in components that people use in the main world.

I feel like with the rendering side of things, it is less clear. With good generic batching, and a desire to reduce 2D overdraw, I feel like getting rid of the custom sprite batching that only draws quads in a hand-crafted way and instead support sprites on top of 2D meshes would be much more flexible and maintainable. Then people could leverage custom materials on sprites for example.

I've even had ideas about not having as much special 2D rendering code as well, but to leverage the 3D foundation for 2D too. I don't know if that would work out but if it did then it could reduce code duplication and new functionality developed for 3D could more readily be used for 2D as well. I could be convinced that this wouldn't work. I haven't thought about it enough yet. But I have felt there is a good chunk of duplication between 2D and 3D mesh/material code.

@tim-blackbird
Copy link
Contributor Author

Sounds good @superdump. I'll leave the rendering internals as-is.

@joseph-gio joseph-gio self-requested a review March 31, 2023 14:32
@tim-blackbird tim-blackbird marked this pull request as ready for review April 5, 2023 18:41
@james7132 james7132 self-requested a review April 5, 2023 19:35
@nicopap nicopap added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 21, 2023
@@ -185,7 +185,7 @@ impl GlobalTransform {

/// Transforms the given `point`, applying shear, scale, rotation and translation.
///
/// This moves `point` into the local space of this [`GlobalTransform`].
/// This moves the `point` from the local space of this entity into global space.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other doc comments, I saw "relative to the reference frame." IMO "into global space" is a bit more evocative. But I think docs should use a consistent vocabulary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can do better with terminology here. I matched the term used here with the existing docs for transform_point on Transform

Comment on lines +84 to +86
unsafe fn propagate_recursive<A, B>(
parent: &B,
transform_query: &Query<(Ref<A>, &mut B, Option<&Children>), With<Parent>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -6,7 +6,7 @@
//! Having sprites out of the camera's field of view should also help stress
//! test any future potential 2d frustum culling implementation.

use std::time::Duration;
use std::{f32::consts::TAU, time::Duration};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using TAU in the stress tests as they don't serve as actual examples (i.e. They're not minimal examples of how to do something with Bevy in an idiomatic way). Instead, They are tools used by engine contributors to test performance.

But those are my thoughts. I'm not 100% sure how others think about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a TAU lover, but I think it's a controversial point.

Copy link
Contributor Author

@tim-blackbird tim-blackbird Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I have to write PI * 2., a part of my soul will die, but I would do it to get this merged :)

@@ -10,6 +10,8 @@
//! Add the `--colored` arg to run with color tinted sprites. This will cause the sprites to be rendered
//! in multiple batches, reducing performance but useful for testing.

use std::f32::consts::TAU;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tim-blackbird
Copy link
Contributor Author

This sounds like a pain for all the third party plugins that want to be dimension-agnostic...

There will be some more effort required for plugin authors, but the improvement to performance, memory-footprint(more than halved), and ergonomics for the users is well worth it.

And for what it's worth. Porting bevy_rapier to this branch was very straightforward and it worked on the first compile.

@idanarye
Copy link

And for what it's worth. Porting bevy_rapier to this branch was very straightforward and it worked on the first compile.

Could it still be used to create games with 3D graphics and 2D physics? Or would you have to add some translation layer to convert between the 2D and 3D transforms?

@nicopap
Copy link
Contributor

nicopap commented May 31, 2023

could you add "closes #2548" to the PR description?

The issue doesn't request a 2d transform but in the "alternatives" section it says

A Transform2D component would be more ideal

@ruifengx
Copy link

This would also solve the long-lasting problem that Transform cannot represent 2D shears. May I know what is currently blocking this PR so that I can help push it forward?

@tim-blackbird
Copy link
Contributor Author

Neither Transform or Transform2d support any shearing as it stands.

I was planning on revisiting this PR soon to try and get it merged for 0.13 :)

@ruifengx
Copy link

ruifengx commented Oct 20, 2023

Neither Transform or Transform2d support any shearing as it stands.

That's a little unfortunate, because I think this would be the best opportunity if we will ever support shearing. I think I got this PR confused with NodeTransform for UI and drew the wrong conclusion.

Performance-wise I'd expect a plain Affine2d to out-perform the current Transform2d encoding, but of course accessing the TRS components would not be as convenient. If it is decided that using using Affine2d for Transform2d is not an option, may I propose we use a diagonalisable matrix (therefore Mat2 instead of Vec2) for Transform2d::scale? In that case, scale would simply become scale: Mat2::from_diagonal(scale), and it would only take 8 extra bytes per entity, but we will be able to faithfully represent each Affine2d in a Transform2d.

Edit: actually we can require that the scaling matrix to be upper triangular (because we have unique QR decomposition for any full-rank matrix). In that case we only need extra 4 bytes (by omitting the lower-left zero element), but I doubt if this optimisation really worth it.

@meepleek
Copy link
Contributor

I would really love this.

Wouldn't adding a 3D suffix to Transform and friends make sense for the sake of consistency?
There could be type aliases for the base names based on a feature defaulting to 3D which would have the same result as is the PR (Transform being an alias for Transform3D), but it would also allow for 2D projects to have the same ergonomics.

This would also close #4149 .

@tim-blackbird
Copy link
Contributor Author

Benchmark

Command used: cargo run --release --example bevymark -- --waves 1 --per-wave 50000 --benchmark

This PR(yellow) vs main(red).
image

There's more gains to be made as this is still using Affine3s on the rendering side and were sending those to the gpu as well :)

@superdump
Copy link
Contributor

@devil-ira hmm. So as noted in a comment above - is the idea to use z only for sorting, and use the Affine2 for the 'mesh uniform'? What about if you want to rotate about the x or y axes? (I'm thinking of spinning coins in platform games.) I mean, you can emulate that with a sprite animation, or with vertical/horizontal scaling, I suppose. Could you emulate a rotation around y = x (or 1,1 if you like)? I'm not sure.

I have a general concern that strictly limiting sprites or 2D meshes to only 2D transforms will work for some but force others to just go 3D, or implement awkward hacks for trying to achieve the same thing that would be simple in 3D.

@cart what are your thoughts on this space?

@tim-blackbird
Copy link
Contributor Author

That is my biggest concern as well. Ideally we maintain enough flexibility that we don't end up railroading users.

We could easily keep allowing the use of 3D transforms with sprites. This would only need a few lines to be tweaked here. I think this is the right choice.

This may create issues if say, a 2d physics plugin, later requires 2D transforms and you want to use 3D ones for your sprites, but I was already planning to support parenting 3D transforms to 2D ones (and not vice versa) to support a 2D game sim + 3D graphics (ie. Metroid Prime).

And introducing skew would provide some flexibility too.
(@ruifengx might have opinions on this.)

@ruifengx
Copy link

And introducing skew would provide some flexibility too.

It would be great if Transform2d provides a skew. Apparently I was overcomplicating the situation and an additional skew: f32 would be enough to support any 2D affine transformation. But we still need to decide what skew means. I can think of three possible interpretations:

  1. scale = Vec2::new(sx, sy) and skew = k together form an upper triangular matrix [[sx, 0], [k, sy]], in place of the current diagonal matrix from sx and sy;
  2. skew = k indicates a [[1, 0], [k, 1]] skew matrix, applied before scale;
  3. skew = k indicates the same skew matrix, but applied after scale;

In my current use cases, the skew always change together with scale, so any of these would work equally well for me (it just means I need to use different formulae for the pre-calculation). The first one saves one multiplication, while the second and the third ones make skew only related to the skewing angle (independent of scaling).

@superdump
Copy link
Contributor

In my opinion, if we can stick to using a 3D affine transform in the render world and use a 2D transform abstraction in the main world if people want to use it, that provides the most flexibility for people to use either a simpler 2D API or a 3D in a 2D kind of world API as they want/need to. It could leave some performance on the table, but also, I feel a bit like there’s more performance to be gained generally, and the level of performance is maybe not as important as flexibility and simplicity in this case. Would you all feel ok if we started with using a 3D affine transform in the render world and then offering both simpler 2D and 3D transforms for use for 2D stuff, and maybe later looking into whether a move to pure 2D in the rendering side is necessary/significantly beneficial?

@tim-blackbird
Copy link
Contributor Author

That sounds good to me :)

@tim-blackbird
Copy link
Contributor Author

I'm currently looking at splitting render ordering into a separate component in another PR so I can get rid of z_translation in this one.

@meepleek
Copy link
Contributor

Any idea when this might land 🙂? I'm guessing it won't be in 0.13.

@NthTensor
Copy link
Contributor

So why do we need a separate GlobalTransform2d? Can't we have Transform2d and Transform both propagate into GlobalTransform if we require them to be disjoint? Seems like this would resolve some of the issues with keeping positions generic for external plugins.

github-merge-queue bot pushed a commit that referenced this pull request Mar 11, 2024
# Objective

Rotating vectors is a very common task. It is required for a variety of
things both within Bevy itself and in many third party plugins, for
example all over physics and collision detection, and for things like
Bevy's bounding volumes and several gizmo implementations.

For 3D, we can do this using a `Quat`, but for 2D, we do not have a
clear and efficient option. `Mat2` can be used for rotating vectors if
created using `Mat2::from_angle`, but this is not obvious to many users,
it doesn't have many rotation helpers, and the type does not give any
guarantees that it represents a valid rotation.

We should have a proper type for 2D rotations. In addition to allowing
for potential optimization, it would allow us to have a consistent and
explicitly documented representation used throughout the engine, i.e.
counterclockwise and in radians.

## Representation

The mathematical formula for rotating a 2D vector is the following:

```
new_x = x * cos - y * sin
new_y = x * sin + y * cos
```

Here, `sin` and `cos` are the sine and cosine of the rotation angle.
Computing these every time when a vector needs to be rotated can be
expensive, so the rotation shouldn't be just an `f32` angle. Instead, it
is often more efficient to represent the rotation using the sine and
cosine of the angle instead of storing the angle itself. This can be
freely passed around and reused without unnecessary computations.

The two options are either a 2x2 rotation matrix or a unit complex
number where the cosine is the real part and the sine is the imaginary
part. These are equivalent for the most part, but the unit complex
representation is a bit more memory efficient (two `f32`s instead of
four), so I chose that. This is like Nalgebra's
[`UnitComplex`](https://docs.rs/nalgebra/latest/nalgebra/geometry/type.UnitComplex.html)
type, which can be used for the
[`Rotation2`](https://docs.rs/nalgebra/latest/nalgebra/geometry/type.Rotation2.html)
type.

## Implementation

Add a `Rotation2d` type represented as a unit complex number:

```rust
/// A counterclockwise 2D rotation in radians.
///
/// The rotation angle is wrapped to be within the `]-pi, pi]` range.
pub struct Rotation2d {
    /// The cosine of the rotation angle in radians.
    ///
    /// This is the real part of the unit complex number representing the rotation.
    pub cos: f32,
    /// The sine of the rotation angle in radians.
    ///
    /// This is the imaginary part of the unit complex number representing the rotation.
    pub sin: f32,
}
```

Using it is similar to using `Quat`, but in 2D:

```rust
let rotation = Rotation2d::radians(PI / 2.0);

// Rotate vector (also works on Direction2d!)
assert_eq!(rotation * Vec2::X, Vec2::Y);

// Get angle as degrees
assert_eq!(rotation.as_degrees(), 90.0);

// Getting sin and cos is free
let (sin, cos) = rotation.sin_cos();

// "Subtract" rotations
let rotation2 = Rotation2d::FRAC_PI_4; // there are constants!
let diff = rotation * rotation2.inverse();
assert_eq!(diff.as_radians(), PI / 4.0);

// This is equivalent to the above
assert_eq!(rotation2.angle_between(rotation), PI / 4.0);

// Lerp
let rotation1 = Rotation2d::IDENTITY;
let rotation2 = Rotation2d::FRAC_PI_2;
let result = rotation1.lerp(rotation2, 0.5);
assert_eq!(result.as_radians(), std::f32::consts::FRAC_PI_4);

// Slerp
let rotation1 = Rotation2d::FRAC_PI_4);
let rotation2 = Rotation2d::degrees(-180.0); // we can use degrees too!
let result = rotation1.slerp(rotation2, 1.0 / 3.0);
assert_eq!(result.as_radians(), std::f32::consts::FRAC_PI_2);
```

There's also a `From<f32>` implementation for `Rotation2d`, which means
that methods can still accept radians as floats if the argument uses
`impl Into<Rotation2d>`. This means that adding `Rotation2d` shouldn't
even be a breaking change.

---

## Changelog

- Added `Rotation2d`
- Bounding volume methods now take an `impl Into<Rotation2d>`
- Gizmo methods with rotation now take an `impl Into<Rotation2d>`

## Future use cases

- Collision detection (a type like this is quite essential considering
how common vector rotations are)
- `Transform` helpers (e.g. return a 2D rotation about the Z axis from a
`Transform`)
- The rotation used for `Transform2d` (#8268)
- More gizmos, maybe meshes... everything in 2D that uses rotation

---------

Co-authored-by: Tristan Guichaoua <[email protected]>
Co-authored-by: Robert Walter <[email protected]>
Co-authored-by: IQuick 143 <[email protected]>
@alice-i-cecile alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Aug 16, 2024
@tim-blackbird
Copy link
Contributor Author

This needs a proper design :)
Tracked in #7876

@tim-blackbird tim-blackbird deleted the transform2d branch November 12, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-Transform Translations, rotations and scales C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Parent scale.z affects child sprite z-index Enhance 2D Transform/Vec2 Quality of Life