Pbr sky fixes metal

[jessebarker]

Please read

Purpose of this PR

The constant buffer for the physically based sky does not have a proper layout for non-DirectX API platforms. 3-component vectors and 4-component vectors share the same size/padding constraints on other APIs, so we had a mismatch in the data layout between C# and the shader binding. This fixes that.

Testing status

Manual Tests: What did you do?

• [ x] Opened test project + Run graphic tests locally
• [ x] Built a player
• Checked new UI names with UX convention
• Tested UI multi-edition + Undo/Redo + Prefab overrides + Alignment in Preset
• [ x] C# and shader warnings (supress 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: 5004, 5005, 5006 tests should now run properly.

Yamato: (Select your branch):
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics

Any test projects to go with this to help reviewers?
Should fix https://fogbugz.unity3d.com/f/cases/1253448/ as well as allow graphics tests 5004, 5005, and 5006 to be enabled at least for Mac.

Comments to reviewers

Notes for the reviewers you have assigned.

[EvgeniiG]

I'm not sure I fully understand the issue here.

First of all, what happens when we declare

float3 A;
float B;

in a constant buffer on Metal? What kind of layout is it?

Vectors are 16-byte aligned in DX, so this results in optimal packing.

Secondly, I can understand if we were generating suboptimal code, but it sounds like we are generating incorrect code? This seems highly error-prone to me. Imagine writing some C code, you declare a struct, re-order a couple of members, your code compiles, but it's complete nonsense... Is it a part of a Metal spec that unless you manually pad everything, you get garbage results? Should we perhaps make the HLSL generation script take care of this for us?

I'd rather avoid tons of error-prone manual labor. It's also important that we do not encounter the same problem again in the future.

[EvgeniiG]

I would also like to avoid wasting space in the constant buffer (and constant cache) if possible.

[sebastienlagarde]

This RP fix current issue with Metal and PBR sky:
https://yamato-artifactviewer.prd.cds.internal.unity3d.com/904f27b8-e05d-4dc2-95b8-0a4097a162ee%2Flogs%2FTestProjects%2FHDRP_Tests%2Ftest-results%2Ftest-results/TestReportV1.html

Agree that we should fix the root cause of the issue but pretty sure that Jesse already have looked at it.
Yes, we can certainly save some space in constant buffer sky. Could you help @EvgeniiG with this, as you know better the code than Jesse. Thanks.

[EvgeniiG]

I have no issues with the current PR and I am sure Jesse tested it and it works. My issue is the manual approach to solving this problem since the issue is not limited to the PBR sky (as well as the wasted space, and it seems we can not use float3 at all anymore just because of Metal?).

[jessebarker]

I'm not sure I fully understand the issue here.

First of all, what happens when we declare

float3 A; float B;

in a constant buffer on Metal? What kind of layout is it?

float3 and float4 both align to 16 bytes. Not just with Metal, but also with Vulkan.
So, your layout is:

'A' has offsetof(0)
'B' has offsetof(16)

Vectors are 16-byte aligned in DX, so this results in optimal packing.

Secondly, I can understand if we were generating suboptimal code, but it sounds like we are generating incorrect code? This seems highly error-prone to me. Imagine writing some C code, you declare a struct, re-order a couple of members, your code compiles, but it's complete nonsense... Is it a part of a Metal spec that unless you manually pad everything, you get garbage results? Should we perhaps make the HLSL generation script take care of this for us?

This is a problem for the cases where we are managing the constant buffers directly via the constant buffers API (i.e., C# code is providing the whole buffer and just telling the graphics device to make it available to the GPU for a given shader stage). As you can see from the above example, you will have put the data for 'B' in the wrong place. Further, we are potentially allocating the wrong size buffer as well because we are not rounding the size up according to the correct alignment constraints. We are also creating suboptimal buffer layouts all over the place, but because those "non-global" buffers are updated member by member, there are no issues with sizing and offsets, we're just not packing things the way we think we are.

[EvgeniiG]

HLSL also aligns all vectors to 16 bytes. See https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-packing-rules

'B' is not a vector, though, so it should be aligned with the next available 4-byte boundary rather than the next 16-byte boundary.

I suppose what you're saying is that all vectors are not just 16-byte aligned but are also 16 bytes in size?

[jessebarker]

In Metal, yes, the float3 and float4 are both actually sized and aligned to 16 bytes, whereas for Vulkan/SPIR-V, I believe only the alignment is the same (the data is the data, but it must be aligned, so you can still have some unexpected results if you're not careful). Metal Shading Language specification section 2.2, and Vulkan specification chapter 14.5 have the gnarly details.

[EvgeniiG]

I see. That makes sense. Thank you.
So sounds like we should avoid using float3 in a constant buffer if we want optimal performance on Metal.
Vulkan appears to behave the same way as DX according to the spec (correct me if I am wrong).

[jessebarker]

I might be misunderstanding the DX packing/alignment rules, but in Vulkan/SPIR-V the actual size of the data is as declared (12 bytes for float3, 16 bytes for float4), but the alignment is according to the 4-component vector (so, a float3 would have a 16-byte alignment).

[EvgeniiG]

Actually, you're right, it's slightly different in DX. See https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-packing-rules

In particular,

//  1 x 16byte elements
cbuffer IE
{
    float1 Val1;
    float3 Val2;
};

which would be 32 bytes in VK (if I understand it correctly, section 14.5.4).

So it's not really an alignment restriction but rather the "does not cross a 16-byte boundary" as the docs say.

[jessebarker]

So, the data still occupies 16 bytes in that example, but it occupies it sparsely in a sense. 'Val1' would be at offset 0 and 'Val2' would be at offset 16, but I believe the buffer could technically be 28 bytes because 'Val2' is still only 12 bytes in actual size. For Metal, the buffer would definitely have to be 32 bytes because 'Val2' would actually be 16 bytes in size in that case.