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

OIT aliasing fixes #12916

Merged
merged 3 commits into from
Sep 9, 2022
Merged

OIT aliasing fixes #12916

merged 3 commits into from
Sep 9, 2022

Conversation

CraigFeldspar
Copy link
Member

  • Implemented multisampling for DepthPeeling (works but has artifacts along triangle edges because we need to access multisampled depth in the shader, which cannot be done for now)
  • Made depth peeling compatible with post processes to be able to use FXAA post process

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@sebavan sebavan requested a review from Popov72 August 30, 2022 16:07
Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

Great job!

What about WebGPU: does it work there too? If not and you are not able to fix it, you should create an issue for me so that I can have a look when I come back.

[EDIT] It probably won't work because in WebGPU textures are recreated when the sample count is updated and it's not possible to simply update a texture with the new value, the texture must be recreated. That means you will need to update the loop below so that the sample count is not updated multiple times for the same texture:

        for (let i = 0; i < 2; i++) {
            this._depthMrts[i].samples = value;
            this._colorMrts[i].samples = value;
        }

@Popov72
Copy link
Contributor

Popov72 commented Aug 31, 2022

we need to access multisampled depth in the shader, which cannot be done for now

Is it a limitation of WebGL or Babylon.js? If the latter, maybe we can have a look and see how to add the support (@sebavan)?

@sebavan
Copy link
Member

sebavan commented Sep 5, 2022

@CraigFeldspar, any news on this PR ?

@CraigFeldspar
Copy link
Member Author

@CraigFeldspar, any news on this PR ?

Hey Seb, looking at it shortly

@CraigFeldspar
Copy link
Member Author

we need to access multisampled depth in the shader, which cannot be done for now

Is it a limitation of WebGL or Babylon.js? If the latter, maybe we can have a look and see how to add the support (@sebavan)?

So, this PR has 2 parts :

  • Enabling postprocesses with depth peeling (so we can use FXAA)
  • Enabling MSAA with depth peeling

But after the fact, I realized that MSAA has absolutely no value in our case since in WebGL2, texelFetch doesn't allow to fetch a specific sample in a multisampled fragment - see https://registry.khronos.org/OpenGL-Refpages/gl4/html/texelFetch.xhtml, with the gSampler2DMS overload. This is only OpenGL.

Therefore we will always read and write 1 depth per pixel in the depth layers, so we got no value from multisampling.

But since I already spent time on it, and it still works, even if it's unusable, I figured out it's better to leave it there until the time we can really fetch these multiple samples. That's also why I added the comment that MSAA requires RGBA, so we can be reminded if we pick back up the implementation.

I will hide the samples setter and getter on the depthPeelingRenderer, so no one mistakenly activates MSAA and wonders why it doesn't work (and makes everything slower)

@CraigFeldspar
Copy link
Member Author

Great job!

What about WebGPU: does it work there too? If not and you are not able to fix it, you should create an issue for me so that I can have a look when I come back.

[EDIT] It probably won't work because in WebGPU textures are recreated when the sample count is updated and it's not possible to simply update a texture with the new value, the texture must be recreated. That means you will need to update the loop below so that the sample count is not updated multiple times for the same texture:

        for (let i = 0; i < 2; i++) {
            this._depthMrts[i].samples = value;
            this._colorMrts[i].samples = value;
        }

I think that given my above comment, we should anyway leave the multisample as it is, and pick it back if/when WebGL or WebGPU allows for multisampled texelFetch.
But maybe WebGPU already supports that ? Do you want me to create an issue to investigate ?

@CraigFeldspar
Copy link
Member Author

@sebavan ready for round 2 :)

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@sebavan
Copy link
Member

sebavan commented Sep 7, 2022

I leave @Popov72 comment on it but it loooks fine to me, @Popov72 might know about the limitations regarding WebGPU

@Popov72
Copy link
Contributor

Popov72 commented Sep 7, 2022

It's fine to me, let's just comment out the getter and setter and we are good to go.

@CraigFeldspar
Copy link
Member Author

It's fine to me, let's just comment out the getter and setter and we are good to go.

done !
@sebavan we should be good to go 🚀

@sebavan sebavan merged commit 38d3ad9 into BabylonJS:master Sep 9, 2022
RaananW pushed a commit that referenced this pull request Dec 9, 2022
OIT aliasing fixes

Former-commit-id: 694e91c752f2b23075061383d53f32a44a8a85b2
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.

4 participants