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

Fix for "static const" literals in HLSL #1684

Merged
merged 2 commits into from
Oct 20, 2022

Conversation

num3ric
Copy link
Contributor

@num3ric num3ric commented Sep 1, 2022

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.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 1, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: num3ric / name: Éric Renaud-Houde (586a4fe)

@num3ric num3ric changed the title HLSL WAR_CONST_INITIALIZER fix where literals are meant to be used. Fix for "static const" literals in HLSL Sep 5, 2022
@remia
Copy link
Collaborator

remia commented Sep 5, 2022

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.

@num3ric
Copy link
Contributor Author

num3ric commented Sep 20, 2022

@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
Copy link
Contributor Author

num3ric commented Sep 20, 2022

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
Copy link
Contributor Author

num3ric commented Oct 6, 2022

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.

@michdolan michdolan merged commit 281d39f into AcademySoftwareFoundation:main Oct 20, 2022
@doug-walker
Copy link
Collaborator

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

@num3ric
Copy link
Contributor Author

num3ric commented Jan 12, 2023

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

@doug-walker
Copy link
Collaborator

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.

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.

5 participants