[Merged by Bors] - Add additional image validation #464

rparrett · 2022-09-30T17:35:05Z

Updating this old code on top of the recent improvements to the validator.

Some previous discussion in #443
Some recent motivation in #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.

IceSentry · 2023-01-18T06:13:35Z

I believe this should be updated to only check for file size restrictions and ignore aspect ratio. With the current card layout, using the wrong aspect ratio might not look great, but it isn't an issue like size is.

rparrett · 2023-01-18T14:32:41Z

That's fine.

Unfortunately, image is now choking on a webp image due to image-rs/image#1712

I guess I'll just rip out the rest of the image validation (leaving file size) as well.

With a constant and common aspect ratio on the website to aim for, more users are getting it right.

rparrett · 2023-01-18T15:05:00Z

Okay, that is done.

bevy_ecs_ldtk
  Image file size 4201746 exceeds maximum 2097152 bytes.

Kataster
  Image file size 3046854 exceeds maximum 2097152 bytes.

Error: 2 asset(s) are invalid.

IceSentry · 2023-01-18T20:23:16Z

Unfortunately, image is now choking on a webp

Oh, 😦 that is unfortunate, well, I guess we'll be able to add it back in the future

For the CI failure, #520 this should fix it, it's not caused by this PR

rparrett · 2023-01-18T20:30:45Z

Oh, 😦 that is unfortunate, well, I guess we'll be able to add it back in the future

Yeah, happy to do that later. I am pretty sure that the next release of image will fix the issue.

rparrett · 2023-01-19T20:54:28Z

We will need to do something about the images for kataster and bevy_ecs_ldtk or I assume the next person to submit something to bevy-assets will get stuck with that problem.

@BorisBoutillier

