-
Notifications
You must be signed in to change notification settings - Fork 483
Fix Vulkan texture binding index issue #2226
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 Vulkan texture binding index issue #2226
Conversation
Signed-off-by: Doug Walker <[email protected]>
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.
Pull request overview
This PR fixes a bug in Vulkan texture binding index calculation. Previously, binding indices were incorrectly calculated by counting only textures of the same dimension type (1D or 3D), when they should have been counting all textures regardless of type.
Key Changes:
- Updated texture index calculation to include both 1D and 3D textures in the count
- Added comprehensive test case validating correct binding index sequencing for mixed texture types
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/OpenColorIO/GpuShader.cpp | Fixed texture binding index calculation to count all texture types |
| tests/cpu/GpuShader_tests.cpp | Added test verifying correct Vulkan binding indices for multiple LUT types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
LGTM! |
cozdas
left a comment
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.
LGTM
|
Looks good but this way the index that is returned by addTexture() is a different one than the one that is passed to getTexture() and doesn't correspond to the array index of the texture anymore, this could be a problem. If you just get the number of textures and loop over them to pass them to Vulkan you might get wrong indices. For example, if you do something like this:
If the 3D and 1D/2D texture indices are mixed in the generated shader code, the above code will produce wrong values. You would need another function that returns for each texture the index that was returned when calling the addTexture() function. Something like |
src/OpenColorIO/GpuShader.cpp
Outdated
| unsigned textureIndex = static_cast<unsigned>(m_textures.size()) | ||
| + static_cast<unsigned>(m_textures3D.size()); | ||
| unsigned numDimensions = static_cast<unsigned>(dimensions); | ||
| Texture t(textureName, samplerName, width, height, 1, channel, numDimensions, interpolation, values); |
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.
To clarify my other comment a bit more:
First maybe rename textureIndex to something that fits better like textureShaderBindingIndex.
Extend Texture struct to also store the binding index Texture t(textureName, ..., textureShaderBindingIndex);
Then add methods to the public API to retrieve it uint getTexture[3D]ShaderBindingIndex(textureIndex) { return m_textures[textureIndex].textureShaderBindingIndex;}
This way users have all the information to properly bind textures to the correct indices.
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.
Thank you @michaelHADSK, that was very helpful. Please check if my latest commit does what you have in mind.
Note that the new getTextureShaderBindingIndex methods include the textureBindingStart. I'm considering having the index returned by the addTexture methods include that too, so that the code in the Ops doesn't have to worry about that. What do you think?
And I will add the new methods to the Python binding, once I have the C++ doing what you'd like.
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.
Yes, thanks, that's what I had in mind, approved it. Including the textureBindingStart sounds good to me. I'm not sure when I will be able to test the changes in our software but as far as I can see everything should work this way.
Signed-off-by: Doug Walker <[email protected]>
michaelHADSK
left a comment
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.
LGTM
Signed-off-by: Doug Walker <[email protected]>
Signed-off-by: Doug Walker <[email protected]>
* Fix texture binding index issue Signed-off-by: Doug Walker <[email protected]> * Add texture binding index getters Signed-off-by: Doug Walker <[email protected]> * Adjust addTexture return and fix Python binding Signed-off-by: Doug Walker <[email protected]> * Fix unit tests Signed-off-by: Doug Walker <[email protected]> --------- Signed-off-by: Doug Walker <[email protected]> (cherry picked from commit 9fea546) Signed-off-by: Doug Walker <[email protected]>
- Remove manual sampler declarations in buildShader(), use OCIO-generated ones - Configure shader descriptor with setDescriptorSetIndex(0, 2) for Vulkan to start texture bindings at 2 (after input/output storage buffers) - Use get3DTextureShaderBindingIndex() and getTextureShaderBindingIndex() for correct texture binding indices - Add GPU diagnostic output to verify MoltenVK uses actual GPU hardware This addresses review feedback from PR AcademySoftwareFoundation#2243 to use the new texture binding API added in OCIO 2.5.1 (PR AcademySoftwareFoundation#2226) instead of manually declaring samplers. Test results: 260/263 tests pass (98.9%) Remaining 3 failures are ACES2 precision-sensitive edge cases. GPU confirmed: Apple M2 Pro (Integrated GPU), not CPU emulation. Signed-off-by: pmady <[email protected]>
Addresses defect #2225.
The binding index was only taking into account the number of textures of a given kind (1D or 3D) rather than the total number of textures.