-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fix log depth woes #8600
Fix log depth woes #8600
Conversation
At least with log depth enabled. Without it, things are still strange.
Implement terrible approximation of polygonOffset. And better account for the near plane because with multi-frustum + log depth, the near plane matters (unlike with single frustum log depth).
Thanks for the pull request @kring!
Reviewers, don't forget to make sure that:
|
I spent about 20 minutes poking around at this, and I couldn't reproduce any of the logdepth problems that I know of. This looks really promising! Although from the screenshot above it looks like maybe there's a rivalry with the West-coast Australians going on? 😄 |
I searched github for log depth issues and tried them all out on this branch, but there's no way I got everything. The common theme is polylines still have issues but everything else seems to be fixed. This PR may set the record for most closed issues all at one. Some of these are also in the PR description, but I just wanted to put them in a single place. Add more as you see them. #6103 (fixed) |
Awesome, thanks Sean! I'll take a look at the polyline issues to see if there's a reasonable fix there, too. |
The 7 remaining test failures are all in So I don't know if my log depth changes just make the problem more obvious, or fixed something that previously made the tests just coincidentally pass, or what. Any chance someone who understands the classification code better could take a quick look? It would be really great to get this PR into next week's CesiumJS release, and a shame to hold it up with a problem like this. |
I looked at performance using the following Sandcastle examples:
In all cases, I used my integrated GPU in Chrome, used the built version of Sandcastle, added the Cesium Inspector mixin, and opened in a new window. I put master on one half of my screen and this branch on the other, making sure the sizes of both were identical. Then I turned on the Inspector performance display and watched the frame rate on each side. They were indistinguishable. |
@lilleyse what are your thoughts here? This is definitely an exciting and high impact change, so it would be nice to get it in. From my perspective, if it's ready to be merged and we do it today, that should be more than enough time to catch any surprises before next release. |
I don't think this needs to be held back because of WebGL 2. @kring is there a fix on the horizon for polylines? I'm actually still ok with merging before then if all the other artifacts are fixed. |
There's no easy/obvious fix for the polyline problem. I'm still looking at it, and hope to get it fixed eventually, but I think we should get this in in the meantime. So maybe just exclude the 7 failing WebGL2 tests for now? |
I've disabled the broken WebGL2 tests for now, and added a comment referencing #8629. I think this is ready! |
I spot checked sandcastle and only hit one bug: point cloud eye dome lighting crashes if log depth is false: Sandcastle |
The code looks good to me! |
Point cloud EDL without log depth is fixed, and those unnecessarily-deprecated uniforms have been removed. |
I pushed an eslint fix: e979008 Will merge after CI passes |
Oops, thank you! |
When log depth is enabled, Cesium has problems with large geometries near the camera getting clipped. #6573 and #7189 are some examples of this.
This is a known problem with the log depth approach when it is implemented using only a modification to the vertex shader. It happens when a large triangle's vertices are outside the frustum, but the triangle itself passes through it. The vertex shader adjusts the clip space Z coordinate to be logarithmic. But then the GPU, in between the vertex and fragment shaders, linearly interpolates the Z coordinates of two vertices to see if they intersect the frustum. Linearly interpolating logarithmic values is not a valid thing to do. It clearly produces incorrect results, and in this case it results in the GPU deciding the triangle does not intersect the frustum. So the triangle generates no fragments, and we see giant ugly holes in our geometry.
It can be "fixed" by making sure geometry is small-ish, so that interpolating logarithmic values linearly doesn't produce such wildly wrong results. That would be challenging in Cesium, and feels a bit error-prone in general.
The other approach is to correctly compute the logarithmic depth values in the fragment shader, rather than relying on its interpolation across triangles. The main downsides to this approach are that it requires the
GL_EXT_frag_depth
extension, and that writing depth in the fragment shader disables early-Z optimizations by the GPU.But despite the downsides we think this is a win for Cesium versus the alternative (more frustums), so Cesium has since the beginning written log depth in the fragment shader. Not just when it's needed, but always, whenever we're using log depth. So wait, why do we still have problems with large geometry being clipped?
Well, remember I said the large geometry was getting clipped before it ever got to the fragment shader? So, no fragment shader change, including writing log depth, is going to stop that. It's too late. To fix the problem with large geometries, it's not enough to write log depth in the fragment shader. We also have to not write log depth in the vertex shader.
Cesium actually has a mechanism for doing this, by defining
DISABLE_GL_POSITION_LOG_DEPTH
in the vertex shader. Terrain would define this for upsampled tiles and fill tiles, and other parts of Cesium would try to turn it on at various times as well. But anytime we had large geometry and didn't#define DISABLE_GL_POSITION_LOG_DEPTH
, we'd get geometry clipping problems.But here's the thing: because we're always writing log depth in the fragment shader, we shouldn't ever need to write log depth in the vertex shader. So why not just have
#define DISABLE_GL_POSITION_LOG_DEPTH
on all the time, always use linear depth in the clip space Z coordinate, and no more large geometry artifacts. Simple!Except that creates new artifacts when zoomed out:
What is that?!
After lots of investigation, I came to the conclusion that it's this:
When using log depth, we use a huge near/far ratio. It's so large that vertices that are nowhere near the far plane are frequently deemed to be on the wrong side of it, because so much error is introduced in the projection transformation and the perspective divide. When this happens, the triangle containing the vertex gets clipped before we get to the vertex shader.
So here's my solution, and you're going to have to hold onto your hat for this one: in this PR, we clamp the clip space Z coordinate to be between the near and far planes. That way we always get fragments, and so we can properly evaluate their depth in the fragment shader. Sounds crazy, right? Let me try to convince you:
Now I'm slightly worried about what this will do in a multi-frustum + log depth case, where you ideally wouldn't pass all the geometry overlapping two adjacent frustums through the fragment shader for each frustum. Perhaps we could only clamp depth values that are "reasonably" near the near and far planes to the planes. But that starts to get pretty error prone again. The current approach is reliable, and we can optimize if we can find a scenario where we actually can measure a negative performance impact from it.
So, with these changes, large geometry is always rendered perfectly, and there are no weird z-clipping artifacts like in the image above.
I also spruced up the log depth computation to hopefully make it work better in the (rare?) case that we do have multiple frustums with log depth. Previously, the second frustum wouldn't get as much precision as the first because the log depth values even in the second frustum were measured from a distance of 0, rather than the near plane. In the first frustum those two are basically the same, in the second frustum they're not at all. Actually if I'm thinking about this right, having a second frustum with log depth was totally pointless before. But it's possible I'm not thinking about it right.
I also implemented polygon offset for log depth. This is used in the bivariate depth test for 3D Tiles, and previously worked by turning off fragment shader depth writes (the only place in Cesium that did this!) in order to make fixed function polygonOffset work. But turning off FS depth writes is no longer an option. Please review this part carefully, in writeLogDepth.glsl, cause there's a decent chance I don't know what I'm doing.
Fixes #6573
Fixes #7189
Fixes #6103
Fixes #8465
Fixes #8562
To do before merging: