Rerouting support

[rinigus]

Hi!

I started working on rerouting support (addresses https://github.com/otsaloma/poor-maps/issues/26). This is still work in progress and I am going out for the field test. However, since I am rather new to Poor Maps internals, it would be great to know whether such approach is the expected one.

If it all works in the field as well, missing are:

• how to notify user on reroute calculation error?

• whether an option is needed to allow rerouting?

• distance for rerouting is 200m now, as you used before. Maybe should depend on transportation mode (car, foot, ...)?

Anything else missing?

[rinigus]

I have just returned from the field testing and, in my opinion, it works quite well. Please consider this for merging.

Issues that I encountered were https://github.com/otsaloma/poor-maps/issues/44 and https://github.com/otsaloma/poor-maps/issues/45 .

In addition, the error message is missing (don't know where to add it) as well as possibility to choose whether rerouting is allowed/desired.

Finally, each call for rerouting costs an API call and may quickly start costing money. So, maybe we should consider allowing rerouting only for selected routers.

[otsaloma]

Thanks, I'll review this sometime later. It looks like a good start, but we'll need a lot more details.

In the mean time, more field testing is likely to help. And if you haven't already, check for comparison the rerouting parameters in Modrana.

https://github.com/M4rtinK/modrana/blob/master/modules/mod_turnByTurn/mod_turnByTurn.py

[rinigus]

No problem, I'll continue then working on it by pushing changes into this branch.

I must say that while driving in a city and outside it, all seemed to be rather natural. Rerouting when you are 200 m away from the closest segment was fine and I would prefer not to limit number of reroutes. In practice, when you start deviating from the path, you always get a route that requires you to turn around (especially since we don't have heading info in router yet) and only after few re-calculations, a new route is proposed. So, by limiting the count as in modRana, you would be in trouble when you use rerouting in practice.

Few specific questions:

• do you want to have rerouting available for all routers or would there be a special flag allowing it?

• I am thinking to add rerouting info block (similar to NorthArrow and ScaleBar) that would tell when rerouting calculation started and when it failed. Otherwise it would be blank. I would suggest to position it above ScaleBar. Is that OK? Done in 056637745cb4888600e133164fc81158fc00dd4b

• Do you want user to have an option on whether re-routing is automatically enabled or do we proceed with the logic that this is expected feature anyway and there is no need to clutter with too many options? I think that automatic selection of re-routing as it is now is better and there is no need for extra option.

• I would work on #44 . Done in 32e9a53600dc4506caca9a12b07cfd4a8297fe45

• I would work on #45. For that, would introduce a new property, maybe app.navigationCanStart. Is that OK? This would allow to decouple re-routing from Begin/Pause logic. Done in a25b626ce880d2e68e7239a246c4795d4e1e139d

[rinigus]

This PR is becoming rather large, but its hard to split the commits when they depend on each other. I hope its OK. I will try to field test it today again with all the patches.

[rinigus]

Coming back from testing: heading direction improved OSM Scout Server/Valhalla response a lot (I presume that the same for Mapzen). Now, when you pass the turn, after 200+precision, you will get the route that takes into account your direction as well. In practice, worked very well and I haven't encountered issues during this run.

[otsaloma]

Sorry for the delay, I should have time to look at this a week from now, around 4–6th August.

[rinigus]

No problem, I will try to fix excessive rotation of the map during rerouting meanwhile. I think I know what's a problem and how to fix it as well.

[rinigus]

The last commit (672bb742adb6a6180d2fad7ce5173855f8db3d82) moves most of the logic of applying new route from app to map object. As a result of using separate reRoute method in map, the previous route is replaced without intermediate panning out and rotation of the map. It feels better and map is not disturbed as much as before.

Edit: note that this code has been field tested as well.

[otsaloma]

I have looked through this now. The only bigger issue I see is that can-start (#45), once we sort that out, the rest is small tweaks and details and I'll likely do that myself as it faster to just do it than try to explain it. Comments below for a couple of the open questions.

how to notify user on reroute calculation error?

It should be fine to reuse the RouterInfo.qml thing for all messages. In fact, I might make that a general notification component.

whether an option is needed to allow rerouting?

Yes, we need an option poor.conf.reroute, usually defaulting to true, available as a switch in the NarrativePage.qml. We'll also later have a similar switch or something for voice announcements. But don't add the switch, that narrative page is getting a bit full, I'll need to do some kind of a redesign.

do you want to have rerouting available for all routers or would there be a special flag allowing it?

I think it's OK for now to be allowed for all routers. A flag can be added later if it turns out some router requires it.

distance for rerouting is 200m now, as you used before

I think this is fine, it should work for car and bicycle. When walking, one can easily reroute manually if needed. I'll likely add a manual reroute button to the narrative page, but that too requires a redesign of the page.

[rinigus]

Great! As for start/stop, I think we can do

• Start/Pause & Stop for all modes, as you suggested.

It all sounds like a great plan! I will focus then on the voice announcements and will set poor.conf.voice to true without any GUI for it assuming that there will be new slot for such option.

With the making a difference for rerouting between the modes (car, bicycle VS walk, public transport): it should be somehow clear with this new design that the rerouting is different among different modes. Although, note that you may want to have an ability to enable auto-rerouting while jogging/running. It would still be "walking" in terms of the software, but, as a user, wouldn't want to stop for fiddling with the phone while running.

[rinigus]

I have left a comment regarding one change. All looking good :)

[otsaloma]

I did some changes. In my desktop testing everything seemed to work well. In addition to further testing, RouterInfo and NarrativePage UI are left.

Check especially https://github.com/otsaloma/poor-maps/pull/43/commits/0327c9148f9609a3f4660316a57031615ea2a433, I introduced some conditions to avoid rerouting too often. I'm worried especially about constantly trying to reroute despite it giving errors and also something like driving on roads not included in the map used by the router, thus giving the route starting point over 200 meters away, triggering another reroute and so on.

And, by the way, this is what used for testing: https://gist.github.com/otsaloma/4f19455369ddd4ad294f93a1b6c812be

[rinigus]

First of all, hats off. Very concise style and still nicely readable!

I agree with all the changes, with the exception of rather conservative rerouting time interval. If I get the units right (time in ms for Date.now()), we have 5 minutes for minimal time interval between rerouting calls. When you are on the road and had to take a turn due to some road works, you would like to get the new route ASAP. 200 meters seemed like a reasonable compromise. In that case, 5 minutes makes rerouting basically irrelevant and forces you to stop the car, fiddle with the program, and continue.

I would suggest to drop the line

interval = Math.min(interval, 300000);

in rerouteMaybe and make the route calculation fail or consider failed if you are in the forest (as you specify in your scenario).

For forest case, there is a radius option in Valhalla. Unfortunately, they will ignore it if there is no road specified in a given location, but when the router will find a node that is "within reason" from the given location. What we can do, is to check whether the starting location is within 200 meters or so from the location that we asked for as a starting point for rerouting (note that we do move during the calculations, probably even more when using online service). Having such optional check as a part of poor.app.router.route optional argument would help us to catch the forest case and penalize it via rerouteConsecutiveErrors.

What do you think about it?

[rinigus]

Sorry, I read your code wrong. You had min and I read max. My mistake, don't remove that line. maybe we should set max to a minute though. Again, 5 min is a bit too much.

[otsaloma]

Again, 5 min is a bit too much.

I dropped it now to one minute. And that's after four failed reroutes.

The outside road network case is just an example, there are probably many other cases too. At this point I just want to avoid an eternal loop and later we can tweak any cases that people actually run into.

Any opinion on the total reroute limit of 50?

[rinigus]

Yes, that limit is maybe also problematic. I was thinking about it when I saw it first, but then got carried away by the time interval.

I don't think we actually need this 50 calls limit. I can imagine that its possible to hit it when you drive a longer stretch and stop for gas/coffee/break several times. Could happen to me during such trips, easy (can't always stop navigation software and have to take these rerouting messages until you get to the station).

I would suggest to release without the limit (or impose it only for online routers) and see if it is fine. The only reason for this limit that I can think of is related to costs of API calls. If the number of calls will be too large, we can either limit them for online routers or figure out how users can pay for it.

Having such a hidden limit may cause some frustration with the software just stopping to work without any obvious reason.

[otsaloma]

The only reason for this limit that I can think of is related to costs of API calls.

Yes, that's exactly what I'm worried about. Let's say someone drives for an hour on a motorway, without being able fiddle with the navigator, at say 5 second intervals that could mean 720 API calls (in some special case where the router doesn't return a good results).

Maybe I'll add an offline/online flag to the router JSON files and make a distinction based on that.

[rinigus]

It would make sense to add offline/online or api_calls/free flag in JSON. Then we can ensure that its not going to boil over due to some unfortunate case.

[otsaloma]

I made the rerouting notification more noticeable and also so that it notifies for successes as well. At least with online routers, the result comes usually such fast that an in-progress only notification is just an unreadable quick flash.

[rinigus]

Looks good. Indeed, the previous version had it as a rather fast flash for the routers I tried (for offline valhalla, speed depends on the length of the road and takes few seconds for the route across Sweden). But nothing major and working quite nicely when testing with your positioning class from gist.

[otsaloma]

Narrative page redesign done now, the page has been split in two. There should be nicely room for any voice announcement options as well. I think all pieces are in place now, and if you agree, I'm ready to merge this. I'll still do testing and possible small tweaks, but I can do that in master along with release preparations.

[rinigus]

It looks good! And after thinking about it for some time - I think its a right way to position maneuvers on the second page. I use maneuvers for testing, but, in real life, I glance through the found route, may want to enable/disable something (rerouting and voice) and start going.

I think its ready for merging. And merging would simplify making the next PR :)