Fix broken layerCount for some types of textures #8151
Conversation
Currently mLayerCount for RenderTarget is always updated to the number of depth for attachments, which incurs unintended behaviors for some types of textures. i.e., 2d array, cubemap array, and 3d textures. Fix this by updating mLayerCount only for multiview use case.
| if (attachment.layerCount) { | ||
| FILAMENT_CHECK_PRECONDITION( | ||
| attachment.texture->getTarget() == Texture::Sampler::SAMPLER_2D_ARRAY) | ||
| << "Texture sampler must be of 2d array for multiview"; | ||
| FILAMENT_CHECK_PRECONDITION(attachment.layer + attachment.layerCount <= d) | ||
| << "For multiview, layer + layerCount cannot exceed the number of depth"; | ||
| if (layerCountMultiview) { | ||
| FILAMENT_CHECK_PRECONDITION(layerCountMultiview == attachment.layerCount) | ||
| << "layerCount for multiview should match"; | ||
| } else { | ||
| layerCountMultiview = attachment.layerCount; | ||
| } | ||
| } |
There was a problem hiding this comment.
instead of doing this, I would calculate the min and max layerCount and do the checks outside of the loop:
CHECK( min == max ) // all counts must match
CHECK( min > 1 && SAMPLER_2D || min <= 1 ) // multiview only with sampler 2D
The (attachment.layer + attachment.layerCount <= d) check can be always done (even when attachment.layerCount ==0).
| for (auto const& attachment : mImpl->mAttachments) { | ||
| if (attachment.texture) { | ||
| const uint32_t w = attachment.texture->getWidth(attachment.mipLevel); | ||
| const uint32_t h = attachment.texture->getHeight(attachment.mipLevel); | ||
| const uint32_t d = attachment.texture->getDepth(attachment.mipLevel); | ||
| const uint32_t l = attachment.layerCount; |
There was a problem hiding this comment.
technically speaking, shouldn't we always have at least one layer? Should this be layer here instead of layerCount?
There was a problem hiding this comment.
sorry I disabled auto-merge while I still have this one question.
There was a problem hiding this comment.
layerCount here means the number of layers used for multiview. This variable indicates whether we use multiview . If this value is 0, then it means we don't use multiview for this attachment.
There was a problem hiding this comment.
why not just make it the same as mLayerCount in BuilderDetails so that > 1 means that multiview is enabled? So we don't have two different interpretations of the same variable name.
There was a problem hiding this comment.
That was my original implementation. But the new interface .multiview(attachpoint, layerCount, startingLayer) requires each attachment point to have a layerCount.
| for (auto const& attachment : mImpl->mAttachments) { | ||
| if (attachment.texture) { | ||
| const uint32_t w = attachment.texture->getWidth(attachment.mipLevel); | ||
| const uint32_t h = attachment.texture->getHeight(attachment.mipLevel); | ||
| const uint32_t d = attachment.texture->getDepth(attachment.mipLevel); | ||
| const uint32_t l = attachment.layerCount; |
There was a problem hiding this comment.
why not just make it the same as mLayerCount in BuilderDetails so that > 1 means that multiview is enabled? So we don't have two different interpretations of the same variable name.
Currently mLayerCount for RenderTarget is always updated to the number of depth for attachments, which incurs unintended behaviors for some types of textures. i.e., 2d array, cubemap array, and 3d textures.
Fix this by updating mLayerCount only for multiview use case.
BUGS=[369165616]