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

Add HLSL compilation support #2

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

chances
Copy link

@chances chances commented Nov 18, 2018

Adds a configurable SourceLanguage enum to make use of libshaderc's SetSourceLanguage(shaderc_source_language lang) when compiling HLSL shaders.

@chances chances changed the title Add HLSL support Add HLSL compilation support Nov 18, 2018
chances added a commit to chances/UtilityGrid that referenced this pull request Nov 18, 2018
chances added a commit to chances/UtilityGrid that referenced this pull request Nov 18, 2018
@mellinoe
Copy link
Collaborator

mellinoe commented Nov 19, 2018

This is a cool feature, I didn't realize that shaderc already supported HLSL compilation.

There's a few compilation errors in the current branch, but I was able to fix those locally and test it out. The tests are passing, but I haven't tested any of the output in Veldrid or anything like that. In order to merge this, we will need a few things:

  • Fix the compilation issues
  • Make sure there are no breaking changes in the public API (e.g. preserve all existing overloads, and no behavior changes in them)
  • (Potentially) Add a "SourceCompileOptions" base class for GlslCompileOptions so that you don't need to pass in a "GlslCompileOptions" to compile HLSL shaders.
  • In the test shaders, I'd like to use the [[vk::binding(x, y)]] syntax, since it doesn't seem like the SPIR-V output is particularly usable without that, especially in Veldrid.
  • Test the output in Veldrid. This will test the underlying native library more than the code here, but it will be good to verify that it actually works end-to-end.

@chances
Copy link
Author

chances commented Nov 21, 2018

@mellinoe I've resolved your first point. I'll work to add tests to cover point 2.

Regarding your fourth point, I'm a little lost about how I should map the : register(xX) resource bindings to the [[vk::binding(x, y)]] syntax. I'm not well versed in all of shader intricacies. 😬

What do x and y represent? What's different about the resource number in the register syntax and one given in the other syntax? (Google helped me with this. 😝 ) How do descriptor set and binding numbers map to Veldrid resource sets?

Regarding your point about testing the output in Veldrid, how should I go about doing that? Do you expect some kind of test rendering in the Tests project? Or is a separate proof-of-concept example project adequate?

@mellinoe
Copy link
Collaborator

How do descriptor set and binding numbers map to Veldrid resource sets?

Vulkan descriptor sets map pretty much 1-to-1 with Veldrid ResourceSets and ResourceLayouts. If a SPIR-V resource is has set = 0, binding = 1, then that resource will correspond to the second element (index 1) in the first ResourceLayout (index 0) for a Pipeline.

Do you expect some kind of test rendering in the Tests project?

Eventually, that's something that should definitely be tested in this repo, but currently there aren't any tests of that sort. At the very least we would need to do some basic validation that it works. IMO, the easiest way to test it out would be to just replace one of the shaders in NeoDemo with an HLSL variant. Everything in that project already runs through Veldrid.Spirv.

@chances
Copy link
Author

chances commented Jul 22, 2019

@mellinoe Is there anything else I should do to move along this PR?

@mellinoe
Copy link
Collaborator

@chances I took another look and compared this with a local branch I have for HLSL support. It looks good to me, and I will likely merge this in sometime soon unless I discover any big issues. The one thing missing that I may need to merge in from my version of this is support for different entry points.

How is the current code working, given that you don't pass the entry point around anywhere? This doesn't come up for GLSL at all because "main" is the only valid entry function name. In HLSL it's much more customary to use real, descriptive function names.

@chances
Copy link
Author

chances commented Dec 13, 2019

@mellinoe Regarding your question about how this is working with named entry points: I'm not sure yet if a shader compiled this way works because I've been working around this by precompiling my shaders to spirv with shaderc and specifying the named entry point in the Veldrid ShaderDescription.

What more is needed than setting the entry in a ShaderDescription?

@chances chances requested a review from mellinoe January 27, 2020 22:47
@Ramobo
Copy link

Ramobo commented May 14, 2020

@mellinoe Any news on this?

@KevinGliewe
Copy link

@mellinoe Please don't forget this PR. HLSL support would be such a awesome addition for this library and Veldrid in general.

@chances
Copy link
Author

chances commented Sep 18, 2022

@mellinoe Any update you can give regarding my question in this comment? I'd like to get this PR merged.

smoogipoo pushed a commit to smoogipoo/veldrid-spirv that referenced this pull request Aug 22, 2023
Fix index out of range with version replace
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.

4 participants