Fix placeholder SMAA LUTs#24398
Conversation
With the placeholder SMAA LUTs in place (for context that's without the "smaa_luts" feature) you couldn't actually use them because backends like Vulkan will scream things like: Texture binding 2 expects dimension = D2, but given a view with dimension = D3 This is because the LUTs weren't generated within this crate, but re-uses the LUT placeholder function from the tonemapping crate. While that may work there, the SMAA LUTs aren't 3D (hence the mysterious third dimension entering the picture here.) I moved a copy of this function instead, modifying it to provide a 2D image instead.
|
Welcome, new contributor! Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨ |
kfc35
added
C-Bug
kfc35
left a comment
kfc35
left a comment
There was a problem hiding this comment.
I tested this by running the anti_aliasing example after removing the smaa_luts feature from the 3d_api feature temporarily in Cargo.toml. I am able to reproduce on main (switching to SMAA results in a crash with 2d/3d error message). Using the logic in this PR prevents it from crashing.
Instead of copying and pasting, you could instead modify the lut_placeholder function to take a TextureDimension argument, and have smaa use D2 while tonemapping uses D3. That might be personal preference though, since I don’t really like copy-paste unless I have to.
|
|
||
| let format = TextureFormat::Rgba8Unorm; | ||
| let data = vec![255, 0, 255, 255]; | ||
| let handle = images.add(Image { |
There was a problem hiding this comment.
If you want to keep the copy/paste and to help prevent people from asking “why isn’t this using lut_placeholder"
| let handle = images.add(Image { | |
| // using lut_placeholder() instead would result in warnings | |
| // the smaa placeholder needs to be a 2d texture | |
| let handle = images.add(Image { |
kfc35
added
S-Ready-For-Final-Review
I'll try that tomorrow, it did cross my mind but I didn't know if modifying that function made sense! |
beicause
left a comment
beicause
left a comment
There was a problem hiding this comment.
Instead of copying and pasting, you could instead modify the lut_placeholder function to take a TextureDimension argument, and have smaa use D2 while tonemapping uses D3. That might be personal preference though, since I don’t really like copy-paste unless I have to.
I don't think lut_placeholder function is meant to be reused. Creating Image here is OK as it is meant to be used for smaa.
| size: Extent3d::default(), | ||
| format, | ||
| dimension: TextureDimension::D2, | ||
| label: None, |
4 participants
Objective
With the placeholder SMAA LUTs in place (for context that's without the
smaa_lutsfeature) you couldn't actually use them because backends like Vulkan will scream things like:This is because the LUTs weren't generated within this crate, but re-uses the LUT placeholder function from the tonemapping crate. While that may work there, the SMAA LUTs aren't 3D (hence the mysterious third dimension entering the picture here.)
Solution
I moved a copy of this function instead, modifying it to provide a 2D image instead.
Testing
Enable the
bevy_anti_aliasfeature (while NOT enabling thesmaa_lutsone), and then configure yourCamerato add theSmaatype. I tested it on the Vulkan backend (Linux and Android) but I think the other ones fail too.