CompressedImageSaver revamp redux#24223
Conversation
greeble-dev
left a comment
greeble-dev
left a comment
There was a problem hiding this comment.
Added some small suggestions. I don't feel I can click approve because my experience with texture compression is limited.
Co-authored-by: Greeble <166992735+greeble-dev@users.noreply.github.com>
Co-authored-by: Greeble <166992735+greeble-dev@users.noreply.github.com>
| >>("png"); | ||
| >>(crate::CompressedImageSaver::default().into()); | ||
|
|
||
| for file_extension in ImageLoader::SUPPORTED_FILE_EXTENSIONS { |
There was a problem hiding this comment.
I saw that the PR originally just did PNG and JPEG, but it was then changed to SUPPORTED_FILE_EXTENSIONS. I think the last change breaks processing as its tries and fails to compress already compressed textures.
ERROR bevy_asset::processor: Failed to process asset environment_maps\sky_skybox.ktx2:
Encountered an error while saving the asset: Expected texture_descriptor.mip_level_count to be 1
ERROR bevy_asset::processor: Failed to process asset textures\Ryfjallet_cubemap_bc7.ktx2:
Encountered an error while saving the asset: Unsupported texture format for compression: Bc7RgbaUnormSrgb
I guess it needs a smarter processor that can pass through already compressed textures? But I'd click approve on this PR if it went back to hard coded PNG/JPEG or exposed a setting - seems good enough for the short term.
There was a problem hiding this comment.
The processor can be a separate plugin exposing the setting. I don't think it should belong to ImagePlugin.
There was a problem hiding this comment.
@greeble-dev why is it running for those assets? It should be running only if you set the meta file up for an asset, I thought...
There was a problem hiding this comment.
This code sets a default processor, which runs automatically if an asset doesn't have a meta file.
|
I'm on the fence. My concern is mainly about the current limitations of bevy's asset system, as someone used to Godot's resource loader/saver/importer. Using env variables and the cargo feature to control BC/ASTC/BasisU output is kind of bad (Ideally, it should be extendable and be able to output all and load one at runtime). TBH, this isn't much more convenient than writing your own script or cargo xtask to process assets if you want to export your project to run on mobile. And it currently doesn't support hdr/exr images (My proposal is #24224). |
|
I would click approve as long as #24223 (comment) is sorted and no new issues are raised - I think this PR is enough of a step forward and lays the foundations for future work. I agree that processing is currently awkward, but that's more to do with the asset processor itself (e.g. no story for glTF embedded textures, no standard for platform specific processing). |
|
@beicause I defiently understand your concerns. I'm not a huge fan of the way this PR turned out either. But it's at least something to get us started. We probably need larger asset system changes to get this working better. |
This reverts commit 4aad16c.
There was a problem hiding this comment.
I'm clicking approve with a couple of caveats:
- I don't know much about texture compression, so I couldn't meaningfully review the details.
- Using the compression image saver in practice is still a bit awkward, particularly with processing. But this PR is good progress.
andriyDev
left a comment
andriyDev
left a comment
There was a problem hiding this comment.
LGTM! Excited to see this!
andriyDev
added
S-Ready-For-Final-Review
alice-i-cecile
added
M-Migration-Guide
|
I've filed #24903 to capture some of the issues raised during review. |
6 participants
Redo of #23567 but with: