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

Shadow denoiser feedback #1

Open
turanszkij opened this issue May 18, 2021 · 4 comments
Open

Shadow denoiser feedback #1

turanszkij opened this issue May 18, 2021 · 4 comments

Comments

@turanszkij
Copy link

turanszkij commented May 18, 2021

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.

@rys
Copy link
Contributor

rys commented May 18, 2021

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.

@expenses
Copy link

expenses commented Dec 15, 2021

  • 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 ;)

@turanszkij
Copy link
Author

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

@expenses
Copy link

I did indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants