Implement texture-component-swizzle - #9553
Conversation
56c7392 to
a1c8647
Compare
texture-component-swizzletexture-component-swizzle on Vulkan
This comment was marked as resolved.
This comment was marked as resolved.
a1c8647 to
b397976
Compare
texture-component-swizzle on Vulkantexture-component-swizzle on Vulkan and DX12
9533b09 to
226392c
Compare
226392c to
64b2ed3
Compare
texture-component-swizzle on Vulkan and DX12texture-component-swizzle
…l texture without shader usage
9e12d46 to
9a25432
Compare
9a25432 to
0fb4f24
Compare
|
Agh same here, another long delay. Will also prioritize this PR early this week |
inner-daemons
left a comment
There was a problem hiding this comment.
I'm tired so not everything I wrote will be coherent or correct. Please ask if you are not sure what I mean anywhere.
This is a useful feature but there are some behavioral changes, particularly on metal, that need to be justified.
Sorry for the delay, hopefully my next review will be quicker.
| /// - Metal on Apple2+ or Mac2+ | ||
| /// | ||
| /// Not yet implemented: | ||
| /// - OpenGL |
There was a problem hiding this comment.
Can you note whether it would be possible on OpenGL
There was a problem hiding this comment.
OpenGL has glTexParameter GL_TEXTURE_SWIZZLE_*
There was a problem hiding this comment.
Ok, maybe file an issue and link it in the feature docs
There was a problem hiding this comment.
This remains to be done I think
| usage.intersects( | ||
| Tu::RESOURCE | Tu::DEPTH_STENCIL_READ | Tu::STORAGE_READ_ONLY | Tu::STORAGE_READ_WRITE, | ||
| ), | ||
| usage.intersects(Tu::RESOURCE | Tu::STORAGE_READ_ONLY | Tu::STORAGE_READ_WRITE), |
There was a problem hiding this comment.
This is a weird change, can you explain?
There was a problem hiding this comment.
This is needed by https://github.com/beicause/wgpu/blob/0fb4f24f52e432174057ddc63c9f6e3a3d63285e/wgpu-hal/src/metal/device.rs#L632-L635
I think DEPTH_STENCIL_READ doesn't require ShaderRead. Only DEPTH_STENCIL_READ|RESOURCE (RENDER_ATTACHMENT|TEXTURE_BINDING) together requires ShaderRead.
| at_descriptor.setTexture(Some(&at.target.view.attachment.texture)); | ||
| at_descriptor.setLevel(at.target.view.attachment.base_mip_level as _); | ||
| at_descriptor.setSlice(at.target.view.attachment.base_array_layer as _); |
There was a problem hiding this comment.
Why is this change necessary? It warrants some comments in code
There was a problem hiding this comment.
I commented in https://github.com/beicause/wgpu/blob/0fb4f24f52e432174057ddc63c9f6e3a3d63285e/wgpu-hal/src/metal/device.rs#L708-L709
When used as a texture binding, depth-stencil needs to use the r001 view, but when used as an attachment, depth-stencil cannot use the swizzled view because it's non-renderable. This is a problem if depth-stencil texture has both RENDER_ATTACHMENT and TEXTURE_BINDING, so we always use the original texture + setLevel/setSlice to avoid the non-renderable view.
inner-daemons
left a comment
There was a problem hiding this comment.
Vast majority of comments are the same renames. Im not super attached to the names I chose but I definitely think they are an improvement.
Nothing here that should be blocking.
Apologies again for the delay, lets try to get this in soon!
| } | ||
| } | ||
|
|
||
| fn map_texture_component_swizzle( |
There was a problem hiding this comment.
| fn map_texture_component_swizzle( | |
| fn map_all_texture_components_swizzle( |
There was a problem hiding this comment.
The function names are following the structs TextureComponentSwizzle ComponentSwizzle.
Perhaps the structs should be renamed, but I haven't thought of better names yet.
There was a problem hiding this comment.
Yes I think they should be renamed, since it is not immediatley clear which is which or why IMO.
There was a problem hiding this comment.
Current names is from webgpu.h:
WGPUTextureComponentSwizzle
WGPUComponentSwizzle
There was a problem hiding this comment.
Interesting. I wonder why they made that decision. Regardless, you can ignore all my comments on this topic
There was a problem hiding this comment.
The alternative names I thought of are TextureSwizzle + Swizzle which are shorter with "Component" omitted
But I'll wait for more feedback if we want to change them.
Edit: But webgpu already used texture_component_swizzle everywhere so probably we should stick to TextureComponentSwizzle?
| Ok(wgpu_types::TextureComponentSwizzle { r, g, b, a }) | ||
| } | ||
|
|
||
| fn map_component_swizzle( |
There was a problem hiding this comment.
| fn map_component_swizzle( | |
| fn map_texture_component_swizzle( |
| /// - Metal on Apple2+ or Mac2+ | ||
| /// | ||
| /// Not yet implemented: | ||
| /// - OpenGL |
There was a problem hiding this comment.
This remains to be done I think
|
Can you ping me or request a re-review when you think I should take another look? |
Co-authored-by: Inner Daemons <85136135+inner-daemons@users.noreply.github.com>
inner-daemons
left a comment
There was a problem hiding this comment.
Assuming you haven't changed anything major since my last review, approving. Sorry once again for the delay.
cef3286 to
33e3a16
Compare
Connections
Fixes #1028.
Description
Implementn texture component swizzle feature on vulkan, metal and dx12 (not implemented on gles)
Testing
I added texture component swizzle CTS
Squash or Rebase?
Squash
Checklist
wgpumay be affected behaviorally.CHANGELOG.mdentries for the user-facing effects of this change are present.