Fix billboards when clamped to ground with depthTestAgainstTerrain #6621

hpinkos · 2018-05-23T20:40:48Z

Before:

After:

This only changes billboards using HeightReference.CLAMP_TO_GROUND
When the camera is greater than 2000m away from the billboard, use normal depth test against terrain
- 2000 was picked arbitrarily through testing to see what looked reasonable
When the camera is closer than 2000m to the billboard, check if the top left, top right and anchor points are behind terrain. If all 3 points are behind terrain, hide the billboard. Otherwise, show the billboard.
- This can cause the billboard to show through terrain in some edge cases, but I think overall it's a vast improvement to what we have and brings us closer to enabling terrain by default.

Test code:

var viewer = new Cesium.Viewer('cesiumContainer', {
    terrainProvider: Cesium.createWorldTerrain()
});
viewer.scene.globe.depthTestAgainstTerrain = true;

var e = viewer.entities.add({
    position : Cesium.Cartesian3.fromDegrees(-122.1958, 46.1915),
    billboard : {
        image : '../images/Cesium_Logo_overlay.png',
        heightReference : Cesium.HeightReference.CLAMP_TO_GROUND,
        verticalOrigin: Cesium.VerticalOrigin.BOTTOM
    }
});

viewer.trackedEntity = e;

cesium-concierge · 2018-05-23T20:40:49Z

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

hpinkos · 2018-05-23T20:41:24Z

Thanks for helping me with this @bagnell!

What's the best way to write tests for this kind of thing?

bagnell · 2018-05-23T20:56:09Z

I haven't seen the code yet, but I imagine you need to mock the globe so the clamp to ground just returns some constant height value. Also, see the ground primitive tests
and classification tests. They change the primitive commands to execute during the globe pass which will be written into the globe depth texture.

hpinkos · 2018-05-25T18:37:17Z

@bagnell I tried to write some tests, but everything breaks down trying to do a render test with height referenced billboards because it relies on the QuadtreePrimitive for adjusting the heights. I'm guessing we don't have a good way to mock that and that's why we don't have any render tests for billboards using a height reference.

I think this is ready for review. If you had other ideas for how to add tests let me know

bagnell · 2018-06-04T20:58:30Z

Source/Shaders/BillboardCollectionFS.glsl

