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

Fix Metal Mipmap Behvior #3610

Merged
merged 6 commits into from
Mar 21, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<u8>`) 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,
}
```

Expand Down
2 changes: 1 addition & 1 deletion deno_webgpu/sampler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub struct CreateSamplerArgs {
lod_min_clamp: f32,
lod_max_clamp: f32,
compare: Option<wgpu_types::CompareFunction>,
max_anisotropy: f32,
max_anisotropy: u16,
}

#[op]
Expand Down
18 changes: 15 additions & 3 deletions wgpu-core/src/device/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1322,13 +1322,13 @@ impl<A: HalApi> Device<A> {
});
}

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 {
Expand Down Expand Up @@ -1358,6 +1358,18 @@ impl<A: HalApi> Device<A> {
}
}

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 {
Expand All @@ -1368,7 +1380,7 @@ impl<A: HalApi> Device<A> {
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,
};

Expand Down
27 changes: 5 additions & 22 deletions wgpu-core/src/resource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 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<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: 1.0,
border_color: None,
}
}
}

#[derive(Debug)]
pub struct Sampler<A: hal::Api> {
pub(crate) raw: A::Sampler,
Expand Down Expand Up @@ -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.")]
cwfitzgerald marked this conversation as resolved.
Show resolved Hide resolved
InvalidFilterModeWithAnisotropy {
filter_type: SamplerFilterErrorType,
filter_mode: wgt::FilterMode,
anisotropic_clamp: f32,
anisotropic_clamp: u16,
},
#[error("Cannot create any more samplers")]
TooManyObjects,
Expand Down
2 changes: 1 addition & 1 deletion wgpu-hal/examples/halmark/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ impl<A: hal::Api> Example<A> {
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() };
Expand Down
2 changes: 1 addition & 1 deletion wgpu-hal/src/dx12/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ impl crate::Device<super::Api> 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;
};

Expand Down
9 changes: 5 additions & 4 deletions wgpu-hal/src/gles/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
13 changes: 10 additions & 3 deletions wgpu-hal/src/gles/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -867,9 +867,16 @@ impl crate::Device<super::Api> 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);

Expand Down
6 changes: 4 additions & 2 deletions wgpu-hal/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -921,8 +921,10 @@ pub struct SamplerDescriptor<'a> {
pub mipmap_filter: wgt::FilterMode,
pub lod_clamp: Range<f32>,
pub compare: Option<wgt::CompareFunction>,
// 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<wgt::SamplerBorderColor>,
}

Expand Down
1 change: 1 addition & 0 deletions wgpu-hal/src/metal/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ 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));

// 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);
Expand Down
6 changes: 4 additions & 2 deletions wgpu-hal/src/vulkan/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
cwfitzgerald marked this conversation as resolved.
Show resolved Hide resolved
dl_flags.set(
Df::FRAGMENT_WRITABLE_STORAGE,
self.core.fragment_stores_and_atomics != 0,
Expand Down Expand Up @@ -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()),
Expand Down
10 changes: 4 additions & 6 deletions wgpu-hal/src/vulkan/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1120,14 +1120,12 @@ impl crate::Device<super::Api> 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,
cwfitzgerald marked this conversation as resolved.
Show resolved Hide resolved
// 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 {
Expand Down
1 change: 0 additions & 1 deletion wgpu-hal/src/vulkan/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<rustc_hash::FxHashMap<RenderPassKey, vk::RenderPass>>,
Expand Down
8 changes: 4 additions & 4 deletions wgpu/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<CompareFunction>,
/// 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<SamplerBorderColor>,
}
Expand All @@ -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,
}
}
Expand Down