-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
support for polylines on terrain via entity API #6689
Conversation
@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. 🌍 🌎 🌏 |
f5f1817
to
55da339
Compare
/** | ||
* @private | ||
*/ | ||
function StaticGroundPolylinePerMaterialBatch(orderedGroundPrimitives) { |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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 |
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. |
@bagnell updated! |
👍 |
See #6615