OpenColorIO
OpenColorIO copied to clipboard
Fix for "static const" literals in HLSL
Constant globals in HLSL should be static const
and not just const
. Without it, we get the following warnings as an example:
OpenColorIOTransformShader.ush:10:11: warning: Initializer of external global will be ignored
const int Ocio_grading_rgbcurve_knotsOffsets_0[8] = {-1, 0, -1, 0, -1, 0, 0, 9};
See WAR_CONST_INITIALIZER
(3207) in the following:
https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/hlsl-errors-and-warnings
I have not extensively tested this suggested change yet, but sharing as a conversation starter. The gpu tests seem to be centered around OpenGL only.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: num3ric / name: Éric Renaud-Houde (586a4feca4bbbdafeb0f6f683749c73adb2ddcbd)
Thanks for the PR @num3ric, it looks good to me even though like you said, we don't have DX unit test so we probably need someone to test it manually, would you be able to do that? Meanwhile it would be nice if you could fix the CLA issue by following the link from the above comment so that we can approve the PR when ready.
@remia The CLA is now applied so we should be good there. Regarding testing, I have been testing this change (manually) with a large number of transforms - so far so good. Let me know if there's anything in particular you think should be tested. I haven't had the chance to port automated tests to DX11/12 yet however.
Actually, we do need to confirm this works with both dynamic properties enabled/disabled. My current tests only covered them disabled. Will do so and report back.
Force-fixed the DCO email mismatch error in the previous commit, so this should be good for approval now.
I'm still working on building an independent DX app to fully test this on the side, but with minimal testing all looks good so far. Reading through various operators, the const functions aren't in use when dynamic properties are enabled.
@num3ric , you mentioned above that you were going to build an independent DX app to help testing. I was just wondering if that's something you could share/contribute? It could be quite helpful given the OpenGL limitation of our GPU CI testing.
@doug-walker I'll see what I can do! This is certainly something that would benefit us, so indeed worth pushing.
However what I have currently is a standalone ImGui-based DX12 app, which I built as a means of transferring Vulkan knowledge over to DX12 (skipping over DX11)... not something ready to publish at large just yet. Once I have GPU readback working, it will become more interesting for automated testing.
Thanks for the update, yes that sounds very interesting! Please keep us posted when you might have something to share or if we can help.