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

More sophisticated RTree implementation #719

Merged
merged 9 commits into from May 21, 2013

Conversation

ahocevar
Copy link
Member

This new implementation is based on
http://github.com/imbcmdth/RTree/, with only a few modifications
to add the optional type and provide the API of the previous
implementation.

There is still room for optimization, but this is such an
improvement over the previous RTree already that it's worth
bringing it in now.

This new implementation is based on
http://github.com/imbcmdth/RTree/, with only a few modifications
to add the optional type and provide the API of the previous
implementation.

There is still room for optimization, but this is such an
improvement over the previous RTree already that it's worth
bringing it in now.
@ahocevar
Copy link
Member Author

More work needs to be done to make this work in advanced compilation mode.

@ahocevar
Copy link
Member Author

This is ready for review. I'm sure the Closure integration can be improved, but for now it works.

@tschaub
Copy link
Member

tschaub commented May 20, 2013

Thanks for taking this on @ahocevar. Looks like a few more changes are needed to satisfy JSDoc3. I hope to get a chance to look this over in the next day or two.

Not using Math.max and Math.min increases performance, and by
using ol.extent functions instead of RTree's rectangle
structures and calculations we can get rid of several functions.
Also fixes some issues that were revealed by the new tests.
@ahocevar
Copy link
Member Author

@tschaub: JSDoc3 is also happy now. Don't worry if you don't see anything in the resulting docs - that's ok because there is nothing exported to the public API.

tree.nodes.splice(i + 1, 1); // Remove unsplit node
// workingObject.nodes contains a list of elements removed from the
// tree so far
if (tree.nodes.length > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but it would match the rest of our style to have braces here (and below).

@tschaub
Copy link
Member

tschaub commented May 21, 2013

This is a fantastic improvement @ahocevar. Thanks for taking the time to adapt it.

While it's a silly example, its nice to see the canvas renderer handling 20K features with aplomb: https://gist.github.com/tschaub/5617621

On my machine, these synthetic features take about 1s to create, about 1s to index (with 266ms in goog.array.concat), and under 400ms to render (~130ms in ol.renderer.canvas.VectorRenderer.renderPointFeatures_, ~90ms in ol.style.Shape.createLiteral, ~85ms in ol.layer.Vector.groupFeaturesBySymbolizerLiteral). And there is plenty of room for optimization (for example, for simple cases of a single symbolizer literal, we can check symbolizer.isLiteral() and avoid all the time spent iterating through features, creating literals, and then grouping). It's clear that time spent loading real data will quickly eclipse this rendering time.

I can imagine we'll have discussion about where this gets used. It may only be of use in the Canvas 2d rendering case (in which case it should not be required by the vector layer). But I don't think that should keep this from going in.

Please merge.

ahocevar added a commit that referenced this pull request May 21, 2013
More sophisticated RTree implementation. r=@tschaub
@ahocevar ahocevar merged commit 485245f into openlayers:master May 21, 2013
@ahocevar ahocevar deleted the fast-rtree branch May 21, 2013 20:01
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.

None yet

2 participants