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.

Sign up for GitHub

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

Jump to bottom

Scale sprite z level by their scale #4156

Closed
wants to merge 7 commits into from
Closed

Scale sprite z level by their scale #4156

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5b86b2d
extracted sprites use a 2d transform separated from its z level
mockersf Mar 8, 2022
c182854
simplify mul operation
mockersf Mar 8, 2022
e153172
inline it
mockersf Mar 8, 2022
4492d3f
single line for `mul_vec3`
mockersf Mar 8, 2022
de07ac3
keep the order
mockersf Mar 8, 2022
060d644
doc strings
mockersf Mar 8, 2022
08034fe
fix doc links
mockersf Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 35 additions & 16 deletions crates/bevy_sprite/src/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use bevy_ecs::{
prelude::*,
system::{lifetimeless::*, SystemParamItem},
};
use bevy_math::{const_vec2, Vec2};
use bevy_math::{const_vec2, Quat, Vec2};
use bevy_reflect::Uuid;
use bevy_render::{
color::Color,
Expand Down Expand Up @@ -173,7 +173,8 @@ impl SpecializedPipeline for SpritePipeline {

#[derive(Component, Clone, Copy)]
pub struct ExtractedSprite {
mockersf marked this conversation as resolved.
Show resolved Hide resolved
pub transform: GlobalTransform,
pub transform: Transform2d,
pub z_layer: f32,
pub color: Color,
/// Select an area of the texture
pub rect: Option<Rect>,
Expand All @@ -186,6 +187,20 @@ pub struct ExtractedSprite {
pub flip_y: bool,
}

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

Choose a reason for hiding this comment

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

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.

Copy link

@heavyrain266 heavyrain266 Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

mockersf marked this conversation as resolved.
Show resolved Hide resolved
pub translation: Vec2,
pub scale: Vec2,
pub rotation: Quat,
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@DJMcNab DJMcNab Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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"

}

impl Transform2d {
Copy link
Member

Choose a reason for hiding this comment

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

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.

#[inline]
fn mul_vec2(&self, value: Vec2) -> Vec2 {
(self.rotation * (self.scale * value).extend(0.0)).truncate() + self.translation
}
}

#[derive(Default)]
pub struct ExtractedSprites {
pub sprites: Vec<ExtractedSprite>,
Expand Down Expand Up @@ -240,7 +255,12 @@ pub fn extract_sprites(
// PERF: we don't check in this function that the `Image` asset is ready, since it should be in most cases and hashing the handle is expensive
extracted_sprites.sprites.alloc().init(ExtractedSprite {
color: sprite.color,
transform: *transform,
transform: Transform2d {
translation: transform.translation.truncate(),
scale: transform.scale.truncate(),
rotation: transform.rotation,
},
z_layer: transform.translation.z * transform.scale.z,
// Use the full texture
rect: None,
// Pass the custom size
Expand All @@ -258,7 +278,12 @@ pub fn extract_sprites(
let rect = Some(texture_atlas.textures[atlas_sprite.index as usize]);
extracted_sprites.sprites.alloc().init(ExtractedSprite {
color: atlas_sprite.color,
transform: *transform,
transform: Transform2d {
translation: transform.translation.truncate(),
scale: transform.scale.truncate(),
rotation: transform.rotation,
},
z_layer: transform.translation.z * transform.scale.z,
// Select the area in the texture atlas
rect,
// Pass the custom size
Expand Down Expand Up @@ -393,16 +418,9 @@ pub fn queue_sprites(
transparent_phase.items.reserve(extracted_sprites.len());

// Sort sprites by z for correct transparency and then by handle to improve batching
extracted_sprites.sort_unstable_by(|a, b| {
match a
.transform
.translation
.z
.partial_cmp(&b.transform.translation.z)
{
Some(Ordering::Equal) | None => a.image_handle_id.cmp(&b.image_handle_id),
Some(other) => other,
}
extracted_sprites.sort_unstable_by(|a, b| match a.z_layer.partial_cmp(&b.z_layer) {
Some(Ordering::Equal) | None => a.image_handle_id.cmp(&b.image_handle_id),
Some(other) => other,
});

// Impossible starting values that will be replaced on the first iteration
Expand Down Expand Up @@ -489,12 +507,13 @@ pub fn queue_sprites(
let positions = QUAD_VERTEX_POSITIONS.map(|quad_pos| {
extracted_sprite
.transform
.mul_vec3((quad_pos * quad_size).extend(0.))
.mul_vec2(quad_pos * quad_size)
.extend(extracted_sprite.z_layer)
.into()
});

// These items will be sorted by depth with other phase items
let sort_key = FloatOrd(extracted_sprite.transform.translation.z);
let sort_key = FloatOrd(extracted_sprite.z_layer);

// Store the vertex data and add the item to the render phase
if current_batch.colored {
Expand Down
9 changes: 7 additions & 2 deletions crates/bevy_text/src/text2d.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use bevy_ecs::{
};
use bevy_math::{Size, Vec3};
use bevy_render::{texture::Image, view::Visibility, RenderWorld};
use bevy_sprite::{ExtractedSprite, ExtractedSprites, TextureAtlas};
use bevy_sprite::{ExtractedSprite, ExtractedSprites, TextureAtlas, Transform2d};
use bevy_transform::prelude::{GlobalTransform, Transform};
use bevy_window::{WindowId, Windows};

Expand Down Expand Up @@ -92,7 +92,12 @@ pub fn extract_text2d_sprite(
let transform = text_transform.mul_transform(glyph_transform);

extracted_sprites.sprites.push(ExtractedSprite {
transform,
transform: Transform2d {
translation: transform.translation.truncate(),
scale: transform.scale.truncate(),
rotation: transform.rotation,
},
z_layer: transform.translation.z * transform.scale.z,
Copy link
Member

Choose a reason for hiding this comment

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

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.

color,
rect,
custom_size: None,
Expand Down
7 changes: 2 additions & 5 deletions crates/bevy_transform/src/components/global_transform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,11 +212,8 @@ impl GlobalTransform {

/// Returns a [`Vec3`] of this [`Transform`] applied to `value`.
#[inline]
pub fn mul_vec3(&self, mut value: Vec3) -> Vec3 {
value = self.scale * value;
value = self.rotation * value;
value += self.translation;
value
pub fn mul_vec3(&self, value: Vec3) -> Vec3 {
self.rotation * self.scale * value + self.translation
}

#[doc(hidden)]
Expand Down
7 changes: 2 additions & 5 deletions crates/bevy_transform/src/components/transform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,8 @@ impl Transform {

/// Returns a [`Vec3`] of this [`Transform`] applied to `value`.
#[inline]
pub fn mul_vec3(&self, mut value: Vec3) -> Vec3 {
value = self.scale * value;
value = self.rotation * value;
value += self.translation;
value
pub fn mul_vec3(&self, value: Vec3) -> Vec3 {
self.rotation * self.scale * value + self.translation
Copy link
Member

Choose a reason for hiding this comment

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

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

Let's keep the ordering consistent here.

Copy link
Member Author

@mockersf mockersf Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}

/// Changes the `scale` of this [`Transform`], multiplying the current `scale` by
Expand Down