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.

Sign up for GitHub

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

Jump to bottom

Add a way to specify grid starting point in TextureAtlas #3775

Closed
wants to merge 5 commits into from
Closed

Add a way to specify grid starting point in TextureAtlas #3775

wants to merge 5 commits into from

Conversation

oledfish
Copy link
Contributor

Currently, if one were to use the from_grid and from_grid_with_padding methods to create a TextureAtlas from a texture, the grid will always have the image origin (top left, 0,0) as its origin point. This change allows specifying a different offset, to be used with spritesheets with a margin, or images packing several spritesheets with different tile sizes.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 27, 2022
@alice-i-cecile alice-i-cecile added 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 and removed S-Needs-Triage This issue needs to be labelled labels Jan 27, 2022
alice-i-cecile
alice-i-cecile requested changes Jan 27, 2022
View reviewed changes
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.

These are good docs, and this is a sensible change! However, I think we should condense the API, by merging from_grid, from_grid_with_padding and from_grid_with_padding_and_origin.

The resulting function signature would be

fn from_grid(
        texture: Handle<Image>,
        tile_size: Vec2,
        columns: usize,
        rows: usize,
        padding: Option<Vec2>,
        origin: Option<Vec2>,
)

@oledfish
Copy link
Contributor Author

That would be cleaner, I just didn't want to risk making a breaking change for a first PR but there it is

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 27, 2022
alice-i-cecile
alice-i-cecile reviewed Jan 27, 2022
View reviewed changes
crates/bevy_sprite/src/texture_atlas.rs Outdated
@@ -67,26 +67,19 @@ impl TextureAtlas {
}

/// Generate a `TextureAtlas` by splitting a texture into a grid where each
Copy link
Member

Choose a reason for hiding this comment

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

This doc comment has definitely turned into a proper run in sentence 😄

We should split the doc comment into the first line summary, then a new line, then a list describing each the non-obvious parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would this look?

Generate a TextureAtlas by splitting a texture into a grid.
Each cell in the grid of tile_size corresponds to one texture in the atlas.

  • Cells can have horizontal and/or vertical separation specified by padding
  • The grid can start at a point offset relative to the top-left corner of the texture

Copy link
Member

Choose a reason for hiding this comment

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

This is almost perfect. Just needs an extra new line after the first sentence so the IDEs handle it properly.

alice-i-cecile
alice-i-cecile requested changes Jan 27, 2022
View reviewed changes
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.

Looking good :) This is a very nice little first PR. The doc string needs some cleanup now, but otherwise I really like the look of this change.

rparrett
rparrett approved these changes Jan 28, 2022
View reviewed changes
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.

Tested with
gabe-idle-run-8px-offset-2px-padding

    let texture_atlas = TextureAtlas::from_grid(
        texture_handle,
        Vec2::new(24.0, 24.0),
        7,
        1,
        Some(Vec2::splat(2.)),
        Some(Vec2::splat(8.)),
    );

@rparrett rparrett 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 1, 2022
@rparrett
Copy link
Contributor

rparrett commented Sep 1, 2022

@oledfish could you add this or something similar to the PR description?

## 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_with_padding(texture_handle, Vec2::new(24.0, 24.0), 7, 1, Some(Vec2::new(4.0, 4.0)), None)

@alice-i-cecile
Copy link
Member

@oledfish once the Migration Guide is in and merge conflicts are resolved I'll merge this in for you.

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Sep 19, 2022
@BorisBoutillier
Copy link
Contributor

I am planning to adopt this PR to merge conflicts and include migration information.

@BorisBoutillier BorisBoutillier mentioned this pull request Sep 22, 2022
Closed
@alice-i-cecile
Copy link
Member

Adopted in #6057 :)

bors bot pushed a commit that referenced this pull request Sep 24, 2022
@BorisBoutillier @oledfish
Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_gr…
2b736e8 
…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 <[email protected]>
bors bot pushed a commit that referenced this pull request Sep 24, 2022
@BorisBoutillier @oledfish
Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_gr…
b91945b 
…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 <[email protected]>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
@BorisBoutillier @oledfish @james7132
Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_gr…
634bc50 
…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 <[email protected]>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@BorisBoutillier @oledfish @james7132
Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_gr…
a97d307 
…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 <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@BorisBoutillier @oledfish @ItsDoot
Merge TextureAtlas::from_grid_with_padding into TextureAtlas::from_gr…
c74afab 
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rparrett rparrett rparrett approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@IceSentry IceSentry IceSentry approved these changes

Assignees
No one assigned
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-Adopt-Me The original PR author has no intent to complete this work. Pick me up! 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oledfish @rparrett @alice-i-cecile @BorisBoutillier @IceSentry