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

Add support for GL_EXT_texture_offset_non_const #3782

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rg3igalia
Copy link
Contributor

Closes #3765

@rg3igalia rg3igalia force-pushed the textureoffset_non_const branch 2 times, most recently from d198780 to 6fb8abf Compare October 29, 2024 14:18
@rg3igalia
Copy link
Contributor Author

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.

@arcady-lunarg arcady-lunarg self-requested a review November 4, 2024 16:35
Copy link
Contributor

@arcady-lunarg arcady-lunarg left a 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.

glslang/MachineIndependent/ParseHelper.cpp Outdated Show resolved Hide resolved
@rg3igalia
Copy link
Contributor Author

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!

There are a few checks that were missing for sparse variants of these
functions.
@rg3igalia
Copy link
Contributor Author

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?

@arcady-lunarg
Copy link
Contributor

We can wait until the SPIRV-Tools change is merged and the you can update the known_good.json to point to the new SPIRV-Tools commit.

@rg3igalia
Copy link
Contributor Author

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.

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.

Allow Offset in addition to ConstOffset for texture operations
2 participants