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] - add bevy_mod_ui_texture_atlas_image #253

Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - add bevy_mod_ui_texture_atlas_image #253

wants to merge 8 commits into from

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Sep 30, 2022

No description provided.

@alice-i-cecile
Copy link
Member

bors r+

@mockersf
Copy link
Member

hello! you should know that your image will be displayed at https://bevyengine.org/assets/, and it currently doesn't have the best ratio

bors bot pushed a commit that referenced this pull request Sep 30, 2022
@alice-i-cecile
Copy link
Member

bors r-

@bors
Copy link

bors bot commented Sep 30, 2022

Canceled.

@ickshonpe
Copy link
Contributor Author

hello! you should know that your image will be displayed at https://bevyengine.org/assets/, and it currently doesn't have the best ratio

Yes, you are right , I changed it.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 30, 2022
@bors
Copy link

bors bot commented Sep 30, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title add bevy_mod_ui_texture_atlas_image [Merged by Bors] - add bevy_mod_ui_texture_atlas_image Sep 30, 2022
@bors bors bot closed this Sep 30, 2022
yopox pushed a commit to yopox/bevy-assets that referenced this pull request Oct 5, 2022
bors bot pushed a commit to bevyengine/bevy-website that referenced this pull request Jan 31, 2023
Updating this old code on top of the recent improvements to the validator.

Some previous discussion in #443
Some recent motivation in [#253](bevyengine/bevy-assets#253)

## Objective

- Shorten the feedback loop for users submitting their projects to `bevy_assets`
- Help maintainers enforce restrictions related to image size
- ~~Improve the overall look and feel of the assets page by enforcing a reasonable aspect ratio~~
- Improve the performance of the assets page and reduce the repository size by enforcing a maximum file size
- Make validation errors output a little more clear for users

## Example Output

Note: Some of these validations are no longer present in this PR.

```
Fish Fight: Punchy
  Image dimensions must not exceed 1000x1000 px.
  Image aspect ratio must be between 1 and 2.

taipo
  Description must be at most 100 chars in length.
  Image file not found.

Bevy Combat
  Image file must be at most 1000000 bytes.

Typey Birb
  Image aspect ratio must be between 1 and 2.

Cheaters Never Win
  Image dimensions must not exceed 1000x1000 px.
  Image aspect ratio must be between 1 and 2.

Error: 28 asset(s) are invalid.
```

## Discussion Points

This is currently a draft because many existing images do not currently meet these restrictions.

For file size restrictions, we could manually fix up the files.

But for image aspect ratio, I think we would need to find a way to "grandfather in" the old images. I contemplated a static list of grandfathered assets, but that would prevent validations from working if users attempt to update those in the future.

Also, see #448 where we decided to use a single aspect ratio on the website. If that were fixed, we might want to rework the some of the validations like

- Enforce a fixed image size rather than a maximum
- Remove the separate aspect ratio check all-together

The current file size limit in the PR seems clearly too large. I think something like 250kb would be more appropriate, but that would require a lot more work on existing assets.
bors bot pushed a commit to bevyengine/bevy-website that referenced this pull request Jan 31, 2023
Updating this old code on top of the recent improvements to the validator.

Some previous discussion in #443
Some recent motivation in [#253](bevyengine/bevy-assets#253)

## Objective

- Shorten the feedback loop for users submitting their projects to `bevy_assets`
- Help maintainers enforce restrictions related to image size
- ~~Improve the overall look and feel of the assets page by enforcing a reasonable aspect ratio~~
- Improve the performance of the assets page and reduce the repository size by enforcing a maximum file size
- Make validation errors output a little more clear for users

## Example Output

Note: Some of these validations are no longer present in this PR.

```
Fish Fight: Punchy
  Image dimensions must not exceed 1000x1000 px.
  Image aspect ratio must be between 1 and 2.

taipo
  Description must be at most 100 chars in length.
  Image file not found.

Bevy Combat
  Image file must be at most 1000000 bytes.

Typey Birb
  Image aspect ratio must be between 1 and 2.

Cheaters Never Win
  Image dimensions must not exceed 1000x1000 px.
  Image aspect ratio must be between 1 and 2.

Error: 28 asset(s) are invalid.
```

## Discussion Points

This is currently a draft because many existing images do not currently meet these restrictions.

For file size restrictions, we could manually fix up the files.

But for image aspect ratio, I think we would need to find a way to "grandfather in" the old images. I contemplated a static list of grandfathered assets, but that would prevent validations from working if users attempt to update those in the future.

Also, see #448 where we decided to use a single aspect ratio on the website. If that were fixed, we might want to rework the some of the validations like

- Enforce a fixed image size rather than a maximum
- Remove the separate aspect ratio check all-together

The current file size limit in the PR seems clearly too large. I think something like 250kb would be more appropriate, but that would require a lot more work on existing assets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants