-
Notifications
You must be signed in to change notification settings - Fork 844
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
Add support for GL_EXT_texture_offset_non_const #3782
base: main
Are you sure you want to change the base?
Conversation
d198780
to
6fb8abf
Compare
I noticed the constness of the offset argument was not being checked with sparse texture functions, so I've added a commit on top with some additional code and tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these test cases be generating SPIR-V? I feel like that might be useful here, since ultimately this is an extension for use with Vulkan and SPIR-V.
Yes, it makes total sense. I wasn't aware of the distinction between the different test groups as I'm not familiar with glslang's code base. Thanks for the tip! |
6fb8abf
to
67acc4f
Compare
There are a few checks that were missing for sparse variants of these functions.
67acc4f
to
2dcce0b
Compare
I see the CI failures are related to the shaders failing validation and suggesting something like this: --- Test/baseResults/validation_fails.txt 2024-11-05 14:19:47.844071917 +0000
+++ - 2024-11-05 14:25:25.835244652 +0000
@@ -68,4 +68,6 @@
Test/baseResults/spv.separate.frag.out
Test/baseResults/spv.sparseTextureClamp.frag.out
Test/baseResults/spv.sparseTexture.frag.out
+Test/baseResults/spv.sparsetextureoffset_non_const.vert.out
+Test/baseResults/spv.textureoffset_non_const.vert.out
Test/baseResults/vk.relaxed.errorcheck.vert.out Which may be solved once the SPIR-V Tools issue is solved. So what should I do in the mean time? Update the file in this MR? |
We can wait until the SPIRV-Tools change is merged and the you can update the |
Alright, feel free to merge if you want in the mean time. The CTS tests using this are still under review so I'm not in a hurry. |
Closes #3765