Fix Metal Mipmap Behvior (#3610)

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<u8>`) 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<NonZeroU32>` to `Option<u32>`. 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)

deno_webgpu/sampler.rs

@@ -40,7 +40,7 @@ pub struct CreateSamplerArgs {
  lod_min_clamp: f32,
  lod_max_clamp: f32,
  compare: Option<wgpu_types::CompareFunction>,
- 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
  };
 

wgpu-core/src/device/mod.rs

@@ -1310,36 +1310,64 @@ impl<A: HalApi> Device<A> {
  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<A: HalApi> Device<A> {
  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,

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<wgt::CompareFunction>,
- /// Valid values: 1, 2, 4, 8, and 16.
- pub anisotropy_clamp: Option<NonZeroU8>,
+ /// 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<wgt::SamplerBorderColor>,
 }
 
-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<A: hal::Api> {
  pub(crate) raw: A::Sampler,
@@ -724,14 +707,42 @@ pub struct Sampler<A: hal::Api> {
  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<f32>),
- #[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.

wgpu-hal/examples/halmark/main.rs

@@ -355,9 +355,9 @@ impl<A: hal::Api> Example<A> {
  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() };

wgpu-hal/src/dx12/device.rs

@@ -583,13 +583,14 @@ impl crate::Device<super::Api> 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<super::Api> 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 })

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),

wgpu-hal/src/gles/device.rs

@@ -864,14 +864,17 @@ impl crate::Device<super::Api> 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,
+ )
  };
  }
 

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<Range<f32>>,
+ pub lod_clamp: Range<f32>,
  pub compare: Option<wgt::CompareFunction>,
- pub anisotropy_clamp: Option<NonZeroU8>,
+ // Must in the range [1, 16].
+ //
+ // Anisotropic filtering must be supported if this is not 1.
+ pub anisotropy_clamp: u16,
  pub border_color: Option<wgt::SamplerBorderColor>,
 }
 

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),

wgpu-hal/src/metal/device.rs

@@ -423,14 +423,13 @@ impl crate::Device<super::Api> for super::Device {
  &self,
  desc: &crate::SamplerDescriptor,
  ) -> DeviceResult<super::Sampler> {
- 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,
@@ -442,18 +441,11 @@ impl crate::Device<super::Api> 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));