-
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
Adjust polygon position heights before removing duplicates if not perPositionHeight #6731
Conversation
@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, 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 can you review. Please update CHANGES. |
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.
Just two small things in a couple places.
@@ -217,6 +217,13 @@ define([ | |||
var outerRing = outerNode.positions; | |||
var holes = outerNode.holes; | |||
|
|||
var i; | |||
if (!perPositionHeight) { | |||
for (i = 0; i < outerRing.length; i++) { |
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.
Should this get set on a temp var before the loop?
var holePositions = arrayRemoveDuplicates(hole.positions, Cartesian3.equalsEpsilon, true); | ||
var holePositions = hole.positions; | ||
if (!perPositionHeight) { | ||
for (j = 0; j < holePositions.length; ++j) { |
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.
Same as above.
while (queue.length !== 0) { | ||
var outerNode = queue.dequeue(); | ||
var outerRing = outerNode.positions; | ||
if (!perPositionHeight) { | ||
for (i = 0; i < outerRing.length; i++) { |
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.
Same as above.
if (hole.positions.length < 3) { | ||
var holePositions = hole.positions; | ||
if (!perPositionHeight) { | ||
for (j = 0; j < holePositions.length; ++j) { |
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.
Same as above.
}; | ||
|
||
var geometry = PolygonOutlineGeometry.createGeometry(new PolygonOutlineGeometry({ polygonHierarchy : hierarchy, perPositionHeight: true })); | ||
expect(geometry).not.toBeUndefined(); |
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.
.toBeDefined()
Specs/Core/PolygonGeometrySpec.js
Outdated
}; | ||
|
||
var geometry = PolygonGeometry.createGeometry(new PolygonGeometry({ polygonHierarchy : hierarchy, perPositionHeight: true })); | ||
expect(geometry).not.toBeUndefined(); |
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.
.toBeDefined()
Thanks @likangning93! Updated |
Fixes crash reported in #4863 (comment)
If
perPositionHeight: false
, the positions passed toPolygonGeometry
are scaled to the ellipsoid surface then adjusted forheight
andextrudedHeight
. However, this needs to happen beforearrayRemoveDuplicates
for the case where there are positions with the same lon/lat but different heights.