@BorisBoutillier with your permission, I'd like to swap out Kataster's image with this smaller `webp` version. ([link to animated preview](https://raw.githubusercontent.com/bevyengine/bevy-assets/f1ad2af8b53adaff68622cbe981791f66b80e974/Apps/Games/Kataster.webp)) <img width="309" alt="image" src="https://user-images.githubusercontent.com/200550/213757436-e4919ad1-6df4-4b14-9628-1cce7bcb7135.png"> This is a re-recording that hopefully captures the same spirit and quality. I took some liberties to remove the title bar and match the game's aspect ratio to what's displayed on the website. Context: I'm trying to add image size validation to the CI (bevyengine/bevy-website#464), and there are a few images that are currently over the size limit. If you'd prefer to cut a new recording yourself, that's great.

@Trouv

@Trouv with your permission, I'd like to swap out `bevy_ecs_ldtk`'s image with this smaller version. You can see it animated side by side with the previous version in the changed files tab. <img width="311" alt="image" src="https://user-images.githubusercontent.com/200550/213820696-40d1b246-fa25-4feb-8ebc-5b3a4f401576.png"> This is a re-recording that hopefully captures the same spirit and image quality. I took some liberties to match the game's aspect ratio to what's displayed on the website and get the file size down. Context: I'm trying to add image size validation to the CI (bevyengine/bevy-website#464), and there are a few images that are currently over the size limit. If you'd prefer to cut a new recording yourself, that's great. There's no rush. I just thought it would be rude to ask you to re-do it without giving you an easy option.

alice-i-cecile · 2023-01-31T20:02:57Z

Fingers crossed this should work now.

bors r+

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 · 2023-01-31T20:04:13Z

Build failed:

generate-wasm-examples

rparrett · 2023-01-31T20:13:15Z

error: usage of sparse registries requires -Z sparse-registry

well that's new and exciting

mockersf · 2023-01-31T23:08:14Z

bors r+

mockersf · 2023-01-31T23:09:36Z

error: usage of sparse registries requires -Z sparse-registry

well that's new and exciting

I'm going to go with cargo bug on the server side 🤷

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 · 2023-01-31T23:19:55Z

Pull request successfully merged into master.

Build succeeded:

@BorisBoutillier

@BorisBoutillier with your permission, I'd like to swap out Kataster's image with this smaller `webp` version. ([link to animated preview](https://raw.githubusercontent.com/bevyengine/bevy-assets/f1ad2af8b53adaff68622cbe981791f66b80e974/Apps/Games/Kataster.webp)) <img width="309" alt="image" src="https://user-images.githubusercontent.com/200550/213757436-e4919ad1-6df4-4b14-9628-1cce7bcb7135.png"> This is a re-recording that hopefully captures the same spirit and quality. I took some liberties to remove the title bar and match the game's aspect ratio to what's displayed on the website. Context: I'm trying to add image size validation to the CI (bevyengine/bevy-website#464), and there are a few images that are currently over the size limit. If you'd prefer to cut a new recording yourself, that's great.

@Trouv

@Trouv with your permission, I'd like to swap out `bevy_ecs_ldtk`'s image with this smaller version. You can see it animated side by side with the previous version in the changed files tab. <img width="311" alt="image" src="https://user-images.githubusercontent.com/200550/213820696-40d1b246-fa25-4feb-8ebc-5b3a4f401576.png"> This is a re-recording that hopefully captures the same spirit and image quality. I took some liberties to match the game's aspect ratio to what's displayed on the website and get the file size down. Context: I'm trying to add image size validation to the CI (bevyengine/bevy-website#464), and there are a few images that are currently over the size limit. If you'd prefer to cut a new recording yourself, that's great. There's no rush. I just thought it would be rude to ask you to re-do it without giving you an easy option.

alice-i-cecile added A-Assets The collection of ecosystem crates found on the Bevy Assets page A-Build-System C-Usability labels Nov 12, 2022

rparrett added 3 commits January 18, 2023 07:58

Add additional image validation

8b48e42

Remove aspect ratio validation

532b6e2

With a constant and common aspect ratio on the website to aim for, more users are getting it right.

Rip out other image validation and improve error messages

7325437

rparrett force-pushed the validate-images branch from 7738f32 to 7325437 Compare January 18, 2023 15:00

The official limit is 2MB now

dbc94bc

rparrett marked this pull request as ready for review January 19, 2023 20:51

rparrett mentioned this pull request Jan 20, 2023

[Merged by Bors] - Use new smaller image for Kataster bevyengine/bevy-assets#287

Closed

rparrett mentioned this pull request Jan 20, 2023

[Merged by Bors] - Use new smaller image for bevy_ecs_ldtk bevyengine/bevy-assets#288

Closed

IceSentry approved these changes Jan 20, 2023

View reviewed changes

rparrett requested a review from mockersf January 31, 2023 19:52

rparrett mentioned this pull request Jan 31, 2023

[Merged by Bors] - bevy_ui_exact_image bevyengine/bevy-assets#292

Closed

alice-i-cecile approved these changes Jan 31, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review Ready for a maintainer to consider for merging label Jan 31, 2023

mockersf approved these changes Jan 31, 2023

View reviewed changes

bors bot changed the title ~~Add additional image validation~~ [Merged by Bors] - Add additional image validation Jan 31, 2023

bors bot closed this Jan 31, 2023

rparrett mentioned this pull request Jul 16, 2024

List maximum description length in the CI error message bevyengine/bevy-assets#159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add additional image validation #464

[Merged by Bors] - Add additional image validation #464

rparrett commented Sep 30, 2022 •

edited

Loading

IceSentry commented Jan 18, 2023

rparrett commented Jan 18, 2023 •

edited

Loading

rparrett commented Jan 18, 2023

IceSentry commented Jan 18, 2023

rparrett commented Jan 18, 2023

rparrett commented Jan 19, 2023

alice-i-cecile commented Jan 31, 2023

bors bot commented Jan 31, 2023

rparrett commented Jan 31, 2023

mockersf commented Jan 31, 2023

mockersf commented Jan 31, 2023

bors bot commented Jan 31, 2023

[Merged by Bors] - Add additional image validation #464

[Merged by Bors] - Add additional image validation #464

Conversation

rparrett commented Sep 30, 2022 • edited Loading

Objective

Example Output

Discussion Points

IceSentry commented Jan 18, 2023

rparrett commented Jan 18, 2023 • edited Loading

rparrett commented Jan 18, 2023

IceSentry commented Jan 18, 2023

rparrett commented Jan 18, 2023

rparrett commented Jan 19, 2023

alice-i-cecile commented Jan 31, 2023

bors bot commented Jan 31, 2023

rparrett commented Jan 31, 2023

mockersf commented Jan 31, 2023

mockersf commented Jan 31, 2023

bors bot commented Jan 31, 2023

rparrett commented Sep 30, 2022 •

edited

Loading

rparrett commented Jan 18, 2023 •

edited

Loading