In v5, forcing image, rows and columns in `Spritesheet::new` is an ergonomics regression

[mgi388]

I'm in the middle of upgrading to v5 and Bevy 0.17.

One issue is that previously I would construct animations and those animations could be used by many different units, each unit with a different image. Think of an RTS: There's an "Infantry" animation and it has the exact same sequence, indices, etc., and I'd create that animation once and use it for all my infantry units/spritesheets. When spawning different units I'd use the same animation ID (or handle now) but a different image handle.

This means that this v5 code is problematic:

let spritesheet = Spritesheet::new(&image, 8, 8);

Because at the time I construct the animation handle, I don't actually have access to the image.

If we look at where image and image() are actually used, it's just for the convenience/helper methods:

Example:

bevy_spritesheet_animation/src/components/generator.rs

Lines 66 to 68 in 65f0376

	pub fn sprite(&self, atlas_layouts: &mut Assets<TextureAtlasLayout>) -> Sprite {
	Sprite::from_atlas_image(self.spritesheet.image().clone(), self.atlas(atlas_layouts))
	}

If we go deeper, you might be tempted to suggest using, say, the sprite3d() helper:

bevy_spritesheet_animation/src/components/generator.rs

Lines 105 to 107 in 65f0376

	pub fn sprite3d(&self, atlas_layouts: &mut Assets<TextureAtlasLayout>) -> Sprite3d {
	Sprite3d::from_atlas_image(self.spritesheet.image().clone(), self.atlas(atlas_layouts))
	}

But that method isn't side effect free. It actually creates a TextureAtlasLayout for you:

bevy_spritesheet_animation/src/components/generator.rs

Lines 240 to 248 in 65f0376

	let layout = TextureAtlasLayout::from_grid(
	UVec2::new(cell_width, cell_height),
	self.spritesheet.columns() as u32,
	self.spritesheet.rows() as u32,
	None,
	None,
	);
	let layout_handle = atlas_layouts.add(layout);

But that's arguably wrong because it doesn't actually allow passing in the index to pass to TextureAtlas (it uses default() so defaults to 0). However if we look at some code I've got in my game, the default being 0 is usually never right.

ImageNode {
    image: s.texture.clone(),
    texture_atlas: Some(TextureAtlas {
        layout: s.texture_atlas_layout.clone(),
        index: disabled_texture_atlas_index,
    }),
    ..Default::default()
},

This one spawns disabled, so the index is the index of that frame, not 0.

This is an example for spawning my unit which uses a Sprite3d:

let direction = calculate_sprite_direction(...);
let (animation_handle, maybe_texture_atlas_index) = &animation_handles[direction];

// ...

Sprite3d {
    image: sprite_sheet.texture.clone(),
    texture_atlas: Some(TextureAtlas {
        layout: sprite_sheet.texture_atlas_layout.clone(),
        index: maybe_texture_atlas_index.unwrap_or(0),
    }),
    unlit: true,
    ..Default::default()
},

This one spawns using the texture atlas index for the unit's facing direction, and so defaulting to 0 would be wrong.

So that is all to say that those helper methods that helpfully try to components for me and create a TextureAtlasLayout won't ever be used by me because they are arguably "wrong" for my game (and possibly most games unless by luck every texture atlas index should start at 0).

And so forcing image to be passed during the constructor is problematic for me. I have to create a dummy image and pass it just to get the code to compile.

If we go deeper, we actually see that the rows and columns that Spritesheet::new are only used for constructing the TextureAtlasLayout as well (the one I've just described as being "wrong" for my game). So, I'm also forced to pass something to that without actually needing it as well.

The ultimate problem with this is that I'm forced to use Spritesheet::new to actually get an Animation. AnimationBuilder requires one, and Animation can only be constructed with a builder.

Any ideas?

In case this issue comes across as ungrateful, it's not :) The move to use assets to manage animations is a huge improvement from earlier versions, so thanks again for the work on this crate.

[merwaaan]

Hi @mgi388

As much as I would liek to work a bit on the crate right now, I'm severely out of time :)

If you have some ideas for making things more ergonomic, PR are always welcome.

[mgi388]

@merwaaan thanks. I'll look when I get a chance as well. While I will check, my gut feel is that a PR that I'd make would likely revert the things that v5 introduced related to my description above.