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

Kosta/fix index creation (fix for #13) #14

Merged
merged 4 commits into from
Mar 9, 2015
Merged

Kosta/fix index creation (fix for #13) #14

merged 4 commits into from
Mar 9, 2015

Conversation

Kosta-Github
Copy link

This is a potential fix for #13

@mourner
Copy link
Member

mourner commented Mar 9, 2015

Thanks! Looks good to me. I'll now check if we can avoid getting vertex length on each vertex twice, and calculate dimensionality once instead...

@Kosta-Github
Copy link
Author

how about this?

@Kosta-Github
Copy link
Author

you could also make the number of dimensions variable (but the same for all vertices) and not just limiting it to 2 and 3 by explicitly using node.p.length for dim and not clamp it to 3. then you could replace https://github.com/mapbox/earcut/blob/master/src/earcut.js#L170-L172:

        vertices.push(node.p[0]);
        vertices.push(node.p[1]);
        if (node.p.length > 2) vertices.push(node.p[2]);

by

    vertices.concat(node.p);

@mourner
Copy link
Member

mourner commented Mar 9, 2015

concat produces a new array, so it's slow.
Looks good now. Do you think there are use cases that require more than 3 dimensions?

@Kosta-Github
Copy link
Author

ah yeah, right about concat...

I could imagine to store additional data with the vertices, such as normals, colors, uv-coordinates for texture mapping, and then use interleaved buffer arrays for that data to render it with WebGL...

But leaving that out for now works for me. If/when I have a concrete use case for that I can create another PR...

mourner added a commit that referenced this pull request Mar 9, 2015
@mourner mourner merged commit 7855909 into mapbox:master Mar 9, 2015
@Kosta-Github Kosta-Github deleted the Kosta/fix_index_creation branch March 9, 2015 16:43
@mourner
Copy link
Member

mourner commented Mar 9, 2015

Published in 1.4.0. Also fixed the code to work with any number of dimensions.

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