Skip to content
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

Add ImageSamplerDescriptor as an image loader setting #9982

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

Kanabenki
Copy link
Contributor

@Kanabenki Kanabenki commented Oct 1, 2023

Closes #9946

Objective

Add a new type mirroring wgpu::SamplerDescriptor for ImageLoaderSettings to control how a loaded image should be sampled.

Fix issues with texture sampler descriptors not being set when loading gltf texture from URI.

Solution

Add a new ImageSamplerDescriptor and its affiliated types that mirrors wgpu::SamplerDescriptor, use it in the image loader settings.


Changelog

Added

  • Added new types ImageSamplerDescriptor, ImageAddressMode, ImageFilterMode, ImageCompareFunction and ImageSamplerBorderColor that mirrors the corresponding wgpu types.
  • ImageLoaderSettings now carries an ImageSamplerDescriptor field that will be used to determine how the loaded image is sampled, and will be serialized as part of the image assets .meta files.

Changed

  • Image::from_buffer now takes the sampler descriptor to use as an additional parameter.

Fixed

  • Sampler descriptors are set for gltf textures loaded from URI.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen A-Assets Load files from disk to use for things like images, models, and sounds labels Oct 1, 2023
@@ -57,7 +57,8 @@ bevy_tasks = { path = "../bevy_tasks", version = "0.12.0-dev" }
image = { version = "0.24", default-features = false }

# misc
wgpu = { version = "0.16.0", features=["naga"] }
wgpu = { version = "0.16.0", features = ["naga"] }
wgpu-types = { version = "0.16.0", features = ["trace", "replay"] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is wgpu-types needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to enable the trace and replay features unconditionally only for the types re-exported in wgpu from there (all types used in the fields of wgpu::SamplerDescriptor), without enabling the trace/replay related stuff in wgpu itself. But I can alos move the enabled features to wgpu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to enable these if we're using a proxy type? wgpu is already pretty slow to compile as is, so I'd prefer to avoid making it take even longer by enabling serde.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still needed since the proxy type is used for the Serialize/Deserialize impl of wgpu::SamplerDescriptor, but the Serialize/Deserialize impl of its fields is pulled from the serde derives in wgpu_types. This should only impact the compile time of wgpu_types which is just a collection of types re-exported in wgpu, from some quick testing the compile times of wgpu itself aren't affected, and wgpu_types goes from 1.2s to 1.7s to build when those features are enabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw if we're going with this solution I'd rather we go with declaring a dependency on wgpu-types and enable the serde features on it (trace+replay), vs enabling them for the whole wgpu crate. (wgpu always includes wgpu-types, so this isn't adding another dependency, it's just not slowing down our compiles as much as enabling trace+replay for wgpu would).

Comment on lines 163 to 181
/// Proxy type used to implement [`Serialize`] and [`Deserialize`] for [`ImageSampler`].
#[derive(Serialize, Deserialize)]
#[serde(remote = "wgpu::SamplerDescriptor<'static>")]
struct WgpuSamplerDescriptor {
#[serde(skip)]
label: wgpu::Label<'static>,
address_mode_u: wgpu::AddressMode,
address_mode_v: wgpu::AddressMode,
address_mode_w: wgpu::AddressMode,
mag_filter: wgpu::FilterMode,
min_filter: wgpu::FilterMode,
mipmap_filter: wgpu::FilterMode,
lod_min_clamp: f32,
lod_max_clamp: f32,
compare: Option<wgpu::CompareFunction>,
anisotropy_clamp: u16,
border_color: Option<wgpu::SamplerBorderColor>,
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we newtype the wgpu SamplerDescriptor type instead of reimplementing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, that's not possible using serde derive, a manual Serialize/Deserialize impl would need to be added for ImageSampler. Is that preferable ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to think what would be most maintainable. I am concerned about desync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of questions.

@mockersf
Copy link
Member

mockersf commented Oct 1, 2023

this issue used to be solved before Asset V2 by loading images directly from their path inside the gltf loader instead of calling the image loader

@Kanabenki
Copy link
Contributor Author

Kanabenki commented Oct 2, 2023

I see, if that is preferable I'll do it that way. However won't we lose the parallelization of the textures loading by not going through the ImageLoader ?

@mockersf
Copy link
Member

mockersf commented Oct 9, 2023

We were doing parallelisation directly inside the loader: https://github.com/bevyengine/bevy/blob/release-0.11.3/crates/bevy_gltf/src/loader.rs#L383-L429

And the gltf texture loader was able to handle both path and data directly: https://github.com/bevyengine/bevy/blob/release-0.11.3/crates/bevy_gltf/src/loader.rs#L566-L616

@cart cart added this to the 0.12 milestone Oct 10, 2023
@cart
Copy link
Member

cart commented Oct 10, 2023

this issue used to be solved before Asset V2 by loading images directly from their path inside the gltf loader instead of calling the image loader

I think the "load" approach is preferable for a number of reasons:

  1. It allows these images to be pre-processed (compressed / gpu-friendly format) cleanly and separately. It is possible with embedded images, just more complicated. "one to many processed assets files" will make the "embedded processing" story a bit cleaner.
  2. It allows users to configure each image, if they want, by configuring the metadata files on each image.

We might want to make this configurable, but imo "separate image" is the correct default.

@mockersf
Copy link
Member

I think the "load" approach is preferable

For me, the most important is to use the same path to load the data, wether it's inline or in a separate file. That difference introduced a lot of bugs in the past, and introduce new ones now

@Elabajaba
Copy link
Contributor

Needs to be updated since we landed the wgpu update PR.

@Kanabenki
Copy link
Contributor Author

What would be the best course of action for this PR while one to many asset loading is not supported ? For embedded textures, would using the label part of the AssetPath to distinguish those be ok ?

Otherwise I could revert to the pre-asset V2 way with everything in the gltf loader, or keep the split handling between embedded/external texture, and in both case handle everything through ImageLoader in a future PR once one to many asset loading is implemented.

@cart
Copy link
Member

cart commented Oct 13, 2023

I think the approach in this PR is the right path. Loading images (and other assets) rather than embedding them is a pattern we will use for other scene types too (ex: Bevy Scenes). I still think it is the right path here.

Additionally, we want to add sampler descriptor configuration to the ImageLoader settings regardless of what we choose to do in this PR. When a developer defines a sprite png file, they will want to be able to define the filters on a per-asset basis. Configurable per-image-asset filters will likely be one of the highest-frequency use cases for "asset meta files".

I do share concerns about surfacing wgpu types directly here. Not only for the desync risk, but also because this will be serialized to meta files. We need to be in full control of that schema so we can detect and do migrations automatically across Bevy versions. The docs for these types should call out that changes to the schema will require migrations in some cases. (migrations have not yet been implemented ... this is a must-have feature for the next release cycle).

For this PR, I think we should create a new ImageSamplerDescriptor type. Regrettably, I think we will want to mirror the other types too (AddressMode, FilterMode, CompareFunction, SamplerBorderColor). These type names should also have an Image prefix. These types should all derive Serialize/Deserialize and we should manually define conversion to/from the wgpu types.

I think we should re-contextualize this PR to focus on configurable samplers in meta files / the title should be adjusted to accommodate that). Or alternatively, that should be broken into a separate PR and merged first. Imo defining samplers in meta files is a "big ticket" feature that should be the focus of this change set (from a user perspective).

@Kanabenki
Copy link
Contributor Author

Kanabenki commented Oct 15, 2023

Thanks for the detailed answer. I'll submit a separate PR to add bevy-defined sampling settings and rework this one to use those, then.

One thing I'm now wondering is if it actually make sense for the sampling settings to be part of the ImageLoaderSettings ? Having different sampling configuration for the same image seems like a valid use case (and is possible in gltf since a texture combine an image with a sampler), which doesn't seem possible if the sampling configuration is directly tied to an asset path. Maybe that would be better fitted for a setting on each material that use the texture, or on an intermediate "texture" asset that would group an image with a sampling setting.

That's a bit tangential to the gltf texture loading since in that case we can pass different ImageLoaderSettings, although I suppose it could still be a problem if one were to use an AssetProcessor with compressed texture saving, since it could map several gltf textures with differents gltf samplers but the same image to an unique compressed image for which only one sampling setting would be saved.

@Diddykonga
Copy link

Want to mention, when helping people with Godot, one of the changes they made was to remove Filter Mode from Image Assets, as many people were upset that they had to duplicate their images, just to have different Filtering applied to them, they moved this to the Material/Drawer that used them, I believe.

load_context.load_with_settings(path, move |settings: &mut ImageLoaderSettings| {
settings.is_srgb = is_srgb;
settings.sampler_descriptor = sampler_descriptor.clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
settings.sampler_descriptor = sampler_descriptor.clone();
settings.sampler_descriptor = sampler_descriptor;

@@ -644,6 +649,7 @@ async fn load_image<'a, 'b>(
supported_compressed_formats: CompressedImageFormats,
) -> Result<ImageOrPath, GltfError> {
let is_srgb = !linear_textures.contains(&gltf_texture.index());
let sampler_descriptor = ImageSampler::Descriptor(texture_sampler(&gltf_texture));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use this descriptor on line 664 instead of creating a new one

supported_compressed_formats,
is_srgb,
)?;
image.sampler_descriptor = sampler_descriptor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the sampler should probably be an argument to Image::from_buffer - it is manually set after calling in 3 of 4 callsites (and it's easy to construct the default variant in the remaining case).

Comment on lines 163 to 181
/// Proxy type used to implement [`Serialize`] and [`Deserialize`] for [`ImageSampler`].
#[derive(Serialize, Deserialize)]
#[serde(remote = "wgpu::SamplerDescriptor<'static>")]
struct WgpuSamplerDescriptor {
#[serde(skip)]
label: wgpu::Label<'static>,
address_mode_u: wgpu::AddressMode,
address_mode_v: wgpu::AddressMode,
address_mode_w: wgpu::AddressMode,
mag_filter: wgpu::FilterMode,
min_filter: wgpu::FilterMode,
mipmap_filter: wgpu::FilterMode,
lod_min_clamp: f32,
lod_max_clamp: f32,
compare: Option<wgpu::CompareFunction>,
anisotropy_clamp: u16,
border_color: Option<wgpu::SamplerBorderColor>,
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kanabenki
Copy link
Contributor Author

Kanabenki commented Oct 21, 2023

I've updated this PR with the discussed changes instead of doing a separate one, since that one would have ended up with mostly nothing in it.

Some outstanding questions:

  • As discussed in the last comments, are we sure we want to have all sampling-related settings on the ImageLoaderSettings instead of allowing different sampling settings for an image, whether that be at the material level or through an additional asset type (or anything else)?
  • I didn't add a wgpu::SamplerDescriptor -> ImageSamplerDescriptor conversion since there's no use for it in the bevy codebase currently, do we still want to provide it for the users? (edit: nevermind, it is needed for the compressed texture saver)
  • I choose to mirror the wgpu types exactly, that includes some wgpu feature gated settings like ImageSamplerBorderColor. Is that ok or should those be removed?

@Kanabenki Kanabenki changed the title Set sampler descriptors for gltf image URI Add ImageSamplerDescriptor as an image loader setting Oct 21, 2023
@Kanabenki
Copy link
Contributor Author

Want to mention, when helping people with Godot, one of the changes they made was to remove Filter Mode from Image Assets, as many people were upset that they had to duplicate their images, just to have different Filtering applied to them, they moved this to the Material/Drawer that used them, I believe.

Indeed, I discussed that on Discord but forgot to add that info here! From what I saw most mainstream engines still put all sampling-related settings at the image import level, except for Godot which made the switch for 4.0.

@@ -30,6 +33,10 @@ impl AssetSaver for CompressedImageSaver {
compressor_params.set_basis_format(basis_universal::BasisTextureFormat::UASTC4x4);
compressor_params.set_generate_mipmaps(true);
let is_srgb = image.texture_descriptor.format.is_srgb();
let sampler_descriptor = match &image.sampler_descriptor {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too sure about that one, if we want to use the actual default sampler from the render plugin, it would require either passing its descriptor here or changing ImageLoaderSettings::sampler_descriptor to an ImageSampler and using ImageSamplerDescriptor instead of wgpu::SamplerDescriptor in ImageSampler.

@cart
Copy link
Member

cart commented Oct 25, 2023

As discussed in the last comments, are we sure we want to have all sampling-related settings on the ImageLoaderSettings instead of allowing different sampling settings for an image, whether that be at the material level or through an additional asset type (or anything else)?

For now: I think the answer is yes. Moving it to the material level will require more time for discussion than we have for 0.12. The current approach is consistent with the current Image / Texture model.

I do think its worth having that conversation, just not here/now.

I choose to mirror the wgpu types exactly, that includes some wgpu feature gated settings like ImageSamplerBorderColor. Is that ok or should those be removed?

I think thats the right move. We can just ignore the settings when they aren't supported.

@cart
Copy link
Member

cart commented Oct 25, 2023

I have a number of changes queued up that accomplish a couple of goals:

  1. Treat ImageSamplerDescriptor as the "public descriptor api" in all places (rather than leaking the wgpu internals)
  2. Make it possible to use the "runtime configured default" sampler in meta files. (this addresses your open comment)

I can't push those changes as it looks like you've restricted the ability for maintainers to push changes to your branch. I think I might just merge this as-is and then create a new PR for my changes.

@cart
Copy link
Member

cart commented Oct 25, 2023

Also found a bug that was preventing the configured samplers from being used for Basis, KTX2, and DDS textures. Fixed that on my branch.

@cart
Copy link
Member

cart commented Oct 25, 2023

PR is here: #10254

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right path. I do have a few changes queued up, which we can review and merge after this.

@cart cart added this pull request to the merge queue Oct 25, 2023
Merged via the queue into bevyengine:main with commit 756fb06 Oct 25, 2023
21 checks passed
@Kanabenki
Copy link
Contributor Author

I can't push those changes as it looks like you've restricted the ability for maintainers to push changes to your branch. I think I might just merge this as-is and then create a new PR for my changes.

I don't remember changing that if that isn't the default, I'll make sure to have it enabled for my next PRs 🙂

github-merge-queue bot pushed a commit that referenced this pull request Oct 26, 2023
# Objective

- Build on the changes in #9982
- Use `ImageSamplerDescriptor` as the "public image sampler descriptor"
interface in all places (for consistency)
- Make it possible to configure textures to use the "default" sampler
(as configured in the `DefaultImageSampler` resource)
- Fix a bug introduced in #9982 that prevents configured samplers from
being used in Basis, KTX2, and DDS textures

---

## Migration Guide

- When using the `Image` API, use `ImageSamplerDescriptor` instead of
`wgpu::SamplerDescriptor`
- If writing custom wgpu renderer features that work with `Image`, call
`&image_sampler.as_wgpu()` to convert to a wgpu descriptor.
@cart cart mentioned this pull request Oct 30, 2023
43 tasks
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
)

Closes bevyengine#9946 

# Objective

Add a new type mirroring `wgpu::SamplerDescriptor` for
`ImageLoaderSettings` to control how a loaded image should be sampled.

Fix issues with texture sampler descriptors not being set when loading
gltf texture from URI.

## Solution

Add a new `ImageSamplerDescriptor` and its affiliated types that mirrors
`wgpu::SamplerDescriptor`, use it in the image loader settings.

---

## Changelog

### Added

- Added new types `ImageSamplerDescriptor`, `ImageAddressMode`,
`ImageFilterMode`, `ImageCompareFunction` and `ImageSamplerBorderColor`
that mirrors the corresponding wgpu types.
- `ImageLoaderSettings` now carries an `ImageSamplerDescriptor` field
that will be used to determine how the loaded image is sampled, and will
be serialized as part of the image assets `.meta` files.

### Changed

- `Image::from_buffer` now takes the sampler descriptor to use as an
additional parameter.

### Fixed

- Sampler descriptors are set for gltf textures loaded from URI.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

- Build on the changes in bevyengine#9982
- Use `ImageSamplerDescriptor` as the "public image sampler descriptor"
interface in all places (for consistency)
- Make it possible to configure textures to use the "default" sampler
(as configured in the `DefaultImageSampler` resource)
- Fix a bug introduced in bevyengine#9982 that prevents configured samplers from
being used in Basis, KTX2, and DDS textures

---

## Migration Guide

- When using the `Image` API, use `ImageSamplerDescriptor` instead of
`wgpu::SamplerDescriptor`
- If writing custom wgpu renderer features that work with `Image`, call
`&image_sampler.as_wgpu()` to convert to a wgpu descriptor.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
)

Closes bevyengine#9946 

# Objective

Add a new type mirroring `wgpu::SamplerDescriptor` for
`ImageLoaderSettings` to control how a loaded image should be sampled.

Fix issues with texture sampler descriptors not being set when loading
gltf texture from URI.

## Solution

Add a new `ImageSamplerDescriptor` and its affiliated types that mirrors
`wgpu::SamplerDescriptor`, use it in the image loader settings.

---

## Changelog

### Added

- Added new types `ImageSamplerDescriptor`, `ImageAddressMode`,
`ImageFilterMode`, `ImageCompareFunction` and `ImageSamplerBorderColor`
that mirrors the corresponding wgpu types.
- `ImageLoaderSettings` now carries an `ImageSamplerDescriptor` field
that will be used to determine how the loaded image is sampled, and will
be serialized as part of the image assets `.meta` files.

### Changed

- `Image::from_buffer` now takes the sampler descriptor to use as an
additional parameter.

### Fixed

- Sampler descriptors are set for gltf textures loaded from URI.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Build on the changes in bevyengine#9982
- Use `ImageSamplerDescriptor` as the "public image sampler descriptor"
interface in all places (for consistency)
- Make it possible to configure textures to use the "default" sampler
(as configured in the `DefaultImageSampler` resource)
- Fix a bug introduced in bevyengine#9982 that prevents configured samplers from
being used in Basis, KTX2, and DDS textures

---

## Migration Guide

- When using the `Image` API, use `ImageSamplerDescriptor` instead of
`wgpu::SamplerDescriptor`
- If writing custom wgpu renderer features that work with `Image`, call
`&image_sampler.as_wgpu()` to convert to a wgpu descriptor.
dekirisu pushed a commit to dekirisu/bevy_gltf_trait that referenced this pull request Jul 7, 2024
# Objective

- Build on the changes in bevyengine/bevy#9982
- Use `ImageSamplerDescriptor` as the "public image sampler descriptor"
interface in all places (for consistency)
- Make it possible to configure textures to use the "default" sampler
(as configured in the `DefaultImageSampler` resource)
- Fix a bug introduced in #9982 that prevents configured samplers from
being used in Basis, KTX2, and DDS textures

---

## Migration Guide

- When using the `Image` API, use `ImageSamplerDescriptor` instead of
`wgpu::SamplerDescriptor`
- If writing custom wgpu renderer features that work with `Image`, call
`&image_sampler.as_wgpu()` to convert to a wgpu descriptor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Textures don't render correctly on Bistro
8 participants