Configure image attributes using ImageLoaderSettings

[kurtkuehnert]

trafficstars

Objective

• Fixes #11200

Solution

I added some of the TextureDescriptor fields to the ImageLoaderSettings struct. They are optional and can be used to overwrite the default values/values defined by the ImageFormat.

I did not add all fields (label, size, etc.). Also, the wgpu types are not serializable and deserialized, so I am skipping them.

---

Changelog

Added optional texture_dimension, texture_format, and texture_usage fields to the ImageLoaderSettings.

[kurtkuehnert]

Thanks for the review. Yeah we could wait for .meta files to expose these settings, but I am unsure how far away this is.

I tried keeping this PR as simple as possible (thus no documentation) without breaking existing code, so we can use this temporarily, and wait for the better API later.

[doonv]

without breaking existing code

Unfortunately adding elements to struct without the #[non_exhaustive] tag is considered a breaking change.

---

I'm not sure how Bevy handles "temporary" pull requests and stuff, so I have no idea if this is a good idea.

[kurtkuehnert]

Sure it's technically not none-breaking, but I expect for the most caeses, where ImageLoaderSettings is constructed, only some fields are set and the rest are overwritten with default anyway.

[mockersf]

I think you should enable (de)serialization for the types, and not skip serde so that it would work directly with .meta files.

I would also prefer that you pass the values to Image::from_buffer, same as the other settings.

Maybe we should wait until .meta files are finished. Then we could add [insert what .meta files are parsed into here ] as a parameter to ImageLoaderSettings?

Not sure what you mean? .meta files are finished, and what they are parsing is ImageLoaderSettings.

[cart]

I think you should enable (de)serialization for the types, and not skip serde so that it would work directly with .meta files.

IIRC this is a pretty big lever to pull / enables a lot of stuff on the wgpu side.

Not sure what you mean? .meta files are finished, and what they are parsing is ImageLoaderSettings.

Yup meta files "work" today and they are driven by serde. As a general rule, settings should be serializable.

The general strategy for ImageLoaderSettings has been to define a new backend-agnostic Image settings API that is serializable. See ImageSamplerDescriptor for an example of the pattern.

[mockersf]

I think you should enable (de)serialization for the types, and not skip serde so that it would work directly with .meta files.

IIRC this is a pretty big lever to pull / enables a lot of stuff on the wgpu side.

Yeah, definitely don't enable serde for all of wgpu! Mirror enums owned by Bevy should be small enough to maintain on our side.

[kurtkuehnert]

Thank you for your input.

So ideally, we can configure all of the attributes of an image using the ImageLoaderSettings, and by extension also in a .meta file.

To make this work, we would have to either implement and enable serde for all wgpu texture types, which is not desired, or we would have to copy them and maintain a mirrored version.

This encompasses the following types: wgpu::Label => WgpuLabel (Option<&str> should be easy) wgpu::Extent3d => ImageExtend3D (Image prefix or Wgpu prefix?) wgpu::TextureDimension => ImageDimension (straight-forward only 3 enum variants) wgpu::TextureFormat => ImageFormat (image format already exists and refers to the image encoding, so we would have to rename one of them, also this enum has very many variants, which makes mirroring them very verbose). wgpu::TextureUsages => ImageUsages (how do we handle bitfield serialization? a simple u32? this would not be very readable)

Additionally, we might also like to mirror TextureViewDescriptor and TextureAspect.

If we were to mirror all of these types, it would be a logical next step, to flatten the Image type to look somewhat like this:

//old:
pub struct Image {
    pub data: Vec<u8>,
    // TODO: this nesting makes accessing Image metadata verbose. Either flatten out the descriptor or add accessors
    pub texture_descriptor: wgpu::TextureDescriptor<'static>,
    pub sampler: ImageSampler,
    pub texture_view_descriptor: Option<TextureViewDescriptor<'static>>,
    pub asset_usage: RenderAssetUsages,
}

//new:
pub struct Image {
    pub data: Vec<u8>,
    pub label: ImageLabel,
    pub size: ImageExtent3d,
    pub mip_level_count: u32,
    pub sample_count: u32,
    pub dimension: ImageDimension,
    pub format: ImageFormat,
    pub usage: ImageUsages,
    pub view_formats: Vec<ImageFormat>,
    pub sampler: ImageSampler,
    pub texture_view_descriptor: Option<TextureViewDescriptor<'static>>, // rework this as well
    pub asset_usage: RenderAssetUsages,
}

Since implementing (de)serialization for all texture types is not trivial, I would debate, that it might be beneficial to merge this PR as is, since this change already improves our API for loading images. Additionally, I would create a new issue detailing how we can get this functionality to work with .meta files, as well as, how we can use these newly mirrored types, to improve/flatten our image abstraction.

What do you think? @mockersf @cart

[cart]

If we were to mirror all of these types, it would be a logical next step, to flatten the Image type to look somewhat like this:

For now I think it probably makes sense to keep the TextureDescriptor model (aka ImageDescriptor):

1. It reduces boilerplate:

• It allows us to share the type / its fields across both ImageLoaderSettings and Image
• It allows us to implement a single From<ImageDescriptor> for TextureDescriptor<'static> and use it anywhere we need to use an ImageDescriptor

2. It makes it easier to conceptualize the Image / wgpu::Texture connection.

Since implementing (de)serialization for all texture types is not trivial, I would debate, that it might be beneficial to merge this PR as is, since this change already improves our API for loading images.

I would prefer to hold the line on "loader settings must be serializable". I don't think the mirroring has many unknowns / we should be able to merge it relatively quickly. And that way we don't break anyone by rolling out two versions of the api back-to-back across releases.

Additionally, I would create a new issue detailing how we can get this functionality to work with .meta files, as well as, how we can use these newly mirrored types, to improve/flatten our image abstraction.

I'm down for that.

[cart]

Or alternatively we could embrace ImageLoaderSettings as the ImageDescriptor and store that on Image?

[ethereumdegen]

can we please have this . I need it for doing stuff w warbler grass

It is unacceptable that the TextureFormat is always assumed by Bevy and I cannot specify it. It is also unacceptable to use .meta files because i am doing dynamic paint textures per chunk.

[cart]

I agree that this is necessary, but its worth doing this in a new PR introducing mirrored configuration API.