-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Configure image attributes using ImageLoaderSettings #11294
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
pub sample_count: Option<u32>, | ||
#[serde(skip)] | ||
pub dimension: Option<TextureDimension>, | ||
#[serde(skip)] | ||
pub texture_format: Option<TextureFormat>, | ||
#[serde(skip)] | ||
pub usage: Option<TextureUsages>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More documentation can be added here
Thanks for the review. 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. |
Unfortunately adding elements to struct without the I'm not sure how Bevy handles "temporary" pull requests and stuff, so I have no idea if this is a good idea. |
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. |
I think you should enable (de)serialization for the types, and not skip serde so that it would work directly with I would also prefer that you pass the values to
Not sure what you mean? |
IIRC this is a pretty big lever to pull / enables a lot of stuff on the wgpu side.
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. |
Yeah, definitely don't enable serde for all of wgpu! Mirror enums owned by Bevy should be small enough to maintain on our side. |
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: 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. |
For now I think it probably makes sense to keep the TextureDescriptor model (aka
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.
I'm down for that. |
Or alternatively we could embrace ImageLoaderSettings as the ImageDescriptor and store that on Image? |
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. |
I agree that this is necessary, but its worth doing this in a new PR introducing mirrored configuration API. |
Objective
Solution
I added some of the
TextureDescriptor
fields to theImageLoaderSettings
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
, andtexture_usage
fields to theImageLoaderSettings
.