Closed JiayinCao closed 1 year ago
Thanks again! The script now:
NRD.hlsli
in any caseI 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.
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
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.
@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?
Yes, this sounds like a good solution.
Thanks for offering the help.
OK, then I will include this into the upcoming release.
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.
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, thecopy_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
andNRD_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
andNRDIntegration.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