add index_range for TextureAtlas

[useyourfeelings]

Objective

To simplify index updating for TextureAtlas when playing animation.

Solution

Add index_range and advance_index() for TextureAtlas.

So

// init
let animation_indices = AnimationIndices { first: 1, last: 6 };

TextureAtlas {
    layout: texture_atlas_layout,
    index: animation_indices.first,
},

// update
atlas.index = if atlas.index == indices.last {
    indices.first
} else {
    atlas.index + 1
};

will become

// init
TextureAtlas {
    layout: texture_atlas_layout,
    index: 1,
    index_range: (1, 6)
},

// update
atlas.advance_index();

Migration Guide

1. Provide index_range: (my_first_index, my_last_index) for all your TextureAtlas initializations.

TextureAtlas {
    layout: texture_atlas_layout,
    index: 1,
    index_range: (1, 6), // add this as defualt index range
},

2. Instead of manually updating index like

atlas.index = if atlas.index == indices.last {
    indices.first
} else {
    atlas.index + 1
};

just call

atlas.advance_index(); // index += 1

3. Remove unnecessary code like

let animation_indices = AnimationIndices { first: 1, last: 6 }; // delete this

Testing

Tested all examples related to TextureAtlas.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

I really like these changes, but I had a hard time understanding the docs at first. I've left a simple suggestion to improve that.

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[BenjaminBrienen]

This looks like it should just use a saturating add.

[BenjaminBrienen]

This is supposed to cycle

[rparrett]

Sort of opposed to this change. I feel like it's of way too limited utility to be given prime real estate in the TextureAtlas struct, and I'm not sure that animation metadata belongs in there anyway.

Definitely want to add stuff to help with 2d animations... I just think that whatever we end up with will look nothing like what is being done in that PR.

I'd maybe implement it as an optional AtlasAnimation { frames: Vec<usize>, index: usize } or AtlasAnimation { start: usize, end: usize, index: usize} component with helper methods or something. But that last one feels pretty restrictive, and they're both missing timing information. But it might be a better starting point.

[mgi388]

For me this change doesn't stand out as a net improvement.

The name advance_index implies to me that it advances the index, not that it advances and possibly wraps back around.

It feels weird to me that you'd now have to pass index_range to your TextureAtlas (even if using Default to have it receive a default value).

Some problems I have with index_range:

1. index_range does not imply that it's a range for the first and last index. Some valid ranges might just be a section of the atlas (e.g. index 0 to 7 out of an atlas with 256 frames), so index_range's name and functionality seems limited. Further, then advance_index only works on the range 0 to 7.
2. An entity with a TextureAtlas probably has multiple valid ranges, not just one. A texture atlas can have animations in it for walking north, south, etc., running north, running south, etc. See https://docs.rs/bevy_spritesheet_animation/1.0.0/bevy_spritesheet_animation/spritesheet/struct.Spritesheet.html for an example crate which can create different "ranges" and how there is a wide variety, certainly more than just "start to end incrementing by 1".
3. All my existing code that constructs TextureAtlas would now have to pass index_range even though it may not even be relevant for my entity. A concrete example follows. I have a UI image with a TextureAtlas. I have an atlas of textures and my UI image comes from index 48.

commands
    .spawn((
        ImageBundle {
            style: Style {
                // ...
                ..Default::default()
            },
            image: UiImage::new(hud_sprite_sheet.texture.clone()),
            ..Default::default()
        },
        TextureAtlas {
            layout: hud_sprite_sheet.texture_atlas_layout.clone(),
            index: 48,
        },
    ))
    .id();

I never need to animate this UI image. So if I was forced to pass index_range here, even if indirectly through Default, it doesn't really make sense to me.

[mgi388]

FWIW, this change looks subtly wrong to me. Previously the logic was:

• If the index is not the last, then increment it (inline).

Now the logic is (due to advance_index):

• If the index is not the last, then increment it (inside advance_index).
• (Inside advance_index) If the index is more than the last set it to the index of the first.

Even if this particular change is the same as before (I didn't completely check because it looks subtly wrong and it's no longer intuitive, at least to me), my main concerns are left in a comment at the end of the review.