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

Added support for ray binning for ray tracing in XR (case 1346374). #5038

Merged
merged 4 commits into from Jul 9, 2021

Conversation

anisunity
Copy link
Contributor

https://fogbugz.unity3d.com/f/cases/1346374/
Support ray binning for RTR and RTGI in performance. Visually should not change anything.

Testing status
Most test pass locally, we need to run yamato to check if the fails are local or not.

@github-actions
Copy link

github-actions bot commented Jul 2, 2021

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page)
and cancel any jobs that started on Yamato.
See the PR template for more information.
Thank you!

@remi-chapelain
Copy link
Contributor

Launched the test on yamato.

// A really nice tip is to dispatch the rays as a 1D array instead of 2D, the performance difference has been measured.
uint dispatchSize = (uint)(bufferSizeX * bufferSizeY);
ctx.cmd.DispatchRays(data.parameters.gBufferRaytracingRT, m_RayGenGBufferBinned, dispatchSize, 1, 1);
ctx.cmd.DispatchRays(data.parameters.gBufferRaytracingRT, m_RayGenGBufferBinned, dispatchSize, (uint)data.parameters.viewCount, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention we've been using the dispatch Z for the view count in all dispatch calls. Could you update this code ?

@@ -176,6 +178,8 @@ void RayGenGBufferBinned()
// Grab the dimensions of the current raytrace shader
uint3 LaunchIndex = DispatchRaysIndex();

UNITY_XR_ASSIGN_VIEW_INDEX(LaunchIndex.y);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment about convention to use Z as view index.

@@ -187,11 +191,11 @@ void RayGenGBufferBinned()

// Read and unpack the coordiante to screen space coordinates
uint globalBinIndex = tileIndex * RAY_BINNING_TILE_SIZE * RAY_BINNING_TILE_SIZE + localTileIndex;
uint packedPixelCoordinate = _RayBinResult[globalBinIndex];
uint packedPixelCoordinate = _RayBinResult[globalBinIndex + _RayBinViewOffset * LaunchIndex.y];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here for Z

uint2 currentPixelCoord = uint2((packedPixelCoordinate & 0xffff0000) >> 16, packedPixelCoordinate & 0xffff);

// If the local index of this pixel is beyond the valid count, no need to compute it
if (localTileIndex > _RayBinSizeResult[tileIndex])
if (localTileIndex > _RayBinSizeResult[tileIndex + _RayBinTileViewOffset * LaunchIndex.y])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here for Z

Copy link
Contributor

@fabien-unity fabien-unity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
See small comment about using the Z dispatch index for the view index

@anisunity
Copy link
Contributor Author

Review fixes done

@github-actions
Copy link

github-actions bot commented Jul 6, 2021

The following job should be run on every PR:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252F_abv.yml%2523General_Checks_2021.2

It looks like you're changing an HDRP Package or project
Please run the following jobs on your branch before merging your PR:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have the hardware to test, trusting user feedbacks, automated tests and the rest of the reviewers ✔️

@sebastienlagarde sebastienlagarde merged commit 82e1753 into master Jul 9, 2021
@sebastienlagarde sebastienlagarde deleted the HDRP/ray-binning-xr branch July 9, 2021 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants