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

Turn Costs Cleanup #269

Closed
2 of 5 tasks
karussell opened this issue Oct 22, 2014 · 3 comments
Closed
2 of 5 tasks

Turn Costs Cleanup #269

karussell opened this issue Oct 22, 2014 · 3 comments

Comments

@karussell
Copy link
Member

open work here:

  • get turn information via 'from-edge' instead of the 'via node'. Seems not yet worth it, it might be okay to just add the shortcut turn-info to the current node-based turn-info, maybe a sorted insert to possibly return earlier while searching. Because an edge-based turn information storage requires some changes and makes the turn information retrieval slightly more complex. We would need to store the turn information onto the 'from-edge' but also on the 'to-edge' when we traverse the graph in 'reverse' direction. Then also accessing the 'to-edge' turn information needs refactoring as we do not want to call graph.getEdgeProps for every edge (see good alternatives in this issue). Or the turn information retrieval is inefficient for 'to-edges'
  • sorted turn information insert to make lookup faster
  • clean up flagencoders somehow to remove OSMReader dependency
  • it could be valueable to create a reusable variant of graph.getEdgeProps to avoid heavy object creation. E.g. edgeExplorer.getEdge(edgeId, adjNode) and/or even very low level edgeExplorer.getField(edgeId, edgeRowIndex)
  • Algorithm.updateBestPath requires EdgeIteratorState which is only necessary to substract the weight of the overlapping edge - can we avoid it?
@karussell karussell added this to the 0.4 milestone Oct 22, 2014
@karussell karussell removed this from the 0.4 milestone Feb 23, 2015
@Azbesciak
Copy link
Contributor

Maybe a little bit out-of-the-game, but two questions:

  1. did you consider expanding implementation to take into account also longer than 2 edges turn relation? As I saw in the code, the lowest and most efficient to the current implementation way would be to keep start and end of relation, via node and relation length. So basically, now it would require one additional field in turn cost table, but also something which allows to pass "edge history" to weight function, so something like SPTEntry... :/ not so good, also won't work with CH.
  2. If so, maybe implementation based on graph would be more efficient and usefull? I meant - remove junctions and create 'direct' routes where there is a turn prevention. I think this pattern is used in OSRM, but could not find it. Also, it is mentioned in http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.721.8922&rep=rep1&type=pdf (OSRM based on it while implementing MLD algorithm). I mean something like this (beautiful, I know :) - all edges are not directed except the one with arrow in the result graph):
    image

@easbar
Copy link
Member

easbar commented Jul 15, 2020

did you consider expanding implementation to take into account also longer than 2 edges turn relation?

This is #446

remove junctions and create 'direct' routes where there is a turn prevention.

See this forum discussion: https://discuss.graphhopper.com/t/node-based-ch-with-turn-cost/5677/12

get turn information via 'from-edge' instead of the 'via node'.

This might be an option for #2084, but why was this bullet point resolved, the turn costs are still (or again?) stored by via node.

sorted turn information insert to make lookup faster

This also might be an option for #2084

it could be valueable to create a reusable variant of graph.getEdgeProps to avoid heavy object creation. E.g. edgeExplorer.getEdge(edgeId, adjNode) and/or even very low level edgeExplorer.getField(edgeId, edgeRowIndex)

We have now (for some time) have graph.getEdgeIteratorState(edge, adjNode), but that has some problems as well or what was meant here? And how is this related to this issue?

Algorithm.updateBestPath requires EdgeIteratorState which is only necessary to substract the weight of the overlapping edge - can we avoid it?

We still do this subtraction, but only for non-CH. Why is this a problem and how is it related to this issue?

I think we can close here in favor of #2084?

@karussell
Copy link
Member Author

Why is this a problem and how is it related to this issue?

I think is issue is very old (6 years) and I just wanted to write down some possible improvements after the big refactoring that introduced turn costs. So it is more a issue collection.

I think we can close here in favor of #2084?

Sure :)

@easbar easbar closed this as completed Jul 15, 2020
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

3 participants