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

Resize mode for sprite #430

Merged
merged 1 commit into from
Sep 8, 2020
Merged

Conversation

naithar
Copy link
Contributor

@naithar naithar commented Sep 3, 2020

Related issue and discussion: #411

Tested on sprite, breakout and texture_atlas examples on macOS.
Also tested in project from #411 on iOS and macOS with Automatic and Manual modes.
ResizeMode is exposed in case developer would need to change resize_mode value in systems.

Most (if not all) of the credit goes to @TheNeikos

Copy link
Contributor

@TheNeikos TheNeikos left a comment

Choose a reason for hiding this comment

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

I think a render compatible way (so as not to copy data to the GPU needlessly) would be to use SpriteResizeMode (or just ResizeMode, but one has to make sure it doesn't name-clash in the future) as a component and add it to the SpriteComponents bundle.

crates/bevy_sprite/src/sprite.rs Outdated Show resolved Hide resolved
@karroffel karroffel added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Sep 3, 2020
@naithar
Copy link
Contributor Author

naithar commented Sep 3, 2020

Remade resize_mode into Component. I guess this should be better with the way bevy works (ecs and stuff).
This would probably change default behavior in case Sprite is used separately from SpriteComponents bundle.

Edit:

CI failed, but I think it's not related to the PR.

error: failed to download from `https://crates.io/api/v1/crates/atom/0.3.5/download

Caused by:
  failed to get 200 response from `https://crates.io/api/v1/crates/atom/0.3.5/download`, got 503
##[error]Process completed with exit code 101.

@cart
Copy link
Member

cart commented Sep 4, 2020

This is definitely a useful feature! I actually would prefer it if this lived in the Sprite component as its logic thats central to how a sprite is rendered (and it pairs nicely with Sprite.size). But we also don't want to pass this in as data to the shader. We can resolve this with the following changes:

sprite.rs

#[derive(Default, RenderResources)]
pub struct Sprite {
    pub size: Vec2,
    #[render_resources(ignore)]
    pub resize_mode: SpriteResizeMode,
}

/// Determines how `Sprite` resize should be handled
#[derive(Debug)]
pub enum SpriteResizeMode {
    Manual,
    Automatic,
}

impl Default for SpriteResizeMode {
    fn default() -> Self {
        SpriteResizeMode::Automatic
    }
}

pub fn sprite_system(
    materials: Res<Assets<ColorMaterial>>,
    textures: Res<Assets<Texture>>,
    mut query: Query<(&mut Sprite, &Handle<ColorMaterial>)>,
) {
    for (mut sprite, handle) in &mut query.iter() {
        match sprite.resize_mode {
            SpriteResizeMode::Manual => continue,
            SpriteResizeMode::Automatic => {
                let material = materials.get(&handle).unwrap();
                if let Some(texture_handle) = material.texture {
                    if let Some(texture) = textures.get(&texture_handle) {
                        sprite.size = texture.size;
                    }
                }
            }
        }
    }
}

sprite.vert

layout(set = 2, binding = 1) uniform Sprite_size {
    vec2 size;
};

void main() {
    v_Uv = Vertex_Uv;
    vec3 position = Vertex_Position * vec3(size, 1.0);
    gl_Position = ViewProj * Model * vec4(position, 1.0);
}

@naithar
Copy link
Contributor Author

naithar commented Sep 5, 2020

@cart thanks for the input. It also looks almost like the first implementation. :)

Is there a reason to remove #[render_resources(from_self)] from Sprite component? Or #[render_resources(ignore)] wouldn't work otherwise?

It seems like #[render_resources(from_self)] just makes shader use exact same name as struct to bind values, while without it all values are binded like {struct_name}_{field_name} am I correct? So it's useful in case we have multiple fields, like TextureAtlas? But TextureAtlasSprite seems to use #[render_resources(from_self)] and works fine =/

@TheNeikos
Copy link
Contributor

TheNeikos commented Sep 5, 2020

I didn't know about #[render_resources(ignore)] ! That is useful. I am wondering if it would be better to use a Changed<Handle<ColorMaterial>> so that it doesn't get set every run?

@cart
Copy link
Member

cart commented Sep 6, 2020

@naithar Yeah we need to remove from_self because it assumes the whole struct is byte-convertable. But now that we're ignoring fields, it isn't directly byte convertible. We're currently discussing ways to improve the RenderResources derive to make it a bit more flexible, but for now I think the code above is our best bet.

TextureAtlasSprite doesn't ignore fields, so it is directly byte-convertible.

@TheNeikos We cant just use Change<Handle<ColorMaterial>> because that only runs the system when the handle itself changes (aka set to a new material). But we also want to run this system when the asset itself changes. We can track these changes by subscribing to AssetEvents, but it would complicate this code a lot. I think we should keep this pr scoped to adding resize mode.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Alrighty i think this is good to go after we remove repr(C)!

renderer::{RenderResource, RenderResources},
texture::Texture,
};
use bevy_render::{renderer::RenderResources, texture::Texture};

#[repr(C)]
Copy link
Member

Choose a reason for hiding this comment

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

lets remove repr(C) as its no longer required.

Adds a 'resize_mode' field for 'Sprite'.
This allows different resize handling based on 'SpriteResizeMode' enum value.

Co-authored-by: Marcel Müller <[email protected]>
@cart cart merged commit 52ae217 into bevyengine:master Sep 8, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
Adds a 'resize_mode' field for 'Sprite'.
This allows different resize handling based on 'SpriteResizeMode' enum value.
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 C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants