Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_grid through option arguments #6057

Closed

Conversation

BorisBoutillier
Copy link
Contributor

@BorisBoutillier BorisBoutillier commented Sep 22, 2022

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)

@Nilirad Nilirad added A-Rendering Drawing game state to the screen A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 22, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@rparrett
Copy link
Contributor

rparrett commented Sep 22, 2022

There's an error in the migration guide section I suggested. The last line should use from_grid, and the code section should be in a code block.

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 from_grid pointing at from_grid_with_padding. I'm not particularly bothered either way though.

@BorisBoutillier
Copy link
Contributor Author

There's an error in the migration guide section I suggested. The last line should use from_grid, and the code section should be in a code block.

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 from_grid pointing at from_grid_with_padding. I'm not particularly bothered either way though.

Thanks for the catch, I copied it a bit too blindindly , my bad.
Update done.

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 with_padding but also support offset, so would deserve an updated name like from_grid_with_padding_and_offset
And as far as I've seen in #help, external texture atlas often have an offset or some padding (or both), having the single method requiring both additional arguments incite the user to think about the whole organization of the texture atlas he is using.

@mockersf
Copy link
Member

@mockersf @rparrett @IceSentry, what's your takes?

I don't like the new API, but it's less bad than the one before I guess 😄

Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also convinced that this is less bad.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 23, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 24, 2022
…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>
@bors
Copy link
Contributor

bors bot commented Sep 24, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Sep 24, 2022
…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>
@bors bors bot changed the title Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_grid through option arguments [Merged by Bors] - Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_grid through option arguments Sep 24, 2022
@bors bors bot closed this Sep 24, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
…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>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…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>
@dubrowgn
Copy link
Contributor

dubrowgn commented Dec 7, 2022

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 (Vec2::ZERO) values to the other. Now, we have to wrap all padding and offset values in Some(), and we get a new sentinel value for zero (e.g. now Some(Vec2::ZERO) == None).

Why is one API better than two? And, if we really want one API, why do we need Options instead of using Vec2 directly?

// 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));

@rparrett
Copy link
Contributor

rparrett commented Dec 7, 2022

Definitely a bit late, but that sort of feedback would be valuable here now: #5103

@dubrowgn dubrowgn mentioned this pull request Dec 7, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants