[Merged by Bors] - Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_grid through option arguments #6057
Conversation
Nilirad
added
A-Rendering
There was a problem hiding this comment.
LGTM on the technical side. I think I prefer this single-method approach to reduce logic duplication, but I'd like to hear other opinions. If this method was called more often I'd be against it on the grounds of boilerplate.
@mockersf @rparrett @IceSentry, what's your takes?
|
There's an error in the migration guide section I suggested. The last line should use I completely missed #4836 being merged and I personally prefer the way the functions are laid out prior to this PR. I'd rather just see a doc link added to |
Thanks for the catch, I copied it a bit too blindindly , my bad. I will rebase to fix the merge conflicts with doc updates. Regarding two functions or one, I think if we keep two functions than the second one has a strange incomplete names, as it is |
BorisBoutillier
force-pushed
the
textureatlas-origin
branch
from
34d4f27
to
043f6ef
Compare
I don't like the new API, but it's less bad than the one before I guess 😄 |
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
…id through option arguments (#6057) This is an adoption of #3775 This merges `TextureAtlas` `from_grid_with_padding` into `from_grid` , adding optional padding and optional offset. Since the orignal PR, the offset had already been added to from_grid_with_padding through #4836 ## Changelog - Added `padding` and `offset` arguments to `TextureAtlas::from_grid` - Removed `TextureAtlas::from_grid_with_padding` ## Migration Guide `TextureAtlas::from_grid_with_padding` was merged into `from_grid` which takes two additional parameters for padding and an offset. ``` // 0.8 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, None, None) // 0.8 TextureAtlas::from_grid_with_padding(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Vec2::new(4.0, 4.0)); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Some(Vec2::new(4.0, 4.0)), None) ``` Co-authored-by: olefish <88390729+oledfish@users.noreply.github.com>
|
Build failed (retrying...): |
…id through option arguments (#6057) This is an adoption of #3775 This merges `TextureAtlas` `from_grid_with_padding` into `from_grid` , adding optional padding and optional offset. Since the orignal PR, the offset had already been added to from_grid_with_padding through #4836 ## Changelog - Added `padding` and `offset` arguments to `TextureAtlas::from_grid` - Removed `TextureAtlas::from_grid_with_padding` ## Migration Guide `TextureAtlas::from_grid_with_padding` was merged into `from_grid` which takes two additional parameters for padding and an offset. ``` // 0.8 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, None, None) // 0.8 TextureAtlas::from_grid_with_padding(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Vec2::new(4.0, 4.0)); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Some(Vec2::new(4.0, 4.0)), None) ``` Co-authored-by: olefish <88390729+oledfish@users.noreply.github.com>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
…id through option arguments (bevyengine#6057) This is an adoption of bevyengine#3775 This merges `TextureAtlas` `from_grid_with_padding` into `from_grid` , adding optional padding and optional offset. Since the orignal PR, the offset had already been added to from_grid_with_padding through bevyengine#4836 ## Changelog - Added `padding` and `offset` arguments to `TextureAtlas::from_grid` - Removed `TextureAtlas::from_grid_with_padding` ## Migration Guide `TextureAtlas::from_grid_with_padding` was merged into `from_grid` which takes two additional parameters for padding and an offset. ``` // 0.8 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, None, None) // 0.8 TextureAtlas::from_grid_with_padding(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Vec2::new(4.0, 4.0)); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Some(Vec2::new(4.0, 4.0)), None) ``` Co-authored-by: olefish <88390729+oledfish@users.noreply.github.com>
…id through option arguments (bevyengine#6057) This is an adoption of bevyengine#3775 This merges `TextureAtlas` `from_grid_with_padding` into `from_grid` , adding optional padding and optional offset. Since the orignal PR, the offset had already been added to from_grid_with_padding through bevyengine#4836 ## Changelog - Added `padding` and `offset` arguments to `TextureAtlas::from_grid` - Removed `TextureAtlas::from_grid_with_padding` ## Migration Guide `TextureAtlas::from_grid_with_padding` was merged into `from_grid` which takes two additional parameters for padding and an offset. ``` // 0.8 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, None, None) // 0.8 TextureAtlas::from_grid_with_padding(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Vec2::new(4.0, 4.0)); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Some(Vec2::new(4.0, 4.0)), None) ``` Co-authored-by: olefish <88390729+oledfish@users.noreply.github.com>
|
I'm obviously a little late to the party, but I'm curious what makes this better than the previous approach? Before, you got two API's that did what you expected, one providing sane default ( Why is one API better than two? And, if we really want one API, why do we need // TextureAtlas::from_grid(texture_handle, Vec2::splat(24.0), 7, 1, None, None);
TextureAtlas::from_grid(texture_handle, Vec2::splat(24.0), 7, 1, Vec2::ZERO, Vec2::ZERO);
// TextureAtlas::from_grid(texture_handle, Vec2::splat(24.0), 7, 1, Some(Vec2::splat(1.0)), None);
TextureAtlas::from_grid(texture_handle, Vec2::splat(24.0), 7, 1, Vec2::splat(1.0), Vec2::ZERO);
// TextureAtlas::from_grid(texture_handle, Vec2::splat(24.0), 7, 1, Some(Vec2::splat(1.0)), Some(Vec2::new(24.0, 0.0)));
TextureAtlas::from_grid(texture_handle, Vec2::splat(24.0), 7, 1, Vec2::splat(1.0), Vec2::new(24.0, 0.0)); |
|
Definitely a bit late, but that sort of feedback would be valuable here now: #5103 |
…id through option arguments (bevyengine#6057) This is an adoption of bevyengine#3775 This merges `TextureAtlas` `from_grid_with_padding` into `from_grid` , adding optional padding and optional offset. Since the orignal PR, the offset had already been added to from_grid_with_padding through bevyengine#4836 ## Changelog - Added `padding` and `offset` arguments to `TextureAtlas::from_grid` - Removed `TextureAtlas::from_grid_with_padding` ## Migration Guide `TextureAtlas::from_grid_with_padding` was merged into `from_grid` which takes two additional parameters for padding and an offset. ``` // 0.8 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, None, None) // 0.8 TextureAtlas::from_grid_with_padding(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Vec2::new(4.0, 4.0)); // 0.9 TextureAtlas::from_grid(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Some(Vec2::new(4.0, 4.0)), None) ``` Co-authored-by: olefish <88390729+oledfish@users.noreply.github.com>
This is an adoption of #3775
This merges
TextureAtlasfrom_grid_with_paddingintofrom_grid, adding optional padding and optional offset.Since the orignal PR, the offset had already been added to from_grid_with_padding through #4836
Changelog
paddingandoffsetarguments toTextureAtlas::from_gridTextureAtlas::from_grid_with_paddingMigration Guide
TextureAtlas::from_grid_with_paddingwas merged intofrom_gridwhich takes two additional parameters for padding and an offset.