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

Fix the position generation for extruded polygons with holes #447

Merged
merged 2 commits into from
Mar 22, 2017

Conversation

howtimeflies0
Copy link

@howtimeflies0 howtimeflies0 commented Mar 22, 2017

@@ -87,6 +87,7 @@ export class PolygonTesselatorExtruded {
}

function countVertices(vertices) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete empty line?

const topVertices = [].concat(vertices);
const positions = groupedVertices.map(
vertices => {
const topVertices = Array.prototype.concat.apply([], vertices);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the difference here?

Copy link
Author

@howtimeflies0 howtimeflies0 Mar 22, 2017

Choose a reason for hiding this comment

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

Yeah, the bug is that applying double map() to groupedVertices changes the sequence of the vertices. So I reverted back to a single map() as in ExtrudedChoroplethLayer.

However, to use single map(), I need to use concat.apply() to have topVertices in the flattened format for the baseVertices to be correctly calculated on the next line.

It took me some time to figure out that
[].concat([[1, 2, 3]]) is [[1, 2, 3]],
Array.prototype.concat.apply([], [[1, 2, 3]]) is [1, 2, 3]

Learned a Javascript lesson....

Fix the polygon layer example in the layer browser
@howtimeflies0 howtimeflies0 merged commit 468ed74 into master Mar 22, 2017
@howtimeflies0 howtimeflies0 deleted the extruded-polygon-holes branch March 22, 2017 19:15
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.

2 participants