Deno

gfx-rs · Mar 21, 2023 · 748f7fa · 748f7fa
1 parent 88f034b
commit 748f7fa
Show file tree

Hide file tree

Showing 11 changed files with 132 additions and 82 deletions.
diff --git 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<NonZeroU32>` to `Option<u32>`. 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
@@ -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: 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
@@ -1310,37 +1310,53 @@ 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 !(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<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,
+            anisotropy_clamp: desc.anisotropy_clamp,
             border_color: desc.border_color,
         };
 

diff --git 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<wgt::CompareFunction>,
-    /// Valid values: 1, 2, 4, 8, and 16.
-    pub anisotropy_clamp: Option<NonZeroU8>,
+    /// 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<wgt::SamplerBorderColor>,
@@ -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<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 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
@@ -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.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
@@ -587,9 +587,7 @@ impl crate::Device<super::Api> for super::Device {
             | 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);
+            | (desc.anisotropy_clamp != 1.0).then_some(d3d12_ty::D3D12_FILTER_ANISOTROPIC);
 
         let border_color = conv::map_border_color(desc.border_color);
 
@@ -602,10 +600,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 })

diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs
@@ -864,16 +864,12 @@ 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 {
-            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
@@ -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<Range<f32>>,
+    pub lod_clamp: Range<f32>,
     pub compare: Option<wgt::CompareFunction>,
-    pub anisotropy_clamp: Option<NonZeroU8>,
+    // 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<wgt::SamplerBorderColor>,
 }
 

diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs
@@ -415,6 +415,9 @@ impl crate::Device<super::Api> 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<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 _);
-            }
+            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
@@ -1103,8 +1103,6 @@ impl crate::Device<super::Api> for super::Device {
         &self,
         desc: &crate::SamplerDescriptor,
     ) -> Result<super::Sampler, crate::DeviceError> {
-        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,25 +1111,23 @@ impl crate::Device<super::Api> 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
                 .compare_enable(true)
                 .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 {