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

Conversation

cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Mar 20, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description

Couple issues I found when debugging a problem:

  • The mipmap filter was set to NotMipmapped (which unconditionally samples mip 0) when the LodClamp was None. This is incorrect, LodClamp of None just means there's no restrictions on the Lod.
  • Mipmap Lod averaging was turned on. This has ramifications when neighboring pixels are texelFetch-ing texels of different mipmaps. This would potentially blend the mipmap levels in an unintended way.

Also resolves #3275.

Testing

Tested using rend3.

@cwfitzgerald cwfitzgerald added the PR: needs back-porting PR with a fix that needs to land on crates label Mar 20, 2023
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other things that don't look right with lod_clamp but could be addressed by a different PR.

  • lod_max_clamp needs to default to 32 (right now it defaults to f32::MAX)
  • lod_clamp is set to None if lod_max_clamp > 32 which doesn't seem right
  • dx12 and vulkan then map a lod_clamp: None to 0.0..16.0 which also doesn't seem right; I think we should make lod_clamp required at the hal level

CHANGELOG.md Outdated Show resolved Hide resolved
wgpu-hal/src/metal/device.rs Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member Author

K, went a little hog wild fixing up sampler stuff. I also brought aniso behavior inline with the spec and made the errors better.

@cwfitzgerald cwfitzgerald requested a review from teoxoy March 21, 2023 13:34
@cwfitzgerald cwfitzgerald force-pushed the fix-mipmap-behavior-metal branch 2 times, most recently from 13babec to 748f7fa Compare March 21, 2023 13:46
@cwfitzgerald cwfitzgerald requested a review from crowlKats as a code owner March 21, 2023 13:46
@cwfitzgerald cwfitzgerald force-pushed the fix-mipmap-behavior-metal branch from 748f7fa to 3c4ee19 Compare March 21, 2023 13:54
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LOD changes look good, thanks for addressing all my concerns in this PR!

I left a comment about the type and validation of anisotropy_clamp.
It also looks like this PR will fix/close #3275 🚀

wgpu/src/lib.rs Outdated Show resolved Hide resolved
@teoxoy
Copy link
Member

teoxoy commented Mar 21, 2023

Ah, actually one remaining thing regarding the LOD changes, could you also update the Default impl of the sampler descriptors? lod_max_clamp needs to default to 32 but it currently defaults to f32::MAX.

I'm not sure why we have 2 types of SamplerDescriptor (one in wgpu-core and another in wgpu), they seem to have the same-ish data.

@cwfitzgerald cwfitzgerald force-pushed the fix-mipmap-behavior-metal branch from d323eb9 to efd0c8b Compare March 21, 2023 15:42
wgpu-core/src/resource.rs Outdated Show resolved Hide resolved
@cwfitzgerald cwfitzgerald force-pushed the fix-mipmap-behavior-metal branch 2 times, most recently from 3d98183 to b2aab81 Compare March 21, 2023 15:53
@cwfitzgerald cwfitzgerald force-pushed the fix-mipmap-behavior-metal branch from b2aab81 to e948314 Compare March 21, 2023 16:00
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
@cwfitzgerald cwfitzgerald force-pushed the fix-mipmap-behavior-metal branch from 0222918 to 00e795d Compare March 21, 2023 16:14
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Someday I'll stop pushing commits to this damn PR

I think you can now :)

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) March 21, 2023 16:29
@cwfitzgerald cwfitzgerald merged commit 0c3ca5c into gfx-rs:master Mar 21, 2023
@cwfitzgerald cwfitzgerald deleted the fix-mipmap-behavior-metal branch March 21, 2023 18:15
cwfitzgerald added a commit to cwfitzgerald/wgpu that referenced this pull request Mar 22, 2023
@cwfitzgerald cwfitzgerald removed the PR: needs back-porting PR with a fix that needs to land on crates label Mar 22, 2023
cwfitzgerald added a commit to cwfitzgerald/wgpu that referenced this pull request Mar 22, 2023
cwfitzgerald added a commit that referenced this pull request Mar 22, 2023
Fix Metal Mipmap Behvior (#3610)
tychedelia added a commit to tychedelia/nannou that referenced this pull request Oct 10, 2023
zmitchell pushed a commit to zmitchell/splatter that referenced this pull request Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relax max_anisotropy validation
2 participants