Implements distance / speed limit voice announcement and displaying for speed cameras (#3721 and #8108, android part) + minor fix for alert icon resizing on large screens #12923

AlexNcUa · 2021-10-04T01:05:40Z

There is attempt to implement issues #3721 (except traffic light camera) and #8108.

osmandapp/OsmAnd-resources#724 is prerequisite for this PR.

Tested in simulation mode for:

Speed camera with assigned max speed same as route segment max speed
Speed camera with assigned max speed different from the route segment max speed (speed camera max speed expected to be displayed / announced)
Speed camera without assigned max speed (route max speed expected to be displayed / announced)

Changes description:

RouteCalculationResult attachAlarmInfo method extended to add max speed to speed camera AlarmInfo taken either from speed camera point (preferred) or from RouteSegmentResult.
WaypointHelper getMostImportantAlarm and announceVisibleLocations updated to attach actual distance to speed camera AlarmInfo.
AlarmWidget extended with ability to show max speed for speed camera when available.
VoiceRouter.announceAlarm extended with special handling for speed camera announcement to announce distance and max speed when available.
CommandBuilder / JsCommandBuilder extended with new speedCameraAlarm method, which allows announce speed camera with distance and max speed.
TestVoiceActivity extended to test speed camera with distance and max speed announcement.

- RouteCalculationResult attachAlarmInfo method extended to add max speed to speed camera AlarmInfo taken either from speed camera point (preferred) or from RouteSegmentResult. - WaypointHelper getMostImportantAlarm and announceVisibleLocations updated to attach actual distance to speed camera AlarmInfo. - AlarmWidget extended with ability to show max speed for speed camera when available. - VoiceRouter.announceAlarm extended with special handling for speed camera announcement to announce distance and max speed when available. - CommandBuilder / JsCommandBuilder extended with new speedCameraAlarm method, which allows announce speed camera with distance and max speed. - TestVoiceActivity extended to test speed camera with distance and max speed announcement.

- Rework icons to be more consistent with american signs (separated icons added for american region) and less confusing with regular speed limit. - Distance to camera visualization added. - Additionally fix alarm icon re-sizing on large screens (related not only to speed cameras, icon re-sizing was missed, as result bottom text was out of icon area)

AlexNcUa · 2021-10-29T22:55:16Z

PR was updated:

Design of icons was changed to be less confusing with regular "current" speed limit alert, and to be more consistent with american signs (when US region selected).
Visual distance indication added.
Additionally fixed issue (not directly related to speed cameras) with resizing alert icon on bigger screens (tablets), which was leading to bottom text out of icon area (for example for tunnels).

New screenshots attached.

vshcherb · 2021-11-01T17:40:22Z

OsmAnd/src/net/osmand/plus/voice/CommandBuilder.java

@@ -30,6 +30,7 @@
 	protected static final String C_REACHED_POI = "reached_poi";
 	protected static final String C_THEN = "then";
 	protected static final String C_SPEAD_ALARM = "speed_alarm";
+	protected static final String C_SPEED_CAMERA_ALARM = "speed_camera_alarm";


This approach will require to change all voice configuration that could be too much. So incremental approach or check could be implemented probably.

I have added corresponding fallback in the voice configuration files as part of the related PR https://github.com/osmandapp/OsmAnd-resources/pull/724/files

For all languages that contains
dictionary["distance"] + (tts ? " - " : " ") + distance(dist) as part of the function route_recalc(dist, seconds) i have added implementation, once this part of the speed camera announcement will be the same. As well as the part of announcement with the speed limit will be the same as corresponding announcement from "regular" speed limit alarm. For all other languages I have added fallback to original announcement without speed / distance.

Only one thing that I am not sure, it is how to force voice configuration files update before (or during) latest version installation. It seems that the voice files downloaded separately from the application itself. Probably you can suggest how to check if the latest voice files are available on device (or if voice file contains corresponding function) from the CommandBuilder / JsCommandBuilder, I have not found something similar....

Voice files are updated automatically checked each android version update (probably you have same android version number).

We've already implemented before backward compatible voice changes, so old voice packages could continue to work with isJsCommandExists(C_TAKE_EXIT)

@vshcherb thank you for refer the isJsCommandExists, have missed it.

So, you recommend to add fallback in Java code even if the voice files will be updated as part of another already prepared PR?

The fallback also will require passing additional argument String fallbackAttentionType to the speedCameraAlarm method, to avoid dependency between JsCommandBuilder and AlarmInfoType (the attentionType should be String.valueOf(AlarmInfoType.SPEED_CAMERA)) or hardcoding "SPEED_CAMERA" value in the speedCameraAlarm method, which itself could be slightly "dirty".

You can never say if somebody doesn't have custom tts package. Secondly, good practice is to update only packages / languages that you are more or less sure. So in that case to not add commands that don't support it.

Indeed you need to pass parameter that make sense only for speed camera, so probably making a new method make sense but it should be checked anyway that this method exists and fallback should be called.

…ap fix.

vshcherb · 2021-12-20T16:53:53Z

OsmAnd/src/net/osmand/plus/views/mapwidgets/widgets/AlarmWidget.java

@@ -138,29 +143,60 @@ public boolean updateInfo(DrawSettings drawSettings, boolean drawBitmap) {
 							icon.setImageResource(info.locImgId);
 						}
 					}
