Skip to content
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

Add clamped vector polylines #9398

Merged
merged 24 commits into from
Mar 12, 2021
Merged

Add clamped vector polylines #9398

merged 24 commits into from
Mar 12, 2021

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Mar 1, 2021

Opening as a draft to make sure things are in their place before adding specs.

  • Moves decodeVectorPolylinePositions logic to its own file in Core
  • Adds examineVectorLinesFunction and minimumMaximumVectorHeights properties to Cesium3DTileset
  • Adds the Vector3DTileClampedPolylinesclass
  • Adds logic for floating vs clamped polylines to Vector3DTileContent
  • Adds vert and frag shaders for clamped polylines
  • Adds a worker for creating clamped polylines

@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

  • ✔️ 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.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

@ebogo1 ebogo1 requested a review from lilleyse March 1, 2021 23:09
@lilleyse lilleyse requested a review from likangning93 March 2, 2021 00:39
Source/Scene/Cesium3DTileset.js Show resolved Hide resolved
Source/Scene/Cesium3DTileset.js Show resolved Hide resolved
sources: [PolylineCommon, vsSource],
});
var fs = new ShaderSource({
defines: ["VECTOR_TILE"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where DEBUG_SHOW_VOLUME would go if you wanted to debug the volumes, CC https://github.com/CesiumGS/cesium/blob/master/Source/Scene/GroundPolylinePrimitive.js#L406.

If we wanted it to be toggleable we'd probably have to wire something through to Cesium3DTileset.

Source/Scene/Vector3DTileClampedPolylines.js Outdated Show resolved Hide resolved

var previousCompressedCartographicScratch = new Cartographic();
var currentCompressedCartographicScratch = new Cartographic();
function removeDuplicates(uBuffer, vBuffer, heightBuffer, counts) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be handy to have elsewhere in vector tiles too, wherever positions are getting unpacked from those quantized u/v buffers. It'll need a more specific name if it becomes its own file though.

Source/Shaders/Vector3DTileClampedPolylinesVS.glsl Outdated Show resolved Hide resolved
@ebogo1 ebogo1 marked this pull request as ready for review March 3, 2021 17:19
@ebogo1
Copy link
Contributor Author

ebogo1 commented Mar 3, 2021

I removed the unused property in Cesium3DTileset, marked the callback as @experimental, and renamed CornerIds + shader variables in a2ac850. Do you think any of the other changes are in scope for this PR?

There is also the line from #9399 (comment).

@ebogo1
Copy link
Contributor Author

ebogo1 commented Mar 3, 2021

@likangning93 had some minor changes for CI to pass but should be ready for another look. I added the examine callback function as an option for the tileset constructor along the way.

Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
Source/Shaders/Vector3DTileClampedPolylinesFS.glsl Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

lilleyse commented Mar 3, 2021

@ebogo1 you mentioned it above, but make sure to address #9399 (comment) soon because otherwise polylines won't be clamped at all. (never mind, but still this is important to have for the b3dm + vctr use case)

@lilleyse
Copy link
Contributor

lilleyse commented Mar 3, 2021

Is there a sandcastle that we could play with? Screenshots? Maybe we can tile some non-customer data. @likangning93 do you know if we have any data like that?

Normally I would also suggest a new official sandcastle example but given that this is for the older vctr format I don't think we need one. However we should add a vector tile polylines (clamped) option to the Development/Picking sandcastle to easily test the picking behavior.

Also, we should have units tests for the new behavior.

@lilleyse
Copy link
Contributor

lilleyse commented Mar 4, 2021

The code is currently crashing because the startFaceNormalAndVertexCorners property rename was not applied to createVectorTileClampedPolylines.js. Just to reiterate a point, it's always a good idea to be testing new features with a sandcastle example of some sort to catch mistakes like this (unit tests being the other line of defense).

Here's a sandcastle I put together for testing: Sandcastle

Selection_682

What's interesting is that even after that problem is fixed there's still some issue where the polylines don't show up at all. Is it because of our usage of ApproximateTerrainHeights and the area being flat on the ocean?

@lilleyse
Copy link
Contributor

lilleyse commented Mar 4, 2021

This one's kind of random, but in #8726 I had to render shadow volumes with premultiplied alpha blending instead of regular alpha blending since alpha blending against a black framebuffer like the globe translucency framebuffer would result in darker colors than normal. The same sort of change needs to happen here - I pushed the change: 0351120.

This is what I was starting to test in the comment above. The commented out lines will enable globe translucency.

I think similar changes will be needed in #9399 as well and I'll look at that once the changes here are confirmed to work.

@lilleyse
Copy link
Contributor

lilleyse commented Mar 9, 2021

What's interesting is that even after that problem is fixed there's still some issue where the polylines don't show up at all. Is it because of our usage of ApproximateTerrainHeights and the area being flat on the ocean?

False alarm, the sandcastle was wrong. classificationType: Cesium.ClassificationType.CESIUM_3D_TILE should have been classificationType: Cesium.ClassificationType.TERRAIN.

Updated sandcastle

@ebogo1
Copy link
Contributor Author

ebogo1 commented Mar 9, 2021

Can confirm clamping behavior is working on Cartersville TIN mesh after 4668ca6
image

@lilleyse
Copy link
Contributor

lilleyse commented Mar 9, 2021

I realized I had to make some adjustments to the premultiplied alpha code because it wasn't taking into account the fact that the batch table overwrites gl_FragColor. I pushed the changes in 7ada1a7. This also makes b3dm classification tileset use premultiplied alpha which was missed in #8726.

These are the sandcastles I used for testing:

vctr-sandcastle, b3dm-classification-sandcastle, geom-sandcastle

@ebogo1
Copy link
Contributor Author

ebogo1 commented Mar 11, 2021

Pushed some specs for the new code, based on Vector3DTilePolylineSpec. I used xit for now since the rendering is failing due to a bug I found:
image
When viewing a clamped polyline tileset from above, artifacts like this begin to appear. I don't believe this is related to different LODs, and it happens both in Chrome and FF. This behavior doesn't occur with regular polylines with clampToGround = true.

@likangning93
Copy link
Contributor

likangning93 commented Mar 11, 2021

When viewing a clamped polyline tileset from above, artifacts like this begin to appear. I don't believe this is related to different LODs, and it happens both in Chrome and FF. This behavior doesn't occur with regular polylines with clampToGround = true.

Here's the relevant chunks of Vector3DTilesClampedPolylines:

import CullFace from "./CullFace.js";

//...

function getRenderState(mask3DTiles) {
  return RenderState.fromCache({
    cull: {
      enabled: true, // prevent double-draw. Geometry is "inverted" (reversed winding order) so we're drawing backfaces.
      face: CullFace.FRONT
    },

I don't know why this is the case though, since the geometry code in the vertex shader and the indices for each volume should basically be creating "inside-out" geometry already.
This also doesn't seem to be a complete fix, so there might still be something else at play. My guess is there's something going wrong in that geometry code in the vertex shader still.

Here's the Sandcastle I'm testing with

@likangning93
Copy link
Contributor

likangning93 commented Mar 11, 2021

There might also be something about the CullFace.FRONT thing combining with clamping the "min depth" of the polyline volumes to about -1000 (the value it's getting from ApproximateTerrainHeights right now seems to be -100000) also producing better results, but... that's yet another mystery.

[EDIT] I did notice that when we use logarithmic depth, depthClamp.glsl doesn't seem to do anything, and it's also true that the draw command for the bounding volume for the clamped vector tiles doesn't take the shadow volume into account (I think...) which would prevent Cesium from taking that into account when setting near/far planes (again, I think...). So maybe our two problems causing artifacts are:

  1. faces inexplicably drawing backwards
  • the "drawing backwards" part is easy to solve
  • the "inexplicable" part, not so much, but maybe I've just been looking at it too much
  1. the "far" part of the shadow volumes being past the far plane in logarithmicDepth mode
  • maybe solvable by using a shadow-volume-aware bounding volume for the DrawCommands in Vector3DTileClampedPolylines

@likangning93
Copy link
Contributor

likangning93 commented Mar 12, 2021

Investigating "When viewing a clamped polyline tileset from above, artifacts like this begin to appear."

I made some shader and render state changes off 6e4d7ef to try to get to the bottom of these, resulting in the two commits below. Here's the Sandcastle I used for testing.

fix clamped vector polylines not drawing when camera is inside volume by culling front faces da38f78

The algorithm we're using to draw clamped line segments does something like:

  1. draw a "decal" that covers the whole onscreen area the line segment is likely to cover onscreen. Conceptually this is a little bit like a shadow volume, except it doesn't involve stencil buffers etc.
  2. for each fragment in the decal, figure out if that fragment is part of the line segment

The goal is to draw the "inside" faces of the volume instead of "outside," so that when the camera enters the volume it should still see the decal instead of backface-culling it all.
The diagrams and REFERENCE_INDICES in createVectorTileClampedPolylines.js indicate that we try to use winding order to accomplish this "inside-drawing," but for some reason that doesn't seem to work. I simplified the vertex shader for clamped vector polylines to make the volumes a little easier to grok, and noticed that when we crash through the surface of the volume, the volume (and therefor the clamped polyline) is no longer drawn:

crashThroughTheSurface

In other words, we were drawing front/outside faces, not back/inside faces. Flipping the face culling (or reversing the indices in the geometry) fixes the problem, but I still don't fully understand why the geometry is drawing front/outside faces in the first place.
Here's a patch for further investigation:

investigateFaceFlip6e4d7ef.patch.zip

use bounding volume that covers whole shadow volume for draw commands in Vector3DTileClampedPolylines 6602501

With face-flipping turned on and no other changes, "smaller" chunks of the polyline were still missing. Moving the camera along the line segment caused the artifacts to "slide" instead of staying where they can't hurt us.

whereTheyCantHurtUs

This looked a little like parallax to me, so I switched the debug bounding volumes back on. Lo and behold! Even drawing back/inside faces correctly (as evidenced by the "sides" of the current volume being visible), the bottom of the volume wasn't drawing, hence the lack of classification!

farFromTheShallow

Now, this was drawing using the actual-size shadow volumes, which go something like -100,000 meters below and 9,000 meters above the ellipsoid in this area.
I noticed that the draw command bounding volumes though were using the 3D Tile's bounding sphere, not its enormously deep and tall bounding volume. I think Cesium was using those "shallow" bounding spheres to compute the far plane for the top-down view. As a result the bottom part of the shadow volume was getting clipped by the depth plane, since in the top-down view it's far from the shallow now. Updating the draw command to use a bounding volume based on the shadow volume seems to fix that problem.

Here's a patch to reproduce the gifs for this part:

investigateDepthClip6e4d7ef.patch.zip

Side-note, I also noticed that czm_depthClamp (depthClamp.glsl) doesn't do anything when the renderer is in log-depth mode, which was also a helpful observation pointing at depth clipping being the problem.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Mar 12, 2021

With some more help from @likangning93 I was able to get rendering and picking specs to pass as well. I think this is good for another round of review!

@lilleyse
Copy link
Contributor

lilleyse commented Mar 12, 2021

I renamed noClassificationModels to vectorClassificationOnly and marked it as @experimental since other vctr-related properties are also marked as experimental.

EDIT: we'll see how CI handles it #9398 (comment)

Make sure to do the same rename in SC once the newer version of CesiumJS is pulled in.

@lilleyse
Copy link
Contributor

lilleyse commented Mar 12, 2021

I also left a comment above: #9398 (review)

EDIT: is was a simple change so I pushed it 4fd49c5

@lilleyse
Copy link
Contributor

By the way, excellent detective work @likangning93

@lilleyse
Copy link
Contributor

Thanks @ebogo1 @likangning93 !

@lilleyse lilleyse merged commit 2315c51 into master Mar 12, 2021
@lilleyse lilleyse deleted the clamp-vector-tiles branch March 12, 2021 22:58
@ebogo1 ebogo1 restored the clamp-vector-tiles branch February 18, 2022 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants