Support of "via" points #147

Closed
wants to merge 36 commits into
from

Conversation

Projects
None yet
2 participants
@ratrun
Contributor

ratrun commented Jan 28, 2014

I have started with an implementation of "via" points. The final goal is that one may not specify only "From" and "To", but also an ordered list of "via" points. Unfortunately the current implementation does not work as intended with CH. The test DijkstraBidirectionCHTest is failing.

Could you please take a look and provide feedback: Are the current problems with CH caused by a wrong approach or can it be solved by clearing some temporary data within the routing algorithms?

ratrun added some commits Jan 28, 2014

Support of "via" points: Initial implementation of public List<Path> …
…calcPathList( int[] from_via_to_list ) as suggested by Peter. The new test in the AbstracRoutingAlgorithmTester works with every algorithm, except within DijkstraBidirectionCHTest.
@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Feb 4, 2014

Member

Thanks!

I would like to have the reset thing in a separate issue as we need to make this really carefully. 'Reset' could clear the map or create a new map etc which is different regarding performance. But I really wonder: can we really do this as QueryGraph is different for every query - we need to protect users from problematic behaviour. E.g. a reset for OneToManyDijkstra is harder than it looks.

Member

karussell commented Feb 4, 2014

Thanks!

I would like to have the reset thing in a separate issue as we need to make this really carefully. 'Reset' could clear the map or create a new map etc which is different regarding performance. But I really wonder: can we really do this as QueryGraph is different for every query - we need to protect users from problematic behaviour. E.g. a reset for OneToManyDijkstra is harder than it looks.

@ratrun

This comment has been minimized.

Show comment
Hide comment
@ratrun

ratrun Feb 5, 2014

Contributor

Thanks for your feedback.
I'm sorry, but I have troubles in interpreting your answer. I agree when you write that you would treat the "reset" function as a separate issue. I would consider my current implementation as a first (buggy) implementation attempt, which needs to be cleand up by you. I believe that only you could implement it correctly - as I am a just user of the algorithms.
If you believe that it is technically possible to implement a "reset" function even for CH, then I could go on with the implementation by iterating over the List . Further on, I could start with extending the http interface ...
If you say that you don't know a way to implement the reset correctly for all algorithms, then my approach is rubbish and the whole "via" issue needs to be solved somehow differently. In this case my next question would be, if you could describe in a few words which approach you would choose.

Contributor

ratrun commented Feb 5, 2014

Thanks for your feedback.
I'm sorry, but I have troubles in interpreting your answer. I agree when you write that you would treat the "reset" function as a separate issue. I would consider my current implementation as a first (buggy) implementation attempt, which needs to be cleand up by you. I believe that only you could implement it correctly - as I am a just user of the algorithms.
If you believe that it is technically possible to implement a "reset" function even for CH, then I could go on with the implementation by iterating over the List . Further on, I could start with extending the http interface ...
If you say that you don't know a way to implement the reset correctly for all algorithms, then my approach is rubbish and the whole "via" issue needs to be solved somehow differently. In this case my next question would be, if you could describe in a few words which approach you would choose.

@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Feb 5, 2014

Member

What do you think about the following:

instead of supporting 'via' points in every Algorithm, why not loop over the via points in GraphHopper.route for now and create a new RoutingAlgorithm object for every pair of points. I know it is a huge waste of everything but this way you can implement the UI independently from me (not sure when I find time for a proper reset method) and the calculation is not incomplete.

Or is it more problematic this way when merging the Path/Instructions etc?

Member

karussell commented Feb 5, 2014

What do you think about the following:

instead of supporting 'via' points in every Algorithm, why not loop over the via points in GraphHopper.route for now and create a new RoutingAlgorithm object for every pair of points. I know it is a huge waste of everything but this way you can implement the UI independently from me (not sure when I find time for a proper reset method) and the calculation is not incomplete.

Or is it more problematic this way when merging the Path/Instructions etc?

@ratrun

This comment has been minimized.

Show comment
Hide comment
@ratrun

ratrun Feb 6, 2014

Contributor

Thank you for the feedback. I will try how far I get with this approach.

Contributor

ratrun commented Feb 6, 2014

Thank you for the feedback. I will try how far I get with this approach.

ratrun added some commits Feb 13, 2014

Via routing moved into new Class ViaRouting. Instead of supporting 'v…
…ia' points in every Algorithm, now loop over the via points in GraphHopper.route for and create a new RoutingAlgorithm object for every pair of points.
Dynamic adding of via buttons. Now It is possible to use From, To or …
…From, To with a single via. Multiple "via" still do not work, altogh it is already possible to create multiple buttons.
Merge graphhoppervia/master
Conflicts:
	core/src/test/java/com/graphhopper/routing/GraphHopperIT.java
@ratrun

This comment has been minimized.

Show comment
Hide comment
@ratrun

ratrun Feb 19, 2014

Contributor

Now the intended via functionality seems to be working for me. Could you please check? What do you think?

Contributor

ratrun commented Feb 19, 2014

Now the intended via functionality seems to be working for me. Could you please check? What do you think?

@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Feb 20, 2014

Member

Thanks - I'll have a look!

Member

karussell commented Feb 20, 2014

Thanks - I'll have a look!

@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Mar 17, 2014

Member

Sorry for the delay. This works nicely :)!

Regarding the core I would like to have only one GHRequest class and the current functionality of GHViaRequest is merged into GHRequest. So instead of two points in GHRequest there will be a list of points.

The web UI currently does not properly work if you create new routes. E.g. insert a via point and then create a new route.

One nice thing would be to be able to drag the route at any point. I'm sure this is possible with leaflet. I'll look up how this could be possible.

I minor thing which I can also try to fix is: the '+' button should add a destination and the destination will be then converted to a via point.

Member

karussell commented Mar 17, 2014

Sorry for the delay. This works nicely :)!

Regarding the core I would like to have only one GHRequest class and the current functionality of GHViaRequest is merged into GHRequest. So instead of two points in GHRequest there will be a list of points.

The web UI currently does not properly work if you create new routes. E.g. insert a via point and then create a new route.

One nice thing would be to be able to drag the route at any point. I'm sure this is possible with leaflet. I'll look up how this could be possible.

I minor thing which I can also try to fix is: the '+' button should add a destination and the destination will be then converted to a via point.

@ratrun

This comment has been minimized.

Show comment
Hide comment
@ratrun

ratrun Mar 17, 2014

Contributor

Thanks for the feedback.
Regarding merging GHViaRequest into GHRequest: I didn't dare this because I thought that it breaks the API, which existing applications might be using. Anyhow if you say so, I can implement this change.

Regarding the web UI: The extensions were my first experiments in javascript and I was glad that it works at least somehow. If I understood you correctly then you intend to automatically add a flag on pressing the "+". But where do you want to draw the flag?

By the way, dragging of any point (From, To, Via) should be possible already.
Anyhow, please be prepared that you need to fix and improve the web UI yourself because I don't think I'm able to get this working 100%.

Contributor

ratrun commented Mar 17, 2014

Thanks for the feedback.
Regarding merging GHViaRequest into GHRequest: I didn't dare this because I thought that it breaks the API, which existing applications might be using. Anyhow if you say so, I can implement this change.

Regarding the web UI: The extensions were my first experiments in javascript and I was glad that it works at least somehow. If I understood you correctly then you intend to automatically add a flag on pressing the "+". But where do you want to draw the flag?

By the way, dragging of any point (From, To, Via) should be possible already.
Anyhow, please be prepared that you need to fix and improve the web UI yourself because I don't think I'm able to get this working 100%.

@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Mar 17, 2014

Member

Why do you think it will brake existing applications?

If I understood you correctly then you intend to automatically add a flag on
pressing the "+". But where do you want to draw the flag?

I meant dragging the line and creating a new via from this action.

By the way, dragging of any point (From, To, Via) should be possible already.

Yes, this works fine.

Anyhow, please be prepared that you need to fix and improve the web UI yourself because I don't think I'm able to get this working 100%.

ok, I hope to get the time to make this working

Member

karussell commented Mar 17, 2014

Why do you think it will brake existing applications?

If I understood you correctly then you intend to automatically add a flag on
pressing the "+". But where do you want to draw the flag?

I meant dragging the line and creating a new via from this action.

By the way, dragging of any point (From, To, Via) should be possible already.

Yes, this works fine.

Anyhow, please be prepared that you need to fix and improve the web UI yourself because I don't think I'm able to get this working 100%.

ok, I hope to get the time to make this working

ratrun added some commits Mar 19, 2014

Merge origin/master
Fix conflicts in:
	core/src/main/java/com/graphhopper/GraphHopper.java
	core/src/main/java/com/graphhopper/util/InstructionList.java
Fix conflicts in:
	core/src/main/java/com/graphhopper/GraphHopper.java
	core/src/main/java/com/graphhopper/util/InstructionList.java

ratrun added some commits Mar 19, 2014

Merge GHViaRequest into GHRequest. Note: This change might slow down …
…the calculation time a bit because of the aggregation with a for loop of the segmets in ViaRouting:getPoints and ViaRouting:calcInstruction.
Merge GHViaRequest into GHRequest. Note: This change might slow down …
…the calculation time a bit because of the aggregation with a for loop of the segmets in ViaRouting:getPoints and ViaRouting:calcInstruction.
@ratrun

This comment has been minimized.

Show comment
Hide comment
@ratrun

ratrun Mar 19, 2014

Contributor

Merge GHViaRequest into GHRequest is done now. Note: This change might slow down the calculation time a bit because of the aggregation with a for loop of the segments in ViaRouting:getPoints and ViaRouting:calcInstruction.

From my point of view now the implementation is finished. Web UI improvements can be made in a later step by somebody more familiar with the javascipt/leaflet possibilities.

Contributor

ratrun commented Mar 19, 2014

Merge GHViaRequest into GHRequest is done now. Note: This change might slow down the calculation time a bit because of the aggregation with a for loop of the segments in ViaRouting:getPoints and ViaRouting:calcInstruction.

From my point of view now the implementation is finished. Web UI improvements can be made in a later step by somebody more familiar with the javascipt/leaflet possibilities.

@karussell karussell added this to the 0.3 milestone Mar 31, 2014

@karussell karussell modified the milestones: 0.4, 0.3 Mar 31, 2014

@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Mar 31, 2014

Member

I've integrated your changes, thanks! The UI part is still TBD (in 0.4) so I leave this open ...

Member

karussell commented Mar 31, 2014

I've integrated your changes, thanks! The UI part is still TBD (in 0.4) so I leave this open ...

@karussell karussell removed this from the 0.4 milestone May 9, 2014

@karussell

This comment has been minimized.

Show comment
Hide comment
@karussell

karussell Jun 27, 2014

Member

The UI part will be fixed via #227

Member

karussell commented Jun 27, 2014

The UI part will be fixed via #227

@karussell karussell closed this Jun 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment