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

Improve performance of trimmed(from:,to:) #192

Merged
merged 1 commit into from Jul 19, 2022

Conversation

jonkan
Copy link
Contributor

@jonkan jonkan commented Jun 16, 2022

So, I found the performance of the LineString.trimmed(from:to:) to be unusably slow and that it's caused by this distance calculation.

Calculating the distance between coordinates i and i+1 can be done with a simple/cheap distance(to:), but instead the LineString.distance(from:to:) is used, which kills performance because it allows from/to to not be on the line and has to calculate which line coordinate is closest using closestCoordinate(to:), a O(n) operation, which leaves trimmed(from:to:) at a terrible O(n^2).

Please let me know if I'm missing anything.

@azarovalex
Copy link
Member

@jonkan Thanks for contributing!

Nice catch, looks like we've used the wrong distance method the whole time. Original turf-js repo uses this method for distance and that's exactly the method you use in this PR.

@1ec5 please take a final look at the PR and let's merge this.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Great catch – this is a tricky part of porting anything from Turf.js to turf-swift, since turf-swift uses object-oriented methods instead of global functions. Thank you for your contribution!

@1ec5 1ec5 merged commit a58b995 into mapbox:main Jul 19, 2022
@jonkan
Copy link
Contributor Author

jonkan commented Jul 19, 2022

Happy to help! ❤️

@azarovalex azarovalex mentioned this pull request Aug 11, 2022
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.

None yet

3 participants