fix spacing being added to atlas size #301
Conversation
|
@StarArawn I noticed that you mentioned this bit of code is contentious, so I thought what better to do than jump into as well. |
|
@antoinePaulinB7 Thanks for this PR! We just merged in some changes that are causing a conflict with your branch. Would you mind merging |
|
@bzm3r done :) |
There was a problem hiding this comment.
This makes sense to me at first glance, but I really think we need to put together a proper test for it. @antoinePaulinB7 Can you put in an asset and an example that clearly shows why this works, and the old method would not?
| (texture_size.x + tile_spacing.x) / (tile_size.x + tile_spacing.x); | ||
| let tile_count_y = | ||
| (texture_size.y + tile_spacing.y) / (tile_size.y + tile_spacing.y); | ||
| let tile_count_x = (texture_size.x) / (tile_size.x + tile_spacing.x); |
There was a problem hiding this comment.
Is this making the assumption that every tile, including the last one in the atlas will have padding? Is that the right assumption here?
There was a problem hiding this comment.
That is indeed the assumption.
If we wanted to get rid of this assumption, I propose that we take inspiration from bevy's own bevy_sprite from_grid_with_padding.
There was a problem hiding this comment.
@antoinePaulinB7 I have been thinking about it, and I think it's a fine assumption, because it provides a lot of regularity and eliminates special cases. Perhaps it is something we can document, so users know that this is what we expect. I will create a separate issue for it if this PR gets merged, to work on documenting that.
|
@bzm3r Hopefully, I'll have time to put together a test before the end of the week. |
|
@StarArawn Can you also review this PR, and see if it makes sense to you? It looks good to me! |
If you want the proper count of the number of tiles / columns, the atlas size should be taken as is.