osmandapp / OsmAnd

OsmAnd
https://osmand.net
Other
4.64k stars 1.01k forks source link

Increasing default speed causes turn announcements to play too early #12636

Closed Zero3 closed 2 years ago

Zero3 commented 3 years ago

Description

Changing the default speed from 45 km/h to 80 km/h (which is the legal default in Denmark) causes the "turn left/right now" voice announcements (also known as "passing" announcements, I believe?) to play way too early.

With the default setting of 45 km/h, the announcement will be made ~50 meter before a turn, which is reasonable. With a setting of 80 km/h, the announcement will be made several hundred meters before the turn, which is confusing in cities where it then often directs you to turn down the wrong road.

Setting the announcement timing setting to "at the last meters" does not fix this. The screen containing that setting also displays "20 - 25 m" as the "passing" distance, which seems very incorrect when the actual announcements are made several hundred meters before. Something weird is going on with the math here.

I realize that this is partly by design (as far as I understand https://docs.osmand.net/en/main@latest/development/algorithms/voice-prompt-triggering). I believe this design is flawed. Changing the default speed for unknown roads should not change how early voice announcements are made in intersections (for example).

Your Environment

OsmAnd Version: 4.0.5

vshcherb commented 3 years ago

Default speed has nothing to do with unknown roads cause for car / bicycle navigation all roads are known. So indeed it's by design and there is extra setting to compensate it. BTW speed is taken from current speed and not default (https://docs.osmand.net/en/main@latest/development/algorithms/voice-prompt-triggering)

Zero3 commented 3 years ago

@vshcherb I believe there might be something lost in translation, or maybe some kind of misunderstandings going on here. I will try to reply to your points:

Default speed has nothing to do with unknown roads...

The description text at the top of the modal with the setting literally says "Predict arrival time for unknown road types ...", at least in the Danish translation. So if the setting is not used for that, what is it for then? What is the purpose of the default 45 km/h (for car routing)?

... cause for car / bicycle navigation all roads are known.

There exists roads of unknown type in the OSM data, for example tagged as highway=road. Maybe you are talking about the existence of roads, where I am talking about the type of existent roads? I am a bit confused here.

sonora commented 3 years ago

In my opinion 'default speed' is a typical example of a parameter needed for an algorithm, but should never be exposed to the user. I can only advise to leave this untouched, unless you really love to experiment, then also trying to compensate via Announcement time compensation.

In my experience from user feedback, many got frustrated playing with this, ultimately anyway not fiinding any better solutions than the default.

Zero3 commented 3 years ago

@sonora thanks for the additional information.

If I understand what you and @vshcherb write correctly, you say that this parameter is only used for timing voice announcements, and nothing else. Is that correct? The thing is, there are two places that say the opposite, which is why I am a bit confused:

1) The GUI, which says "Predict arrival time for unknown road types ..." 2) The Navigation Voice Prompt Triggering docs, which says "Note: The Default speed also affects the calculated route time"

So which is right? Is the parameter only for timing voice announcements, or also for routing and arrival time estimation for unknown road types?

@vshcherb I noticed that you closed this issue, but I think it is still relevant. Depending on what you think about the question above, either the bug is still present as described, or there is a bug in the way it is presented in the GUI, since it is presented such that the user will believe he should put in the legal default speed, which will mess up voice announcements. Would you mind reopening? :)

vshcherb commented 3 years ago

I closed because it's considered to be a desired behavior this is how we end up to have voice router after spending 100 times configuring for different vehicles with different base speed. There is 1 more setting to compensate it and it varies from 0.1 to 2 (announcement time) it should be enough to compensate default speed

Zero3 commented 3 years ago

@vshcherb thank you for continuing the discussion.

I think parts of the discussion are getting lost in translation, as I did not get answers or follow-ups to most of my questions and points. But to jump to the conclusion you just provided:

I closed because it's considered to be a desired behavior

If the "Default speed" setting is indeed supposed to be left at 45 km/h for voice navigation to work correctly, I humbly suggest that this setting is removed from the GUI. The way it is presented at the moment encourages users to set it to a bad value.

vshcherb commented 3 years ago

Well, I'm not jumping to the conclusion too quickly, I'm just saying I don't see enough arguments why should it be changed. Again for driving navigation is the least to have considered for voice navigation, mostly "current speed of driving" is used and "announcement distance setting" (far or close up).

No we shouldn't delete "default speed" neither from settings nor from formulas cause it could be used for boat / pedestrian navigation