-					if (!Algorithms.objectEquals(info.text, cachedText) || cachedRegion != info.region) {
+
+					boolean isRegionChanged = cachedRegion != info.region;


We follow previous code style cause it's shorter version and simpler.

@vshcherb Kindly note, that it is not only code style related.

Most likely the initial implementation has an issue with if statement condition in L#193 (previous L#156):

if (!Algorithms.objectEquals(info.bottomText, cachedBottomText) || cachedRegion != info.region)

because the cachedRegion could be already updated in L#144 (previous numbering) above, so the second if statement will not detect the region change.

The codestyle and technique we use is very simple.
1 - if statement that checks all cached variable
2- in case any state is changed the whole widget / icon is rerendered / recalculated, so pseudocode is always like

if( new_state_A != cached_state_A || new_state_B != cached_state_B || ... ) { cached_state_A = new_state_A; cached_state_B = new_state_B; .... <CODE using new_state_A, new_state_B, ... > }

vshcherb · 2021-12-20T17:06:46Z

OsmAnd/src/net/osmand/plus/routing/RouteCalculationResult.java

+				}
+
+				if (maxSpeed > 0 && maxSpeed != RouteDataObject.NONE_MAX_SPEED) {
+					if (sc.imperial) {


Looks strange that here we store converted data, I guess it should happen at the end UI / Voice prompt cause here it's to early to do conversions

@vshcherb To be honest, I also had strong doubts when writing this code.

On the right hand it is not the best place for conversion, on the left hand the AlarmInfo.intValue also used for speed limit alarm, and in that case the field will contain converted value.

It could be very confusing if the same field will contain speed value in different units of measurement.

Probably, it would be better to introduce dedicated explicit field in AlarmInfo for meters per second speed and provide getter method to provide converted speed. But it will require re-factoring in multiple places, and also I guess there was signifact reason to use several common fields like "floatValue", "intValue" etc. instead of "maxSpeedInMetersPerSecond", "distance" etc...

vshcherb · 2021-12-20T17:11:26Z

OsmAnd/src/net/osmand/plus/views/mapwidgets/widgets/AlarmWidget.java

 						}
 					}
