Add support for Extended Dynamic State 3 #3071
Conversation
|
|
||
| ret.colorBlend.blends.resize(p.attachments.size()); | ||
| for(size_t i = 0; i < p.attachments.size(); i++) | ||
| { | ||
| ret.colorBlend.blends[i].enabled = p.attachments[i].blendEnable; | ||
| if(i < state.colorBlendEnable.size()) | ||
| ret.colorBlend.blends[i].enabled = state.colorBlendEnable[i] != VK_FALSE; |
There was a problem hiding this comment.
Similar to the earlier comment, this should reset the values to a default (in this case false, similar for other values) if it's not specified.
I also had to check the VUs and I'm not 100% sure - can you clarify what the behaviour should be regarding these counts matching? In most cases for vulkan the counts are required to match when specified in multiple different places, but the VU for this one seems to say you only need to specify a value for "active color attachments". Does that mean if there are attachments which are unused this check may be necessary and it would be valid for the application to have only specified some smaller number of values in these color blend arrays?
There was a problem hiding this comment.
The changes here now worry me - as they do the opposite from what they did before by assuming that the arrays are all matching in size. I couldn't find any VU actually requiring that which is why I was looking for clarification in the comment above.
Checking again, I can also see this:
attachmentCountis the number ofVkPipelineColorBlendAttachmentStateelements inpAttachments. It is ignored if the pipeline is created with VK_DYNAMIC_STATE_COLOR_BLEND_ENABLE_EXT, VK_DYNAMIC_STATE_COLOR_BLEND_EQUATION_EXT, and VK_DYNAMIC_STATE_COLOR_WRITE_MASK_EXT dynamic states set, and eitherVK_DYNAMIC_STATE_COLOR_BLEND_ADVANCED_EXTset oradvancedBlendCoherentOperationsis not enabled on the device.
Which seems like it's possible that p.attachments could be entirely empty even though the pipeline does have attachments which it expects to write to (and possibly blend to) depending on dynamic state.
This definitely needs a solid understanding of where these counts are intended to match and where they can be different, as it is right now this doesn't look correct to me.
stephen-bjore
force-pushed
the
extended-dynamic-state-3-support
branch
from
d5fb0e6
to
a627dd6
Compare
|
Updated the branch based on the comments above. Please let us know if you have additional comments, or would like responses to any of the previous comments. |
|
There's one outstanding issue that I think needs to be resolved - what happens with matching counts for dynamic arrays of state. |
stephen-bjore
force-pushed
the
extended-dynamic-state-3-support
branch
2 times, most recently
from
81e7f15
to
289d586
Compare
|
Updated again based on previous comments and conversations. As always, feedback is appreciated! |
| for(size_t i = 0; i < p.attachments.size(); i++) | ||
| // find size if no static pipeline state | ||
| size_t numAttach = std::max({state.colorBlendEnable.size(), state.colorBlendEquation.size(), | ||
| state.colorWriteEnable.size(), state.colorWriteMask.size()}); |
There was a problem hiding this comment.
Something I appreciate might feel nitpicky, generally I avoid std:: where possible. This std::max overload might be supported as the docs seem to suggest it's C++11, but I would bet good money that Android will probably ruin that somehow as it always does - I don't know if the Android compilers we target fully support C++11 as I've had problems with that in the past.
I'd rather you use RDCMAX here and do it one by one for each value.
There was a problem hiding this comment.
This has been changed in the latest update.
stephen-bjore
force-pushed
the
extended-dynamic-state-3-support
branch
from
289d586
to
d9f7053
Compare
Description
Add support for Extended Dynamic State 3.