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

Optimize Polygon triangulation #2305

Merged
merged 13 commits into from
Dec 3, 2014
Merged

Optimize Polygon triangulation #2305

merged 13 commits into from
Dec 3, 2014

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Dec 3, 2014

For #2074.

I still want to look at the performance, but the code can be reviewed. You might want to look at the individual commits before and after the fourth one; I moved all of the functions around so they were defined before they were called.

@pjcozzi This doesn't change the subdivision as we discussed. If its better than how its done now, it'll be in a separate PR.

@shunter
Copy link
Contributor

shunter commented Dec 3, 2014

I suspect you'll get additional peformance gains by removing getRandomIndex and related functions and using the mersenne twister from Math.nextRandomNumber instead.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 3, 2014

For performance, can we look at:

  • Our standard big datasets for this.
  • Simple convex and really complicated non-convex. These could be extracted from the above.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 3, 2014

Also fixes #1089.

@@ -512,6 +512,25 @@ define([
};

/**
* The modulo operation that also works for negative dividends.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to CHANGES.md. Once we know the performance gain, we should also mention it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 3, 2014

Code looks fine. @bagnell can you also profile this as part of the performance testing?

@bagnell
Copy link
Contributor Author

bagnell commented Dec 3, 2014

Total time triangulating in ms.

Polygons master This branch
US states 49.5 23.2
US counties 7754.9 2737.3
Country borders 6690.7 2606.6

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 3, 2014

CC @DavidHudlow

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 3, 2014

Very nice. Let's look at subdivision in a separate pull request.

pjcozzi added a commit that referenced this pull request Dec 3, 2014
@pjcozzi pjcozzi merged commit 3db3652 into master Dec 3, 2014
@pjcozzi pjcozzi deleted the triangulateCleanUp branch December 3, 2014 21:34
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