Scale sprite z level by their scale by mockersf · Pull Request #4156 · bevyengine/bevy

mockersf · 2022-03-08T20:21:14Z

Objective

Fix Parent scale.z affects child sprite z-index #4149
Apply the sprite own scale on the z axis
- the scale was already applied to other axis
- the scale on z axis of parents was already applied
- => only the z axis scale of a sprite own transform was not used

Solution

scale the sprite z axis by its scale.
- as the multiplication was done several times, I factored it in its own parameter to do it only once
- after extracting it, keeping the transform in 3d isn't as useful, so I changed it to a 2d transform
- changing the new mul_vec2 to a single line has an impact, so I also changed mul_vec3 on transforms

fps	before	after
`many_sprites`	68~69	77~79
`bevymark 1300 100`	49~50	52
`bevymark 10000 100`	7	8~10

mockersf · 2022-03-08T20:22:43Z

crates/bevy_sprite/src/render/mod.rs

+pub struct Transform2d {
+    pub translation: Vec2,
+    pub scale: Vec2,
+    pub rotation: Quat,


it feels wrong to keep a Quat for rotations, but I'm not sure if keeping only two axis of rotations would be enough here. I feel it would, which would probably make it faster?

Don't we only need a single 2D rotation axis?

heh 3d rotations in 2d are one of the things where I give up usually...

I think we can use Quat::mul_vec3 with Vec3::X, and truncate that. We then get the angle from that Vec2, e.g. Vec2::angle_between with Vec2::X.

I think we can use Quat::mul_vec3 with Vec3::X

that's exactly what local_x() does, so it's simply

Vec2::X.angle_between(transform.local_x().truncate())

"Get the angle between the global x-axis and the local x unit-vector"

alice-i-cecile · 2022-03-08T20:32:16Z

Are the numbers in the table linked above FPS?

mockersf · 2022-03-08T20:33:34Z

~~yes but I 🤦 in the name of the columns~~ fixed

alice-i-cecile · 2022-03-08T20:35:21Z

crates/bevy_transform/src/components/transform.rs

-        value += self.translation;
-        value
+    pub fn mul_vec3(&self, value: Vec3) -> Vec3 {
+        self.rotation * self.scale * value + self.translation


Suggested change

self.rotation * self.scale * value + self.translation

self.scale * self.rotation * value + self.translation

Let's keep the ordering consistent here.

I'm not sure those are commutatives. I added parentheses to make sure it happens in the right order

Is this change necessary (turning the 3 lines into 1)? I would think the compiler optimises it to the same result, and the existing notation is a lot clearer about the order of operations imo.

Though mockersf is right, it definitely should be scale applied to value first, then multiplied by rotation, then lastly translation added. perhaps translation + rotation * (scale * value) is nicer?

I like to be clear that we're following SRT here: it's much cleaner at a glance if we're adding Translation last.

It's a matter of perspective I guess; If you were to consider Scale, Rotation, Translation as independent transformations that you compose like

translate(rotate(scale(value)))

Then it's a lot more visually similar imo, since the order of operations is

translation + (rotation * (scale * value))

It only looks inside out compared to the "SRT" acronym because the inner-most (farthest right) function is applied to the value first

alice-i-cecile · 2022-03-08T20:36:36Z

crates/bevy_sprite/src/render/mod.rs

+    pub rotation: Quat,
+}
+
+impl Transform2d {


There are a large number of convenience methods and conversions I would like for this, but IMO we should keep the scope of this PR small.

alice-i-cecile · 2022-03-08T20:38:37Z

crates/bevy_sprite/src/render/mod.rs

 }

+#[derive(Clone, Copy)]
+pub struct Transform2d {


I feel quite strongly that this should be used in the public APIs for both UI and sprites, along with a public z-layer component. I'm not sure if this PR is the place for it though.

This should be done in it's own PR to not go out of scope for this one.

I renamed the struct to make it clear it is not to be used as a general Transform2d

crates/bevy_sprite/src/render/mod.rs

alice-i-cecile · 2022-03-08T20:43:06Z

So, as much as I desperately want "dedicated 2D coordinates in user-facing APIs", I don't think this PR is the place to make that change.

It will require quite a bit of churn (and lots of convenience methods). I'd rather not block a serious performance win + bug fix on that; we can use the types defined here in SpriteBundle and so on later.

DJMcNab

The perf changes seem good.

DJMcNab · 2022-03-08T21:34:33Z

crates/bevy_text/src/text2d.rs

+                        scale: transform.scale.truncate(),
+                        rotation: transform.rotation,
+                    },
+                    z_layer: transform.translation.z * transform.scale.z,


Suggested change

z_layer: transform.translation.z * transform.scale.z,

z_layer: transform.translation.z,

The sprite has 0 width, changing the depth here based on the scale doesn't make sense.

DJMcNab · 2022-03-08T21:50:25Z

crates/bevy_sprite/src/render/mod.rs

+pub struct Transform2d {
+    pub translation: Vec2,
+    pub scale: Vec2,
+    pub rotation: Quat,


I think we can use Quat::mul_vec3 with Vec3::X, and truncate that. We then get the angle from that Vec2, e.g. Vec2::angle_between with Vec2::X.

mockersf · 2022-03-13T00:59:03Z

we should move to a more complete 2d Transform rather than a in between solution like this

mockersf added 4 commits March 8, 2022 19:15

extracted sprites use a 2d transform separated from its z level

5b86b2d

simplify mul operation

c182854

inline it

e153172

single line for mul_vec3

4492d3f

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 8, 2022

mockersf commented Mar 8, 2022

View reviewed changes

mockersf changed the title ~~extracted sprites use a 2d transform separated from its z level~~ Scale sprite z level by their scale Mar 8, 2022

alice-i-cecile added A-Rendering Drawing game state to the screen A-Transform Translations, rotations and scales C-Bug An unexpected or incorrect behavior and removed S-Needs-Triage This issue needs to be labelled labels Mar 8, 2022

alice-i-cecile reviewed Mar 8, 2022

View reviewed changes

crates/bevy_sprite/src/render/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Mar 8, 2022

View reviewed changes

crates/bevy_sprite/src/render/mod.rs Show resolved Hide resolved

mockersf added 2 commits March 8, 2022 21:45

keep the order

de07ac3

doc strings

060d644

alice-i-cecile approved these changes Mar 8, 2022

View reviewed changes

fix doc links

08034fe

DJMcNab reviewed Mar 8, 2022

View reviewed changes

mockersf closed this Mar 13, 2022

	self.rotation * self.scale * value + self.translation
	self.scale * self.rotation * value + self.translation

	z_layer: transform.translation.z * transform.scale.z,
	z_layer: transform.translation.z,

Uh oh!

Conversation

mockersf commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Mar 8, 2022

Uh oh!

mockersf commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mockersf Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heavyrain266 Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Mar 8, 2022

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mockersf commented Mar 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mockersf commented Mar 8, 2022 •

edited

Loading

DJMcNab Mar 8, 2022 •

edited

Loading

mockersf commented Mar 8, 2022 •

edited

Loading

mockersf Mar 8, 2022 •

edited

Loading

heavyrain266 Mar 8, 2022 •

edited

Loading

DJMcNab Mar 8, 2022 •

edited

Loading