Clip point in PointPrimitiveCollection properly #8542
Conversation
|
Thanks for the pull request @Samulus!
Reviewers, don't forget to make sure that:
|
| @@ -135,7 +135,7 @@ void main() | |||
| float nearSq = distanceDisplayConditionAndDisableDepth.x; | |||
| float farSq = distanceDisplayConditionAndDisableDepth.y; | |||
| if (lengthSq < nearSq || lengthSq > farSq) { | |||
| positionEC.xyz = vec3(0.0); | |||
| positionEC.xyz = vec3(1.0); | |||
There was a problem hiding this comment.
So instead of putting the position at the center of the earth we're putting it at 1,1,1 and that fixes it? Seems kind of odd to me and maybe masking a bigger issue?
Random idea, is the problem here that positionEC is actually a vec4 and not a vec3 and something like:
positionEC.xyz = vec3(0.0);
positionEC.w = 1.0;Is the more correct fix?
There was a problem hiding this comment.
My thinking is that positionEC.xyz = vec3(1.0); puts the point behind the camera since negative Z is forward in eye space. It should get clipped afterwards in the pipeline.
Probably a clearer fix is to do positionEC.xyz = vec3(0.0, 0.0, 1.0); and have an inline comment explaining why.
There was a problem hiding this comment.
What's the range for eye coordinates again? Is 1.0 on the near plane? Maybe use 2.0?
There was a problem hiding this comment.
Makes sense, I was reading poistionEC as earth-centric, now eye coordinates. (forgetting we use WC for world coordinates)
There was a problem hiding this comment.
It's still a little strange that positionEC.xyz = vec3(0.0); doesn't work. That should be putting the point behind the near plane. I just noticed that the bug only happens when log depth is true. And it doesn't matter if I change the log-depth near plane default from 0.1 to 1.0.
Still, positionEC.xyz = vec3(0.0, 0.0, 1.0); should be safe because it is 1 meter behind the camera, and either 1.1 or 2.0 meters behind the near plane (for log-depth and multi-frustum respectively).
|
I've verified that the clipping in the |
|
Looks like that's the only sandcastle that involves points and distance display conditions. Can confirm that both that sandcastle and the one in #7557 are fixed by this PR. I'm going to open a separate issue for standardizing how we hide/clip geometry in shaders. Looking around the code there are several different methods.. all slightly different. |
|
Opened that issue here: #8547. No need to work on it right now, I just wanted to get all my thoughts together. |
|
Thanks @Samulus! |
Fixes #7557 , still unsure if it has any unattended side effects! Posting what I have now.