[Merged by Bors] - add_texture returns index to texture #2864
Conversation
inodentry
added
A-Assets
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `rect` - The section of the atlas that contains the texture to be added, | ||
| /// from the top-left corner of the texture to the bottom-right corner | ||
| pub fn add_texture(&mut self, rect: Rect) { | ||
| pub fn add_texture(&mut self, rect: Rect) -> u32 { |
There was a problem hiding this comment.
Taking a look at get_texture_index we are returning the index as usize. I think we should do the same here.
There was a problem hiding this comment.
My preference would actually be to change get_texture_index to return a u32 too. TextureAtlasSprite and the shader for it takes a u32.
There was a problem hiding this comment.
I think we should come up with a guideline to keep Bevy consistent. My opinion would be to use usize for indices and cast them to u32 when passing them to shaders. Initial responses on Discord suggest the same. Making all this consistent should be another PR though, so I wouldn't block this one based on the inconsistency.
There was a problem hiding this comment.
For now, this needs to be u32 because we directly use the TextureAtlasSprite type when generating shader uniform buffers. In the renderer rework, this is no longer the case. And I agree that we should favor usize in user facing types for more natural indexing.
I think we should merge this as-is. Can you create another PR for the pipelined-rendering branch and make the relevant types in bevy_sprites2 use usize?
NiklasEi
added
S-Ready-For-Final-Review
|
bors r+ |
If you need to build a texture atlas from an already created texture that is not match a grid, you need to use new_empty and add_texture to create it. However it is not straight forward to get the index to be used with TextureAtlasSprite. add_texture should be changed to return the index to the texture.
Currently you can do something like this:
```rs
let texture = asset_server.load::<Texture>::("texture.png");
let texture_atlas = TextureAtlas::new_empty(texture, Vec2::new(40.0, 40.0));
texture_atlas.add_texture(Rect {
min: Vec2::new(20.0, 20.0),
max: Vec2::new(40.0, 40.0),
});
let index = (texture_atlas.len() - 1) as u32;
let texture_atlas_sprite = TextureAtlasSprite {
index,
Default::default()
};
```
But this is more clear
```rs
let index = texture_atlas.add_texture(Rect {
min: Vec2::new(20.0, 20.0),
max: Vec2::new(40.0, 40.0),
});
```
bors
bot
changed the title
Per this comment #2864 (comment), I have done a pass at changing all the public facing indexes for `TextureAtlas` to usize.
If you need to build a texture atlas from an already created texture that is not match a grid, you need to use new_empty and add_texture to create it. However it is not straight forward to get the index to be used with TextureAtlasSprite. add_texture should be changed to return the index to the texture.
Currently you can do something like this:
But this is more clear