Enable improved depth testing for all billboards #6802

hpinkos · 2018-07-13T18:11:08Z

Now that we can do the billboard depth texture lookups in the vertex shader, I think it's safe to enable this option for all billboards instead of only when they're clamped to ground.

I did some performance testing using some of our sandcastle examples like the KML, BillboardClampToGround and Billboards Instancing Sandcastle examples and saw no differences in performance.

@bagnell can you review?

TODO

Fix disableDepthTestDistance causes weird mouse interaction #6840
Fix Billboard ordering incorrect when using disableDepthTestDistance #6838 (or do something different than use disableDepthTestDistance)
CHANGES

cesium-concierge · 2018-07-13T18:11:09Z

Thanks for the pull request @hpinkos!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

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

🌍 🌎 🌏

hpinkos · 2018-07-13T19:23:21Z

I also bumped up the default depth testing range from 2000 to 4000. I found 2000 to be a little too close in practice.

hpinkos · 2018-07-13T19:43:28Z

Well, now that I increased the range I do see a bit of a performance hit for a lot of billboards in a small area.

Sandcastle

I think I'll put this behind a flag instead. I'm thinking it should default to true and people can disable it if they're seeing a performance impact and they don't need the feature.

hpinkos · 2018-07-13T20:08:21Z

Actually, I'm not sure if it's a performance issue. There's something weird with zooming in and out though when you're at this angle:

The FPS doesn't drop and panning around still works fine, but zooming in and out kind of stops working.
@bagnell do you have any idea why that would be?

bagnell · 2018-07-16T23:52:15Z

Looks like the zooming problem is the camera picking the depth buffer for the collision detection and it's picking the billboards. The problem is that billboards are pushed to the near plane when within the disabled depth test distance. I'm not sure how we fix this. There is a fallback for picking the terrain using ray-triangle intersections but we rely on picking the depth buffer so the camera doesn't go through 3D Tiles.

hpinkos · 2018-07-17T14:50:57Z

@bagnell okay, so this is an existing issue with disableDepthTestDistance

@pjcozzi what do you recommend we do here?

pjcozzi · 2018-07-18T13:33:54Z

The problem is that billboards are pushed to the near plane...

What are the other options:

Separate depth buffer for collision detection / environment - doesn't need to be a separate render pass could just GPU-to-GPU copy the depth buffer - this is also needed for polylines on 3D Tiles, CC @ggetz
Maybe a gl.clear at the right time with a separate render pass for billboards/labels/points. I dunno.
Bump by a fudge factor instead of all the way to the near plane? Could be hard in practice.

We should also ask if we ever want collisions with billboards/labels/points, I suspect not.

Perhaps also chat with @lilleyse if he has bandwidth and ping me if you run out of ideas.

bagnell · 2018-07-18T20:01:45Z

Separate depth buffer for collision detection / environment - doesn't need to be a separate render pass could just GPU-to-GPU copy the depth buffer - this is also needed for polylines on 3D Tiles, CC @ggetz

There are no GPU-to-GPU copies for depth buffers.

Maybe a gl.clear at the right time with a separate render pass for billboards/labels/points. I dunno.

We can't clear the depth since we determine whether to depth test in the vertex shader. Some billboards may need to be depth tested while others are moved to the near plane so they are not.

Bump by a fudge factor instead of all the way to the near plane? Could be hard in practice.

This could work, but I also think it could be hard in practice.

Other options:

Have two depth buffers and use the previous depth to render the billboards and the current depth for collision detection.
Render billboards last and have them enable the depth test but disable depth writes.

With both of these options, pickPosition wouldn't work on billboards.

bagnell · 2018-07-18T20:54:13Z

For testing, search for depthMask in BillboardCollection.js and set them to false.

Is there anything we can to for top-down views? The billboards blink in and out when zooming.

pjcozzi · 2018-07-19T00:54:09Z

With both of these options, pickPosition wouldn't work on billboards.

In general, we might need to embrace multiple depth buffers / masks - this is for collision, this is for picking, this is for rendering, etc.

If we resort to multi-pass, don't forget MRT...

If possible, would like to limit the amount of major re-architecture we need to do now.

hpinkos · 2018-07-19T19:57:17Z

Is there anything we can to for top-down views? The billboards blink in and out when zooming.

@bagnell I added a value to the depth check. It seems to fix the flickering without causing the billboards to show through the terrain when they shouldn't.

bagnell · 2018-07-19T23:24:23Z

Top-down views look much better for the linked Sandcastle example.

bagnell · 2018-07-19T23:26:20Z

Source/Shaders/BillboardCollectionVS.glsl

@@ -328,16 +328,18 @@ if (lengthSq < disableDepthTestDistance) {
    vec4 pEC1 = addScreenSpaceOffset(positionEC, dimensions, scale, vec2(0.0), origin, labelTranslate, pixelOffset, alignedAxis, validAlignedAxis, rotation, sizeInMeters, rotationMatrix, mpp);
    float globeDepth1 = getGlobeDepth(pEC1);

-    if (globeDepth1 != 0.0 && pEC1.z < globeDepth1)
+    float depthsilon = 10.0;


Where did you come up with 10.0? Have you tested other areas of terrain?

Great variable name.

Yeah, I tested on both flat areas and mountainy areas. 10 was the lowest value that mostly got rid of the flickering. Other values I tested like 5 and 7 still had a lot of flickering when the terrain had more drastic slopes

bagnell · 2018-07-20T23:06:06Z

I'm seeing 10 test failures for billboards and labels.

hpinkos · 2018-07-23T18:41:59Z

@bagnell fixed the test failures. I changed billboard.disableDepthTestDistance to default to undefined instead of 0. That way the end user can explicitly set it to 0. Otherwise it defaults to 5000 for the depth test thing.

Let me know when you're free to chat about the camera collision thing

bagnell · 2018-07-24T19:55:49Z

Specs/Scene/BillboardCollectionSpec.js

@@ -682,14 +684,16 @@ defineSuite([
    it('adds and renders a billboard', function() {
        billboards.add({
            position : Cartesian3.ZERO,
-            image : greenImage
+            image : greenImage,
+            disableDepthTestDistance : 0.0


Why do you have to explicitly set disableDepthTestDistance for this to pass?

I wouldn't think we have to, but when they're both set to the default 5000 the wrong one renders in front. Should I open an issue? I'll see if I can reproduce it for a normal case. Would it be weird for this since there's no globe and it's at 0,0,0?

@bagnell yeah it looks like it's a bug with disableDepthTestDistance. I can reproduce it pretty easily, I'll open a new issue

I think it's because the disableDepthTestDistance is greater than the distance from the camera to the billboard

hpinkos · 2018-07-25T19:16:02Z

After a discussion with @mramato, I don't want to merge this in until #6840 (and ideally #6838) are fixed. I ran the KML example in sandcastle and you can run into the bad mouse behavior pretty easily. I don't think it makes sense to put this behind a flag because then we're more-or-less pushing through a feature that is fundamentally broken for no good reason other than to get it in. It's currently enabled for billboards that are clamped to ground which I think covers the most common use case for this anyway. We'll just leave it at that for now.

hpinkos · 2018-07-25T19:35:16Z

I had a few minor fixes in this PR, I'll open up a new PR for them

cesium-concierge · 2018-12-11T22:00:20Z

Thanks again for your contribution @hpinkos!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

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

🌍 🌎 🌏

hpinkos · 2018-12-12T20:14:41Z

@cesium-concierge stop

hpinkos · 2019-02-21T15:55:35Z

Closing this since we probably won't be able to merge this in the near term

enable better depth testing for all billboards

00d94d9

bump up range

54f6994

hpinkos added 2 commits July 19, 2018 15:19

Merge branch 'master' into enable-billboard-depth

b4baa2a

wiggle room

c2a35f4

fix context

b218f4a

bagnell reviewed Jul 19, 2018

View reviewed changes

Merge branch 'master' into enable-billboard-depth

912e14b

hpinkos added 2 commits July 23, 2018 14:09

default disableDepthTestDistance to undefined

3293844

remove depth test distance ifdef

534c7b8

bagnell reviewed Jul 24, 2018

View reviewed changes

hpinkos mentioned this pull request Jul 24, 2018

disableDepthTestDistance causes weird mouse interaction #6840

Open

hpinkos mentioned this pull request Jul 25, 2018

Billboard clamp to ground cleanup #6844

Merged

hpinkos closed this Feb 21, 2019

lilleyse mentioned this pull request Apr 28, 2020

Separate pass for billboards #8810

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable improved depth testing for all billboards #6802

Enable improved depth testing for all billboards #6802

hpinkos commented Jul 13, 2018 •

edited

Loading

cesium-concierge commented Jul 13, 2018

hpinkos commented Jul 13, 2018

hpinkos commented Jul 13, 2018

hpinkos commented Jul 13, 2018

bagnell commented Jul 16, 2018

hpinkos commented Jul 17, 2018

pjcozzi commented Jul 18, 2018

bagnell commented Jul 18, 2018

bagnell commented Jul 18, 2018

pjcozzi commented Jul 19, 2018

hpinkos commented Jul 19, 2018

bagnell commented Jul 19, 2018

bagnell Jul 19, 2018

hpinkos Jul 20, 2018

bagnell commented Jul 20, 2018

hpinkos commented Jul 23, 2018

bagnell Jul 24, 2018

hpinkos Jul 24, 2018

hpinkos Jul 24, 2018

hpinkos Jul 24, 2018

hpinkos commented Jul 25, 2018

hpinkos commented Jul 25, 2018

cesium-concierge commented Dec 11, 2018

hpinkos commented Dec 12, 2018

hpinkos commented Feb 21, 2019

Enable improved depth testing for all billboards #6802

Enable improved depth testing for all billboards #6802

Conversation

hpinkos commented Jul 13, 2018 • edited Loading

cesium-concierge commented Jul 13, 2018

hpinkos commented Jul 13, 2018

hpinkos commented Jul 13, 2018

hpinkos commented Jul 13, 2018

bagnell commented Jul 16, 2018

hpinkos commented Jul 17, 2018

pjcozzi commented Jul 18, 2018

bagnell commented Jul 18, 2018

bagnell commented Jul 18, 2018

pjcozzi commented Jul 19, 2018

hpinkos commented Jul 19, 2018

bagnell commented Jul 19, 2018

bagnell Jul 19, 2018

Choose a reason for hiding this comment

hpinkos Jul 20, 2018

Choose a reason for hiding this comment

bagnell commented Jul 20, 2018

hpinkos commented Jul 23, 2018

bagnell Jul 24, 2018

Choose a reason for hiding this comment

hpinkos Jul 24, 2018

Choose a reason for hiding this comment

hpinkos Jul 24, 2018

Choose a reason for hiding this comment

hpinkos Jul 24, 2018

Choose a reason for hiding this comment

hpinkos commented Jul 25, 2018

hpinkos commented Jul 25, 2018

cesium-concierge commented Dec 11, 2018

hpinkos commented Dec 12, 2018

hpinkos commented Feb 21, 2019

hpinkos commented Jul 13, 2018 •

edited

Loading