CompressedImageSaver revamp#23567
Conversation
|
FYI I'm working on a basisu asset processor in beicause/bevy_basisu_loader#33 |
| return Err(CompressedImageSaverError::UninitializedImage); | ||
| }; | ||
|
|
||
| if image.texture_descriptor.mip_level_count != 1 { |
There was a problem hiding this comment.
Note ctt can deal with any source amount of mips
| stride: image.width() * bytes_per_pixel, | ||
| format: input_format, | ||
| color_space, | ||
| alpha: ctt::AlphaMode::Straight, // TODO: User-configurable? |
There was a problem hiding this comment.
Yeah this should be user configurable.
| container: ctt::Container::Ktx2, | ||
| quality: ctt::Quality::default(), | ||
| output_color_space: None, | ||
| output_alpha: None, |
There was a problem hiding this comment.
The correct default here is Some(premultiplied), though should be user configurable
andriyDev
left a comment
There was a problem hiding this comment.
Overall LGTM. We're just calling another lib!
I haven't tested this locally, but the code looks fine (barring one complaint).
| }) | ||
| } | ||
|
|
||
| #[cfg(feature = "compressed_image_saver_universal")] |
There was a problem hiding this comment.
I am really not a fan of this. Saying we just straight up can't compile either case if both features are enabled kinda sucks.
WDYT about just having two CompressedImageSavers? CompressedImageSaverCtt and CompressedImageSaverUniversal? Then just pick one in the plugin? This way if users want to use the old compression, they can. You could also have a wrapper CompressedImageSaver for compatibility (which internally holds one of the backends), so that we keep old meta files working.
I am personally not a fan of mutually exclusive feature flags, since it means I can't just test with --all-features.
There was a problem hiding this comment.
Generally the idea was that you'd always be using CompressedImageSaver (same meta files), and then depending on what platform you want to target, you'd enable different backends.
There was a problem hiding this comment.
I think you can use feature gate within a custom asset processor instead of asset savers, instead of using LoadTransformAndSave.
There was a problem hiding this comment.
I want to double down on this. I think we should support both. I'd rather users have the capacity to remix these however they want in their own processors, rather than being limited to whatever processors we give them.
| @@ -20,6 +20,7 @@ Cargo.lock | |||
|
|
|||
| # Bevy Assets | |||
| assets/**/*.meta | |||
There was a problem hiding this comment.
I think we could remove this now that we don't auto generate meta files, but let's not do that in this PR.
| fn default() -> Self { | ||
| Self { | ||
| input_alpha_mode: AlphaMode::Straight, | ||
| output_alpha_mode: AlphaMode::Premultiplied, |
There was a problem hiding this comment.
This is a surprising default to me. Why are we changing this on users in the default config?
There was a problem hiding this comment.
If I hand an image processor an image. I expect nothing else to be changed during processing. Why do we expect Straight in but return PMA out?
There was a problem hiding this comment.
I don't know what the correct API design is for bevy, however if any kind of blending is involved, you need premultiplied alpha for the math to work out. iff you are nearest sampling you can do the premultiplication in the shader, but if you're doing any kind of linear sampling you need premultiplied alpha in the texture.
There was a problem hiding this comment.
Yeah, this is generally the best default for users.
alice-i-cecile
left a comment
There was a problem hiding this comment.
We need to move the assets out into the bevy-assets repo still 🙂
I've also caught a few smaller problems, which should be addressed.
@alice-i-cecile I think this is merged too quickly. The assets have not moved to bevy asset repo. Even if we delete these files later, the git history is permanently bloated unless you force push to rewrite history. |
Objective
Closes #14671.
Solution
Testing
Showcase