bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.24k stars 3.57k forks source link

Change from_grid_with_padding API #1086

Closed ryanleecode closed 1 year ago

ryanleecode commented 3 years ago

The current API of from_grid_with_padding seems fairly strange because it forces you to specify the number of rows & columns when that should be computed instead. I propose changing it to take the texture dimensions instead.

I also think the API should add padding to 0th column because texture clamping can cause it to bleed without.

New Proposed API

pub fn from_grid_with_padding(
    texture: Handle<Texture>,
    tile_size: Vec2,
    texture_dimensions: Vec2,
    padding: Vec2,
) -> TextureAtlas {
    let mut sprites = Vec::new();
    let rows = (texture_dimensions.x() / (tile_size.x() + (padding.x() * 2.0))) as i32;
    let columns = (texture_dimensions.y() / (tile_size.y() + (padding.y() * 2.0))) as i32;

    for y in 0..rows {
        for x in 0..columns {
            let rect_min = Vec2::new(
                (tile_size.x() + (padding.x() * 2.0)) * x as f32 + padding.x(),
                (tile_size.y() + (padding.y() * 2.0)) * y as f32 + padding.y(),
            );

            sprites.push(Rect {
                min: rect_min,
                max: Vec2::new(rect_min.x() + tile_size.x(), rect_min.y() + tile_size.y()),
            })
        }
    }

    TextureAtlas {
        size: texture_dimensions,
        textures: sprites,
        texture,
        texture_handles: None,
    }
}

Old API:

pub fn from_grid_with_padding(
    texture: Handle<Texture>,
    tile_size: Vec2,
    columns: usize,
    rows: usize,
    padding: Vec2,
) -> TextureAtlas {
    let mut sprites = Vec::new();
    let mut x_padding = 0.0;
    let mut y_padding = 0.0;

    for y in 0..rows {
        if y > 0 {
            y_padding = padding.y();
        }
        for x in 0..columns {
            if x > 0 {
                x_padding = padding.x();
            }

            let rect_min = Vec2::new(
                (tile_size.x() + x_padding) * x as f32,
                (tile_size.y() + y_padding) * y as f32,
            );

            sprites.push(Rect {
                min: rect_min,
                max: Vec2::new(rect_min.x() + tile_size.x(), rect_min.y() + tile_size.y()),
            })
        }
    }

    TextureAtlas {
        size: Vec2::new(
            ((tile_size.x() + x_padding) * columns as f32) - x_padding,
            ((tile_size.y() + y_padding) * rows as f32) - y_padding,
        ),
        textures: sprites,
        texture,
        texture_handles: None,
    }
}
mockersf commented 3 years ago

This API was done in #460, adding padding to the 0th column would make it fails for the SpriteSheet mentioned there...

From looking a little at how spritesheets are done in general, there seems to be no standard, but something often done is having both spacing (between sprites) and margin (between all sprites and image border)

ryanleecode commented 3 years ago

Maybe creating a padding specification struct would solve this.

#[derive(Debug, Default)]
pub struct PaddingSpecification {
    pub left: f32,
    pub top: f32,
    pub right: f32,
    pub bottom: f32,
}

impl PaddingSpecification {
    pub fn new(left: f32, top: f32, right: f32, bottom: f32) -> Self {
        Self {
            left,
            top,
            right,
            bottom,
        }
    }

    pub fn uniform<T>(value: T) -> Self
    where
        T: Into<Vec2>,
    {
        let vec2: Vec2 = value.into();

        Self::new(vec2.x(), vec2.y(), vec2.x(), vec2.y())
    }
}

impl From<Vec2> for PaddingSpecification {
    fn from(vec2: Vec2) -> Self {
        PaddingSpecification::uniform(vec2)
    }
}
pub fn from_grid_with_padding<PS>(
    texture: Handle<Texture>,
    tile_size: Vec2,
    texture_dimensions: Vec2,
    padding: PS,
) -> TextureAtlas
where
    PS: Into<PaddingSpecification>,
{
    let padding: PaddingSpecification = padding.into();
    let mut sprites = Vec::new();
    let padding_x = padding.left + padding.right;
    let padding_y = padding.top + padding.bottom;
    let rows = (texture_dimensions.x() / (tile_size.x() + padding_x)) as i32;
    let columns = (texture_dimensions.y() / (tile_size.y() + padding_y)) as i32;

    for y in 0..rows {
        for x in 0..columns {
            let rect_min = Vec2::new(
                (tile_size.x() + padding_x) * x as f32 + padding.left,
                (tile_size.y() + padding_y) * y as f32 + padding.top,
            );

            sprites.push(Rect {
                min: rect_min,
                max: Vec2::new(rect_min.x() + tile_size.x(), rect_min.y() + tile_size.y()),
            })
        }
    }

    TextureAtlas {
        size: texture_dimensions,
        textures: sprites,
        texture,
        texture_handles: None,
    }
}
ryanleecode commented 3 years ago

cc: @sY9sE33

photex commented 3 years ago

Chiming in as someone who got bit by a late night miscount of the cols and row count; I totally like this proposal. In particular, when I was reading the interface to the function it wasn't clear how padding was calculated. It worked with the kenney tilesheet, but I had no idea really if it would. Having a more directly configured padding makes a lot of sense to me.

photex commented 3 years ago

@ryanleecode why not just submit a PR of this?

james7132 commented 1 year ago

TextureAtlas::from_grid_with_padding doesn't exist anymore. Is this still relevant?

mockersf commented 1 year ago

everything is now available in TextureAtlas::from_grid