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

Earcut.js #3998

Merged
merged 18 commits into from
Jun 6, 2016
Merged

Earcut.js #3998

merged 18 commits into from
Jun 6, 2016

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jun 3, 2016

Uses earcut.js for triangulation. Fixes #2788. Fixes some of the files uploaded to #3044, but the ones that still don't work look better. The files in #1209 work. The problem with the US states happens because of the intersecting edges.

TODO:

  • Remove eliminateHoles code
  • Remove triangulate code
  • Update tests
  • Profile
  • Clean up
  • Update LICENSE.md
  • Update CHANGES.md

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2016

All looks good so far!

//>>includeStart('debug', pragmas.debug);
if (!defined(positions)) {
throw new DeveloperError('positions is required.');
}
if (positions.length < 3) {
throw new DeveloperError('At least three positions are required.');
if (!defined(holes)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required, it doesn't look like earcut requires it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks.

@bagnell
Copy link
Contributor Author

bagnell commented Jun 3, 2016

@pjcozzi This is ready for another look.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2016

All looks good to me.

@mramato can you test the OSM NYC dataset offline?

@bagnell are there any other datasets you want to test with?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2016

Do you have a representative total performance improvement for a large dataset?

@bagnell
Copy link
Contributor Author

bagnell commented Jun 6, 2016

@pjcozzi Triangulation when loading all of the country borders takes 305.322ms on average for the whole dataset in the branch and 641.598ms in master.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 6, 2016

Sounds great. Can you add this example performance improvement to CHANGES.md?

Is anything else needed before we can merge this?

@mramato
Copy link
Contributor

mramato commented Jun 6, 2016

FYI, I ran this on the full NYC set I was working with last week and it fixed tons of problems that we were having with the old triangulation code. So this is a big 👍 from me.

@bagnell
Copy link
Contributor Author

bagnell commented Jun 6, 2016

CHANGES.md was updated. I think this is good to go.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 6, 2016

@bagnell given that this will be of interest to many users, can you please email the forum to let them know this is coming in Cesium 1.23 on July 1?

@pjcozzi pjcozzi merged commit f27237e into master Jun 6, 2016
@pjcozzi pjcozzi deleted the earcut branch June 6, 2016 18:20
@@ -5,6 +5,9 @@ Change Log

* Add a `rotatable2D` option to to `Scene`, `CesiumWidget` and `Viewer` to enable a rotatable map in 2D. [#3897](https://github.com/AnalyticalGraphicsInc/cesium/issues/3897)
* `Camera.setView` and `Camera.flyTo` will now use the `orientation.heading` parameter in 2D if the map is rotatable.
* Added `packArray` and `unpackArray` functions to `Cartesian2`, `Cartesian3`, and `Cartesian4`.
* Fix some large polygon triangulations. [#2788](https://github.com/AnalyticalGraphicsInc/cesium/issues/2788)
* Improved performance and accuracy of polygon triangulation by using the [earcut](https://github.com/mapbox/earcut) library. Loading a GeoJSON with polygons for each country was ~50% faster.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean 2x faster, i.e., 50% less time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I'll fix it in master.

@mourner
Copy link

mourner commented Jun 6, 2016

Awesome work, happy to see it used in Cesium!

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