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

add barycentrics ordering check onto existing barycentrics test #4635

Merged
merged 8 commits into from
Dec 15, 2022

Conversation

bob80905
Copy link
Collaborator

@bob80905 bob80905 commented Sep 3, 2022

The existing barycentrics test failed to account for an extra part of the specification.
No matter in which order the vertices are passed to the pixel shader, GetAttributeAtVertex on the svid of each vertex should equal the numbered vertex that's given to the GetAttributeAtVertex function.
So, this test was added inside of the barycentrics test, with an extra shader to accomplish the test.
If this spec is met, then the associated pixel will write a float4 of all 1.0's to the render target view. These pixels are specifically tested to see if the ordering constraint is met.

@bob80905 bob80905 requested a review from tex3d September 3, 2022 01:09
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

I still have to look more closely at the changes, but here are a few things that might help make that easier and would improve the final result as well

tools/clang/unittests/HLSL/ExecutionTest.cpp Outdated Show resolved Hide resolved
tools/clang/unittests/HLSL/ExecutionTest.cpp Outdated Show resolved Hide resolved
tools/clang/unittests/HLSL/ExecutionTest.cpp Outdated Show resolved Hide resolved
<RootSignature>RootFlags(ALLOW_INPUT_ASSEMBLER_INPUT_LAYOUT)</RootSignature>
<Resource Name="VBuffer" Dimension="BUFFER" Width="1024" Flags="ALLOW_UNORDERED_ACCESS" InitialResourceState="COPY_DEST" Init="ByName" ReadBack="true">
</Resource>
<Resource Name="RTarget" Dimension="TEXTURE2D" Width="64" Height="64" Format="R32G32B32A32_FLOAT" Flags="ALLOW_RENDER_TARGET" InitialResourceState="COPY_DEST" ReadBack="true" />
Copy link
Member

Choose a reason for hiding this comment

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

If we can make the render targets the same size, you can use different entry point targets to make one ubershader and prevent duplicating the shader code above with slight modifications.

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

2 == GetAttributeAtVertex(input.svid, 2))
{
// special value of all 1's in a float4 will be returned if the ordering is maintained for this pixel.
return float4(1.0, 1.0, 1.0, 1.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative way to do this (without the if) is:

return float4(float3(GetAttributeAtVertex(input.svid, 0),
                     GetAttributeAtVertex(input.svid, 1),
                     GetAttributeAtVertex(input.svid, 2)) * 0.5, 1.0)
```
then change the expected result to (0, 0.5, 1.0, 1.0) which makes out-of-order failure case reveal the ordering used in the result.

Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

I'm curious if the change to the round() makes any difference in the required precision.

I think tex's suggestion is a good one.

tools/clang/unittests/HLSL/ExecutionTest.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@tex3d tex3d left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for sticking with this!

@bob80905 bob80905 changed the title add barycentrics ordering check onto existing barycentrics test, with extra shader add barycentrics ordering check onto existing barycentrics test Dec 15, 2022
@AppVeyorBot
Copy link

@bob80905 bob80905 merged commit ee0994e into microsoft:main Dec 15, 2022
hekota pushed a commit to hekota/DirectXShaderCompiler that referenced this pull request Dec 16, 2022
…osoft#4635)

* add barycentrics ordering check onto existing barycentrics test, with extra shader

(cherry picked from commit ee0994e)
hekota added a commit that referenced this pull request Dec 16, 2022
Cherry-pick of PIX and HLK changes for the 2212 release

Changes for PIX:
20bb3d0 PIX: Modify root sigs in place (plus fix root sig memory leak) (PIX: Modify root sigs in place (plus fix root sig memory leak) #4876)
2c3d965 dxcopt: Support full container and restore extra data to module (dxcopt: Support full container and restore extra data to module #4845)
21cf36a Fix hitgroup metadata argument order

HLK Test Updates:
ee0994e add barycentrics ordering check onto existing barycentrics test (add barycentrics ordering check onto existing barycentrics test #4635)
6acd11b ConvertFloat32ToFloat16: Use DirectXMath conversion functions (ConvertFloat32ToFloat16: Use DirectXMath conversion functions #4855)
e7aac8e Include TestConfig.h only if DEFAULT_TEST_DIR is not defined (Include TestConfig.h only if DEFAULT_TEST_DIR is not already defined #4884)
5decc4a Do not include TestConfig.h for all HLK build (Do not include TestConfig.h for all HLK build #4887)
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

Successfully merging this pull request may close these issues.

4 participants