NVIDIAGameWorks / RayTracingDenoiser

NVIDIA Ray Tracing Denoiser
Other
504 stars 46 forks source link

More Cmake Options for Compiling Shaders to Different Backends #63

Closed Sirtsu55 closed 1 year ago

Sirtsu55 commented 1 year ago

It would be nice to have more control over precompiled shaders and what backend they are compiled for. For example, not every Win32 application uses DXIL shaders or SPIR-V; therefore, having more control over what shaders are compiled would be great. Right now the assumption is that for Win32 application shaders are compiled to all backends, even though those aren't necessarily used.

I could add a PR addressing the issue/enhancement if that is okay. Thanks

dzhdanNV commented 1 year ago

Yes, create a PR, please. The logic behind the current solution is:

But if your request is easy to implement & support... why not?

Sirtsu55 commented 1 year ago

PR Opened: #64

dzhdanNV commented 1 year ago

Testing...

dzhdanNV commented 1 year ago

Thanks! Integrated! Note, that I fixed typos, auto-formatted the file and refactored a bit in my local version. My edited version will sneak on GitHub with the next update.

Logically, the next step would be to introduce more explicit:

NRD_HAS_SPIRV_SHADERS
NRD_HAS_DXBC_SHADERS
NRD_HAS_DXIL_SHADERS

instead of hard-to-follow-what's-going-on-here:

NRD_USE_PRECOMPILED_SHADERS
NRD_PRECOMPILE_SPIRV_SHADERS
NRD_PRECOMPILE_D3D_SHADERS
NRD_ONLY_SPIRV_SHADERS_AVAILABLE

Let me know what you think...

Sirtsu55 commented 1 year ago

Yes, sounds great! I'm a bit unfamiliar with the codebase so I opted to just separate into D3D and SPIR-V. The specifics would be a great addition. Let's close the issue once you add the new options.

dzhdanNV commented 1 year ago

@Sirtsu55 Implemented! Please, review this Cmake file (I also made a ton of changes in cpp files, but you definitely don't want to see them): CMakeLists.txt

dzhdanNV commented 1 year ago

Reworked a bit:

option (NRD_EMBEDS_SPIRV_SHADERS "NRD embeds SPIRV shaders" ON)
option (NRD_EMBEDS_DXIL_SHADERS "NRD embeds DXIL shaders" ${IS_WIN})
option (NRD_EMBEDS_DXBC_SHADERS "NRD embeds DXBC shaders" ${IS_WIN})
option (NRD_DISABLE_SHADER_COMPILATION "Disable shader compilation" OFF)
Sirtsu55 commented 1 year ago

Nice implementation! I noticed you added --useAPI to to the general arguments, so the linux build could fail because the ShaderMake docs say it's Windows-only. Apart from that, thanks for implementing this.

dzhdanNV commented 1 year ago

Yes! Will fix right now... Argh!

dzhdanNV commented 1 year ago

With the fix: CMakeLists.txt

Do you give your blessing? ;)

Sirtsu55 commented 1 year ago

Looks good. Bless the CMake! :)

dzhdanNV commented 1 year ago

Thanks! It will be included into the next release (next week I hope).

dzhdanNV commented 1 year ago

Implemented!