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

Validate descriptor component type #299

Merged
merged 4 commits into from
Aug 28, 2018

Conversation

chrisforbes
Copy link
Contributor

No description provided.

@chrisforbes chrisforbes force-pushed the validate-descriptor-component-type branch 4 times, most recently from 0fa43bd to a61adcf Compare August 28, 2018 00:13
@chrisforbes
Copy link
Contributor Author

Validates that isampler* is correctly matched with SINT formats, etc.

@mark-lunarg
Copy link
Contributor

@tobine would want a test...

@chrisforbes
Copy link
Contributor Author

Adding a test and rebasing.

@chrisforbes chrisforbes force-pushed the validate-descriptor-component-type branch 2 times, most recently from 3404355 to bf7bdfd Compare August 28, 2018 16:34
Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

Some explanation in the test would be handy

TEST_DESCRIPTION(
"Test that an error is produced when the component type of an imageview disagrees with the type in the shader.");

m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_ERROR_BIT_EXT, "SINT component type, but bound descriptor");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a VUID string, or UNASSIGNED string matching this? I don't see one in the code above...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way DescriptorSet::ValidateDrawState is written forces everything to use the same VUID tag. We should probably rework this function at some point to support case-specific VUIDs.

pipe.AddShader(&fs);
pipe.AddDefaultColorAttachment();

VkTextureObj texture(m_device, nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

By default the texture is in a UNORM format, which (if I'm understanding this correctly) why the Draw validation will report the mismatch, yes? If so, some commentary might be handy as so much of this is hidden behind utility objects. If not some commentary might be handy to educate the maintain about the "theory of operation"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another case of the test framework being bizarrely inside out -- the details you care about are implicit, all kinds of other noise is exposed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have the spare cycles to do any kind of major surgery to this -- I just need the check enforced to make CTS more correct.

V2: Add comment clarifying why this test works
@chrisforbes chrisforbes force-pushed the validate-descriptor-component-type branch from bf7bdfd to 3c5b959 Compare August 28, 2018 17:19
Copy link
Contributor

@tobine tobine left a comment

Choose a reason for hiding this comment

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

LGTM

@chrisforbes chrisforbes merged commit 81fc74d into master Aug 28, 2018
@mark-lunarg mark-lunarg deleted the validate-descriptor-component-type branch August 15, 2019 20:55
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