FidelityFX-Denoiser icon indicating copy to clipboard operation
FidelityFX-Denoiser copied to clipboard

Shadow denoiser feedback

Open turanszkij opened this issue 4 years ago • 4 comments

Hello,

Thank you for this amazing library, it looks and performs wonderfully. The integration was fairly straight forward for this amount of complexity. I just had a few minor clarifications I'd like to write down for anyone wanting to integrate it into their own progam in the future.

  • Velocity buffer is expected to be computed by the application (per pixel) as: clipspacePos.xy/clipspacePos.w - clipspacePosPrev.xy/clipspacePosPrev.w (the denoiser library converts velocity from clip space to uv space internally as velocity * float2(0.5, -0.5) ).
  • You can #define INVERTED_DEPTH_RANGE if using a reverse depth buffer, this matters when choosing closest velocity in neighborhood (reverse depth buffer means near=1, far=0)
  • matrices you provide are multiplied in the shader as mul(matrix, vector) so the library assumes hlsl default column_major matrix layout
  • Worth to clarify texture formats: "moments" textures should use FORMAT_R11G11B10_FLOAT, the "reprojection results" textures should use FORMAT_R16G16_FLOAT
  • Metadata buffer size? Worth to clarify if this is for the 8x4 tiles or the 8x8 tiles. (As it's suggested that the classification and filter passes could use different tile size).
  • Denoising multiple lights: I chose to use the Z Dispatch dimension to choose which light to denoise in the denoiser passes. For that I currently had to declare a groupshared uint light_index; which I assign with light_index = Gid.z (SV_GroupIndex Z component). The light_index is used to simply index resource bindings for separate light denoiser textures. This is because the library only uses XY dimensions of all thread IDs in the user functions. It would be cool if the denoiser had a notion of light index in the user functions (or just provide XYZ components of thread IDs)

Also, there are a lot of user defined functions. It would have been much easier if the library would expose a stub of all these functions instead of having to go through the library code and look for undefined functions (or look at compiler errors).

I don't mean to complain though, it is amazing that this library is provided as open source. The existing documentation was also very helpful.

turanszkij avatar May 18 '21 11:05 turanszkij

Thanks for the really great feedback, János, we'll make documentation and library updates based on what you found with your integration. I'll mirror this in a separated set of issues in our internal issue tracker and feed them into the next dev cycle, and leave this issue open here so that others can find it in the interim.

rys avatar May 18 '21 14:05 rys

  • Worth to clarify texture formats: "moments" textures should use FORMAT_R11G11B10_FLOAT, the "reprojection results" textures should use FORMAT_R16G16_FLOAT

FORMAT_R11G11B10_FLOAT is VK_FORMAT_B10G11R11_UFLOAT_PACK32 in Vulkan, in case anyone else was struggling to find it ;)

expenses avatar Dec 15 '21 21:12 expenses

FORMAT_R16G16_FLOAT is VK_FORMAT_B10G11R11_UFLOAT_PACK32 in Vulkan, in case anyone else was struggling to find it ;)

I think you mistyped it, FORMAT_R16G16_FLOAT = VK_FORMAT_R16G16_SFLOAT, and FORMAT_R11G11B10_FLOAT = VK_FORMAT_B10G11R11_UFLOAT_PACK32

turanszkij avatar Dec 20 '21 16:12 turanszkij

I did indeed.

expenses avatar Dec 20 '21 16:12 expenses