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 shaders and improve tooling #961

Open
wants to merge 91 commits into
base: main
Choose a base branch
from

Conversation

SaschaWillems
Copy link
Collaborator

@SaschaWillems SaschaWillems commented Mar 2, 2024

Description

With the framework now being able to consume shaders in SPIR-V format, this PR will start adding actual HLSL shaders to the repository for at least some samples. I'll be starting with adding HLSL shaders to my samples and maybe other not so complex samples.

✅ What this PR currently does:

  • Adds a way to specify a set of HLSL shaders for a sample similar to how GLSL shaders can be added to a sample (so they show up in the IDE)
  • Adds a new overload for the (hpp)api sample base class that handles toggling between glsl and hlsl
  • Adds an optional CMake argument for passing additional arguments to DXC (e.g. to enable a given extension) called DXC_ADDITIONAL_ARGUMENTS
  • Has some minor fixes for HLSL offline compilation introduced in Add offline compilation for HLSL shaders with usage of DXC #990
  • Restructures folders for shaders, so samples can have a set of shaders in a glsl sub folder and a hlsl sub folder
    • shaders\sample_name (e.g. ray_tracing_basic)
      • glsl
        • raygen.rgen
        • closesthit.rchit
        • miss.rmiss
      • hlsl
        • raygen.rgen.hlsl
        • closesthit.rchit.hlsl
        • miss.rmiss.hlsl
    • I'm adding .hlsl to the file names as that automatically gives you HLSL syntax highlighting in e.g. MSVC
    • The only downside to this is that we need to exclude these files from the build process, or else MSVC want's to compile them with it's own DXC version for the D3D target
  • Adds some basic information on the shaders used in this repo incl. links to the HLSL chapters in the Vulkan Guide
  • Fixes some GLSL shaders to make mapping to HLSL possible/easier (e.g. HDR sample shaders)
  • Adds shaders to some samples where they were missing from the CMake file (and thus wouldn't show up in the IDE)
  • Adds a way to switch between GLSL and HLSL shaders via command line using a plugin
    • e.g. vulkan_samples ray_tracing_basic --shading-language hlsl
    • The sample base class defaults to glsl, so omitting above argument will try to load glsl shaders

📃List of samples that got HLSL shaders added

For a complete list of samples see #1014
This PR adds HLSL shaders (based on their GLSL counterparts) for the following samples:

  • calibrated_timestamp
  • color_write_enable
  • compute_nbody (+hpp variant)
  • conditional_rendering
  • conservative_rasterization
  • debug_utils
  • descriptor_buffer_basic
  • descriptor_indexing
  • dynamic_blending
  • dynamic_line_rasterization
  • dynamic_primitive_clipping
  • dynamic_rendering
  • dynamic_uniform_buffers (+hpp variant)
  • fragment_shading_rate
  • graphics_pipeline_library
  • hdr (+hpp variant)
  • instancing (+hpp variant)
  • logic_op_dynamic_state
  • memory_budget
  • mesh_shading (+hpp variant)
  • open_cl_interop (needs Update OpenCL interop samples to latest OpenCL spec #1021 to work)
  • open_gl_interop
  • patch_control_points
  • portability
  • profiles (can't test, see Profiles sample no longer working #1008)
  • push_descriptors
  • ray_tracing_basic
  • ray_tracing_extended
  • ray_queries
  • shader_debugprintf
  • separate_image_sampler (+hpp variant)
  • sparse_image
  • synchronization_2
  • terrain_tessellation (+hpp variant)
  • texture_compression_basisu
  • texture_loading (+hpp variant)
  • texture_mipmap_generation (+hpp variant)
  • timestamp_queries (+hpp variant)
  • vertex_dynamic_state

❌ What this won't do

💬Up for discussion

  • ✅ Offline compilation from Add offline compilation for HLSL shaders with usage of DXC #990 will result in .spv files generated at build-time. Should we add them to the repo, or exclude them?
    • As per the call on 2024-04-08 we decided to add the compiled .spv files, so people can test/run this without requiring DXC
  • How to handle if someone requests hlsl shaders but a sample doesn't have them? Should we just fail or fall back?
  • One thing that feels kinda odd now is that we also have a realtime shader selection plugin that was merged from another PR, which defines enums and code that now feels out of place. We should discuss if we want to remove that one. It's not used anywhere.

🚧Opens

  • Add some more documentation for maintainers on how to get HLSL shaders added to a sample
  • Due to the way DXC optimizes vertex inputs, some of the samples display a validation layer performance message for that. It's not critical and nothing that should stop merging us this PR

Fixes #892
Fixes #179

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

Stop-gap until we have added a more convenient way of compiling HLSL
MSVC automatically compiles any HLSL shader using it's built-in DXC version with DX syntax, which obviously fails for VK HLSL shaders
@SaschaWillems SaschaWillems added the hlsl Everything related to getting HLSL support added label Mar 2, 2024
@SaschaWillems
Copy link
Collaborator Author

Quick note: While adding HLSL shaders I noticed that several samples are broken on main or at least display validation errors. So when reviewing, most errors that show up are probably also visible on main. I didn't want to fix it in order not to pollute this PR.

@JoseEmilio-ARM JoseEmilio-ARM self-requested a review May 20, 2024 16:05
@JoseEmilio-ARM
Copy link
Collaborator

Quick note: While adding HLSL shaders I noticed that several samples are broken on main or at least display validation errors. So when reviewing, most errors that show up are probably also visible on main. I didn't want to fix it in order not to pollute this PR.

Tested most samples and found no issues, sorry for the delay. I did notice that Performance/WaitIdle does not launch on Android. I thought it was unrelated, but the same sample opens fine in #1051 , maybe it's an issue from a commit in between the two?

@SaschaWillems
Copy link
Collaborator Author

I did notice that Performance/WaitIdle does not launch on Android. I thought it was unrelated

That is indeed unrelated to this PR. That sample was broken on master for quite some time and was fixed by a different PR that I haven't merged. So it shouldn't matter for this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hlsl Everything related to getting HLSL support added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add HLSL shaders HLSL shader support
4 participants