Closed mdagois closed 8 months ago
@asuessenbach I had to update a few copyright year that are not from my company (to please the copyright quality check). Is it OK?
When starting this sample, I get lots of warnings from the Vulkan Configurator:
Validation Warning: [ UNASSIGNED-BestPractices-TransitionUndefinedToReadOnly ] | MessageID = 0x9f63e654 | vkCmdPipelineBarrier(): pImageMemoryBarriers[0] VkImageMemoryBarrier is being submitted with oldLayout VK_IMAGE_LAYOUT_UNDEFINED and the contents may be discarded, but the newLayout is VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL, which is read only.
on the call tovkb::image_layout_transition
in oit_depth_peeling.cpp, line 163.
I think I fixed this issue. I was transitioning a yet unused depth texture in the first gather pass. Could you please check again?
I'm seeing quite a lot of flickering triangles when I reduce the object opacity. I'll see if I can attach a small video.
I'm seeing quite a lot of flickering triangles when I reduce the object opacity. I'll see if I can attach a small video.
Oh, interesting. Thanks for the video!
Could you tell me which GPU you tested on? The algorithm should never produce those flickers, but I have been quite aggressive with the barriers and it may be causing sync issues.
I only ever tested on AMD GPU.
Could you tell me which GPU you tested on? The algorithm should never produce those flickers, but I have been quite aggressive with the barriers and it may be causing sync issues.
It's our own Broadcom Videocore VII GPU
So, the flickering looks like it might be an issue on our side, which I'm investigating.
So, the flickering looks like it might be an issue on our side, which I'm investigating.
Hey @gary-sweet, Did you find out if the flickering issue was on your driver side? Or might it be in the sample?
Did you find out if the flickering issue was on your driver side?
@mdagois The flickering is an issue on our side related to a tiny mismatch between the fixed-function Z value and the value of gl_FragCoord.z in certain circumstances on older hardware.
While I was pondering why we hadn't seen such an issue previously, I started to think about how depth-peeling is usually implemented, and I would suggest that it normally just uses the fixed-function depth comparison operations rather than having to modify the shaders to do the depth comparison. Having to modify the fragment shaders to be able to use them with depth-peeling isn't an optimal solution, so we may want to think about modifying this sample (which is presumably supposed to be showing good-practice) to use fixed-function depth instead.
Did you find out if the flickering issue was on your driver side?
@mdagois The flickering is an issue on our side related to a tiny mismatch between the fixed-function Z value and the value of gl_FragCoord.z in certain circumstances on older hardware.
While I was pondering why we hadn't seen such an issue previously, I started to think about how depth-peeling is usually implemented, and I would suggest that it normally just uses the fixed-function depth comparison operations rather than having to modify the shaders to do the depth comparison. Having to modify the fragment shaders to be able to use them with depth-peeling isn't an optimal solution, so we may want to think about modifying this sample (which is presumably supposed to be showing good-practice) to use fixed-function depth instead.
Doing that also has other advantages, like only needing a single depth-buffer - not the eight that you have now for example.
Ah, I see. That sounds more like a hardware issue than an algorithm one though. I doubt any modern hardware would have that problem.
The original version of depth peeling (link below) relies on both fixed-function Z and a depth texture read to "fake" having a second depth buffer. The paper is from the early 2000's, so the algorithm was working on pretty old hardware. I believe the sample is doing exactly what the paper presents. It uses the fixed-function Z to get the closest layer, but use the texture read (from the depth buffer of the previous gather pass) to reject fragments that were already rendered. There are two assumptions that makes this work afaik:
From what I understand, the old hardware you are running the sample on has an issue with assumption (2). The problem is that other depth peeling algorithms do rely on the same trick. At least, dual depth peeling does. I believe back-first depth peeling would too, otherwise, I am not sure how it could discard back layers that have already been rendered.
Even then, the advantage of the original depth peeling is that you can stop early. Dual depth peeling or back first depth peeling cannot do that. They of course have other advantages, like less gather passes or lower resource usage.
Ultimately, I think the sample should probably implement all three algorithms, and let the user choose which algorithm to use. I can add a TODO if necessary and list pros/cons of each technique to the README. I just cannot commit a given timeline for implementing those though, as I mostly do that on my free time.
Btw, the algorithm does not use 8 depth buffers, but just 2. It does however use 8 color attachments. I assume that's what you meant.
Original depth peeling: https://developer.download.nvidia.com/SDK/10/opengl/src/dual_depth_peeling/doc/DualDepthPeeling.pdf
The original version of depth peeling (link below) relies on both fixed-function Z and a depth texture read to "fake" having a second depth buffer .................
I was actually just writing to say that what I had suggested wasn't sensible having thought about it more (and that I realised only 2 depth buffers were in use) and then noticed your message. I clearly wasn't having a good day yesterday! Apologies for that.
The original version of depth peeling (link below) relies on both fixed-function Z and a depth texture read to "fake" having a second depth buffer .................
I was actually just writing to say that what I had suggested wasn't sensible having thought about it more (and that I realised only 2 depth buffers were in use) and then noticed your message. I clearly wasn't having a good day yesterday! Apologies for that.
No problem at all! I think you and @asuessenbach made actually good points with those other algorithms. They should definitely be added through some dropbox in the GUI.
Is there any more issue to fix? I think I can support more depth peeling algorithms, but I'd rather do it from a base that's already merged.
By default, object opacity is at 1.0. So in order to actually see transparency, one needs to lower that first. Can you put that at e.g. 0.75 by default instead?
I set it to 0.5. The background is now visible from the start through the transparent geometry.
And the sample code does not have a single code comment. The readme does have some information, but it's light-weight. Since we want people to learn from our samples, at least some code comments should be added so people have an easier time following the code.
I added comments in the fragment shaders and in the function that records the command buffer. Please let me know if that's enough.
And a minor one: The readme for the sample has a "Tests" paragraph. Can you remove that? It doesn't fit into that readme.
I removed the section.
One small thing I realised recently is that you don't actually have any intersecting geometry in the sample. Depth-peeling works even with intersecting geometry so I wonder if it's worth adding a separate object that intersects the first one to show that?
If you're planning to enhance the sample in a later PR it could wait until then. Maybe a checkbox that toggles an intersecting object for example.
One small thing I realised recently is that you don't actually have any intersecting geometry in the sample. Depth-peeling works even with intersecting geometry so I wonder if it's worth adding a separate object that intersects the first one to show that?
If you're planning to enhance the sample in a later PR it could wait until then. Maybe a checkbox that toggles an intersecting object for example.
Oops, you are right! And it is true for the other OIT sample too (oit_linked_lists). I will make a new PR once this one is in to fix the issue for both sample.
Merging - 3 approvals
Description
Added a new sample that implements an order-independent transparency algorithm using depth peeling.
General Checklist:
I have tested every sample to ensure everything runs correctly[x] This PR describes the scope and expected impact of the changes I am making
Note: The Samples CI runs a number of checks including:
Sample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:
If the sample is vendor-specific, I have tagged it appropriately