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

format function #4

Closed
calvinmetcalf opened this issue Jul 19, 2013 · 8 comments
Closed

format function #4

calvinmetcalf opened this issue Jul 19, 2013 · 8 comments

Comments

@calvinmetcalf
Copy link

add the ability to pass a function to the format argument, something like a function that returns a bbox given an object

@mourner
Copy link
Owner

mourner commented Jul 19, 2013

The reason I didn't do this is that item sorting functions (sort by minX, minY, etc.) heavily affect the performance of the library, and they need to be as simple and direct as possible to perform well. If I accepted a format function that returns a bbox from an item, I would be forced to call it from inside the sorting functions, and performance would drop dramatically. Function compilation performs much better.

@mourner mourner closed this as completed Jul 19, 2013
@calvinmetcalf
Copy link
Author

What I was thinking was a function that would be called when the feature was added not one to be used internally.

@mourner
Copy link
Owner

mourner commented Jul 19, 2013

I see... This has some implications as well, as you would have to keep all item bboxes separately from items. Is there a really important use case for this that I'm missing, or is it just more of a nice to have for convenience?

@mourner mourner reopened this Jul 19, 2013
@calvinmetcalf
Copy link
Author

off the top of my head the use case was consuming geojson that doesn't have the bbox property set but also coordinate transformations would be another conceivable use

@mourner
Copy link
Owner

mourner commented Jul 19, 2013

I think I'd suggest data pre-processing in this case. E.g. go over all features and set a bbox property. Also, next GeoJSON spec will restrict coordinates to WGS84, so no need for coords convertions. geojson/draft-geojson#6

@calvinmetcalf
Copy link
Author

Those are two separate thoughts btw but I was actually thinking in terms of someone wanting to have the tree be in a projected coordinate system not the other way around.

Pre-processing would be the optimal way but that can be applied to having the data in any other format besides the default. If done right (and I can send a pull request) it shouldn't slow down anyone who doesn't use it.

@mourner
Copy link
Owner

mourner commented Jul 21, 2013

But if it indeed does slow down the algorithm if used, I'd rather encourage data preprocessing than allow using RBush with expectedly subpar performance. And so far I couldn't come up with a way to introduce a format function without affecting the performance much due to the way sorting is used in the algorithm. If you do, please let me know.

@mourner
Copy link
Owner

mourner commented Nov 21, 2013

Fixed in a way by #14

@mourner mourner closed this as completed Nov 21, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants