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

Useless/Wrong GLSL code in depth test #5621

Closed
SunBlack opened this issue Jul 12, 2017 · 2 comments · Fixed by #5735
Closed

Useless/Wrong GLSL code in depth test #5621

SunBlack opened this issue Jul 12, 2017 · 2 comments · Fixed by #5735

Comments

@SunBlack
Copy link

#5166 introduced depth test for billboards, points and labels. But the GLSL code makes no sense:

gl_Position.z = min(gl_Position.z, gl_Position.w);
bool clipped = gl_Position.z < -gl_Position.w || gl_Position.z > gl_Position.w;

This code I found in BillboardCollectionVS.glsl and PointPrimitiveCollectionVS.glsl (didn't found depth test code for labels now).

Case 1 (gl_Position.z < gl_Position.w):

gl_Position.z = min(0.0, 1.0); //gl_Position.z => 0.0, if gl_Position.z = 0.0 and gl_Position.w = 1.0
bool clipped = 0.0 < -1.0 || 0.0 > 1.0; //false

Case 2 (gl_Position.z > gl_Position.w):

gl_Position.z = min(1.0, 0.0); //gl_Position.z => 0.0, if gl_Position.z = 1.0 and gl_Position.w = 0.0
bool clipped = 0.0 < -0.0 || 0.0 > 0.0; //false

Case 3 (gl_Position.z == gl_Position.w):

gl_Position.z = min(0.0, 0.0); //gl_Position.z => 0.0, if gl_Position.z = 0.0 and gl_Position.w = 0.0
bool clipped = 0.0 < -0.0 || 0.0 > 0.0; //false

So clipped will be always false.

PS: Sorry, I have no idea how you handle code review if code is already merged ;).

@emackey
Copy link
Contributor

emackey commented Jul 12, 2017

This sounds like it could be the cause of #5501 and/or #5331.

/cc @rahwang

@emackey
Copy link
Contributor

emackey commented Aug 10, 2017

Thanks for pointing out this code, @SunBlack. It's fixed in master, and will be released with 1.37.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants