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 LocationIndexTree.query(BBox) #1485

Merged
merged 18 commits into from Apr 17, 2019

Conversation

@karussell
Copy link
Member

karussell commented Oct 16, 2018

This PR adds a new query(BBox) method. Currently we can fetch only nodes from one leaf and so with this querying areas is inefficient. Related to #1127 and #1324.

Furthermore a possibility to visualize the index

image

and some description for the index is added. The description now makes clear that we have a Quadtree implementation with some special tuning. E.g. for bigger areas one node branches not only into 4 subtrees but can branch into 16. Previously also 64 was possible, but we removed this as this has no advantages in terms of storage, query or creation speed. (Measured it for Germany and world wide)

TODOs:

  • to make this correct we need to add +1/2 cell on every side of the BBox. The problem is that we do not add the node to both cells due to the bresenham line to cell limitation Does not seem to be necessary (no problems e.g. in #1572).
  • make it working with Polygon too (use JTS Polygon e.g. via embedding in ours so that we can implement Shape)
  • use PrepareGeometry -> #1589
  • use intersect and contains!
  • use quality vs. speed properties like UNIQUE_NODES (just make sure nodes come to the Visitor just once), FILTERED_NODES (unique nodes plus filtered again through the shape), HIGH_PRECISION (filtered node but use fetchGeometry instead of just junction nodes)
karussell added 4 commits Oct 15, 2018
…s what's different to usual implementation
@karussell karussell added this to the 0.12 milestone Oct 16, 2018
if (bbox.contains(lat, lon))
indexNodeList.add(nodeId);

// TODO should be done behind the scene?

This comment has been minimized.

Copy link
@karussell

karussell Oct 16, 2018

Author Member

Will make it possible via a SimpleVisitor class or something. Often this iteration has to be done again on the client side e.g. to fetch the edges and so this work would be an unnecessary overhead then.

karussell added 3 commits Oct 16, 2018
…d EdgeVisitor
@karussell karussell added the algorithm label Feb 20, 2019
@karussell karussell modified the milestones: 0.12, 0.13 Feb 25, 2019
@karussell karussell referenced this pull request Mar 19, 2019
7 of 7 tasks complete
@michaz

This comment has been minimized.

Copy link
Member

michaz commented Apr 11, 2019

Nice, this closes some open ends for me. Is the current state mergeable, meaning, do we think that the bounding-box query does more or less what it should? Because I'd like to.

@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 12, 2019

Well, this is not fully ready unfortunately. Currently all nodes are returned where the tile intersects with the shape. This is ok for visualisation, but for other use cases we have to filter these nodes and call "shape.contains(node)" as a post processing step. Now the problem is that this could be done cheap like this or in a more expensive manner if we need high precision via "shape.intersects(edge.fetchWayGeometry)". Currently exploring the options on how to do this so that this also works for a Polygon (which basically means using JTS Polygon behind our Polygon class). But we could also bring it into a mergable state faster and I'll create a new issue for the open tasks.

@michaz

This comment has been minimized.

Copy link
Member

michaz commented Apr 12, 2019

Oh, that's exactly what I need: "May include more nodes than are contained, but never fails to return return a node that is contained". I think that's also how other implementations work.

@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 12, 2019

Oh, that's exactly what I need: "May include more nodes than are contained, but never fails to return return a node that is contained". I think that's also how other implementations work.

Ah, ok. I think there are indeed two use cases. This one, which you need and we need for fast visualization. But also the other to e.g. block certain edges but not too many!

Maybe I'll bring this PR in a mergable form for the first use case and explicitly mention this and work later on the second "high precision" use case?

@michaz

This comment has been minimized.

Copy link
Member

michaz commented Apr 12, 2019

Maybe I'll bring this PR in a mergable form for the first use case and explicitly mention this and work later on the second "high precision" use case?

Yes, please! For me, this doesn't even mean that this is currently unfinished -- filtering the result to the desired level of exclusivity is a second task, and can happen outside of the current method. This one is finished.

@michaz

This comment has been minimized.

Copy link
Member

michaz commented Apr 12, 2019

And this method can still narrow down what it returns later on without changing its promise.

@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 12, 2019

filtering the result to the desired level of exclusivity is a second task

This is exactly where I'm unsure because the method index.query(shape) could also imply that this should be an exact filter and no post-processing work needs to be done.

@michaz

This comment has been minimized.

Copy link
Member

michaz commented Apr 12, 2019

Well, "query" as a method name is vague enough that I think it can't be misunderstood as promising anything specific. ;-)

Let's add a Javadoc which explains what this method does and does not (not how it does that), and it'll be fine.

@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 12, 2019

Ok :D

Will do this ... let me just do some minor stuff.

@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 12, 2019

I did the following changes (let me know if this was too much):

  • moved the Visitor interfaces and the query method to the LocationIndex interface
  • removed the extra visualize method and made it possible via the Visitor setting isTileInfo (now Visitor has to be an abtract class as default methods are not possible in 7). onTile now directly includes the depth not the too-visualization-specific width.
  • avoid duplicate nodes (and edges for the EdgeVisitor)
  • introduced a new "queryBBox.contains" branch to avoid bbox checks on greater depths, should make the query slightly faster
  • renamed Shape.intersect to intersects, also to make later migrations to JTS easier
  • renamed *cell* to *tile*
  • made some methods non-public
@karussell karussell requested a review from michaz Apr 12, 2019
karussell and others added 5 commits Apr 12, 2019
/**
* This interface allows to visit every node stored in the leafs of a LocationIndex.
*/
abstract class Visitor {

This comment has been minimized.

Copy link
@michaz

michaz Apr 14, 2019

Member

Just as a small remark: This Visitor is not very Java-8-friendly (see my call of the new query method in the commit I added - I can't do a lambda there, because the visitor is not a single-method interface but a multi-method abstract class), and also a bit unusual. If I understand it correctly, as a client, I have to override isTileInfo if I want onTile to be called? I think interfaces are most clear when control flows in one direction only: An interface is either for me to call, or for me to implement and be called.

This comment has been minimized.

Copy link
@michaz

michaz Apr 14, 2019

Member

An interface is either for me to call, or for me to implement and be called.

Okay, that's actually the case here. :-) But the information flows both ways.

This comment has been minimized.

Copy link
@karussell

karussell Apr 14, 2019

Author Member

Yes, I tried with a separate QueryOptions class but this was not much prettier. Should I revert it back to this to make it easier with lambda?

This comment has been minimized.

Copy link
@michaz

michaz Apr 14, 2019

Member

Ah no, it's okay.

This comment has been minimized.

Copy link
@michaz

michaz Apr 14, 2019

Member

Thanks! I'm doing more Isochrone experiments with this right now... ,-)

@michaz
michaz approved these changes Apr 14, 2019
@michaz michaz merged commit fa78298 into master Apr 17, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.