Clears globe depth even when terrain depth testing on #12821
Conversation
|
Thank you for the pull request, @mzschwartz5! ✅ We can confirm we have a CLA on file for you. |
| @@ -317,18 +317,17 @@ void main() | |||
| if (lengthSq < disableDepthTestDistance) { | |||
There was a problem hiding this comment.
I removed the labelTranslate / pass in 0 now. I honestly don't know the original reason for it - the visual results seem much cleaner to me without it. With it, there's like padding around the billboards that causes it to render way after it's occluded by terrain.
|
This definitely seems improved at the global scale. However I noticed some weird "flickering" when zooming more locally. I assume this is just due to sampling or terrain loading or I just got lucky with the billboard "point" between some rocks that only obstruct it at certain angles. Probably still better than it was but thought it was worth noting. simplescreenrecorder-2025-08-19_11.45.00.mp4I also wanted to confirm, is this expected to impact 3dTiles or only Terrain? I'm still seeing it clip with the google 3d tiles |
|
I'm inclined to think the flickering predates this PR, as my changes should only make things more permissive on a global scale. Unless the Interesting that it's still clipping on 3D tiles... I expected this to work on both terrain and 3D tiles, and I thought it did, in my testing. edit- nope, this fix definitely only works for terrain. Will continue investigation into whether it can be extended to 3D tiles. |
This reverts commit 4c186a2.
| @@ -315,6 +314,8 @@ function BillboardCollection(options) { | |||
| ]; | |||
|
|
|||
| this._highlightColor = Color.clone(Color.WHITE); // Only used by Vector3DTilePoints | |||
| this._coarseDepthTestDistance = 50000.0; | |||
| this._threePointDepthTestDistance = 5000.0; | |||
There was a problem hiding this comment.
Previously, the distance at which the three-point depth testing technique occurred was set to the disableDepthTestDistance, or 5km by default if not set. Setting it to disableDepthTestDistance was a strange and buggy choice, though, because the original author misinterpreted the setting. They reversed the meaning to only 3-point depth test within that distance.
Since the original intent was to make close up viewing of billboards clamped to terrain better, I think always having it be 5km (while respecting the disableDepthTestDistance value) is okay.
| @@ -1365,9 +1376,6 @@ function writeCompressedAttribute3( | |||
| const clampToGround = | |||
| isHeightReferenceClamp(billboard.heightReference) && | |||
| frameState.context.depthTexture; | |||
| if (!defined(disableDepthTestDistance)) { | |||
| disableDepthTestDistance = clampToGround ? 5000.0 : 0.0; | |||
There was a problem hiding this comment.
This is what I mentioned above. Removing in favor of always using a 5km distance.
| @@ -2023,8 +2031,7 @@ BillboardCollection.prototype.update = function (frameState) { | |||
| ) { | |||
| this._rsOpaque = RenderState.fromCache({ | |||
| depthTest: { | |||
| enabled: true, | |||
| func: WebGLConstants.LESS, | |||
| enabled: false, | |||
There was a problem hiding this comment.
The crux of this PR's change. Disable automatic depth testing and do it manually in the shaders so we have more control of how it happens.
Note: Given the comment below about translucency, I'm a little wary I may have introduced regressions, as I don't do a LEQUAL check in the shaders, but it seems to work better this way (see test case in PR description)
There was a problem hiding this comment.
Also note- we still leave write to the depth buffer on, we just don't test against it automatically.
| in vec4 v_compressed; // x: eyeDepth, y: applyTranslate & enableDepthCheck, z: dimensions, w: imageSize | ||
| const float SHIFT_LEFT1 = 2.0; | ||
| const float SHIFT_RIGHT1 = 1.0 / 2.0; | ||
|
|
There was a problem hiding this comment.
These changes, among several others in this PR, are just moving variables outside of macro checks. These used to be only used if #ifdef FRAGMENT_DEPTH_CHECK was defined, but now they're used all the time
| float getGlobeDepthAtCoords(vec2 st) | ||
| { | ||
| float logDepthOrDepth = czm_unpackDepth(texture(czm_globeDepthTexture, st)); | ||
| if (logDepthOrDepth == 0.0) | ||
| { | ||
| return 0.0; // not on the globe | ||
| } |
There was a problem hiding this comment.
This isn't a new function as much as it is breaking the old getGlobeDepth function into two pieces, so I can call this piece separately.
| float temp2 = (temp - floor(temp)) * SHIFT_LEFT1; | ||
| bool enableDepthTest = temp2 != 0.0; | ||
| bool applyTranslate = floor(temp) != 0.0; | ||
| if (!enableDepthCheck) return; |
There was a problem hiding this comment.
More moving of variables outside compiler macros... And now we check if enableDepthCheck is turned on outside of the macro. This used to just mean "is the 3-point depth check enabled," but I repurposed it to mean "is depth testing enabled at all".
If it is, we do all forms of depth testing below (3-point, and regular)
There was a problem hiding this comment.
I would recommend reviewing the VS before the FS.
| vec2 fragSt = gl_FragCoord.xy / czm_viewport.zw; | ||
| float globeDepth = getGlobeDepthAtCoords(fragSt); | ||
| if (globeDepth != 0.0) { | ||
| float distanceToEllipsoidCenter = -length(czm_viewerPositionWC); // depth is negative by convention | ||
| float testDistance = (eyeDepth > -u_coarseDepthTestDistance) ? globeDepth : distanceToEllipsoidCenter; | ||
| if (eyeDepth < testDistance) { | ||
| discard; | ||
| } | ||
| } |
There was a problem hiding this comment.
Again, the crux of this PR. This the manual depth testing that replaces automatic depth testing. Except, now, with manual control, we can depth test against a flat plane at global scales.
| #if defined(FRAGMENT_DEPTH_CHECK) || defined(VERTEX_DEPTH_CHECK) | ||
| float eyeDepth = positionEC.z; | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Was never really necessary - eyeDepth is used in one place exactly. The point was to store off the value of positionEC.z before it gets mutated, but that's not necessary either. Now (down below) we just use this value where it's needed, but before the important mutations occur. (...but after we potentially set eye depth to 0 if the billboard fails the 3-point test).
| float enableDepthCheck = 1.0; | ||
| #ifdef DISABLE_DEPTH_DISTANCE | ||
| float disableDepthTestDistance = compressedAttribute3.z; | ||
| if (disableDepthTestDistance == 0.0 && czm_minimumDisableDepthTestDistance != 0.0) | ||
| { | ||
| disableDepthTestDistance = czm_minimumDisableDepthTestDistance; | ||
| } | ||
|
|
||
| if (lengthSq < disableDepthTestDistance || disableDepthTestDistance < 0.0) | ||
| { | ||
| enableDepthCheck = 0.0; | ||
| } |
There was a problem hiding this comment.
I repurposed enableDepthCheck - it used to mean "is the three-point depth check applied?" But now it just means what it sounds like "do we depth test at all?"
To accommodate this change, I had to move some chunks upwards in the vert shader. But it's actually more appropriate in this location anyway; i.e. all the disableDepthTestDistance logic is together now.
| float depthsilon = 10.0; | ||
|
|
||
| vec2 labelTranslate = textureCoordinateBoundsOrLabelTranslate.xy; |
There was a problem hiding this comment.
I honestly don't know what the purpose of labelTranslate here was, but it had the (bad) effect of making 3-point testing not work until a label was wayyyy occluded by terrain. Separate from all other changes in this PR, removing labelTranslate and using vec2(0.0) looks way better.
Description
Previously, when when using terrain, clamping entities to ground, and enabling
depthTestingAgainstTerrain, billboards, labels, and points would noisily clip against the terrain at global scales. (See images https://github.com/iTwin/platform-bentley-community/issues/234).This issue isn't unexpected: billboards and others will face the camera as it moves, causing them to dip into the globe and become occluded. And when clamped to ground, the problem is exacerbated, with noisy terrain sporadically poking through the entities.
Ideally, billboards would not get clipped at this global scale, but would continue to respect terrain-depth-testing at local scales (e.g. a billboard should become invisible when behind a mountain). The solution this PR targets is described in this comment.
NOTE:
Please review commit-by-commit (starting at Moves to manual billboard depth testing in shaders). Specifically, I did some refactoring in Refactors Billboard fragment shader depth testing into functions, which will make it difficult to review all the changes together. Also, I'm going to leave comments on individual commits to help explain changes, and those will only appear in the right places if looking at those commits.
Issue number and link
#12410
Testing plan
Basic billboard clamped to terrain:
Current release vs. localhost
These should behave the same at local scales. At global scales, the PR-version behaves much better - as if terrain is disabled.
Basic billboard clamped to terrain, with
disableDepthTestDistance:Current release vs. localhost
Try this with various depth test distances: 0, really large (non-infinite) numbers, positive infinity, etc. Note: in the current release, there's a bug. The special 3-point depth testing of billboards ONLY applies within the disabled depth test distance region. For instance, if you set the disable distance to 1e20, you would expect pretty much no depth testing - but the 3-point testing occurs all the time now. This bug is fixed in this PR.
Another difference: with this PR, the 3-point depth testing only applies within 5000m, whereas before it would apply within whatever the depth testing distance was (or 5000m if none was set).
And, as before, on global scales, the PR-version behaves much better (as if there's no terrain to occlude).
Basic billboard, on terrain but not clamped:
Current release vs. localhost
These should behave the same at a local scale. Try playing around with different
disableDepthTestingDistancesettings as well; both examples should continue to behave the same, and no 3-point testing will be done.Again, on global scales, the PR-version behaves much better.
Billboards among 3D models (regression test)
Current release vs. localhost
Should have identical behavior. However, if you turn on
CLAMP_TO_3D_TILE, the current build's billboard clips through the models (because, I believe, with clampingdisableDepthTestDistancedefaults to 5000m, but with no terrain, it doesn't do the 3-point testing). In the PR-version, this is fixed.Basic billboard on GP3D tiles
Current release vs. localhost
At global scales, PR-version behaves much better. At local scales, current release has the same bug described before: it clips through the globe because of the default 5km depth test distance and no globe). The PR-version behaves better, but doesn't do the 3-point testing because, again, no globe or terrain.
Billboards with translucent background (no terrain)
Current release vs. localhost
The behavior here differs slightly, but I think it's improved. In the current version, if the blue billboard is on top of the red one, you just see blue. If the red one is on top, they blend to make purple (as expected). In the PR-version, you get purple no matter which is on top.
Current billboard translucency:
CurrentTranslucency.mov
PR billboard translucency:
DevTranslucency.mov
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change