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

support for polylines on terrain via entity API #6689

Merged
merged 4 commits into from
Jun 18, 2018

Conversation

likangning93
Copy link
Contributor

See #6615

@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 am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@likangning93 likangning93 force-pushed the polylinesOnTerrain-entity branch from f5f1817 to 55da339 Compare June 14, 2018 19:58
/**
* @private
*/
function StaticGroundPolylinePerMaterialBatch(orderedGroundPrimitives) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly copied from StaticGroundGeometryPerMaterialBatch, it just allows a lot more flexibility in batching and needs its own primitive-type.

onselect : function() {

if (!Cesium.Entity.supportsPolylinesOnTerrain(viewer.scene)) {
console.log('Polylines on terrain is not supported on this platform');
Copy link
Contributor

Choose a reason for hiding this comment

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

is -> are

rectangle : {
coordinates : Cesium.Rectangle.fromDegrees(-99.5, 31.0, -90.0, 41.0),
material : Cesium.Color.BLUE,
zIndex: 1
}
});

if (!Cesium.Entity.supportsPolylinesOnTerrain(viewer.scene)) {
console.log('Polylines on terrain is not supported on this platform, Z-index will be ignored');
Copy link
Contributor

Choose a reason for hiding this comment

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

is -> are for here and below.

var that = this;

// Load terrain heights
if (!this._terrainHeightsReady) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check GroundPolylinePrimitive._initialized or make a private function so this doesn't wait on the promise for every instance.

@@ -513,8 +575,11 @@ define([

this._line = line;
this._primitives = primitives;
this._groundPrimitives = groundPrimitives;
this._groundPolylinePrimitive = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this could be a line in a PolylineCollection of a GroundPolylinePrimitive, delay the PolylineCollection creation and the line added until you know which.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could potentially create and destroy primitives a lot if clampToGround is dynamic. What do you think @mramato?

@bagnell
Copy link
Contributor

bagnell commented Jun 15, 2018

Should we automatically clamp to ground when all of the position heights are zero?

@likangning93
Copy link
Contributor Author

Should we automatically clamp to ground when all of the position heights are zero?

I was wondering if this would be kind of a breaking change, but I guess I can't see a use case where users would be counting on intentional heights of 0 behaving like they do now.

It does put additional onus on the Entity API layer to check position heights though, which seems like it could duplicate a lot of the cartesian-to-cartographic work in GroundPolylineGeometry.

@mramato
Copy link
Contributor

mramato commented Jun 16, 2018

I was wondering if this would be kind of a breaking change, but I guess I can't see a use case where users would be counting on intentional heights of 0 behaving like they do now.

It does put additional onus on the Entity API layer to check position heights though, which seems like it could duplicate a lot of the cartesian-to-cartographic work in GroundPolylineGeometry.

I would recommend against this change. There are certainly places in the world where the ground is below 0, so it's not even a guarantee on the ground is what is desired, plus there are performance issues to consider as well. We might change our mind later (when terrain is on by default) but I wouldn't suggest it for this PR.

@likangning93
Copy link
Contributor Author

@bagnell updated!

@bagnell
Copy link
Contributor

bagnell commented Jun 18, 2018

👍

@bagnell bagnell merged commit 6d55550 into CesiumGS:master Jun 18, 2018
@likangning93 likangning93 deleted the polylinesOnTerrain-entity branch June 18, 2018 20:59
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