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

Polylines on terrain via Primitive API #6615

Merged
merged 41 commits into from
Jun 13, 2018

Conversation

likangning93
Copy link
Contributor

@likangning93 likangning93 commented May 22, 2018

Opening a PR to indicate that this is "stable" enough to preview and for preliminary review, but there's still a decent amount of work to be done.

Features:

  • support for PolylineMaterialAppearance
  • mitered polylines
  • batching multiple instances with per-instance show and line width

Implementation notes:

More details in #2172 (comment).
We're using a single-pass, globe-depth-based algorithm.
For each line segment in the polyline we generate a geometry instance volume that has several planes encoded in the batch table. For each fragment on the volume, we look up the globe depth to compute a terrain position in eyespace and clip this position based on the planes from the batch table.
The planes are also used to compute the position's distance from the center and ends of the line for material support.

Due to the batch table dependence and mapping of attributes from multiple instances to a single pick ID, this PR introduces a new Primitive type, GroundPolylinePrimitive.
There's also a new user-facing geometry, GroundPolylineGeometry, that doesn't actually have a createGeometry function but rather gets decomposed into individual volumes that are then used to generate vertex buffers and attributes.

This is a little troublesome for asynchronous geometry since GroundPolylineGeometry's computation currently happens on the main thread, with the worker threads mostly just converting chunks of this into the combined vertex buffer. In practice this doesn't seem to be a huge problem, but it's something we should talk about.

For each line segment in the polyline we generate a volume with vertex attributes that describe the line segment using a series of planes. For each fragment on the volume, we look up the globe depth to compute a terrain position in eyespace and clip this position based on the planes from the vertex attributes.
The planes are also used to compute the position's distance from the center and ends of the line for material support.

The planes differ between 2D and 3D, and I haven't figured out yet how to interpolate them for morphing, so there's a new Primitive type that issues draw commands with different shaders for 2D and morph. When morphing we actually just draw the volumes themselves with materials, which seems acceptable because the camera goes so far away during the morph anyway.

  • I should also do load and runtime comparisons with a hack that attempts to simulate polylines using corridors or something.

TODO:

Couple Post-PR roadmap items:

  • Entity API integration (priority)
  • Investigate smear reduction with something like screen-space partial derivatives
  • terrain height and volume length improvements
  • Evaluate whether dashed lines should "swim" or stay put
  • Investigate fixes for floating point problems with very long arrow lines

@cesium-concierge
Copy link

Signed CLA is on file.

@likangning93, thanks for the pull request! Maintainers, we have a signed CLA from @likangning93, 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.

🌍 🌎 🌏

componentDatatype: ComponentDatatype.FLOAT,
componentsPerAttribute: 3,
normalize: false,
value : [lengthSoFar, segmentLength, totalLength]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't most of these vertex attributes? If it's per segment, it should be a vertex attribute and if it's per polyline then it should be in the batch table. We do the same thing for billboards and other polylines. Yes, you're duplicating data but it cuts down texture reads in the shader. We only started adding more to the batch table when we ran out of vertex attributes (guaranteed to be at least 8).

Copy link
Contributor

Choose a reason for hiding this comment

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

The other polylines are using 13 vertex attributes so expect the maximum to be at least 16.

@bagnell
Copy link
Contributor

bagnell commented May 24, 2018

For some views, the line width is much bigger than what was set because of the slope of the terrain.
image
Change the width of polyline2 to 32. Here is the view:

viewer.camera.setView({
        destination : new Cesium.Cartesian3(-1919172.892822556, -4782158.333815719, 3749491.554081307),
        orientation : {
            heading : 3.877303220462619,
            pitch : -0.11656491800053193
        }
    });

// An object that, on setting an attribute, will set all the instances' attributes.
var userAttributeNames = ['width', 'show'];
var userAttributeCount = userAttributeNames.length;
function InstanceAttributeSynchronizer(batchTable, firstInstanceIndex, lastInstanceIndex, batchTableAttributeIndices) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Once you remove the per segment data from the batch table, you can remove this too.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also move most of the code from GroundLineSegmentGeometry to GroundPolylineGeometry and remove decompose.

attribute vec3 position3DLow;
attribute float batchId;

varying vec4 v_startPlaneEC;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably limit the number of varyings to 8. MAX_VARYING_VECTORS

Copy link
Contributor

@bagnell bagnell May 24, 2018

Choose a reason for hiding this comment

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

Never mind, the fragment shader only uses 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can trim it down... the polyline material shaders also swallow a couple varyings that don't show in the FS.

@likangning93
Copy link
Contributor Author

For some views, the line width is much bigger than what was set because of the slope of the terrain.

This is kind of inevitable for shadow-volume-type algorithms... part of the motivation for a single-pass algorithm was possible hybridization with @pjcozzi's wall-based algorithm, and I think it's also worth exploring if the per-fragment terrain slope can help us with this. We were hoping those could be "future work" if the current smearing is acceptable in most use cases though.

@likangning93
Copy link
Contributor Author

likangning93 commented May 25, 2018

if the per-fragment terrain slope can help us with this

Aaaaand there we go, I'm nerd sniped... right now we're doing per-fragment point-plane distances, hence the smearing problem. But if we took the local terrain slope into account (which we can already compute pretty reliably for ground primitive materials) we could do ray-plane distance checks, I think this could work! Will definitely take some fiddling though, I'll leave that until after I've addressed existing PR comments...

[EDIT] or maybe even until after this PR and the followup Entity API integration is done... motivation!

@likangning93
Copy link
Contributor Author

likangning93 commented May 25, 2018

Integrate with changes in #6434 for better geometry heights

@bagnell ideally we'd want to do this per-segment, unlike GroundPrimitive which seems to do this over the entire GroundPrimitive, but I ran a little experiment that suggests doing ~64800 does take a nonzero amount of time (maybe 0.1-0.2 seconds on my mid-to-high-range CPU) on the main thread. Is it possible/advisable to do these lookups on the geometry worker? That would also simplify point interpolation, which is possibly a big source of additional height lookups.

Some concerns about the additional cost on worker spinup, but does this happen very frequently?
I'm also not sure about the complexity of doing this since it looks like all the geometry creation tasks are currently synchronous on the worker side.

@bagnell
Copy link
Contributor

bagnell commented May 25, 2018

@bagnell ideally we'd want to do this per-segment, unlike GroundPrimitive which seems to do this over the entire GroundPrimitive

Ah, OK. You can definitely move it to a worker. Worker spin up only happens once on the first post message.

One thing you can do in the future is group line segments by terrain tile. This would reduce the number of times you need to get the min/max heights. Right now, with that many segments, it wouldn't be bad. If we ever need to query a server for the min/max heights, you don't want to make that many HTTP requests.

@likangning93
Copy link
Contributor Author

likangning93 commented May 30, 2018

@bagnell there's a problem with polylines that have large bounding volumes:

var viewer = new Cesium.Viewer('cesiumContainer');

var polylineCartographics = [
    -50, -50,
    -50,  50,
     50,  50,
     50, -50
];

var instance = new Cesium.GeometryInstance({
    geometry : new Cesium.GroundPolylineGeometry({
        loop : true,
        width : 16,
        positions : Cesium.Cartesian3.fromDegreesArray(polylineCartographics)
    })
});

var polylineOnTerrainPrimitive = new Cesium.GroundPolylinePrimitive({
    geometryInstances : instance,
    debugShowShadowVolume : false,
    debugShowBoundingVolume : false
});
viewer.scene.primitives.add(polylineOnTerrainPrimitive);

GeometryPipeline tries to split this (although this one doesn't actually cross the IDL) and I don't think it handles the custom vertex attributes correctly. Is this a problem with regular polylines too?
[EDIT] ah I guess not if they don't use GeometryPipeline at all...

@hpinkos
Copy link
Contributor

hpinkos commented May 30, 2018

I don't think it handles the custom vertex attributes correctly

Sure doesn't! See #4739

You'll have to add something to GeometryPipeline for each attribute you added. You can see the changes I made for my new attribute in #6607

@likangning93
Copy link
Contributor Author

tears

I need to take a deeper look, but has there been any discussion of letting it take fully generic attributes? I might end up giving that a try...

@hpinkos
Copy link
Contributor

hpinkos commented May 30, 2018

has there been any discussion of letting it take fully generic attributes?

It's a little tricky. I'm not sure there's a generic way to figure out what the value is at the new vertex that works for every kind of attribute. For example, mines a boolean attribute so you can't interpolate it, you need to decide what the new value should be based on what the other vertices of the triangle are

@likangning93
Copy link
Contributor Author

likangning93 commented May 30, 2018

It's a little tricky. I'm not sure there's a generic way to figure out what the value is at the new vertex that works for every kind of attribute. For example, mines a boolean attribute so you can't interpolate it, you need to decide what the new value should be based on what the other vertices of the triangle are

Playing with fire a little bit, but if the boolean is true for the top cap and false for the bottom cap of the geometry you might still be able to just interpolate and clamp and expect this to just work because the IDL should be splitting bottoms and tops instead of walls? I guess it's still tricky since there might not be a good way to indicate a clampable bool for safety, but it seems like it might interpolate okay?

The attributes in here are fortunately (painfully...) identical for each vertex in a triangle, so barycentric interpolation is actually overkill :P

Wonder how far I could get by having GeometryPipeline detect triangles crossing the IDL and just make sure the IDL splits are handled in my createGeometry function... hmmmm...

@likangning93
Copy link
Contributor Author

splitting bottoms and tops instead of walls

Erm, clarification. Splitting walls, but vertically instead of horizontally if that makes sense.

v0 -- S -- v1
     \      |
       \    |
         \ v2

So if the values at V0 and V1 are the same, S === V0 === V1?

@likangning93
Copy link
Contributor Author

The line starts disappearing in 2D when it crosses the anti-meridian:

This is partially my fault, I noticed some weirdness with crossing segments in CV as well, but I think part of this is also a problem with the depth buffer in 2D.

wat

Debug views seem to show the depth values drop off moving west-to-east towards the IDL, varying with view distance. The effect is worse if there's things drawn over a wider range of longitudes (wider rectangle).

here on Sandcastle

@likangning93
Copy link
Contributor Author

Terrain heights over parts of the ocean have min/max heights below the ellipsoid, so lines in these areas on the ellipsoid itself look weird. This is (-100, 0), (-100.01, 0):

watnow

Should I clamp terrain heights to the ellipsoid in this case? If not I might just open an issue after the PR closes.

@likangning93
Copy link
Contributor Author

likangning93 commented Jun 11, 2018

Did the early discards impact performance at all?

I'm getting noticeable performance improvements on Windows/Nvidia and Linux/Intel, will test Windows/Intel soon. Hopeful.

[EDIT] nice performance boost for Windows/Intel too, in both Chrome and Edge.

@likangning93
Copy link
Contributor Author

@pjcozzi @bagnell I think this is up-to-date for feedback.

I'll add roadmap stuff to the top of this PR and move it into a dedicated issue when this is merged.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 12, 2018

@bagnell please do a final review and merge. It would be nice to get this into master soon so it is well before the release.

}

// Update each geometry for framestate.scene3DOnly = true
geometryInstance.geometry._scene3DOnly = frameState.scene3DOnly;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do the same thing for projection?

attribute float batchId;

varying vec3 v_forwardDirectionEC;
varying vec3 v_texcoordNormalization_and_halfWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the underscores.

@bagnell
Copy link
Contributor

bagnell commented Jun 12, 2018

This puts us over 10,000 tests:
image

@bagnell
Copy link
Contributor

bagnell commented Jun 12, 2018

This crashes in IE.

@bagnell
Copy link
Contributor

bagnell commented Jun 12, 2018

I'm seeing some overdraw in Edge.
lines_on_terrain_overdraw

@likangning93
Copy link
Contributor Author

I'm seeing some overdraw in Edge.

This is happening in Chrome too, it's because we're permitting much steeper angles now before breaking the miter.

So the problem is that the solution for this thing doesn't actually work:

41000750-f45cc6fe-68dc-11e8-9413-84d0df60109a

It works almost okay for the two volumes on either side of the sharp turn, but the next set of volumes don't have enough information to avoid overdraw. In other words, it was only fixing a part of the original overdrawn region.

Here's what the original overdrawn region can look like for very steep angles:

trianglePunt

Note that there are 3 regions of overdraw here, with only one region (the furthest to the right) exclusively involving the two segments immediately adjacent to the turn. Only that region is getting resolved correctly by my solution, leaving overdraw in all other areas:

blaw

This problem becomes much less noticeable when granularity in the examples is set to zero, which isn't practical for long-distance lines.

Resolving this completely might require giving those longer rectangular regions information about the turn, which is another pile of vertex attributes (definitely some high precision ones) and could be kind of terrible.
Had an offline discussion with @bagnell and @pjcozzi, again, the overdraw is only a problem with transparent materials, is mostly noticeable for very wide lines, and also might not even be a huge deal to most users. I'm going to revert to breaking less acute angles and accepting the overdraw for now and open an issue with notes from this PR for further investigation/experimentation.

@likangning93
Copy link
Contributor Author

@bagnell up-to-date and passing

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.

6 participants