From 89579aeaee5a37d31be37a62d980670c6ce03880 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Mon, 20 Mar 2023 19:02:56 -0400 Subject: [PATCH 1/6] Fix metal mipmap behavior --- wgpu-hal/src/metal/adapter.rs | 1 - wgpu-hal/src/metal/device.rs | 8 -------- wgpu-hal/src/metal/mod.rs | 1 - 3 files changed, 10 deletions(-) diff --git a/wgpu-hal/src/metal/adapter.rs b/wgpu-hal/src/metal/adapter.rs index a61b628d8f..e7fb3317bd 100644 --- a/wgpu-hal/src/metal/adapter.rs +++ b/wgpu-hal/src/metal/adapter.rs @@ -540,7 +540,6 @@ impl super::PrivateCapabilities { MUTABLE_COMPARISON_SAMPLER_SUPPORT, ), sampler_clamp_to_border: Self::supports_any(device, SAMPLER_CLAMP_TO_BORDER_SUPPORT), - sampler_lod_average: { version.at_least((11, 0), (9, 0), os_is_mac) }, base_instance: Self::supports_any(device, BASE_INSTANCE_SUPPORT), base_vertex_instance_drawing: Self::supports_any(device, BASE_VERTEX_INSTANCE_SUPPORT), dual_source_blending: Self::supports_any(device, DUAL_SOURCE_BLEND_SUPPORT), diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index 52cc215126..6084a7164a 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -409,16 +409,12 @@ impl crate::Device for super::Device { &self, desc: &crate::SamplerDescriptor, ) -> DeviceResult { - let caps = &self.shared.private_caps; objc::rc::autoreleasepool(|| { let descriptor = metal::SamplerDescriptor::new(); descriptor.set_min_filter(conv::map_filter_mode(desc.min_filter)); descriptor.set_mag_filter(conv::map_filter_mode(desc.mag_filter)); descriptor.set_mip_filter(match desc.mipmap_filter { - wgt::FilterMode::Nearest if desc.lod_clamp.is_none() => { - metal::MTLSamplerMipFilter::NotMipmapped - } wgt::FilterMode::Nearest => metal::MTLSamplerMipFilter::Nearest, wgt::FilterMode::Linear => metal::MTLSamplerMipFilter::Linear, }); @@ -437,10 +433,6 @@ impl crate::Device for super::Device { descriptor.set_lod_max_clamp(range.end); } - if caps.sampler_lod_average { - descriptor.set_lod_average(true); // optimization - } - if let Some(fun) = desc.compare { descriptor.set_compare_function(conv::map_compare_function(fun)); } diff --git a/wgpu-hal/src/metal/mod.rs b/wgpu-hal/src/metal/mod.rs index 57083b585d..ffb6832de7 100644 --- a/wgpu-hal/src/metal/mod.rs +++ b/wgpu-hal/src/metal/mod.rs @@ -151,7 +151,6 @@ struct PrivateCapabilities { shared_textures: bool, mutable_comparison_samplers: bool, sampler_clamp_to_border: bool, - sampler_lod_average: bool, base_instance: bool, base_vertex_instance_drawing: bool, dual_source_blending: bool, From 88f034bc31a3397f8eb52d976d68b63f88dd6ccf Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Mon, 20 Mar 2023 19:11:39 -0400 Subject: [PATCH 2/6] Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index de634e06a6..28241eb7b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Bottom level categories: --> ## Unreleased + ### Major changes #### TextureFormat info API @@ -113,6 +114,9 @@ By @teoxoy in [#3534](https://github.com/gfx-rs/wgpu/pull/3534) ### Bug Fixes +#### Metal +- Fix incorrect mipmap being sampled when using `MinLod <= 0.0` and `MaxLod >= 32.0` or when the fragment shaders in the same squad sample different Lods. By @cwfitzgerald in [#3610](https://github.com/gfx-rs/wgpu/pull/3610). + #### DX12 - Fix DXC validation issues when using a custom `dxil_path`. By @Elabajaba in [#3434](https://github.com/gfx-rs/wgpu/pull/3434) From 3c4ee19425a82e04fda958a16ae2200284acd2da Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Tue, 21 Mar 2023 09:33:16 -0400 Subject: [PATCH 3/6] Deno --- CHANGELOG.md | 18 +++++++- deno_webgpu/sampler.rs | 4 +- wgpu-core/src/device/mod.rs | 70 +++++++++++++++++++------------ wgpu-core/src/resource.rs | 44 +++++++++++++++---- wgpu-hal/examples/halmark/main.rs | 4 +- wgpu-hal/src/dx12/device.rs | 15 +++---- wgpu-hal/src/gles/device.rs | 14 +++---- wgpu-hal/src/lib.rs | 7 ++-- wgpu-hal/src/metal/device.rs | 13 +++--- wgpu-hal/src/vulkan/device.rs | 24 +++++------ wgpu/src/lib.rs | 8 ++-- 11 files changed, 137 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28241eb7b5..8942f6d34e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,21 @@ The following `Features` have been renamed. By @teoxoy in [#3534](https://github.com/gfx-rs/wgpu/pull/3534) +#### Anisotropic Filtering + +Anisotropic filtering has been brought in line with the spec. The anisotropic clamp is now a f32 which must be between 1.0 and 16.0 inclusive. + +If the anisotropy clamp is not 1.0, all the filters in a sampler must be `Linear`. + +```diff +SamplerDescriptor { +- anisotropic_clamp: None, ++ anisotropic_clamp: 1.0, +} +``` + +By @cwfitzgerald in [#3610](https://github.com/gfx-rs/wgpu/pull/3610). + #### General - Change type of `mip_level_count` and `array_layer_count` (members of `TextureViewDescriptor` and `ImageSubresourceRange`) from `Option` to `Option`. By @teoxoy in [#3445](https://github.com/gfx-rs/wgpu/pull/3445) @@ -99,6 +114,7 @@ By @teoxoy in [#3534](https://github.com/gfx-rs/wgpu/pull/3534) - Improve attachment related errors. By @cwfitzgerald in [#3549](https://github.com/gfx-rs/wgpu/pull/3549) - Make error descriptions all upper case. By @cwfitzgerald in [#3549](https://github.com/gfx-rs/wgpu/pull/3549) - Don't include ANSI terminal color escape sequences in shader module validation error messages. By @jimblandy in [#3591](https://github.com/gfx-rs/wgpu/pull/3591) +- Bring anisotropic filtering in line with the spec. #### WebGPU @@ -115,7 +131,7 @@ By @teoxoy in [#3534](https://github.com/gfx-rs/wgpu/pull/3534) ### Bug Fixes #### Metal -- Fix incorrect mipmap being sampled when using `MinLod <= 0.0` and `MaxLod >= 32.0` or when the fragment shaders in the same squad sample different Lods. By @cwfitzgerald in [#3610](https://github.com/gfx-rs/wgpu/pull/3610). +- Fix incorrect mipmap being sampled when using `MinLod <= 0.0` and `MaxLod >= 32.0` or when the fragment shader samples different Lods in the same quad. By @cwfitzgerald in [#3610](https://github.com/gfx-rs/wgpu/pull/3610). #### DX12 diff --git a/deno_webgpu/sampler.rs b/deno_webgpu/sampler.rs index e5f230b2dd..6a9fe285d3 100644 --- a/deno_webgpu/sampler.rs +++ b/deno_webgpu/sampler.rs @@ -40,7 +40,7 @@ pub struct CreateSamplerArgs { lod_min_clamp: f32, lod_max_clamp: f32, compare: Option, - max_anisotropy: u8, + max_anisotropy: f32, } #[op] @@ -67,7 +67,7 @@ pub fn op_webgpu_create_sampler( lod_min_clamp: args.lod_min_clamp, lod_max_clamp: args.lod_max_clamp, compare: args.compare, - anisotropy_clamp: std::num::NonZeroU8::new(args.max_anisotropy), + anisotropy_clamp: args.max_anisotropy, border_color: None, // native-only }; diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 5f6a148129..3967d226da 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -1310,37 +1310,53 @@ impl Device { self.require_features(wgt::Features::ADDRESS_MODE_CLAMP_TO_ZERO)?; } - if desc.lod_min_clamp < 0.0 || desc.lod_max_clamp < desc.lod_min_clamp { - return Err(resource::CreateSamplerError::InvalidLodClamp( - desc.lod_min_clamp..desc.lod_max_clamp, + if desc.lod_min_clamp < 0.0 { + return Err(resource::CreateSamplerError::InvalidLodMinClamp( + desc.lod_min_clamp, )); } + if desc.lod_max_clamp < desc.lod_min_clamp { + return Err(resource::CreateSamplerError::InvalidLodMaxClamp { + lod_min_clamp: desc.lod_min_clamp, + lod_max_clamp: desc.lod_max_clamp, + }); + } - let lod_clamp = if desc.lod_min_clamp > 0.0 || desc.lod_max_clamp < 32.0 { - Some(desc.lod_min_clamp..desc.lod_max_clamp) - } else { - None - }; + if !(1.0..=16.0).contains(&desc.anisotropy_clamp) { + return Err(resource::CreateSamplerError::InvalidAnisotropy( + desc.anisotropy_clamp, + )); + } - let anisotropy_clamp = if let Some(clamp) = desc.anisotropy_clamp { - let clamp = clamp.get(); - let valid_clamp = - clamp <= hal::MAX_ANISOTROPY && conv::is_power_of_two_u32(clamp as u32); - if !valid_clamp { - return Err(resource::CreateSamplerError::InvalidClamp(clamp)); + if desc.anisotropy_clamp != 1.0 { + if !matches!(desc.min_filter, wgt::FilterMode::Linear) { + return Err( + resource::CreateSamplerError::InvalidFilterModeWithAnisotropy { + filter_type: resource::SamplerFilterErrorType::MinFilter, + filter_mode: desc.min_filter, + anisotropic_clamp: desc.anisotropy_clamp, + }, + ); } - if self - .downlevel - .flags - .contains(wgt::DownlevelFlags::ANISOTROPIC_FILTERING) - { - std::num::NonZeroU8::new(clamp) - } else { - None + if !matches!(desc.mag_filter, wgt::FilterMode::Linear) { + return Err( + resource::CreateSamplerError::InvalidFilterModeWithAnisotropy { + filter_type: resource::SamplerFilterErrorType::MagFilter, + filter_mode: desc.mag_filter, + anisotropic_clamp: desc.anisotropy_clamp, + }, + ); } - } else { - None - }; + if !matches!(desc.mipmap_filter, wgt::FilterMode::Linear) { + return Err( + resource::CreateSamplerError::InvalidFilterModeWithAnisotropy { + filter_type: resource::SamplerFilterErrorType::MipmapFilter, + filter_mode: desc.mipmap_filter, + anisotropic_clamp: desc.anisotropy_clamp, + }, + ); + } + } //TODO: check for wgt::DownlevelFlags::COMPARISON_SAMPLERS @@ -1350,9 +1366,9 @@ impl Device { mag_filter: desc.mag_filter, min_filter: desc.min_filter, mipmap_filter: desc.mipmap_filter, - lod_clamp, + lod_clamp: desc.lod_min_clamp..desc.lod_max_clamp, compare: desc.compare, - anisotropy_clamp, + anisotropy_clamp: desc.anisotropy_clamp, border_color: desc.border_color, }; diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 648b7f9e7e..5f4fcf83f1 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -11,7 +11,7 @@ use crate::{ use smallvec::SmallVec; use thiserror::Error; -use std::{borrow::Borrow, num::NonZeroU8, ops::Range, ptr::NonNull}; +use std::{borrow::Borrow, ops::Range, ptr::NonNull}; /// The status code provided to the buffer mapping callback. /// @@ -689,8 +689,8 @@ pub struct SamplerDescriptor<'a> { pub lod_max_clamp: f32, /// If this is enabled, this is a comparison sampler using the given comparison function. pub compare: Option, - /// Valid values: 1, 2, 4, 8, and 16. - pub anisotropy_clamp: Option, + /// Valid values between 1 and 16 inclusive. If this is not 1.0, all filter modes must be linear. + pub anisotropy_clamp: f32, /// Border color to use when address_mode is /// [`AddressMode::ClampToBorder`](wgt::AddressMode::ClampToBorder) pub border_color: Option, @@ -707,7 +707,7 @@ impl Default for SamplerDescriptor<'_> { lod_min_clamp: 0.0, lod_max_clamp: std::f32::MAX, compare: None, - anisotropy_clamp: None, + anisotropy_clamp: 1.0, border_color: None, } } @@ -724,14 +724,42 @@ pub struct Sampler { pub(crate) filtering: bool, } +#[derive(Copy, Clone)] +pub enum SamplerFilterErrorType { + MagFilter, + MinFilter, + MipmapFilter, +} + +impl std::fmt::Debug for SamplerFilterErrorType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match *self { + SamplerFilterErrorType::MagFilter => write!(f, "magFilter"), + SamplerFilterErrorType::MinFilter => write!(f, "minFilter"), + SamplerFilterErrorType::MipmapFilter => write!(f, "mipmapFilter"), + } + } +} + #[derive(Clone, Debug, Error)] pub enum CreateSamplerError { #[error(transparent)] Device(#[from] DeviceError), - #[error("Invalid lod clamp lod_min_clamp:{} lod_max_clamp:{}, must satisfy lod_min_clamp >= 0 and lod_max_clamp >= lod_min_clamp ", .0.start, .0.end)] - InvalidLodClamp(Range), - #[error("Invalid anisotropic clamp {0}, must be one of 1, 2, 4, 8 or 16")] - InvalidClamp(u8), + #[error("Invalid lodMinClamp: {0}. Must be greater or equal to 0.0")] + InvalidLodMinClamp(f32), + #[error("Invalid lodMaxClamp: {lod_max_clamp}. Must be greater or equal to lodMinClamp (which is {lod_min_clamp}).")] + InvalidLodMaxClamp { + lod_min_clamp: f32, + lod_max_clamp: f32, + }, + #[error("Invalid anisotropic clamp: {0}. Must be in the range 1 to 16 inclusive.")] + InvalidAnisotropy(f32), + #[error("Invalid filter mode for {filter_type:?}: {filter_mode:?}. When anistropic clamp is not 1.0 (it is {anisotropic_clamp}), all filter modes must be linear.")] + InvalidFilterModeWithAnisotropy { + filter_type: SamplerFilterErrorType, + filter_mode: wgt::FilterMode, + anisotropic_clamp: f32, + }, #[error("Cannot create any more samplers")] TooManyObjects, /// AddressMode::ClampToBorder requires feature ADDRESS_MODE_CLAMP_TO_BORDER. diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index b4f25c9179..d70908fc6c 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -355,9 +355,9 @@ impl Example { mag_filter: wgt::FilterMode::Linear, min_filter: wgt::FilterMode::Nearest, mipmap_filter: wgt::FilterMode::Nearest, - lod_clamp: None, + lod_clamp: 0.0..32.0, compare: None, - anisotropy_clamp: None, + anisotropy_clamp: 1.0, border_color: None, }; let sampler = unsafe { device.create_sampler(&sampler_desc).unwrap() }; diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index 24fea55663..16b9918f38 100644 --- a/wgpu-hal/src/dx12/device.rs +++ b/wgpu-hal/src/dx12/device.rs @@ -583,13 +583,14 @@ impl crate::Device for super::Device { Some(_) => d3d12_ty::D3D12_FILTER_REDUCTION_TYPE_COMPARISON, None => d3d12_ty::D3D12_FILTER_REDUCTION_TYPE_STANDARD, }; - let filter = conv::map_filter_mode(desc.min_filter) << d3d12_ty::D3D12_MIN_FILTER_SHIFT + let mut filter = conv::map_filter_mode(desc.min_filter) << d3d12_ty::D3D12_MIN_FILTER_SHIFT | conv::map_filter_mode(desc.mag_filter) << d3d12_ty::D3D12_MAG_FILTER_SHIFT | conv::map_filter_mode(desc.mipmap_filter) << d3d12_ty::D3D12_MIP_FILTER_SHIFT - | reduction << d3d12_ty::D3D12_FILTER_REDUCTION_TYPE_SHIFT - | desc - .anisotropy_clamp - .map_or(0, |_| d3d12_ty::D3D12_FILTER_ANISOTROPIC); + | reduction << d3d12_ty::D3D12_FILTER_REDUCTION_TYPE_SHIFT; + + if desc.anisotropy_clamp != 1.0 { + filter |= d3d12_ty::D3D12_FILTER_ANISOTROPIC; + }; let border_color = conv::map_border_color(desc.border_color); @@ -602,10 +603,10 @@ impl crate::Device for super::Device { conv::map_address_mode(desc.address_modes[2]), ], 0.0, - desc.anisotropy_clamp.map_or(0, |aniso| aniso.get() as u32), + desc.anisotropy_clamp as u32, conv::map_comparison(desc.compare.unwrap_or(wgt::CompareFunction::Always)), border_color, - desc.lod_clamp.clone().unwrap_or(0.0..16.0), + desc.lod_clamp.clone(), ); Ok(super::Sampler { handle }) diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index d994aa1d56..208fd0a9bc 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -864,16 +864,12 @@ impl crate::Device for super::Device { unsafe { gl.sampler_parameter_f32_slice(raw, glow::TEXTURE_BORDER_COLOR, &border) }; } - if let Some(ref range) = desc.lod_clamp { - unsafe { gl.sampler_parameter_f32(raw, glow::TEXTURE_MIN_LOD, range.start) }; - unsafe { gl.sampler_parameter_f32(raw, glow::TEXTURE_MAX_LOD, range.end) }; - } + unsafe { gl.sampler_parameter_f32(raw, glow::TEXTURE_MIN_LOD, desc.lod_clamp.start) }; + unsafe { gl.sampler_parameter_f32(raw, glow::TEXTURE_MAX_LOD, desc.lod_clamp.end) }; - if let Some(anisotropy) = desc.anisotropy_clamp { - unsafe { - gl.sampler_parameter_i32(raw, glow::TEXTURE_MAX_ANISOTROPY, anisotropy.get() as i32) - }; - } + unsafe { + gl.sampler_parameter_f32(raw, glow::TEXTURE_MAX_ANISOTROPY, desc.anisotropy_clamp) + }; //set_param_float(glow::TEXTURE_LOD_BIAS, info.lod_bias.0); diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 814c451f06..bafa4d8427 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -87,7 +87,7 @@ pub mod api { use std::{ borrow::{Borrow, Cow}, fmt, - num::{NonZeroU32, NonZeroU8}, + num::NonZeroU32, ops::{Range, RangeInclusive}, ptr::NonNull, sync::atomic::AtomicBool, @@ -919,9 +919,10 @@ pub struct SamplerDescriptor<'a> { pub mag_filter: wgt::FilterMode, pub min_filter: wgt::FilterMode, pub mipmap_filter: wgt::FilterMode, - pub lod_clamp: Option>, + pub lod_clamp: Range, pub compare: Option, - pub anisotropy_clamp: Option, + // Must be in the range 1.0 to 16.0 inclusive. Anisotropic filtering must be supported if this is not 1.0. + pub anisotropy_clamp: f32, pub border_color: Option, } diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index 6084a7164a..48a87757bd 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -415,6 +415,9 @@ impl crate::Device for super::Device { descriptor.set_min_filter(conv::map_filter_mode(desc.min_filter)); descriptor.set_mag_filter(conv::map_filter_mode(desc.mag_filter)); descriptor.set_mip_filter(match desc.mipmap_filter { + wgt::FilterMode::Nearest if desc.lod_clamp == (0.0..0.0) => { + metal::MTLSamplerMipFilter::NotMipmapped + } wgt::FilterMode::Nearest => metal::MTLSamplerMipFilter::Nearest, wgt::FilterMode::Linear => metal::MTLSamplerMipFilter::Linear, }); @@ -424,14 +427,10 @@ impl crate::Device for super::Device { descriptor.set_address_mode_t(conv::map_address_mode(t)); descriptor.set_address_mode_r(conv::map_address_mode(r)); - if let Some(aniso) = desc.anisotropy_clamp { - descriptor.set_max_anisotropy(aniso.get() as _); - } + descriptor.set_max_anisotropy(desc.anisotropy_clamp as _); - if let Some(ref range) = desc.lod_clamp { - descriptor.set_lod_min_clamp(range.start); - descriptor.set_lod_max_clamp(range.end); - } + descriptor.set_lod_min_clamp(desc.lod_clamp.start); + descriptor.set_lod_max_clamp(desc.lod_clamp.end); if let Some(fun) = desc.compare { descriptor.set_compare_function(conv::map_compare_function(fun)); diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index 1d10d69b0a..b476ad7d75 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -1103,8 +1103,6 @@ impl crate::Device for super::Device { &self, desc: &crate::SamplerDescriptor, ) -> Result { - let lod_range = desc.lod_clamp.clone().unwrap_or(0.0..16.0); - let mut vk_info = vk::SamplerCreateInfo::builder() .flags(vk::SamplerCreateFlags::empty()) .mag_filter(conv::map_filter_mode(desc.mag_filter)) @@ -1113,8 +1111,8 @@ impl crate::Device for super::Device { .address_mode_u(conv::map_address_mode(desc.address_modes[0])) .address_mode_v(conv::map_address_mode(desc.address_modes[1])) .address_mode_w(conv::map_address_mode(desc.address_modes[2])) - .min_lod(lod_range.start) - .max_lod(lod_range.end); + .min_lod(desc.lod_clamp.start) + .max_lod(desc.lod_clamp.end); if let Some(fun) = desc.compare { vk_info = vk_info @@ -1122,16 +1120,14 @@ impl crate::Device for super::Device { .compare_op(conv::map_comparison(fun)); } - if let Some(aniso) = desc.anisotropy_clamp { - if self - .shared - .downlevel_flags - .contains(wgt::DownlevelFlags::ANISOTROPIC_FILTERING) - { - vk_info = vk_info - .anisotropy_enable(true) - .max_anisotropy(aniso.get() as f32); - } + if self + .shared + .downlevel_flags + .contains(wgt::DownlevelFlags::ANISOTROPIC_FILTERING) + { + vk_info = vk_info + .anisotropy_enable(true) + .max_anisotropy(desc.anisotropy_clamp); } if let Some(color) = desc.border_color { diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index 5d937db745..8d4efbe561 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -19,7 +19,7 @@ use std::{ fmt::{Debug, Display}, future::Future, marker::PhantomData, - num::{NonZeroU32, NonZeroU8}, + num::NonZeroU32, ops::{Bound, Deref, DerefMut, Range, RangeBounds}, sync::Arc, thread, @@ -1008,8 +1008,8 @@ pub struct SamplerDescriptor<'a> { pub lod_max_clamp: f32, /// If this is enabled, this is a comparison sampler using the given comparison function. pub compare: Option, - /// Valid values: 1, 2, 4, 8, and 16. - pub anisotropy_clamp: Option, + /// Valid values between 1 and 16 inclusive. If this is not 1.0, all filter modes must be linear. + pub anisotropy_clamp: f32, /// Border color to use when address_mode is [`AddressMode::ClampToBorder`] pub border_color: Option, } @@ -1028,7 +1028,7 @@ impl Default for SamplerDescriptor<'_> { lod_min_clamp: 0.0, lod_max_clamp: std::f32::MAX, compare: None, - anisotropy_clamp: None, + anisotropy_clamp: 1.0, border_color: None, } } From e94831448a3e1a6c0cbb580bce1622d18ffe3321 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Tue, 21 Mar 2023 11:37:23 -0400 Subject: [PATCH 4/6] Various aniso fixed --- CHANGELOG.md | 6 +++--- deno_webgpu/sampler.rs | 2 +- wgpu-core/src/device/mod.rs | 18 +++++++++++++++--- wgpu-core/src/resource.rs | 27 +++++---------------------- wgpu-hal/examples/halmark/main.rs | 2 +- wgpu-hal/src/dx12/device.rs | 2 +- wgpu-hal/src/gles/adapter.rs | 9 +++++---- wgpu-hal/src/gles/device.rs | 13 ++++++++++--- wgpu-hal/src/lib.rs | 6 ++++-- wgpu-hal/src/metal/device.rs | 1 + wgpu-hal/src/vulkan/adapter.rs | 6 ++++-- wgpu-hal/src/vulkan/device.rs | 10 ++++------ wgpu-hal/src/vulkan/mod.rs | 1 - wgpu/src/lib.rs | 8 ++++---- 14 files changed, 58 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8942f6d34e..7c6995d31b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,14 +84,14 @@ By @teoxoy in [#3534](https://github.com/gfx-rs/wgpu/pull/3534) #### Anisotropic Filtering -Anisotropic filtering has been brought in line with the spec. The anisotropic clamp is now a f32 which must be between 1.0 and 16.0 inclusive. +Anisotropic filtering has been brought in line with the spec. The anisotropic clamp is now a u16 (was a `Option`) which must be at least 1. -If the anisotropy clamp is not 1.0, all the filters in a sampler must be `Linear`. +If the anisotropy clamp is not 1, all the filters in a sampler must be `Linear`. ```diff SamplerDescriptor { - anisotropic_clamp: None, -+ anisotropic_clamp: 1.0, ++ anisotropic_clamp: 1, } ``` diff --git a/deno_webgpu/sampler.rs b/deno_webgpu/sampler.rs index 6a9fe285d3..d064ba2ebe 100644 --- a/deno_webgpu/sampler.rs +++ b/deno_webgpu/sampler.rs @@ -40,7 +40,7 @@ pub struct CreateSamplerArgs { lod_min_clamp: f32, lod_max_clamp: f32, compare: Option, - max_anisotropy: f32, + max_anisotropy: u16, } #[op] diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 3967d226da..f4413d1d3d 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -1322,13 +1322,13 @@ impl Device { }); } - if !(1.0..=16.0).contains(&desc.anisotropy_clamp) { + if desc.anisotropy_clamp < 1 { return Err(resource::CreateSamplerError::InvalidAnisotropy( desc.anisotropy_clamp, )); } - if desc.anisotropy_clamp != 1.0 { + if desc.anisotropy_clamp != 1 { if !matches!(desc.min_filter, wgt::FilterMode::Linear) { return Err( resource::CreateSamplerError::InvalidFilterModeWithAnisotropy { @@ -1358,6 +1358,18 @@ impl Device { } } + let anisotropy_clamp = if self + .downlevel + .flags + .contains(wgt::DownlevelFlags::ANISOTROPIC_FILTERING) + { + // Clamp anisotropy clamp to [1, 16] per the wgpu-hal interface + desc.anisotropy_clamp.min(16) + } else { + // If it isn't supported, set this unconditionally to 1 + 1 + }; + //TODO: check for wgt::DownlevelFlags::COMPARISON_SAMPLERS let hal_desc = hal::SamplerDescriptor { @@ -1368,7 +1380,7 @@ impl Device { mipmap_filter: desc.mipmap_filter, lod_clamp: desc.lod_min_clamp..desc.lod_max_clamp, compare: desc.compare, - anisotropy_clamp: desc.anisotropy_clamp, + anisotropy_clamp, border_color: desc.border_color, }; diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 5f4fcf83f1..ff04787548 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -689,30 +689,13 @@ pub struct SamplerDescriptor<'a> { pub lod_max_clamp: f32, /// If this is enabled, this is a comparison sampler using the given comparison function. pub compare: Option, - /// Valid values between 1 and 16 inclusive. If this is not 1.0, all filter modes must be linear. - pub anisotropy_clamp: f32, + /// Must be at least 1. If this is not 1, all filter modes must be linear. + pub anisotropy_clamp: u16, /// Border color to use when address_mode is /// [`AddressMode::ClampToBorder`](wgt::AddressMode::ClampToBorder) pub border_color: Option, } -impl Default for SamplerDescriptor<'_> { - fn default() -> Self { - Self { - label: None, - address_modes: Default::default(), - mag_filter: Default::default(), - min_filter: Default::default(), - mipmap_filter: Default::default(), - lod_min_clamp: 0.0, - lod_max_clamp: std::f32::MAX, - compare: None, - anisotropy_clamp: 1.0, - border_color: None, - } - } -} - #[derive(Debug)] pub struct Sampler { pub(crate) raw: A::Sampler, @@ -752,13 +735,13 @@ pub enum CreateSamplerError { lod_min_clamp: f32, lod_max_clamp: f32, }, - #[error("Invalid anisotropic clamp: {0}. Must be in the range 1 to 16 inclusive.")] - InvalidAnisotropy(f32), + #[error("Invalid anisotropic clamp: {0}. Must be at least 1.")] + InvalidAnisotropy(u16), #[error("Invalid filter mode for {filter_type:?}: {filter_mode:?}. When anistropic clamp is not 1.0 (it is {anisotropic_clamp}), all filter modes must be linear.")] InvalidFilterModeWithAnisotropy { filter_type: SamplerFilterErrorType, filter_mode: wgt::FilterMode, - anisotropic_clamp: f32, + anisotropic_clamp: u16, }, #[error("Cannot create any more samplers")] TooManyObjects, diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index d70908fc6c..2810d160b1 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -357,7 +357,7 @@ impl Example { mipmap_filter: wgt::FilterMode::Nearest, lod_clamp: 0.0..32.0, compare: None, - anisotropy_clamp: 1.0, + anisotropy_clamp: 1, border_color: None, }; let sampler = unsafe { device.create_sampler(&sampler_desc).unwrap() }; diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index 16b9918f38..7e14818572 100644 --- a/wgpu-hal/src/dx12/device.rs +++ b/wgpu-hal/src/dx12/device.rs @@ -588,7 +588,7 @@ impl crate::Device for super::Device { | conv::map_filter_mode(desc.mipmap_filter) << d3d12_ty::D3D12_MIP_FILTER_SHIFT | reduction << d3d12_ty::D3D12_FILTER_REDUCTION_TYPE_SHIFT; - if desc.anisotropy_clamp != 1.0 { + if desc.anisotropy_clamp != 1 { filter |= d3d12_ty::D3D12_FILTER_ANISOTROPIC; }; diff --git a/wgpu-hal/src/gles/adapter.rs b/wgpu-hal/src/gles/adapter.rs index 44beb4399c..46bba478f3 100644 --- a/wgpu-hal/src/gles/adapter.rs +++ b/wgpu-hal/src/gles/adapter.rs @@ -315,10 +315,11 @@ impl super::Adapter { && (vertex_shader_storage_blocks != 0 || vertex_ssbo_false_zero), ); downlevel_flags.set(wgt::DownlevelFlags::FRAGMENT_STORAGE, supports_storage); - downlevel_flags.set( - wgt::DownlevelFlags::ANISOTROPIC_FILTERING, - extensions.contains("EXT_texture_filter_anisotropic"), - ); + if extensions.contains("EXT_texture_filter_anisotropic") { + let max_aniso = + unsafe { gl.get_parameter_i32(glow::MAX_TEXTURE_MAX_ANISOTROPY_EXT) } as u32; + downlevel_flags.set(wgt::DownlevelFlags::ANISOTROPIC_FILTERING, max_aniso >= 16); + } downlevel_flags.set( wgt::DownlevelFlags::BUFFER_BINDINGS_NOT_16_BYTE_ALIGNED, !(cfg!(target_arch = "wasm32") || is_angle), diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index 208fd0a9bc..0a1cfaf241 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -867,9 +867,16 @@ impl crate::Device for super::Device { unsafe { gl.sampler_parameter_f32(raw, glow::TEXTURE_MIN_LOD, desc.lod_clamp.start) }; unsafe { gl.sampler_parameter_f32(raw, glow::TEXTURE_MAX_LOD, desc.lod_clamp.end) }; - unsafe { - gl.sampler_parameter_f32(raw, glow::TEXTURE_MAX_ANISOTROPY, desc.anisotropy_clamp) - }; + // If clamp is not 1, we know anisotropy is supported up to 16x + if desc.anisotropy_clamp != 1 { + unsafe { + gl.sampler_parameter_i32( + raw, + glow::TEXTURE_MAX_ANISOTROPY, + desc.anisotropy_clamp as i32, + ) + }; + } //set_param_float(glow::TEXTURE_LOD_BIAS, info.lod_bias.0); diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index bafa4d8427..adb5fdc773 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -921,8 +921,10 @@ pub struct SamplerDescriptor<'a> { pub mipmap_filter: wgt::FilterMode, pub lod_clamp: Range, pub compare: Option, - // Must be in the range 1.0 to 16.0 inclusive. Anisotropic filtering must be supported if this is not 1.0. - pub anisotropy_clamp: f32, + // Must in the range [1, 16]. + // + // Anisotropic filtering must be supported if this is not 1. + pub anisotropy_clamp: u16, pub border_color: Option, } diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index 48a87757bd..75c64989d3 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -427,6 +427,7 @@ impl crate::Device for super::Device { descriptor.set_address_mode_t(conv::map_address_mode(t)); descriptor.set_address_mode_r(conv::map_address_mode(r)); + // Anisotropy is always supported on mac up to 16x descriptor.set_max_anisotropy(desc.anisotropy_clamp as _); descriptor.set_lod_min_clamp(desc.lod_clamp.start); diff --git a/wgpu-hal/src/vulkan/adapter.rs b/wgpu-hal/src/vulkan/adapter.rs index 5efeed35e3..6a281077f7 100644 --- a/wgpu-hal/src/vulkan/adapter.rs +++ b/wgpu-hal/src/vulkan/adapter.rs @@ -345,7 +345,10 @@ impl PhysicalDeviceFeatures { caps.supports_extension(vk::KhrSwapchainMutableFormatFn::name()), ); dl_flags.set(Df::CUBE_ARRAY_TEXTURES, self.core.image_cube_array != 0); - dl_flags.set(Df::ANISOTROPIC_FILTERING, self.core.sampler_anisotropy != 0); + dl_flags.set( + Df::ANISOTROPIC_FILTERING, + self.core.sampler_anisotropy >= 16, + ); dl_flags.set( Df::FRAGMENT_WRITABLE_STORAGE, self.core.fragment_stores_and_atomics != 0, @@ -1320,7 +1323,6 @@ impl super::Adapter { }, vendor_id: self.phd_capabilities.properties.vendor_id, timestamp_period: self.phd_capabilities.properties.limits.timestamp_period, - downlevel_flags: self.downlevel_flags, private_caps: self.private_caps.clone(), workarounds: self.workarounds, render_passes: Mutex::new(Default::default()), diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index b476ad7d75..2421572a5e 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -1120,14 +1120,12 @@ impl crate::Device for super::Device { .compare_op(conv::map_comparison(fun)); } - if self - .shared - .downlevel_flags - .contains(wgt::DownlevelFlags::ANISOTROPIC_FILTERING) - { + if desc.anisotropy_clamp != 1 { + // We only enable the downlevel flag if supports 16x anisotropy, + // and wgpu-hal interface guarentees the clamp is in the range [1, 16] vk_info = vk_info .anisotropy_enable(true) - .max_anisotropy(desc.anisotropy_clamp); + .max_anisotropy(desc.anisotropy_clamp as f32); } if let Some(color) = desc.border_color { diff --git a/wgpu-hal/src/vulkan/mod.rs b/wgpu-hal/src/vulkan/mod.rs index af322e0ee8..fdee547973 100644 --- a/wgpu-hal/src/vulkan/mod.rs +++ b/wgpu-hal/src/vulkan/mod.rs @@ -249,7 +249,6 @@ struct DeviceShared { extension_fns: DeviceExtensionFunctions, vendor_id: u32, timestamp_period: f32, - downlevel_flags: wgt::DownlevelFlags, private_caps: PrivateCapabilities, workarounds: Workarounds, render_passes: Mutex>, diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index 8d4efbe561..ee92091f01 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -1008,8 +1008,8 @@ pub struct SamplerDescriptor<'a> { pub lod_max_clamp: f32, /// If this is enabled, this is a comparison sampler using the given comparison function. pub compare: Option, - /// Valid values between 1 and 16 inclusive. If this is not 1.0, all filter modes must be linear. - pub anisotropy_clamp: f32, + /// Must be at least 1. If this is not 1, all filter modes must be linear. + pub anisotropy_clamp: u16, /// Border color to use when address_mode is [`AddressMode::ClampToBorder`] pub border_color: Option, } @@ -1026,9 +1026,9 @@ impl Default for SamplerDescriptor<'_> { min_filter: Default::default(), mipmap_filter: Default::default(), lod_min_clamp: 0.0, - lod_max_clamp: std::f32::MAX, + lod_max_clamp: 32.0, compare: None, - anisotropy_clamp: 1.0, + anisotropy_clamp: 1, border_color: None, } } From 00e795d88211ec9f991a9c857a88a5309c5326c5 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Tue, 21 Mar 2023 12:01:36 -0400 Subject: [PATCH 5/6] Someday I'll stop pushing commits to this damn PR --- CHANGELOG.md | 1 - wgpu-hal/src/vulkan/adapter.rs | 5 +---- wgpu-hal/src/vulkan/device.rs | 4 ++-- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c6995d31b..796bfce1f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -114,7 +114,6 @@ By @cwfitzgerald in [#3610](https://github.com/gfx-rs/wgpu/pull/3610). - Improve attachment related errors. By @cwfitzgerald in [#3549](https://github.com/gfx-rs/wgpu/pull/3549) - Make error descriptions all upper case. By @cwfitzgerald in [#3549](https://github.com/gfx-rs/wgpu/pull/3549) - Don't include ANSI terminal color escape sequences in shader module validation error messages. By @jimblandy in [#3591](https://github.com/gfx-rs/wgpu/pull/3591) -- Bring anisotropic filtering in line with the spec. #### WebGPU diff --git a/wgpu-hal/src/vulkan/adapter.rs b/wgpu-hal/src/vulkan/adapter.rs index 6a281077f7..ab07b7f854 100644 --- a/wgpu-hal/src/vulkan/adapter.rs +++ b/wgpu-hal/src/vulkan/adapter.rs @@ -345,10 +345,7 @@ impl PhysicalDeviceFeatures { caps.supports_extension(vk::KhrSwapchainMutableFormatFn::name()), ); dl_flags.set(Df::CUBE_ARRAY_TEXTURES, self.core.image_cube_array != 0); - dl_flags.set( - Df::ANISOTROPIC_FILTERING, - self.core.sampler_anisotropy >= 16, - ); + dl_flags.set(Df::ANISOTROPIC_FILTERING, self.core.sampler_anisotropy != 0); dl_flags.set( Df::FRAGMENT_WRITABLE_STORAGE, self.core.fragment_stores_and_atomics != 0, diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index 2421572a5e..09b887772c 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -1121,8 +1121,8 @@ impl crate::Device for super::Device { } if desc.anisotropy_clamp != 1 { - // We only enable the downlevel flag if supports 16x anisotropy, - // and wgpu-hal interface guarentees the clamp is in the range [1, 16] + // We only enable anisotropy if it is supported, and wgpu-hal interface guarentees + // the clamp is in the range [1, 16] which is always supported if anisotropy is. vk_info = vk_info .anisotropy_enable(true) .max_anisotropy(desc.anisotropy_clamp as f32); From ed39d8f190e0d27737c55b64ecaf7dc03695d842 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Tue, 21 Mar 2023 12:27:51 -0400 Subject: [PATCH 6/6] Update wgpu-core/src/resource.rs --- wgpu-core/src/resource.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index ff04787548..9dbf1b3357 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -737,7 +737,7 @@ pub enum CreateSamplerError { }, #[error("Invalid anisotropic clamp: {0}. Must be at least 1.")] InvalidAnisotropy(u16), - #[error("Invalid filter mode for {filter_type:?}: {filter_mode:?}. When anistropic clamp is not 1.0 (it is {anisotropic_clamp}), all filter modes must be linear.")] + #[error("Invalid filter mode for {filter_type:?}: {filter_mode:?}. When anistropic clamp is not 1 (it is {anisotropic_clamp}), all filter modes must be linear.")] InvalidFilterModeWithAnisotropy { filter_type: SamplerFilterErrorType, filter_mode: wgt::FilterMode,