vshcherb commented 3 years ago

I double checked how it's implemented - https://github.com/osmandapp/OsmAnd/blob/e7cae5fc14233b94dcd3f6e225137789636342ee/OsmAnd/src/net/osmand/plus/routing/data/AnnounceTimeDistances.java#L54.

Default speed is used and set to minimum of 40 kmh, default speed is 45 kmh, indeed setting average speed to 90 kmh will increase by factor of 2, which could be compensated with Late arrival setting for some announcements. It could be changed if this trigger is skipped for some situations or turns (https://github.com/osmandapp/OsmAnd/blob/e7cae5fc14233b94dcd3f6e225137789636342ee/OsmAnd/src/net/osmand/plus/routing/data/AnnounceTimeDistances.java#L180).

I will reopen to get feedback from @sonora whether we should / could change it or not

sonora commented 3 years ago

@vshcherb Trying to recall our discussion when we re-did this in 2019 or so, I think my vision was clearly this:

Let us offer to the user only 2 simple scaling factors in the following fashion:

We would simply additionally scale everything (i.e. all prompts, and all estimated segment times) by a simple multiplication with these user-defined factors. We would hide in our UI, but keep in our code (because it works), any mentioning of the "default speed", because it is misleading. I personally would also like to then hide "Early arrival announcement", a singular odd adjustment we have now.

Users could over time give us feedback, e.g. in a survey, which settings work best for them for different means of transport, so we could maybe adjust our defaults over time... and gather experience what people use with profiles we are not too familiar with, like boat, truck, or motorbike.

Lee-Carre commented 3 years ago

Re if the default speed variable is exposed in the UI.

I'll preface by admitting to not having read the code.

Maybe make better use of the value (as user-configurable). How? Well, I live somewhere for which the ‘national’ (read: island) speed limit is 40 mph. Having experience with traditional (dedicated-unit) GPS receivers, the average (moving) speed is only around 20 mph (many winding roads, congestion zones, traffic_calming, stopping & starting, pedestrian crossings, and grockles in rental cars crawling along (well below maxspeed) because they're unaccustomed to narrow roads, among other road-entertainment).

So, perhaps continue to allow users to change this value, so that OsmAnd can be quickly adapted to varying jurisdictions & conditions.

The default value could also be used to better estimate journey duration when in navigation mode, for roads lacking maxspeed, surface, smoothness, and other relevant info needed for accurate ETE & ETA times.

Better still; internally auto-calibrate / auto-adjust based on measured moving speed per jurisdiction.

There obviously must've been some good reason to expose it in the UI, originally.

sonora commented 3 years ago

Well, it's misleading: The "defsult speed" is not your average trip speed. And it enters calculations different from what its name may superficially imply.

vshcherb commented 3 years ago

@sonora I came back to analysis and history and think that propose not to change the strategy.

Default speed (= expected speed) defines first of all class of the vehicle i.e. if it's a buggy with top 40 kmh or slow airplane with top 200 kmh. The range is vehicle is enormous and in order all algorithms working properly, we need to make function continuous rather than discrete and also easily to understand and handle.

So coming back to announcements and algorithms ( I think they are correct now and problem is not there). If we take a cyclist with average speed 8 m/s (in settings) and current speed is only 2 m/s, the voice announcement calculates it's needed just 7 seconds to announce but the current speed estimates 14 m where in reality cyclist could speed up and go back to normal speed with 8 m/s, so the minimal distance to announce = max ( 14m , 56 m) = 56 m.

In that case there is a compensation for speeding (current speed) and compensation for going slower than expected. The reality with people putting unexpected high speed to default speed leads to early announcement. Because if you put a car default speed (= expected speed) to 120 kmh it becomes impossible to go with that speed in the city. So theoretically car can go with 120 kmh but can't go in the specified conditions. Though if the car would be able to speed up from 40 kmh (current speed) to 120 kmh on the highway then early announcement is exactly correct behavior.

Summary next comment.

vshcherb commented 3 years ago

Summary.

There are 2 correct compensation strategies to not overcomplicate settings and not to move to direction that user needs to program himself.

We should stick to default speed as expected average speed for navigation.

In that case we have an issue for segments where it's not nearly possible to reach that speed. Let's articulate that critical point is between factor 1.5-2. So if you expected speed > 1.5 * (maximum possible speed now) - we have a problem of early announcements. So we could take this in account and calculate correctly in Voice Router.

Proposed solutions to detect - (maximum possible speed now):

  1. Take it from Routing as maxspeed from the route itself Pros: exactly correct method Cons: doesn't work for any profile except car-based Cons: doesn't work for GPX
  2. Take it from Routing as speed / time from the route itself Pros: often correct method / works for any profile + GPX Cons: doesn't work precise for any profile except car-based and cycle-based (cause it takes in the end default speed to calculate
  3. Take maximum possible speed now = as current average speed for last 5 seconds * 1.5 Pros: works in any situation for any profile Pros: looks simple to implement Cons: could be some caveats to implement Cons: speed might be errorenous or 0 reported, that's why exactly we have default speed comparision introduce in first place.
Zero3 commented 3 years ago

@vshcherb @sonora Interesting discussion and points!

I clearly do not know about all the cases OsmAnd supports, but I am a bit curious about why a "default speed" setting is necessary in the first place. At first sight, it does not seem like a reasonable setting to have, because the correct value will depend on the situation. For example, for cars it will depend on stuff like whether you are driving in urban or rural areas, and whether there are traffic congestion or not. So no matter what you set it at, it will often be set to something logically wrong.

Maybe the announcement timing calculation could be simplified by removing this concept altogether, and just base the announcement timing on actual speed? Maybe there is a good reason to keep it, but I do not see it (with my very limited knowledge). The only arguments I read for keeping it above are these:

No we shouldn't delete "default speed" neither from settings nor from formulas cause it could be used for boat / pedestrian navigation

Maybe it could be explained why it is needed in those cases? Why not just use actual speed?

[...] cyclist could speed up and go back to normal speed with 8 m/s, so the minimal distance to announce = max ( 14m , 56 m) = 56 m.

I believe this case would already be covered by using just the actual speed in the calculations. The distance until announcement would be recalculated regularly anyway, right? So the new higher speed will be used in the next recalculation, and thus increase the announcement distance. So it should work without the separate "minimum distance based on default speed".

sonora commented 3 years ago

@vshcherb Regarding my above proposal of eliminating what we call "default speed" from the UI: Yes, I meant both adjustment factors selectable on a slider in a continuos fashion e.g. 0.25-2.0. (The text labels are only meant illustratively.)

Regarding your suggestions: All have major caveats, so my gut feeling is rather hesitant to like any of them too much. ;) We would have to go through lots of testing in various different situations, like e.g. devices not reporting speed correctly, momentary loss of GPS, stop and go traffic, and so forth.

We both agree that the current implementation is not fundamentally bad, and the return of invest on a major overhaul is uncertain.

If we want to keep a speed setting in the UI for users to find, we could consider a simple wording change to call it Typical vehicle speed and explain that it should be set to your most valid average use case, which would provide something like a base setting for navigation instruction timing, with subsequent further fine-tuning avalable via the "Announcement time** setting.

That would at least make the UI more in line with what is actually meant?

vshcherb commented 3 years ago

@sonora well the issue is not that we don't provide 1 voice routing slider but it's configured as 2 parts: distance and time.

Obviously slider Announcement time works very tricky.

We should avoid tricky configuration of each turn announcement and even 2 sliders cause nobody will have correct understanding how should it happen.

"Default speed" -> "Expected average speed" gives a lot of reasonable information but it's not enough, so I would still insist that it should be used + 1 more setting and not more. In back of my mind I believe this 1 more setting should be enough.

@Zero3 Removing "Minimum distance to announce" sounds weird and doesn't work correct in traffic jams. Once you start getting normal speed back you won't have enough time to understand and catch coming maneuvre.

Zero3 commented 3 years ago

@vshcherb a "Minimum distance to announce" for each kind of announcement sounds like a good fallback for traffic jam situations. My suggestion was not necessarily to remove this part, but rather to remove the "Default speed" variable.

This could be done by just hiding it in the UI, as @sonora suggests, or even better, replace it with a lookup table in routing.xml, so that values can be individually configured for each profile. For example, for the car profile:

Announcement Minimum time (seconds) Minimum distance (meters)
Turn now 7 45
Turn in X m 22 275
Prepare to turn in X m 1438
... ... ...

Some advantages to this approach:

vshcherb commented 2 years ago

The issue has quite long conversation and didn't lead to any conclusion as it looks

Zero3 commented 1 year ago

@vshcherb I'm sad to see this issue closed, since the problem was not solved. It seems like a waste of time contributing with issue reports if they are closed just because no agreement on the right solution are found immediately :confused: