Tobin gh2080 rp unexpected errors #2082
Conversation
tobine
requested review from
chrisforbes,
TonyBarbour,
mark-lunarg,
mikew-lunarg and
mikes-lunarg
I think the users are more likely to be interested in the message text. It won't kill unique IDs, but it will kill unique messages, no? What is the reasoning behind such change? It does not seem like the ideal solution. But "perfect is the enemy of better"; perhaps leave a |
|
@krOoze the early return makes testing easier for renderPass compatibility. It avoids having to flag a bunch of unexpected errors in the test. With this change, the user will get all of the errors about why input attachments of renderPass cause an incompatibility, then if the user returns "true" to skip the call down, they will miss any potential error messages related to why color or DS attachments are incompatible. If user doesn't skip call down, they'll still see all of the error callbacks. Not sure what ideal solution is here. I could make an argument that validation should queue up all the text for a single VUID and then make a single call. Alternatively, for a complex case like RP compatibility maybe it's better to make multiple calls for same VUID, or perhaps we need more VUIDs to make identifying the exact error simpler for the user. Let me know if you have strong thoughts one way. I don't feel too strongly about how I've structured this update. It just makes testing easier, fixes known unexpected errors in tests, and seems reasonable. If users don't return "true" from callback they'll still see all the errors. |
|
@tobine, can you rebase this? |
tobine
force-pushed
the
tobin_gh2080_rp_unexpected_errors
branch
from
02651f8 to
a22c26f
Compare
|
rebased to master |
Only perform state updates for CmdBeginRenderPass() if we're not skipping the call to the driver. Also update validateSubpassCompatibility() to return early if skip is set to "true." That function will only return a single error code so in the event that we hit a skip case go ahead and just return early as we don't need to flag same code multiple times.
Add check for the case where renderPass and framebuffer used at CmdBeginRenderPass() time are incompatible. Add unexpected error to then allow that CmdBeginRenderPass() call so that draw-time pipeline renderPass incompatible case can then be flagged.
tobine
force-pushed
the
tobin_gh2080_rp_unexpected_errors
branch
from
a22c26f to
ff2b867
Compare
| vkCmdBeginRenderPass(m_commandBuffer->handle(), &rpbi, VK_SUBPASS_CONTENTS_INLINE); | ||
| m_errorMonitor->VerifyFound(); | ||
| // Now we want to bind the RenderPass to trigger Draw-time error so allow the error for this call | ||
| // The better way to do this to avoid the error would be to create separate FB with compatible RP |
There was a problem hiding this comment.
I think we should really do this properly.
There was a problem hiding this comment.
From private chat: This error is no good to hide because it exposes the driver to an incorrect call. It would be perfectly reasonable for the driver to explode or subtly corrupt itself as a result of this.
|
Can/should we close this MR? |
|
#2080 was closed. Is here anything worth preserving? |
|
@tobine, this repository will close for write access on Sunday, 5/13/2018. If it is pushed before that time it will be present in the follow-on Vulkan-ValidationLayers repository on Monday, otherwise a new PR will be required in the new repo. |
Fixes #2080
Refactor CmdBeingRenderPass() to only update state if no skipping call down.
Update RenderPassIncompatible test to hit another case and not have unexpected error.
Note that I updated validateSubpassCompatibility() function to return early if skip case is hit. That function only flags a single error ID so I felt the early return is reasonable since it won't kill any unique VUIDs. This also benefits testing, which isn't a reason to change validation, but given that no new errors are skipped I think the change is ok. In cases where a renderPass is incompatible for multiple reasons, this silences repeated callbacks giving the same VUID with a different reason. I can see a case where some users may want all of the callbacks but in general I'm hopeful that seeing the first error will cause users to examine the entire renderPass and fix it completely. Worst case is that a user only fixes the first error and then sees the next error on second run.