Skip to content
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

Dedicated 2D Transform #82

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Dedicated 2D Transform #82

wants to merge 6 commits into from

Conversation

BobG1983
Copy link

@BobG1983 BobG1983 commented Jun 1, 2024

The ergonomics of using Bevy's existing Transform for 2D games is poor. To counter this a 2D Transform component should be created that has improved ergonomics.

Rendered: https://github.com/BobG1983/bevy_rfcs/blob/main/rfcs/82-transform2d.md

@NthTensor NthTensor self-requested a review June 2, 2024 12:10
Copy link

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

This is a great start, and it outlines the problem very well. I left some comments, and I also want to have a crack at answering some of those unsolved questions.

Do we want to take this burden ourselves?

Yes. I think there's a clear consensus that something like this is needed, and the maintenance burden is just the cost of properly supporting 2d (which is one of bevy's core goals).

Are we happy about using a fairly simple/naive DrawOrder implementation?

I am not, and I suspect I will not be until I see some prototypes. This is not just a question of ergonomics, but also of correctness, flexibility, and performance. It's really difficult to assess these without actually writing and evaluating several z-layering systems.

What are the knock-on effects on UI and its desire for a potential UiTransform/TransformUi

It's vitally important that bevy UI not use these 2d transforms. I think UI deserves it's own layout/transform types which are informed by this work, but which exist in "camera space" rather than "world space" and so will not trip up crates likebig_space.


### The Problem with Rotation

Rotation in bevy is stored as a `Quat` in the Transform component. Again, this is fine for 3D games, and is usable for 2D games, but has poor ergonomics. Rotation in a 2D game in Bevy is always counterclockwise around the Z axis. As with using `Vec3`'s for Translation this causes a lot of boilerplate, extracting the value of a quaternions rotation around Z in radians, performing calculations on that value, then creating a new `Quat` rotating around Z before reassigning. Much as with Translation, this is a poor user experience.
Copy link

@NthTensor NthTensor Jun 4, 2024

Choose a reason for hiding this comment

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

I would say that most but not all rotation is about Z, or possibly about the camera normal. There are reasons to want 2d rotation about a different axis, or affine rotations which mimic the effect of rotation beyond the plane. But I'm being pedantic.

Copy link
Author

Choose a reason for hiding this comment

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

I think it would be fine to give Transform3d to something you wanted the extra flexibility on. I think it should be trivial to mix and match given they all land in the same global transform.

