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

Global constant faceDirection instead of gl_FrontFacing #17620

Closed
wants to merge 12 commits into from

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Sep 30, 2019

Still I did not make any deep-test... but I can finish. Apparently work for normal/bump maps.

Considering that Adreno GPUs not work property gl_FrontFacing, otherwise, it can be more simple.

#17598

This is a WIP

/ping @WestLangley @mrdoob

@sunag sunag closed this Sep 30, 2019
@sunag sunag reopened this Sep 30, 2019
@sunag
Copy link
Collaborator Author

sunag commented Oct 9, 2019

Considering that Adreno GPUs not work property gl_FrontFacing, otherwise, it can be more simple.

This is affirmation is true?
If yes, I think that this will prevent other bugs related, maybe not published in github.
If no, I think that the code can be more simple.

/ping @WestLangley @mrdoob

// Workaround for Adreno GPUs gl_FrontFacing bug. See #15850 and #10331

#15850 (comment)

@makc
Copy link
Contributor

makc commented Oct 9, 2019

fwiw, I have seen like 2 other drivers having problems with gl_FrontFacing, and heard of 3rd one. what I did was

#extension GL_OES_standard_derivatives : enable
vec3 fdx = dFdx(v_position.xyz);
vec3 fdy = dFdy(v_position.xyz);
vec3 faceNormal = normalize(cross(fdx,fdy));
vec3 normal = v_normal;
if (dot (normal, faceNormal) < 0.0) {
	normal *= -1.0;
}

instead of

vec3 normal = v_normal;
if (!gl_FrontFacing) {
    normal *= -1.0;
}

@WestLangley
Copy link
Collaborator

Still I did not make any deep-test... but I can finish.

Thanks, @sunag. I think this is definitely worth further study.

@makc
Copy link
Contributor

makc commented Nov 10, 2019

hmm, funny thing here. my workaround above should be the same as in #17804 (comment) but that one is causing problems for @WestLangley 🤔

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 23, 2019

Notice that #17958 has removed the Adreno workaround.

FWIW: While studying the shader code of Sketchfab, I've noticed that they use gl_FrontFacing in the shader, too. AFAIK they don't use hacks like dot( cross( S, T ), N ), so introducing a global constant seems not necessary. Hence, I vote to close the PR.

@sunag
Copy link
Collaborator Author

sunag commented Nov 26, 2019

...so introducing a global constant seems not necessary. Hence, I vote to close the PR.

@Mugen87 I think so, this PR is too outdated... Maybe in future simplify the operation a *= ( float( gl_FrontFacing ) * 2.0 - 1.0 ) for a *= faceDir can be an option for optimisation. closing...

@sunag sunag closed this Nov 26, 2019
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