-
Notifications
You must be signed in to change notification settings - Fork 66
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
rfcs/82-transform2d.md
Outdated
```rust | ||
pub struct Transform2d { | ||
position: Vec2, | ||
rotation: f32 //RFC Note: We could use Rotation2d here |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
```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. |
There was a problem hiding this comment.
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.
- 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). - 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.
- 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"?
There was a problem hiding this comment.
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.
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. |
I agree
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".
Agreed. |
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. |
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? |
As a user, it would make sense to me to go with |
I agree, incrementalism would be wise here. |
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.
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 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
|
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. |
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. 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. |
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