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

add steiner support and add test cases from earcut.js #23

Closed
wants to merge 3 commits into from

Conversation

mrgreywater
Copy link
Collaborator

Port of steiner points (see #23 (comment)).
Also added missing tests from earcut.js

@mourner
Copy link
Member

mourner commented Feb 10, 2016

Here's the reason why we haven't yet ported the tests: #6 (comment)

If you find out why the tests fail and fix the PR so that the build passes, that would awesome!

@mrgreywater
Copy link
Collaborator Author

@mourner Yes, this PR is based on the port-tests branch, because there were still (failing) new tests in earcut.js that were missing in this branch. This is meant to be merged into the port-tests branch, not the master branch.

@mourner
Copy link
Member

mourner commented Feb 10, 2016

@mrgreywater but the build on Travis still fails for this PR, right? How could this be fixed?

@mrgreywater
Copy link
Collaborator Author

@mourner If I'm reading the output right the build completes and runs, but the tests are failing, so travis reports that the checks failed. This is fine, since the whole purpose of this branch is that the tests may fail.

@mourner
Copy link
Member

mourner commented Feb 11, 2016

@mrgreywater if the tests are failing, Travis looses its sense since we won't be able to track down regressions. We need to figure out how to make them pass before we can merge new tests.

@mrgreywater
Copy link
Collaborator Author

@mourner Then I don't get why the "port-tests" branch exists at all. It's failing at travis already without this PR because it contains failing tests, why not add the rest of those aswell then.

...mumble mumble.. test driven development ..mumble...

@mrgreywater
Copy link
Collaborator Author

This last commit is porting mapbox/earcut@deb0bfe, mapbox/earcut@65c7da5, mapbox/earcut@68da53f and some others. Test cases now match earcut.js again (and work). Closing the other PR for now, since it duplicates some of this PR's changes.

This pretty much resolves #6 and #13.

@mrgreywater mrgreywater changed the title add missing tests from earcut.js add steiner support and add test cases from earcut.js Feb 11, 2016
@mrgreywater
Copy link
Collaborator Author

Performance record (i5-3570K@3.4Ghz, compiled with GCC 4.9.3 with -O3)
Before:

+----------------+--------------------+--------------------+
| Polygon        | earcut             | libtess2           |
+----------------+--------------------+--------------------+
| bad_hole       |      307.127 ops/s |       21.730 ops/s |
| building       |    1.764.854 ops/s |       51.918 ops/s |
| degenerate     |    4.069.767 ops/s |       90.989 ops/s |
| dude           |      103.678 ops/s |       11.735 ops/s |
| empty_square   |    2.673.845 ops/s |       78.770 ops/s |
| water_huge     |          218 ops/s |           60 ops/s |
| water_huge2    |           87 ops/s |           76 ops/s |
| water          |        1.288 ops/s |          157 ops/s |
| water2         |        1.476 ops/s |          715 ops/s |
| water3         |       43.150 ops/s |        5.477 ops/s |
| water3b        |      473.830 ops/s |       32.914 ops/s |
| water4         |        5.412 ops/s |        1.305 ops/s |
+----------------+--------------------+--------------------+

After:

+----------------+--------------------+--------------------+
| Polygon        | earcut             | libtess2           |
+----------------+--------------------+--------------------+
| bad_hole       |      175.355 ops/s |       21.754 ops/s |
| building       |    1.899.877 ops/s |       50.988 ops/s |
| degenerate     |    3.422.676 ops/s |       89.196 ops/s |
| dude           |      107.021 ops/s |       11.673 ops/s |
| empty_square   |    2.373.604 ops/s |       76.254 ops/s |
| water_huge     |           87 ops/s |           59 ops/s |
| water_huge2    |           42 ops/s |           78 ops/s |
| water          |        1.192 ops/s |          157 ops/s |
| water2         |        1.333 ops/s |          712 ops/s |
| water3         |       38.208 ops/s |        5.436 ops/s |
| water3b        |      409.587 ops/s |       32.757 ops/s |
| water4         |        4.874 ops/s |        1.297 ops/s |
+----------------+--------------------+--------------------+

as @mourner said here
mapbox/earcut@deb0bfe causes some performance regression in earcut.hpp, but unless we want to diverge from earcut.js in both library code and in the unit-tests, there is not much we can do about it.

I'm curious about the performance difference (if any) that this commit had to earcut.js itself.

@mourner
Copy link
Member

mourner commented Feb 11, 2016

This is great, thanks for bringing earcut.hpp up to date and fixing tests! The regression from my benchmarks looked a bit worse as far as I remember but that may be just some platform deviation, and after all I think it only regresses on really bad data with lots of self-intersections. When the polygons are clean, the performance should improve (like the building case above). So I'll merge this to master.

@mourner
Copy link
Member

mourner commented Feb 12, 2016

Rebased and merged to master 👍

@mourner mourner closed this Feb 12, 2016
@mrgreywater
Copy link
Collaborator Author

Mmh, that rebase was kind of unnecessary. I would've just merged it onto port-tests and then merged that branch with master. Thanks for adding the changes though.

@mourner
Copy link
Member

mourner commented Feb 12, 2016

@mrgreywater just reducing the number of merge commits in git history, and rebase was clean.

@mrgreywater
Copy link
Collaborator Author

I could've rebased myself if you asked, then this PR would appear as merged and not closed, but it's alright :)
The port-tests branch can probably be removed.

@mrgreywater mrgreywater deleted the port-tests branch February 17, 2016 13:20
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