-					if (!Algorithms.objectEquals(info.bottomText, cachedBottomText) || cachedRegion != info.region) {
+					if (!Algorithms.objectEquals(info.bottomText, cachedBottomText) || isRegionChanged || isAlarmTypeChanged) {


If we talk about this code, then it's simple 2 blocks should be combined into 1 if, otherwise the pattern is broken and it's hard to maintain.

Absolutely agree that single if will be simpler for understanding, will combine it. My initial intention was to reduce number of changes in PR.

vshcherb · 2021-12-20T17:12:03Z

OsmAnd/src/net/osmand/plus/views/mapwidgets/widgets/AlarmWidget.java

 								int topPadding = res.getDimensionPixelSize(R.dimen.map_alarm_text_top_padding);
 								widgetText.setPadding(0, topPadding, 0, 0);
 							} else {
 								widgetText.setPadding(0, 0, 0, 0);
 							}
+


this is already quite big chunk of code so, it makes sense I think to move if block inside to a separate method

- Add fallback for C_SPEED_CAMERA_ALARM in JsCommandBuilder - Combine checks for text change / bottom text change in the AlarmWidget.updateInfo into one conditional statement and extract updateTextWidget / updateBottomTextWidget methods.

- Refactor AlarmInfo to always accept maxSpeed in meters per second, and provide it either in meters per second or in accordance with requested speed constants settings.

AlexNcUa · 2021-12-21T00:52:46Z

@vshcherb I've updated implementation, and have changed AlarmInfo to always keep speed in meters per seconds to avoid early conversion and to provide it according to requested speed constants settings. Could you please check if it is better from the general conception prospective? Also not sure about naming conventions, please correct me if I am wrong somewhere.

vshcherb · 2022-05-24T10:49:34Z

As we planned it for release 4.2, we've decided to postpone it till 4.3 and merge with #13413.

Main ideas behind it:

We probably want to switch pictograms and make speed (exceeding speed) main pictogram and add speedcam pictogram as additional (3rd pictogram to Max speed / Current speed)
To simplify view we probably don't need distance to speed camera cause it won't be readable during driving. We use for tunnels to mimic real traffic sign.

We plan soon to finalize UI but this issue likely won't be merged in 4.2 release.

alensiljak · 2023-08-07T10:44:43Z

Hi, the current version is 4.5.9. Is this feature still on the roadmap?
I find the current speed camera announcements to come too late. Most of the time when camera is right next to the car or after. And this is at an area with the speed limit of 40km/h. This is a problem in countries/areas for which no speed limit exists in OSM (but there is a camera POI).

vshcherb added this to the future-android milestone Oct 10, 2021

AlexNcUa changed the title ~~Attempt to implement #3721 and #8108 (android part)~~ Implements distance / speed limit voice announcement and displaying for speed cameras (#3721 and #8108, android part) + minor fix for alert icon resizing on large screens Oct 29, 2021

vshcherb reviewed Nov 1, 2021

View reviewed changes

vshcherb modified the milestones: future-android, 4.2-android Nov 1, 2021

AlexNcUa added 2 commits December 17, 2021 14:26

Merge branch 'master' into 8108_3721_initial_impl

6327ea5

Fix build after master backmerge + minor AlarmWidget createWidgetBitm…

b829487

…ap fix.

vshcherb reviewed Dec 20, 2021

View reviewed changes

AlexNcUa added 3 commits December 20, 2021 22:11

Merge branch 'master' into 8108_3721_initial_impl

09688ec

osmandapp#8108, osmandapp#3721 Code review fixes part 2:

c70a074

- Refactor AlarmInfo to always accept maxSpeed in meters per second, and provide it either in meters per second or in accordance with requested speed constants settings.

vshcherb modified the milestones: 4.2-android, 4.3-android May 24, 2022

vshcherb modified the milestones: 4.3-android, 4.4-android Sep 29, 2022

vshcherb mentioned this pull request Aug 7, 2023

Speed Limit Icon in Right Panel #12133

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements distance / speed limit voice announcement and displaying for speed cameras (#3721 and #8108, android part) + minor fix for alert icon resizing on large screens #12923

Implements distance / speed limit voice announcement and displaying for speed cameras (#3721 and #8108, android part) + minor fix for alert icon resizing on large screens #12923

AlexNcUa commented Oct 4, 2021 •

edited

Loading

AlexNcUa commented Oct 29, 2021

vshcherb Nov 1, 2021

AlexNcUa Nov 1, 2021 •

edited

Loading

AlexNcUa Nov 1, 2021

vshcherb Dec 20, 2021

vshcherb Dec 20, 2021

AlexNcUa Dec 20, 2021 •

edited

Loading

vshcherb Dec 21, 2021

vshcherb Dec 20, 2021

AlexNcUa Dec 20, 2021 •

edited

Loading

vshcherb Dec 20, 2021

vshcherb Dec 20, 2021

AlexNcUa Dec 20, 2021

vshcherb Dec 20, 2021

AlexNcUa Dec 20, 2021

vshcherb Dec 20, 2021

AlexNcUa commented Dec 21, 2021

vshcherb commented May 24, 2022

alensiljak commented Aug 7, 2023

Implements distance / speed limit voice announcement and displaying for speed cameras (#3721 and #8108, android part) + minor fix for alert icon resizing on large screens #12923

Are you sure you want to change the base?

Implements distance / speed limit voice announcement and displaying for speed cameras (#3721 and #8108, android part) + minor fix for alert icon resizing on large screens #12923

Conversation

AlexNcUa commented Oct 4, 2021 • edited Loading

AlexNcUa commented Oct 29, 2021

Choose a reason for hiding this comment

AlexNcUa Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexNcUa Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexNcUa Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexNcUa commented Dec 21, 2021

vshcherb commented May 24, 2022

alensiljak commented Aug 7, 2023

AlexNcUa commented Oct 4, 2021 •

edited

Loading

AlexNcUa Nov 1, 2021 •

edited

Loading

AlexNcUa Dec 20, 2021 •

edited

Loading

AlexNcUa Dec 20, 2021 •

edited

Loading