-
Notifications
You must be signed in to change notification settings - Fork 475
Adsk Contrib - Vulkan support #2176
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
base: main
Are you sure you want to change the base?
Adsk Contrib - Vulkan support #2176
Conversation
As requested, here is my feedback on the PR (forked the branch to check the code).
so for direct3d an additional samplerBindingStart is required. Also note that registers in Direct3d are zero indexed
There is another function in GpuShaderCreator called :
This is off by default and keeps explicit implementations to use the default "automatic" bindings" (eg : current implementation) , and use explicit as an Opt-In.
becomes :
It would require a new enum in that case GPU_LANGUAGE_HLSL_SM_5_1 But given that it would not break the GpuCreator interface, this could be implemented at a later stage with no risk of breaking existing implementations. |
src/OpenColorIO/GpuShaderUtils.cpp
Outdated
void GpuShaderText::declareUniformFloat(const std::string & uniformName) | ||
{ | ||
newLine() << (m_lang == GPU_LANGUAGE_MSL_2_0 ? "" : "uniform ") << floatKeyword() << " " << uniformName << ";"; | ||
std::string uniformDeclString("uniform"); |
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.
Very minor but should we add the space here and other declaration methods to avoid having declarations beginning with a white space like we see in the 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.
That should work. I agree that the white space at the beginning is not so nice, I'll try to fix it before the 2.5 release.
e3260da
to
c80e919
Compare
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
…tart index. Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Co-authored-by: Aleksi Sapon <[email protected]> Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
Signed-off-by: Michael Horsch <[email protected]>
c80e919
to
77756ec
Compare
OCIO_CHECK_EQUAL(shaderDesc->getDescriptorSetIndex(), 123); | ||
OCIO_CHECK_EQUAL(shaderDesc->getTextureBindingStart(), 456); | ||
|
||
auto getSize = []() -> float { return 2; }; //simulate only 2 elements in the array |
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.
This should fix the broken Windows build.
auto getSize = []() -> float { return 2; }; //simulate only 2 elements in the array | |
auto getSize = []() -> int { return 2; }; //simulate only 2 elements in the array |
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.
Just needs the broken Windows unit tests fixed. Otherwise LGTM!
This PR adds Vulkan support to OCIO.