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

Support read-only DSV in ITextureView. #413

Closed
wants to merge 1 commit into from

Conversation

eugeneko
Copy link
Contributor

Per discussion in Discord.

@eugeneko
Copy link
Contributor Author

It is ready now.

I have written RenderPass-based test in addition to RenderTarget-based test, but I didn't commit it because I don't think it's useful.
Render passes already have all resource transitions explicitly described, so TEXTURE_VIEW_READ_ONLY_DEPTH_STENCIL has no utility when using render passes.

@eugeneko eugeneko force-pushed the master branch 2 times, most recently from aeac1d0 to d600a0c Compare June 27, 2023 09:21
@eugeneko
Copy link
Contributor Author

PS: I've just noticed that my IDE automatically removes trailing spaces. I can revert those if requested.

@eugeneko eugeneko force-pushed the master branch 2 times, most recently from 29ed1f7 to bffc53a Compare July 3, 2023 08:19
Tests/DiligentCoreAPITest/src/ReadOnlyDepthTest.cpp Outdated Show resolved Hide resolved
Tests/DiligentCoreAPITest/src/ReadOnlyDepthTest.cpp Outdated Show resolved Hide resolved
Tests/DiligentCoreAPITest/src/ReadOnlyDepthTest.cpp Outdated Show resolved Hide resolved
Tests/DiligentCoreAPITest/src/ReadOnlyDepthTest.cpp Outdated Show resolved Hide resolved
Tests/DiligentCoreAPITest/src/ReadOnlyDepthTest.cpp Outdated Show resolved Hide resolved
@@ -340,6 +344,7 @@ struct GraphicsPipelineDesc
SubpassIndex == Rhs.SubpassIndex &&
ShadingRateFlags == Rhs.ShadingRateFlags &&
DSVFormat == Rhs.DSVFormat &&
UseReadOnlyDSV == Rhs.UseReadOnlyDSV &&
Copy link
Contributor

@MikhailGorobets MikhailGorobets Jul 5, 2023

Choose a reason for hiding this comment

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

Still, I don't like the idea of additional fields. Can we find a value for a render pass based on DepthStencilState?
@TheMostDiligent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do that, but it will (1) silently break existing user code, and (2) will require extra render target changes in user code. Which is the trade off I am personally not willing to take.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, I don't like the idea of additional fields.

Neither do I.

Can we find a value for a render pass based on DepthStencilState?

Probably. We can check the DepthWriteEnable flag and create the read-only depth. However this will create a problem with binding normal DSV and using it with the read-only depth pipeline.
I was thinking to have two views inside each DSV - one normal, one readonly, but then again there is a problem with mixing normal and read-only pipelines.

In current approach, what happens if you bind read-only depth pipeline, set render targets and then use read-write depth pipeline?

We have the SetRenderTargetsExt call. It looks like we will need to add ReadOnlyDepth flag to the struct it takes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheMostDiligent
Sorry, I don’t follow your question. The act of binding pipeline and setting render targets are still independent from each other. It’s okay if you have mismatched pipeline bound at some point as long as you don’t use it.

This is the reason why I don’t want to make automatic flagless detection — it’s gonna mess this up. We will have to have two pipelines in pipeline object if depth write is disabled. One if user decides to use PSO with read write depth and another if with read only. And this will also create non trivial engagement between setting PSO and render target. I would even say that effective PSO will have to be resolved in prepareDraw.

@@ -81,12 +81,14 @@ class TexFormatToViewFormatConverter
static_assert(TEXTURE_VIEW_RENDER_TARGET == 2, "TEXTURE_VIEW_RENDER_TARGET == 2 expected");
static_assert(TEXTURE_VIEW_DEPTH_STENCIL == 3, "TEXTURE_VIEW_DEPTH_STENCIL == 3 expected");
static_assert(TEXTURE_VIEW_UNORDERED_ACCESS == 4, "TEXTURE_VIEW_UNORDERED_ACCESS == 4 expected");
static_assert(TEXTURE_VIEW_READ_ONLY_DEPTH_STENCIL == 6, "TEXTURE_VIEW_READ_ONLY_DEPTH_STENCIL == 6 expected");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add four flags at once? @TheMostDiligent

Depth: Read, Stencil: Read
Depth: Read, Stencil: Write
Depth: Write, Stencil: Read
Depth: Write, Stencil: Write 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TEXTURE_VIEW_DEPTH_STENCIL directly corresponds to RESOURCE_STATE_DEPTH_WRITE.
TEXTURE_VIEW_READ_ONLY_DEPTH_STENCIL directly corresponds to RESOURCE_STATE_DEPTH_READ.

What would "Depth: Read, Stencil: Write" and "Depth: Write, Stencil: Read" correspond to?

Copy link
Contributor

Choose a reason for hiding this comment

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

In Vulkan there are such flags, in d3d12 respectively there is a way to create such combinations

 VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL
 VK_IMAGE_LAYOUT_DEPTH_READ_ONLY_STENCIL_ATTACHMENT_OPTIMAL
 VK_IMAGE_LAYOUT_DEPTH_ATTACHMENT_STENCIL_READ_ONLY_OPTIMAL
 VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL

Copy link
Contributor Author

@eugeneko eugeneko Jul 5, 2023

Choose a reason for hiding this comment

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

Oh, I didn't know that existed.
I agree that having those may be helpful in general. Just not in my specific usecase.
However, adding those would require extending RESOURCE_STATE, and refactoring that I am afraid I am not quite qualified to do.

If you people decide you want to have all 4 combinations, I would suggest to extract it into separate task, and I will tweak this PR to minimize future changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's wait and see what Egor says. Perhaps it would be better to switch to the option with four states and also consider the possibility of adding dynamic rendering, so that we don't have to add additional fields to the Pipeline State.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think adding four flags makes sense for completeness.
Dynamic rendering would be and ideal solution here, only that this is a very big change.

@eugeneko eugeneko force-pushed the master branch 2 times, most recently from e45bd61 to 3ea255a Compare July 5, 2023 08:42
@eugeneko eugeneko force-pushed the master branch 3 times, most recently from e1936b0 to 15459f4 Compare July 8, 2023 18:32
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.

3 participants