+#ifdef CLAMP_TO_GROUND
+    if (v_eyeDepth > -2000.0) {
+        vec2 adjustedST = v_textureCoordinates - v_textureOffset.xy;
+        adjustedST = adjustedST / (v_textureOffset.z - v_textureOffset.x, v_textureOffset.w - v_textureOffset.y);


I'm surprised this compiled. Add use the vec2 constructor for the denominator.

Thanks haha. I hardly ever glsl so I'm not very familiar with the syntax

bagnell · 2018-06-04T21:02:34Z

Source/Shaders/BillboardCollectionFS.glsl

+        vec2 st1 = ((v_dimensions.xy * (v_depthLookupTextureCoordinate1 - adjustedST)) + gl_FragCoord.xy) / czm_viewport.zw;
+        float logDepthOrDepth = czm_unpackDepth(texture2D(czm_globeDepthTexture, st1));
+        vec4 eyeCoordinate = czm_windowToEyeCoordinates(gl_FragCoord.xy, logDepthOrDepth);
+        float globeDepth1 = eyeCoordinate.z / eyeCoordinate.w;


Make this and the above two lines a function.

bagnell · 2018-06-04T21:06:45Z

Source/Shaders/BillboardCollectionFS.glsl

+            eyeCoordinate = czm_windowToEyeCoordinates(gl_FragCoord.xy, logDepthOrDepth);
+            float globeDepth2 = eyeCoordinate.z / eyeCoordinate.w;
+
+            if (globeDepth2 > v_eyeDepth + czm_epsilon5)


One would think that all of these ifs would increase performance for fragments that don't execute them, but, unfortunately, that's not the case. It would look cleaner if you compute all of the globe depths and then have one if combining all four conditionals for the discard.

bagnell · 2018-06-04T21:27:41Z

Source/Shaders/BillboardCollectionVS.glsl

@@ -152,6 +166,14 @@ void main()

    vec2 imageSize = vec2(floor(temp), compressedAttribute2.w);

+#ifdef CLAMP_TO_GROUND
+    v_textureOffset = textureOffset;
+    v_depthLookupTextureCoordinate1 = (0.0, 0.9) - depthLookupST; //the origin


Are you sure these are right? Say the anchor point is the bottom right, then wouldn't you want to test vec2(-1.0, 1.0) instead of vec2(1.0)?

For the vec* constructors, a single argument duplicates it for all components.

Yeah, you're right. I think I forgot to test different horizontal origins, this only works for center.

hpinkos · 2018-06-05T16:34:18Z

@bagnell ready for another look.
Any ideas for what we can do for labels? I guess each letter is a separate billboard, which doesn't work well with this approach.

var viewer = new Cesium.Viewer('cesiumContainer', {
    terrainProvider: Cesium.createWorldTerrain()
});
viewer.scene.globe.depthTestAgainstTerrain = true;

var e = viewer.entities.add({
    position : Cesium.Cartesian3.fromDegrees(-122.1958, 46.1915),
     label : {
        text : 'Clamped to ground',
        heightReference : Cesium.HeightReference.CLAMP_TO_GROUND,
        horizontalOrigin : Cesium.HorizontalOrigin.CENTER,
        verticalOrigin : Cesium.VerticalOrigin.BOTTOM,
        fillColor : Cesium.Color.BLACK,
        showBackground : true,
        backgroundColor : new Cesium.Color(1, 1, 1, 0.7),
        backgroundPadding : new Cesium.Cartesian2(8, 4)
    }
});

viewer.trackedEntity = e;

bagnell · 2018-06-05T19:36:07Z

Look for translate in the vertex shader. It is the pixel offset to move each billboard in a label.

Did you test this with all of the other billboard options? You'll have to account for pixelOffset, sizeInMeters, *ByDistance, and others.

hpinkos · 2018-06-06T19:33:06Z

@bagnell as far as I can tell, this is working correctly with things like scale, pixelOffset *ByDistance and sizeInMeters.

The problem with the label is that each letter is checking the bounds of that letter instead of checking the bounds of the entire label. That's why each letter flickers individually.

bagnell · 2018-06-06T19:45:19Z

Can you add another attribute for the size of the whole label?

pjcozzi

@hpinkos do you want to write a short tech blog post about this?

pjcozzi · 2018-06-09T18:16:53Z

Source/Scene/BillboardCollection.js

@@ -302,7 +311,9 @@ define([
            BufferUsage.STATIC_DRAW, // SCALE_BY_DISTANCE_INDEX
            BufferUsage.STATIC_DRAW, // TRANSLUCENCY_BY_DISTANCE_INDEX
            BufferUsage.STATIC_DRAW, // PIXEL_OFFSET_SCALE_BY_DISTANCE_INDEX
-            BufferUsage.STATIC_DRAW  // DISTANCE_DISPLAY_CONDITION_INDEX
+            BufferUsage.STATIC_DRAW, // DISTANCE_DISPLAY_CONDITION_INDEX
+            BufferUsage.STATIC_DRAW,  // TEXTURE_OFFSET


Extra whitespace?

pjcozzi · 2018-06-09T18:17:56Z

Source/Scene/BillboardCollection.js

+            usage : buffersUsage[TEXTURE_OFFSET]
+        }, {
+            index : attributeLocations.dimensions,
+            componentsPerAttribute : 2,


Is there anyway to pack this into one component and perhaps store as z in DISTANCE_DISPLAY_CONDITION_INDEX? Haven't looked closely, but perhaps precision requirements are not stiff here and fewer bits can be used?

Is there a way to pack a Cartesian2 into one float? I'm not quite sure how to do this

There are no bit-wise operators in GLSL but you can shift bits by multiplying powers of 2. See any of the writeCompressedAttrib* functions. So if you want to pack a Cartesian2 into a float, you'll have 12 bits for each component. Is that enough precision? It would look something like this for packing:

var LEFT_SHIFT12 = Math.pow(2.0, 12.0); var width = Math.floor(Math.clamp(width, 0.0, LEFT_SHIFT12)); var height = Math.floor(Math.clamp(height, 0.0, LEFT_SHIFT12)); var dimensions = width * LEFT_SHIFT12 + height;

Then, reverse it in the vertex shader to unpack.

pjcozzi · 2018-06-09T18:18:32Z

Source/Scene/BillboardCollection.js

@@ -1176,6 +1197,10 @@ define([
        }

        var disableDepthTestDistance = billboard.disableDepthTestDistance;
+        if (billboard.heightReference === HeightReference.CLAMP_TO_GROUND && disableDepthTestDistance === 0.0) {
+            disableDepthTestDistance = 2000.0;


Should this be configurable at the scene level? I think there are other billboard/label/depth-ish options there.

It's configurable by setting disableDepthTestDistance to a different value

pjcozzi · 2018-06-09T18:21:04Z

Source/Shaders/BillboardCollectionVS.glsl

 #ifdef VECTOR_TILE
 attribute float a_batchId;
 #endif

 varying vec2 v_textureCoordinates;
+#ifdef CLAMP_TO_GROUND


This is going to use a huge amount of memory bandwidth.

Looks easy to pack some varyings together. Also perhaps some can be derived from others or more should be computed in the fragment shader if they are trivial?

Also, use more precise names - if possible - than 1, 2, and 3 for texture coordinates.

pjcozzi · 2018-06-09T18:22:04Z

Source/Shaders/BillboardCollectionVS.glsl

@@ -10,11 +10,25 @@ attribute vec4 eyeOffset;                                  // eye offset in mete
 attribute vec4 scaleByDistance;                            // near, nearScale, far, farScale
 attribute vec4 pixelOffsetScaleByDistance;                 // near, nearScale, far, farScale
 attribute vec3 distanceDisplayConditionAndDisableDepth;    // near, far, disableDepthTestDistance
+#ifdef CLAMP_TO_GROUND
+attribute vec4 textureOffset;                              // the min and max x and y values for the texture coordinates


Should this be named something more precise like textureCoordinatesBounds?

pjcozzi · 2018-06-09T18:23:00Z

Source/Shaders/BillboardCollectionVS.glsl

+    v_depthLookupTextureCoordinate1 = vec2(1.0) - depthOrigin; //the origin
+    if (v_depthLookupTextureCoordinate1.y == 1.0) //vertical origin is top
+    {
+        v_depthLookupTextureCoordinate2 = vec2(0.0, 0.0); //bottom left


Just vec2(0.0)

pjcozzi · 2018-06-09T18:25:48Z

Source/Shaders/BillboardCollectionFS.glsl

+        float epsilonEyeDepth = v_eyeDepth + czm_epsilon5;
+
+        // negative values go into the screen
+        if (globeDepth1 > epsilonEyeDepth && globeDepth2 > epsilonEyeDepth && globeDepth3 > epsilonEyeDepth)


Make this a separate if statement after each call to getGlobeDepth.

On WebGL implementations with dynamic branches, it could exit early and avoid the memory bandwidth for the unnecessary texture reads.

Also, if not already done, order the calls to getGlobeDepth in the order of most likely to fail the depth test - I imagine origin first.

I had it that way initially but @bagnell said it wouldn't work that way. Is dynamic branches something that's only supported in certain browsers?

On a lot of GPUs with WebGL 1 support, there is no dynamic branches, but @pjcozzi is right. Early exits for GPUs with dynamic branches would be faster.

pjcozzi · 2018-06-09T18:29:17Z

CHANGES.md.

hpinkos · 2018-06-12T21:00:48Z

@bagnell this is ready for another review

bagnell · 2018-06-13T18:56:28Z

You can get the background to disappear but not the glyphs. I used the code you posted above but changes the horizontal origin to be LEFT and the text is just 'Clamped'.

bagnell · 2018-06-13T19:22:22Z

Source/Shaders/BillboardCollectionVS.glsl

+    {
+        if (v_originTextureCoordinateAndTranslate.y == 0.0)
+        {
+            v_originTextureCoordinateAndTranslate.y = 0.1;


It had better results than testing the bottom edge when I was first playing around with this, but I'll revisit it because I'm not sure if that's still the case

bagnell · 2018-06-13T19:48:27Z

Source/Shaders/BillboardCollectionFS.glsl

    czm_writeLogDepth();
+
+#ifdef CLAMP_TO_GROUND
+    if (v_eyeDepthAndDistance.x > -v_eyeDepthAndDistance.y) {


How much of a pain would it be to add this to both the vertex and fragment shader?

We would want it to execute in the vertex shader when ContextLimits.maximumVertexTextureImageUnits > 0; otherwise, execute in the fragment shader. That condition just means we can read from textures in the vertex shader. The reason we would want to do this is for performance. Instead of discarding the fragment, you would put the vertex behind the eye so that it gets clipped like we do for distance display conditions or translucency by distance.

I'm going to do this in a separate PR because the math is different since there's no gl_FragCoord in the vertex shader. I'll open an issue

bagnell · 2018-06-13T19:54:55Z

The code looks good to me. The biggest thing would be executing that code in the vertex shader when possible.

Does anyone else want to test?

addressed all comments

hpinkos · 2018-06-19T19:27:13Z

@bagnell I think I've finally worked out all the kinks. Labels were actually looking up pixels in the completely wrong location because the horizontal origin passed in is always "LEFT", so I had to pass in the horizontal offset the user set separately.

There is still occasionally a 1px discrepancy that can lead to this:

I'm honestly not sure how to fix it. I think it's a rounding error. The texture lookup is literally 1px different and it doesn't happen all of the time.

bagnell · 2018-06-19T20:36:24Z

This crashes in IE since it doesn't have depth texture support.

hpinkos · 2018-06-19T20:58:11Z

@bagnell thanks for catching that. I changed IE so it has the behavior currently in master. I also tested Firefox and Edge and they both work great. And all the tests pass locally.

Anything else?

bagnell · 2018-06-19T21:22:25Z

I still see the missing glyphs when the origin is left center. I also see all the glyphs disappear but the background is still visible when the origin is right center.

bagnell · 2018-06-19T21:29:41Z

This is with the scale set to 1.5:

EDIT: I'm seeing the same issue with sizeInMeters : true.

bagnell · 2018-06-19T21:41:15Z

I'm seeing some strange behavior with rotated billboards. This has an origin at the bottom center and a rotation of pi/4.

EDIT: Same issue with aligned axis.

hpinkos · 2018-06-19T21:42:26Z

Dagnabbit. I thought I checked all the weird properties but I guess I missed some stuff 😓

hpinkos · 2018-06-20T18:15:18Z

@bagnell I fixed scale and rotation/alignedAxis. I'm a little stuck on sizeInMeters though

bagnell · 2018-06-20T19:07:59Z

To fix sizeInMeters, multiply anything that has units in pixels with czm_metersPerPixel(eyeCoordinates).

hpinkos · 2018-06-21T18:48:27Z

@bagnell This should be ready for another look
I do still occasionally see blinking letters because of what I mentioned in this comment: #6621 (comment)
The PR you submitted made it better, but it didn't fix it completely. I'm going to play around with adding floor to different pixel calculations to see if that fixes it. Right now the way we compute translate has all kinds of fractional values and I think things like that are causing the lookup to be 1px off every now and then.

But I believe I've fixed everything that was really wrong. I tested a combination of different origins, scale, rotation and it seems to have pretty good behavior.

mramato · 2018-06-21T18:52:59Z

I'm going to play around with adding floor to different pixel calculations to see if that fixes it.

Be careful with this, clamping pixels can lead to very undesired behavior (especially when the text is moving). That's the main reason we stopped clamping to screen space. Not saying it applies here, just something to be aware of.

hpinkos · 2018-06-21T18:59:52Z

Be careful with this, clamping pixels can lead to very undesired behavior (especially when the text is moving)

@mramato thanks for noting that. I should be able to make the location of the depth lookup consistent without effecting the position of the billboards.

hpinkos · 2018-06-21T19:14:35Z

@bagnell nope, that didn't seem to help with the label letters. Do you have any ideas? I feel like it only happens if you're trying really hard and might not be a huge issue in normal application use. Regardless, the behavior here is way better than what's in master now.

bagnell · 2018-06-21T23:45:01Z

This looks good to me. I wasn't able to reproduce the missing letters at all where the last time I commented I could reproduce it reliably.

I'm merging this but don't forget about #6699.

esparano · 2018-06-29T17:21:42Z

Is there a reason that this change only affects billboards with HeightReference.CLAMP_TO_GROUND? I'm having the same issues with billboards that are very close to (but not clamped to) the ground, so I had hoped that those would be fixed by this change.

On a side note, in my specific case, I was looking for a way to hide billboards based on ONLY the anchor point's visibility. Perhaps that could be an option in the future!

pjcozzi · 2018-06-30T16:01:36Z

@esparano good point, this would also be useful for labels "on" 3D buildings, etc.

@hpinkos how hard is it to make this a separate boolean? If easy, maybe do this soon; otherwise, submit an issue. Should it default to true? Is there ever a case when it should be false...hmm, might not need a flag at all if everyone likes the new depth testing...I dunno.

bagnell reviewed Jun 4, 2018

View reviewed changes

oterral mentioned this pull request Jun 5, 2018

Search placemark in 3D geoadmin/mf-geoadmin3#4203

Open

pjcozzi previously requested changes Jun 9, 2018

View reviewed changes

Better billboard depth test against terrain for clamped to ground

da3593d

hpinkos force-pushed the billboard-depth-test branch from e9f7d3f to da3593d Compare June 12, 2018 15:02

hpinkos added 2 commits June 12, 2018 12:47

cleanup

bd63312

compress the things

3f019cc

hpinkos added 2 commits June 13, 2018 10:18

Merge branch 'master' into billboard-depth-test

2a5fb2c

CHANGES.md

072c0c8

bagnell reviewed Jun 13, 2018

View reviewed changes

pjcozzi mentioned this pull request Jun 18, 2018

Billboard/label on terrain flickering #2691

Closed

cleanup

493b8ce

hpinkos force-pushed the billboard-depth-test branch from 65b7cff to 493b8ce Compare June 18, 2018 20:40

fix label picks

6b0e54f

Merge branch 'master' into billboard-depth-test

376a0a6

hpinkos added 2 commits June 19, 2018 16:54

fix ie

be0000c

Merge branch 'master' into billboard-depth-test

2cb5285

hpinkos added 3 commits June 19, 2018 17:46

Merge branch 'master' into billboard-depth-test

dc945c2

fix scale

5883a2b

fix rotation

8d905c6

hpinkos added 3 commits June 20, 2018 18:08

math

9493e4d

Merge branch 'master' into billboard-depth-test

80275d8

size in meters

4c3da3b

bagnell merged commit 0a23971 into master Jun 21, 2018

bagnell deleted the billboard-depth-test branch June 21, 2018 23:45

hpinkos mentioned this pull request Jul 2, 2018

Enable billboard depth testing improvements for non clamped to ground billboards #6762

Open

hpinkos mentioned this pull request Nov 27, 2019

Label letters misaligned when zooming in #7376

Open

Fix billboards when clamped to ground with depthTestAgainstTerrain #6621

Fix billboards when clamped to ground with depthTestAgainstTerrain #6621

Conversation

hpinkos commented May 23, 2018 • edited Loading

cesium-concierge commented May 23, 2018

hpinkos commented May 23, 2018

bagnell commented May 23, 2018

hpinkos commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Jun 5, 2018

bagnell commented Jun 5, 2018

hpinkos commented Jun 6, 2018 • edited Loading

bagnell commented Jun 6, 2018

pjcozzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jun 9, 2018

hpinkos commented Jun 12, 2018

bagnell commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagnell commented Jun 13, 2018

hpinkos commented Jun 19, 2018

bagnell commented Jun 19, 2018

hpinkos commented Jun 19, 2018

bagnell commented Jun 19, 2018

bagnell commented Jun 19, 2018 • edited Loading

bagnell commented Jun 19, 2018 • edited Loading

hpinkos commented Jun 19, 2018

hpinkos commented Jun 20, 2018

bagnell commented Jun 20, 2018

hpinkos commented Jun 21, 2018

mramato commented Jun 21, 2018

hpinkos commented Jun 21, 2018

hpinkos commented Jun 21, 2018

bagnell commented Jun 21, 2018

esparano commented Jun 29, 2018

pjcozzi commented Jun 30, 2018

hpinkos commented May 23, 2018 •

edited

Loading

hpinkos commented Jun 6, 2018 •

edited

Loading

bagnell commented Jun 19, 2018 •

edited

Loading

bagnell commented Jun 19, 2018 •

edited

Loading