```rust
pub struct Transform2d {
position: Vec2,
rotation: f32 //RFC Note: We could use Rotation2d here

Choose a reason for hiding this comment

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

I will 100% want this to be Rotation2d.

Copy link
Author

Choose a reason for hiding this comment

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

Cool. I'll make that change

pub struct Transform2d {
position: Vec2,
rotation: f32 //RFC Note: We could use Rotation2d here
scale: Vec2

Choose a reason for hiding this comment

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

This is sidestepping the problems with nonuniform scale, and is a feature regression from the current. But I'm ok to leave that out for now and tackle 2d/3d scale at the same time in follow up, so long as it lands in the same release cycle.

Copy link
Author

@BobG1983 BobG1983 Jun 4, 2024

Choose a reason for hiding this comment

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

...I'm probably going to need to have someone talk to me live (discord?) about this so I can understand. I've read the threads, I've read a post about it....and I feel like I'm being gaslit when it comes to "the problems with nonuniform scale". So I'm going to leave this for now and come back when I don't feel like a crazy person and clearly understand the problem.

Comment on lines +46 to +50
```rust
pub struct DrawLayer(pub f32);
```

As you can see `DrawLayer` is just a wrapper around an `f32`. Entities with a higher value for their `DrawLayer` will be drawn on top of those with lower values.

Choose a reason for hiding this comment

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

So this is one of the approaches that's been tossed around for z ordering, and personally I'm not a huge fan.

There are a few issues.

  1. Floating points are not totally ordered, and by exposing them to users for ordering we invite them to make mistakes (rather than just using them for ordering internally, where we can ensure correctness). It's not clear to me what should happen if you have a NAN layer, or if negative draw layers should be allowed, or what the ordering properties for negative vs positive zero should be. Negative and positive infinity do seem fine, and I'll note that there are ways to make floats Ord (but they are not free).
  2. You can exhaust the precision of the space; inserting a layer between two other layers can fail, require moving an arbitrary number of layers to make room. I'm not sure how common this will be in practice, but I suspect with enough nesting it will be something people run into repeatedly.
  3. It seems like to determine z position, you sum the layers of all the ancestors and add it to the layer of the entity. This probably means storing a PropigatedLayer(f32) component, which seems like it would add complexity. "Layer" also dosn't seem like the right term to describe this; perhaps "z-offset"?

Copy link
Author

Choose a reason for hiding this comment

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

I thought about this after I wrote the RFC, but honestly, I would prefer this to be a u32 rather than an f32.

Also yes, that's my suggestion, summing all the layers of the parents values and adding it the childs.

@NthTensor
Copy link

Oh, and one last thing. I want to mix 2d and 3d transforms can be mixed seamlessly in the hierarchy; mixing 2d and 3d stuff like this in games is super common. This will need to have an expanded section on the way these should interact.

@BobG1983
Copy link
Author

BobG1983 commented Jun 4, 2024

Yes. I think there's a clear consensus that something like this is needed, and the maintenance burden is just the cost of properly supporting 2d (which is one of bevy's core goals).

I agree

Are we happy about using a fairly simple/naive DrawOrder implementation?

I am not, and I suspect I will not be until I see some prototypes. This is not just a question of ergonomics, but also of correctness, flexibility, and performance. It's really difficult to assess these without actually writing and evaluating several z-layering systems.

Professionally, I've used Z (Transform), seperate draw layer (f32), and "define list of draw layers and pick". Either of the 2nd two is fine IMO, I've no preference. If you have other suggestions I'd love to hear them, but I find the f32 approach the simplest and least "magic".

What are the knock-on effects on UI and its desire for a potential UiTransform/TransformUi

It's vitally important that bevy UI not use these 2d transforms. I think UI deserves it's own layout/transform types which are informed by this work, but which exist in "camera space" rather than "world space" and so will not trip up crates likebig_space.

Agreed.

@NthTensor
Copy link

NthTensor commented Jun 4, 2024

If you have other suggestions I'd love to hear them, but I find the f32 approach the simplest and least "magic".

See my rough proposal here. This issue also has a really good list of criteria for the layering system.

I am most worried about making users worry about manual reshuffling to make room for new intermediate layers. If reshuffling is necessary, I think bevy should try to handle it quietly by default. While we are laying out our preferences, I would strongly prefer using numerics (floating points or integers) over using enums or discrete types for layers/ordering.

@BobG1983
Copy link
Author

BobG1983 commented Jun 4, 2024

So your suggestion is the user sets up layers, that have an f32 depth. Then you assign a sprite to a layer?

Layers are relational? Above/below?

The inner functionality of those layers can then be whatever (your proposal if f32) and we handle relayering "magically" when a new layer is added?

@MScottMcBee
Copy link

As a user, it would make sense to me to go with DrawOrder for the context of this RFC. It seems about the same as current behavior, which let's this RFC focus on reworking the API vs figuring out a new feature. 1275 could be an RFC / working group on its own and I'd rather have Transform2d in one release and a layering system in another than wait for both.

@NthTensor
Copy link

I agree, incrementalism would be wise here.

@MScottMcBee
Copy link

MScottMcBee commented Jun 26, 2024

I took a stab at implementing this RFC in a fork and got the Breakout example to work with just Transform2ds. I'd like to share some thoughts and further questions I ran into.

I think, for now, DrawLayer should be part of Transform2d. It doesn't make sense for a Transform2d to not have a DrawLayer to me. Until required components or an equivalent construct comes online having them separate sounds like it's just asking for users to miss it. I think it's worth having it be its own type rather than a raw f32 to leave room for implementing a more involved layering system later.

I don't know if I agree with having Sprite2dBundle and Sprite3dBundle. At least, I think SpriteBundle should stay and use Transform2d. It might be breaking but I think it's more intuitive

The approach outlined here is nice, but I'm not 100% sold on it. There are two large questions that make me say that.

  1. Should hierarchies mix? Should EntityA with Transform3d be able to have EntityB with a Transform2d as a child? If not, how an we enforce that?

If they can, do they effect each other or is it organizational?

If they can effect each other, what does that even mean? Would EntityB be layered according to EntityA's Z value, or would all sprites be drawn on top of the 3d scene, only using the screen space X & Y from EntityA?

If EntityB is layered into the 3d scene, is that better served with a billboard component that has a Trasnsform3d instead? If it's drawn on top, would it be better served with a different mechanism than parenting with different transform components?

In my branch I modified sync_simple_transforms to work on both Transform3d and Transform2d. It's ugly.

pub fn sync_simple_transforms(
    mut query: ParamSet<(
        Query<
            (
                Option<&Transform3d>,
                Option<&Transform2d>,
                &mut GlobalTransform,
            ),
            (
                Or<(Changed<Transform3d>, Changed<Transform2d>, Added<GlobalTransform>)>,
                Without<Parent>,
                Without<Children>,
                Or<(With<Transform3d>, With<Transform2d>)>,
            ),
        >,
        Query<
            (
                Option<Ref<Transform3d>>,
                Option<Ref<Transform2d>>,
                &mut GlobalTransform,
            ),
            (
                Without<Parent>,
                Without<Children>,
                Or<(With<Transform3d>, With<Transform2d>)>,
            ),
        >,
    )>,
    mut orphaned: RemovedComponents<Parent>,
) {
    // Update changed entities.
    query.p0().par_iter_mut().for_each(
        |(opt_transform3d, opt_transform2d, mut global_transform)| {
            *global_transform = opt_transform3d
                .map(|t| GlobalTransform::from(*t))
                .or(opt_transform2d.map(|t| GlobalTransform::from(*t)))
                .unwrap();
        },
    );
    // Update orphaned entities.
    let mut query = query.p1();
    let mut iter = query.iter_many_mut(orphaned.read());
    while let Some((opt_transform3d, opt_transform2d, mut global_transform)) = iter.fetch_next() {
        if let Some(transform) = opt_transform3d {
            if !transform.is_changed() && !global_transform.is_added() {
                *global_transform = GlobalTransform::from(*transform);
            }
        }
        if let Some(transform) = opt_transform2d {
            if !transform.is_changed() && !global_transform.is_added() {
                *global_transform = GlobalTransform::from(*transform);
            }
        }
    }
}

I'll concede I'm not the most experienced Rust/Bevy dev, so maybe this can be done better, but I'm really not a fan of the ergonomics of having multiple optionals in a query. It's not TOO bad for two transform types, but what if we want something like TransformUI or TransformUI3d later on?

  1. What if an entity has multiple Transform components?
    This would leave which one ends up resulting into the GlobalTransform to system ambiguities. But how would we stop that? I don't know of any way to set components to be exclusive. We could have a system to remove one component if the other was added, but it'd be nice to have something at compile time.

@BobG1983
Copy link
Author

BobG1983 commented Jul 7, 2024

Perhaps the answer is that we don't have transform as a component at all but a query struct. Then the individual components are split out and the only actual Tranform becomes a compute LocalTransform and a computed GlobalTransform.

I think mixed transform hierarchies is an interesting question. I don't have an opinion.

In terms of DrawOrder not being a raw f32. I'm fine with having a type though we may want to start it as a new type of f32 or a deref f32 now.

FYI @NthTensor who is planning on putting a working group together.

@DasLixou
Copy link

DasLixou commented Aug 2, 2024

I dont think a seperate transform component is the way to go. What should happen when an entity has both Transform and Transform2d? What about systems that just use transform, e.g. the transform propagation. You don't want to write them twice.
We could make transform an enum, for example

enum Transform {
    2d {
        position: Vec2,
        depth: f32,
        rotation: f32,
        scale: Vec2,
    },
    3d {
        translation: Vec3,
        rotation: Quat,
        scale: Vec3,
    },
}

But here we also would have to split the logic twice, plus having a breaking change for every 3d user.
When we now reextract the two possibilities into seperate structs and add the methods .2d() and .3d() to it, we would still have a breaking change, because now everyone has to do .3d(), but all the other problems should be solved.
Then the question is if we always store the 3d transform, or store an enum with the dimension from initialization and just return directly or convert on the fly when called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants