OpenColorIO icon indicating copy to clipboard operation
OpenColorIO copied to clipboard

Fix for "static const" literals in HLSL

Open num3ric opened this issue 2 years ago • 5 comments

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.

num3ric avatar Sep 01 '22 01:09 num3ric

CLA Signed

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 avatar Sep 05 '22 19:09 remia

@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.

num3ric avatar Sep 20 '22 13:09 num3ric

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.

num3ric avatar Sep 20 '22 13:09 num3ric

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 avatar Oct 06 '22 17:10 num3ric

@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 avatar Jan 04 '23 03:01 doug-walker

@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.

num3ric avatar Jan 12 '23 18:01 num3ric

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.

doug-walker avatar Jan 13 '23 03:01 doug-walker