Improve light AABB generation #1574

EvgeniiG · 2020-08-11T21:03:31Z

Purpose of this PR

Light AABB generation is the first part of the light list generation algorithm (used for tiled and clustered lighting).

There are two issues with the current implementation:

it's slow (214.5 us for 3 lights on the base PS4);
it's underdocumented, and thus difficult to understand, maintain, and optimize.

I have rewritten the code. Highlights include:

a single unified algorithm (1 kernel) that handles all types of projection matrices;
thoroughly documented code (cannot claim that it's simple, as the task itself is not simple);
high performance: 214.5 us -> 77 us for 3 lights (2.78x speedup), 8 threads/light -> 4 threads/light (2x improvement).

I focused on algorithmic rather than low-level optimization. The generated code is not particularly optimal.

Testing status

Manual Tests: What did you do?

Opened test project + Run graphic tests locally
Built a player
Checked new UI names with UX convention
Tested UI multi-edition + Undo/Redo + Prefab overrides + Alignment in Preset
C# and shader warnings (suppress shader cache to see them)
Checked new resources path for the reloader (in developer mode, you have a button at end of resources that check the paths)
Other:

Automated Tests: Used existing tests.

Yamato: (Select your branch):
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics/tree/HDRP%252Flight-aabb/.yamato%252Fall-hdrp.yml%2523All_HDRP_2020.1

Any test projects to go with this to help reviewers?

Comments to reviewers

QA

test various light setups (many local lights, reflection probes, density volumes, decals) with different camera projection matrices (orthographic, perspective, oblique (used for planar reflection)).

Francesco

review the code;
change the WaveLaneSwizzle intrinsic to take (andMask, orMask, xorMask) as parameters;
test on XBox; if you want to make further optimizations, create a new PR.

Sebastien

review the code;
merge the PR.

github-actions · 2020-08-11T21:04:09Z

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!

FrancescoC-unity · 2020-08-25T12:14:37Z

com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/LightLoop.cs

-                var altDist = Mathf.Sqrt(fAltDy * fAltDy + (true ? 1.0f : 2.0f) * fAltDx * fAltDx);
-                bound.radius = altDist > (0.5f * range) ? altDist : (0.5f * range);       // will always pick fAltDist
-                bound.scaleXY = squeeze ? new Vector2(0.01f, 0.01f) : new Vector2(1.0f, 1.0f);
+                var altDist   = Mathf.Sqrt(fAltDy * fAltDy + (true ? 1.0f : 2.0f) * fAltDx * fAltDx);


You did not add this code, but having true? is a bit weird here :D

FrancescoC-unity · 2020-08-25T12:21:53Z

Cannot change the intrinsic as you use it as on xbox the final mask (as used now) needs to be a constant literal.

FrancescoC-unity · 2020-08-25T12:42:32Z

com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/scrbound.compute

-            const unsigned int uFlag3 = GetClip(vP3);
+/* ------------------------------ Implementation ---------------------------- */
+
+#define DUMB_COMPILER // Improve the quality of generated code at the expense of readability


Unless using an IDE with collapsing of #if, the presence of this makes it a bit harder to follow the code.

I assume it is left as define here to show the more readable version, but couldn't we get rid of that and instead comment the "dumb compiler" part so that we don't need the other version?

The idea is to eventually remove this #define. It's a trivial transformation, and a compiler should be able to do it for us.

We need to report it to Sony and see if it works as expected on Microsoft's machines.

JMargevics

Hey! Here's a test doc for this PR: https://confluence.unity3d.com/display/HDRP/Improve+light+AABB+generation

Generally, we've run through most of light features and areas on all platforms. There are no regressions except Xbox, which is completely broken now. I am still intending to do so performance tests on ps4 and compare it to 8.x.x./release.

EvgeniiG · 2020-08-26T19:02:29Z

Thanks for testing. Xbox should work now.

JMargevics

Hey, @TomasKiniulis verified it on Xbox and it works well now 👍

EvgeniiG · 2020-08-27T16:24:39Z

Excellent! Thanks, guys.

FrancescoC-unity · 2020-08-28T15:02:23Z

com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/scrbound.compute

-    //uint vindex = groupID * NR_THREADS + threadID;
-    unsigned int g = groupID;
-    unsigned int t = threadID;
+float3x3 Invert3x3(float3x3 R)


Feels like this would be useful even outside of this context on its own hlsl file (or already existing common file?) along few of the other generic matrix ops in this file.

Sure. We just need to decide what to move to the core.

We could add those in a "math section" in common.hlsl. Or alternatively create a matrix.hlsl file?

We should avoid creating more files with confusing names. I propose we move this to Common once there is more than 1 use case.

same answer than before, nobody will ever know that such a function exist as it is "hidden" in particular file.
Agree to not add more files, so we could move it to common.hlsl, it is a frequent enough function to be there.

I will see whether it makes sense to share some code after I am done with Z-binning.

sebastienlagarde · 2020-09-16T14:26:43Z

@EvgeniiG this PR is targeting 8.x.x/release, sadly we can't retarget to HDRP/staging, could you recreate the PR which target HDRP staging? thanks

sebastienlagarde · 2020-09-16T14:30:39Z

com.unity.render-pipelines.core/ShaderLibrary/Common.hlsl

@@ -852,11 +852,11 @@ void CompositeOver(real3 colorFront, real3 alphaFront,
 // Space transformations
 // ----------------------------------------------------------------------------

-static const float3x3 k_identity3x3 = {1, 0, 0,
+static const float3x3 k_Identity3x3 = {1, 0, 0,


so I know this is not great but we can't rename it without breaking existing code of users (this is also used by URP).

Here there is not a lot of choice, either we don't rename it, or we do the following.
in common.hlsl:

static const float3x3 k_Identity3x3 = {

(i.e your change).

And in CommonDeprecated.hlsl
we add :
// This is obsolete, don't used, keep for compatiblity.
static const float3x3 k_identity3x3 = k_Identity3x3 ;

this will work and not break existing code.

Note that this is a fix. I actually couldn't find this constant because it was capitalized in the wrong way.
I can fix URP (or already have). I propose we just add a comment in this file. If someone's code breaks, it is trivial to fix.

being it a fix or not isn't the problem. The issue is that it will break users custom shader. The proposal is to allow to fix the nomenclature and still not break existing user code (by putting old version as k_identity3x3 )

"If someone's code breaks, it is trivial to fix"
We can't afford this anymore. Any change like this must be added to the upgrade guide if there is no automatic upgrade, and for this little change, I will suggest we don't break code.

And yes URP code should be fix in this case as well. You should consider make a separate PR for this change as it isn't related to improve AABB generation and touch a lot of files.

sebastienlagarde · 2020-09-16T14:39:32Z

com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/scrbound.compute

+// that is unrolled at the compile time, and the constants are generated during the
+// constant propagation pass of the optimizer. This works fine on PlayStation, but does not work
+// on Xbox. In order to avoid writing hideous code specifically for Xbox, we disable the support
+// of wave intrinsics on Xbox until the Xbox compiler is fixed.


let's add a date (or xbox compiler version, so we know which version we are refering too when we read the comment later.

sebastienlagarde · 2020-09-16T14:43:37Z

com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/scrbound.compute


+// Improve the quality of generated code at the expense of readability.
+// Remove when the shader compiler is clever enough to perform this optimization for us.
+#define DUMB_COMPILER


always define? Even for PS4?

maybe the name isn't good, we should say:
#define MANUAL_CODE_OPTIMIZATION ?

I only tested on the PS4. Don't know about the XBox or Nvidia, I don't have access to the hardware.

ok, still we should avoid naming thing "dumb compiler" in public code :)

sebastienlagarde · 2020-09-16T14:45:48Z

com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/scrbound.compute

-                    const float4 vP1 = mul(g_mProjection, float4(q1, 1));
-                    const float4 vP2 = mul(g_mProjection, float4(q2, 1));
-                    const float4 vP3 = mul(g_mProjection, float4(q3, 1));
+float4 IntersectEdgeAgainstPlane(ClipVertex v0, ClipVertex v1)


Several function should be move to GeometricTool.hlsl but ok for now

Maybe in the future, if we end up reusing them.

so ok ,but the problem is the reverse, people will never been aware that such a function exist, so they will duplicate it. But fair enough.

sebastienlagarde · 2020-09-16T14:53:39Z

@JMargevics Hey, thanks for the detailed report! appreciated. I noticed that for the test of planar reflection, you have just tested if the object was working correctly in the planar. But the goal is to check that multiple reflect correctly at various camera angle.
Have you done that? could you check? thanks.

Note: It is not expected that this PR goes in 8.x, so if you can do the test above once the PR is recreated for HDRP/staging, thanks

EvgeniiG · 2020-09-17T01:59:36Z

See #1906

EvgeniiG added 13 commits August 5, 2020 13:48

Implement clipping and culling (does not consider view frustum corners)

c084cdd

Support orthographic projection

a6f00ce

k_identity -> k_Identity

f8eea29

Turn 'scaleXY' into a scalar

ccbb9e1

Test corners of the view volume

1559d1c

Improve the placeholder for the linear depth

b8e8c83

Fix aspect

676eb5a

Bugfix

433e27e

Optimize

8a2458a

Also store view space Z

0453c6c

Optimize orthographic

7aa331c

Optimize LUT

05a222e

Add wave intrinsic support

1e90134

EvgeniiG marked this pull request as draft August 11, 2020 21:03

github-actions bot added HDRP SRP labels Aug 11, 2020

EvgeniiG added 6 commits August 11, 2020 16:07

Fix group count

9e4b8c6

Reduce the kernel count to 1

f52d29b

Remove old code

64f50ba

Bounds check

1a3e172

Add a profiling marker

3e28378

Fix lane masks

06a8d70

EvgeniiG changed the title ~~Optimize light AABB generation~~ Improve light AABB generation Aug 12, 2020

Fix compiler warning

a7fcd99

EvgeniiG requested review from sebastienlagarde, FrancescoC-unity and a team August 12, 2020 01:50

Remove GPU Pro reference

71004f0

FrancescoC-unity reviewed Aug 25, 2020

View reviewed changes

JMargevics requested changes Aug 26, 2020

View reviewed changes

No instrinsics on Xbox

21c2481

JMargevics approved these changes Aug 27, 2020

View reviewed changes

FrancescoC-unity reviewed Aug 28, 2020

View reviewed changes

sebastienlagarde changed the base branch from 8.x.x/release to HDRP/staging September 16, 2020 14:25

sebastienlagarde changed the base branch from HDRP/staging to 8.x.x/release September 16, 2020 14:26

sebastienlagarde reviewed Sep 16, 2020

View reviewed changes

EvgeniiG mentioned this pull request Sep 17, 2020

Improve light AABB generation #2 #1906

Merged

6 tasks

EvgeniiG closed this Sep 17, 2020

Improve light AABB generation #1574

Improve light AABB generation #1574

Uh oh!

Conversation

EvgeniiG commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Aug 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrancescoC-unity commented Aug 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JMargevics left a comment

Choose a reason for hiding this comment

Uh oh!

EvgeniiG commented Aug 26, 2020

Uh oh!

JMargevics left a comment

Choose a reason for hiding this comment

Uh oh!

EvgeniiG commented Aug 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EvgeniiG Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde commented Sep 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EvgeniiG Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EvgeniiG Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde commented Sep 16, 2020

Uh oh!

EvgeniiG commented Sep 17, 2020

Uh oh!

EvgeniiG commented Aug 11, 2020 •

edited

Loading

EvgeniiG Sep 16, 2020 •

edited

Loading

EvgeniiG Sep 16, 2020 •

edited

Loading

EvgeniiG Sep 16, 2020 •

edited

Loading

sebastienlagarde Sep 16, 2020 •

edited

Loading