diff --git a/CHANGELOG.md b/CHANGELOG.md index de634e06a6..796bfce1f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Bottom level categories: --> ## Unreleased + ### Major changes #### TextureFormat info API @@ -81,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 u16 (was a `Option`) which must be at least 1. + +If the anisotropy clamp is not 1, all the filters in a sampler must be `Linear`. + +```diff +SamplerDescriptor { +- anisotropic_clamp: None, ++ anisotropic_clamp: 1, +} +``` + +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) @@ -113,6 +129,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 shader samples different Lods in the same quad. 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) diff --git a/deno_webgpu/sampler.rs b/deno_webgpu/sampler.rs index e5f230b2dd..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: u8, + max_anisotropy: u16, } #[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..f4413d1d3d 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -1310,36 +1310,64 @@ 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 desc.anisotropy_clamp < 1 { + 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 { + 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, + }, + ); + } + 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, + }, + ); } + } + + 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 { - None + // If it isn't supported, set this unconditionally to 1 + 1 }; //TODO: check for wgt::DownlevelFlags::COMPARISON_SAMPLERS @@ -1350,7 +1378,7 @@ 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, border_color: desc.border_color, diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 648b7f9e7e..9dbf1b3357 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,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: 1, 2, 4, 8, and 16. - pub anisotropy_clamp: Option, + /// 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: None, - border_color: None, - } - } -} - #[derive(Debug)] pub struct Sampler { pub(crate) raw: A::Sampler, @@ -724,14 +707,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 at least 1.")] + InvalidAnisotropy(u16), + #[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, + anisotropic_clamp: u16, + }, #[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..2810d160b1 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, 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..7e14818572 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 { + 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/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 d994aa1d56..0a1cfaf241 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -864,14 +864,17 @@ 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 { + // 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, anisotropy.get() as i32) + gl.sampler_parameter_i32( + raw, + glow::TEXTURE_MAX_ANISOTROPY, + desc.anisotropy_clamp as i32, + ) }; } diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 814c451f06..adb5fdc773 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,12 @@ 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 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/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..75c64989d3 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -409,14 +409,13 @@ 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() => { + wgt::FilterMode::Nearest if desc.lod_clamp == (0.0..0.0) => { metal::MTLSamplerMipFilter::NotMipmapped } wgt::FilterMode::Nearest => metal::MTLSamplerMipFilter::Nearest, @@ -428,18 +427,11 @@ 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 _); - } - - if let Some(ref range) = desc.lod_clamp { - descriptor.set_lod_min_clamp(range.start); - descriptor.set_lod_max_clamp(range.end); - } + // Anisotropy is always supported on mac up to 16x + descriptor.set_max_anisotropy(desc.anisotropy_clamp as _); - if caps.sampler_lod_average { - descriptor.set_lod_average(true); // optimization - } + 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/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, diff --git a/wgpu-hal/src/vulkan/adapter.rs b/wgpu-hal/src/vulkan/adapter.rs index 5efeed35e3..ab07b7f854 100644 --- a/wgpu-hal/src/vulkan/adapter.rs +++ b/wgpu-hal/src/vulkan/adapter.rs @@ -1320,7 +1320,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 1d10d69b0a..09b887772c 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,12 @@ 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 desc.anisotropy_clamp != 1 { + // 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); } 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 5d937db745..ee92091f01 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, + /// 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: None, + anisotropy_clamp: 1, border_color: None, } }