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

Use rbush to speed up probe checks #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use rbush to speed up probe checks #8

wants to merge 1 commit into from

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 31, 2016

Fixes #7. Makes the implementation considerably more complex though, and I have yet to set up a real world benchmark on a variety of polygons — preliminary results show ~3-4 faster times after warming up, which is less than I hoped for. The speedup may be bigger on other platforms though.

@urschrei
Copy link

urschrei commented Sep 1, 2016

Nice! I'll have a look at using https://github.com/ambaxter/spatial-rs for my implementation, but yeah, I have a feeling it might be complex to implement…

@drnextgis
Copy link

drnextgis commented Dec 28, 2016

I've tested this PR on one synthetic polygon. Calculation took about one minute. It looks like no effect compared original version:

[ [ [ 12382242.131689133122563, -7840437.468911342322826 ], [ 12382242.131689133122563, 323370.434348836948629 ], [ 18995007.833730760961771, 323370.434348836948629 ], [ 18995007.833730760961771, -7840437.468911342322826 ], [ 12382242.131689133122563, -7840437.468911342322826 ] ], [ [ 15093476.069526199251413, -6916429.451484249904752 ], [ 16647476.009505981579423, -6916429.451484249904752 ], [ 16647476.009505981579423, -4179464.127069373615086 ], [ 18631305.720118477940559, -4179464.127069373615086 ], [ 18631305.720118477940559, -2660328.417218445800245 ], [ 16647476.009505981579423, -2660328.417218445800245 ], [ 16647476.009505981579423, -106602.746021312093944 ], [ 15093476.069526199251413, -106602.746021312093944 ], [ 15093476.069526199251413, -2660328.417218445800245 ], [ 12878199.559342253953218, -2660328.417218445800245 ], [ 12878199.559342253953218, -4179464.127069373615086 ], [ 15093476.069526199251413, -4179464.12706937501207 ], [ 15093476.069526199251413, -6916429.451484249904752 ] ] ]

@TWiStErRob
Copy link

Something you may want to consider: I like the fact that the code is just 150 lines + a heap base priority queue implementation; e.g. I'm planning to use this in Java on Android. By accepting this pull request you add ~500+50 lines of dependencies, this hurts portability between languages a lot, and may make it harder to understand the algorithm.

@mourner
Copy link
Member Author

mourner commented Dec 28, 2016

@TWiStErRob yes, that's the main reason I haven't pursued merging this much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants