-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[DirectX] Adding support for static samplers in yaml2obj/obj2yaml #139963
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
Conversation
24e77c1
to
aabd424
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than one comment about consistently using the ASSERT_*
vs EXPECT_*
macros in the tests.
ASSERT_EQ(Sampler.MaxAnisotropy, 20u); | ||
ASSERT_EQ(Sampler.ComparisonFunc, 4u); | ||
ASSERT_EQ(Sampler.BorderColor, 0u); | ||
EXPECT_FLOAT_EQ(Sampler.MinLOD, 4.56); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be ASSERT_FLOAT_EQ
? IIUC ASSERT_*
stops immediately if the condition isn't met and EXPECT_*
allow us to diagnose multiple issues. I don't think it makes sense to use one for the floating point fields and the other for the rest of the fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Did you want something to track and update going through all the previously defined parameters and marking them as optional yaml parameters if applicable?
…vm#139963) - Adds support for static samplers ins dxcontainer binary format. - Adds writing logic to mcdxbc - adds reading logic to Object - adds tests Closes: [126636](llvm#126636) --------- Co-authored-by: joaosaffran <joao.saffran@microsoft.com>
Closes: 126636