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

PMREMGenerator: Revert NearestFilter change. #23517

Closed
wants to merge 1 commit into from
Closed

PMREMGenerator: Revert NearestFilter change. #23517

wants to merge 1 commit into from

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Feb 17, 2022

Related issue: #17855 #23468 #23511

Description

I couldn't stop thinking about this and while having lunch I realized I could easily "reproduce" the problem by setting anisotropy: 16 in the PMREM render target.

Sure enough:

Screen Shot 2022-02-17 at 6 09 47 PM

What I learned:

  1. _textureToCubeUV is not affected.

  2. _applyPMREM is affected:

Screen Shot 2022-02-17 at 6 15 29 PM

  1. Even if we skip _applyPMREM we get seams:

Screen Shot 2022-02-17 at 6 19 23 PM

  1. TheminFilter: NearestFilter workaround doesn't fully remove the seams:

Screen Shot 2022-02-17 at 6 23 04 PM

So the problem doesn't only affect the PMREM atlas but also the sampling from the material shader plus we get different renders.

After playing with all this for a while I'm starting to believe we should not add a workaround for this and consider this a "user error" unless someone can prove that the NVIDIA panel come with that override anisotropy setting enabled by default.

PS: @elalish Are you confident that the scissorTest and scissor() calls are needed? I didn't see any change by removing them.

@mrdoob mrdoob added this to the r138 milestone Feb 17, 2022
@elalish
Copy link
Contributor

elalish commented Feb 17, 2022

Hmm, I can't remember anymore. I think at one point I didn't know about scissor and only setting the viewport gave me errors I struggled mightily to debug, and after that I swore to always set both. I doubt I tested the difference on this one.

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 17, 2022

Hmm...

Now I'm realizing we dealt with this years ago already in #17855.

@JordyvanDortmont said this:

Nvidia's default settings enable anisotropic filtering in the control panel for the GTX 1070 and GTX 2060 (Mobile). When you disable anisotropic filtering for Chrome and Firefox, the seams are gone. This is unfortunate, because most users won't disable anisotropic filtering manually, so they'll see the seams.

And @sciecode said this:

To avoid cube map seams, I create an extra pixel around each face. This way when the cube map is sampled by an application later(with a little care by sampling the centre of the texel), the extra 1 border of pixels makes sure that there is no seams artifacts present.

Clearly the creators knew about the possibility of cube map seams and devised a solution to it, but perhaps they failed to considered that anisotropic wouldn't guarantee sampling from the center of the texel.

@elalish
Copy link
Contributor

elalish commented Feb 17, 2022

You're quoting someone quoting me there you know... Anyway, looks like scissor did help? At least at one time: #18045

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 18, 2022

We could try to set anisotropy: 16 temporarily and try to "fix" _applyPMREM.

Another solution would be to do like playcanvas (playcanvas/engine#3783) and use equirectangular in the atlas instead of cube sides:

image

There'd be still seams, but less seams I guess...

Not sure how to solve this.

@elalish
Copy link
Contributor

elalish commented Feb 18, 2022

I think I know what the real problem is, now that it's clear it's caused by anisotropic filtering rather than some random driver bug. I'll work up a PR.

@mrdoob mrdoob mentioned this pull request Feb 22, 2022
@mrdoob mrdoob removed this from the r138 milestone Feb 23, 2022
@mrdoob mrdoob deleted the pmrem branch February 23, 2022 00:05
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.

2 participants