Copy NRD.hlsli by default

[JiayinCao]

Hi,

I'd like to point out a minor thing in the scripts.
As it is necessary to use code in NRD.hlsli in the user ray tracing app that integrates the denoiser as it has a few helper functions that packs data, such as REBLUR_FrontEnd_PackRadianceAndNormHitDist. Of course, an alternative solution is for the users to define the same function in their own code base, but that would cause further upgrading of Denoiser in the app a bit troublesome as there is some implicit connection between their code and the SDK that needs some special attention.

It makes a lot of sense to copy this particular file by default regardless whether the user choose to add the argument copy_shader or not. To my understanding, the copy_shaders option is specifically for people who wants to integrate the SDK with the third varient mentioned here . For people who are interested in method 1 or 2, they won't care about the source code of the shaders, except this NRD.hlsli.

An extra suggestion of the script is that, it would be very nice to generate the macro definition of the two macros, NRD_USE_OCT_NORMAL_ENCODING and NRD_USE_MATERIAL_ID by the script.
My current solution is to take a look at the way I build the NRD project and put the macros right before where I include it. But this could also cause some inconsistency if people won't pay attention to it. The script has everything it needs to generate the two macros and put it in front of the hlsli file to avoid asking user to define it.

And another very minor thing is that maybe it is also worth considering to not copy NRDIntegration.h and NRDIntegration.hpp by default, as these two files are only for the second integration method. If the user chooses to use the first and third method, these two files are not relevant.

Thanks
Jiayin

[dzhdanNV]

Thanks again! The script now:

• copies NRD.hlsli in any case
• NRD integration is copied only if requested

I can't generate macro definitions (at least in a simple way), but I have added this:
In NRDDescs.h:

struct LibraryDesc
{
        ...
        uint8_t maxSupportedMaterialBitNum; // if 0, compiled with NRD_USE_MATERIAL_ID = 0
        bool isCompiledWithOctPackNormalEncoding : 1; // if 0, compield with NRD_USE_OCT_NORMAL_ENCODING = 0
};

and in NRD.hlsli:

// UNORM or OCT-packed normals
#ifndef NRD_USE_OCT_NORMAL_ENCODING
    #error "NRD_USE_OCT_NORMAL_ENCODING needs to be defined as 0 or 1. You can check 'LibraryDesc::isCompiledWithOctPackNormalEncoding' at runtime to know it."
#endif

// Material ID support
#ifndef NRD_USE_MATERIAL_ID
    #error "NRD_USE_MATERIAL_ID needs to be defined as 0 or 1. You can check 'LibraryDesc::maxSupportedMaterialBitNum' at runtime to know it."
#endif

I will update the repo today-tomorrow.

[JiayinCao]

Thanks for the update.

I'd like to share my two cents about the trade off of this solution from a user's perspective.

This macro solution of yours would indeed work. So with this solution, the user would have two choices

• Define one of the macros and in the host app, check the flag to assert it is indeed defined the same way when the library was compiled. This is fine, except that it adds a bit of extra burden for user to integrate the library. Also for pre-compiled library, you would have to indicate which flag is used during compilation. The documentation would be slightly longer as you would have to explain why the flag is there and how we as users could process the information.
• Another alternative solution is for user to define two set of shader code, each with one of the two macro defined. And depending on the flag you added, selectively check which one should be used. Similar as the above solution, this also adds a bit of extra burden to the users. A slightly most concerning issue would be the shader permutation problem as they would have to double the shader compilation time for shaders that use the function. Though this may not be as bad as it sounds as only shaders that uses this header would need to be duplicated. And there should not be that many of such a shader.
  Regardless whichever solution the users choose, it would mean more document and effort for integrating the library. This extra complexity, no matter how smal it is, doesn't seem to be worth it for a good reason. Ideally, it would be nice to totally hide this from the users.

A slightly better solution, which is also simple enough is to use script to generate something like a new file with this content in it.

#define OneOfTheTwoMacros
#define NRD_IMPLEMENTATION
#include "NRD.hlsli"

And then add a line in NRD.hlsli

#if !defined(NRD_IMPLEMENTATION)
#error "Please do not include this file directly, there is a wrapper file that is used to be included."
#endif

The benefit of this approach over yours is the hidden the macro definition entirely from NRD users.

If this extra wrapper header file seems a bit unnecessary, another common approach that can be done is to define something quite noticable and won't compile without processing in the shader. Things like this

<<<ReplaceMeWithAMacroDefine>>>

It would be quite simple to locate such a string and process it to dxc/fxc consumable format with the correct macro defined inside the header.
The catch of this solution is that it would require a bit more steps in the NRD project setup as there must be a step generating the final header before anycode uses this header. But the complexity would be entirely limited inside the NRD project itself with close to nothing exposed to the users.

[dzhdanNV]

@JiayinCao Thanks. I meditated about the problem and I think, that the simplest solution for me would be to add NRDEncoding.hlsli file, which will be automatically generated near NRD.hlsli during project deployment, it will contain:

// Automatically generated file, reflecting Cmake settings
#define NRD_NORMAL_ENCODING 2
#define NRD_ROUGHNESS_ENCODING 1

This file is not needed for Cmake-based integrations (like the NRD sample), but this file can be included prior NRD.hlsli to provide encoding settings on the end-user side (matching NRD project settings). Will it work for you?

[JiayinCao]

Yes, this sounds like a good solution.

Thanks for offering the help.

[dzhdanNV]

OK, then I will include this into the upcoming release.

[dzhdanNV]

In v3.8.0 during project deployment NRDEncoding.hlsli will be created, which can be included prior NRD.hlsli on the app side to provide normal and roughness encoding.