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.

Sign up for GitHub

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

Jump to bottom

[android] Add Speed class #7955

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[android] Add Speed class #7955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions android/app/src/main/cpp/app/organicmaps/Framework.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1936,4 +1936,10 @@ Java_app_organicmaps_Framework_nativeGetKayakHotelLink(JNIEnv * env, jclass, jst
return url.empty() ? nullptr : jni::ToJavaString(env, url);
}

JNIEXPORT jint JNICALL
Java_app_organicmaps_Framework_nativeGetUnits(JNIEnv *, jclass)
{
return static_cast<jint>(measurement_utils::GetMeasurementUnits());
}

} // extern "C"
6 changes: 6 additions & 0 deletions android/app/src/main/java/app/organicmaps/Framework.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ public class Framework
public static final int ROUTER_TYPE_TRANSIT = 3;
public static final int ROUTER_TYPE_RULER = 4;

// Units values shall be the same as the ones defined in c++ in <platform/measurement_utils.hpp>.
public static final int UNITS_METRIC = 0;
public static final int UNITS_IMPERIAL = 1;

@Retention(RetentionPolicy.SOURCE)
@IntDef({ROUTE_REBUILD_AFTER_POINTS_LOADING})
public @interface RouteRecommendationType {}
Expand Down Expand Up @@ -455,4 +459,6 @@ public static native void nativeSetChoosePositionMode(@ChoosePositionMode int mo
@Nullable
public static native String nativeGetKayakHotelLink(@NonNull String countryIsoCode, @NonNull String uri,
long firstDaySec, long lastDaySec, boolean isReferral);

public static native int nativeGetUnits();
}
2 changes: 1 addition & 1 deletion android/app/src/main/java/app/organicmaps/routing/NavigationController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public void update(@Nullable RoutingInfo info)
updateVehicle(info);

updateStreetView(info);
mNavMenu.update(info);
mNavMenu.update(info, Framework.nativeGetUnits());
}

private void updateStreetView(@NonNull RoutingInfo info)
Expand Down
138 changes: 138 additions & 0 deletions android/app/src/main/java/app/organicmaps/util/Speed.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
package app.organicmaps.util;

import android.content.Context;
import android.util.Pair;

import java.text.DecimalFormat;
import java.text.DecimalFormatSymbols;
import java.util.Locale;

import app.organicmaps.Framework;
import app.organicmaps.util.log.Logger;

public class Speed
{
private static String mUnitStringKmh = "km/h";
private static String mUnitStringMiph = "mph";

private static char mDecimalSeparator = Character.MIN_VALUE;

public static double MpsToKmph(double mps) { return mps * 3.6; }
public static double MpsToMiph(double mps) { return mps * 2.236936; }

public static void setUnitStringKmh(String unitStringKmh) { mUnitStringKmh = unitStringKmh; }
public static void setUnitStringMiph(String unitStringMiph) { mUnitStringMiph = unitStringMiph; }

private final static DecimalFormat mDecimalFormatNoDecimal = new DecimalFormat("#");
private final static DecimalFormat mDecimalFormatOneDecimal = new DecimalFormat("#.#");

public static Pair<String, String> formatMeasurements(double speedInMetersPerSecond, int units,
Context context)
{
double speedValue;
String unitsString;

if (units == Framework.UNITS_IMPERIAL)
{
speedValue = MpsToMiph(speedInMetersPerSecond);
unitsString = mUnitStringMiph;
}
else
{
speedValue = MpsToKmph(speedInMetersPerSecond);
unitsString = mUnitStringKmh;
}

long start1 = System.nanoTime();
String formatString = (speedValue < 10.0)? "%.1f" : "%.0f";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this string be pre-cached?

String speedString = String.format(Locale.getDefault(), formatString, speedValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does caching the locale help to speed up the method?

Are there other alternatives to String.format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other alternatives to String.format?

I've made some timing comparison with 2 other alternatives to String.format().

You can find the code for this comparison in the function formatMeasurements().

  • Option 1: String.format(Locale, double)
  • Option 2: DecimalFormat("#.#").format(double)
  • Option 3: Long.toString(Math.round(speedValue * 10.0)) + StringBuffer(String).insert(1, char) (inserting the decimal separator into the string)

These are the average times for each option:

  • Option 1: 250 us
  • Option 2: 55 us
  • Option 3: 15 us

Option 3 is by far the fastest one, though it requires to insert manually the decimal separator into the string (as we are doing in c++ right now).

With option 3, we even get a better performance compared to the current JNI calls. These are the new measurements:

Current / New  / Diff (x1.1f)
    210 /   27 / -184 (x0.1)
    123 /   45 /  -78 (x0.4)
    111 /   20 /  -90 (x0.2)
    112 /   21 /  -91 (x0.2)
    115 /   18 /  -97 (x0.2)
    112 /   25 /  -87 (x0.2)
    132 /   22 / -110 (x0.2)
    126 /   23 / -103 (x0.2)
    123 /   29 /  -94 (x0.2)
    120 /   22 /  -97 (x0.2)
    185 /   28 / -156 (x0.2)
    140 /   44 /  -97 (x0.3)
    123 /   22 / -101 (x0.2)
    130 /   26 / -104 (x0.2)
    111 /   22 /  -89 (x0.2)
    108 /   16 /  -92 (x0.1)
    123 /   20 / -103 (x0.2)
    120 /   29 /  -91 (x0.2)
    127 /   21 / -106 (x0.2)

This 3rd option seems to be a good solution performance-wise, so we could start to implement it in this PR, including:

  1. Delete the native call for speed formatting.
  2. Implement decimal separator change detection.
  3. Update units strings at app startup and when settings are changed.
  4. Implement speed formatting unit tests in Android

I have to check also if Android Auto also uses native calls for speed formatting, and see if this solution also applies there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about separating thousands? There are also Formatter formatter = new Formatter(sb, Locale.US); and java.text.NumberFormat

Note that there are also values displayed in the Place Page (altitude, speed) that also should be formatted. Ideally, doing it in one place and supporting only one implementation would be the best way to go than supporting C++ version and custom Java formatters for Android.

As you've already established a test environment, does it make sense to check how the current C++ implementation can be sped up? Is there any overhead there? Can JNI calls be optimized?

long elapsed1 = System.nanoTime() - start1;

Logger.i("LOCALE_MEASURE", "1) " + speedString);

long start2 = System.nanoTime();
if (speedValue < 10.0)
speedString = mDecimalFormatOneDecimal.format(speedValue);
else
speedString = mDecimalFormatNoDecimal.format(speedValue);
long elapsed2 = System.nanoTime() - start2;

Logger.i("LOCALE_MEASURE", "2) " + speedString);

long start3 = System.nanoTime();
if (speedValue < 10.0)
{
speedString = Long.toString(Math.round(speedValue * 10.0));

StringBuffer buffer = new StringBuffer(speedString);

if (mDecimalSeparator == Character.MIN_VALUE)
mDecimalSeparator = DecimalFormatSymbols.getInstance().getDecimalSeparator();

// For low values (< 1.0), force to have 2 characters in string.
if (buffer.length() < 2)
buffer.insert(0, "0");

buffer.insert(1, mDecimalSeparator);

speedString = buffer.toString();
}
else
speedString = Long.toString(Math.round(speedValue));
long elapsed3 = System.nanoTime() - start3;

Logger.i("LOCALE_MEASURE", "3) " + speedString);

String text = String.format(Locale.US,
"%5d / %5d / %5d",
Math.round(0.001 * elapsed1),
Math.round(0.001 * elapsed2),
Math.round(0.001 * elapsed3));

Logger.i("LOCALE_MEASURE", text);

return new Pair<>(speedString, unitsString);
}

public static Pair<String, String> format(double speedInMetersPerSecond, int units,
Context context)
{
double speedValue;
String unitsString;

if (units == Framework.UNITS_IMPERIAL)
{
speedValue = MpsToMiph(speedInMetersPerSecond);
unitsString = mUnitStringMiph;
}
else
{
speedValue = MpsToKmph(speedInMetersPerSecond);
unitsString = mUnitStringKmh;
}

String speedString;

if (speedValue < 10.0)
{
speedString = Long.toString(Math.round(speedValue * 10.0));

StringBuffer buffer = new StringBuffer(speedString);

if (mDecimalSeparator == Character.MIN_VALUE)
mDecimalSeparator = DecimalFormatSymbols.getInstance().getDecimalSeparator();

// For low values (< 1.0), force to have 2 characters in string.
if (buffer.length() < 2)
buffer.insert(0, "0");

buffer.insert(1, mDecimalSeparator);

speedString = buffer.toString();
}
else
speedString = Long.toString(Math.round(speedValue));

return new Pair<>(speedString, unitsString);
}
}
35 changes: 32 additions & 3 deletions android/app/src/main/java/app/organicmaps/widget/menu/NavMenu.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package app.organicmaps.widget.menu;

import android.location.Location;
import android.util.Log;
import android.util.Pair;
import android.view.View;
import android.widget.Button;
Expand All @@ -15,12 +16,16 @@
import app.organicmaps.routing.RoutingInfo;
import app.organicmaps.sound.TtsPlayer;
import app.organicmaps.util.Graphics;
import app.organicmaps.util.Speed;
import app.organicmaps.util.StringUtils;
import app.organicmaps.util.UiUtils;
import app.organicmaps.util.log.Logger;

import com.google.android.material.progressindicator.LinearProgressIndicator;

import java.time.LocalTime;
import java.time.format.DateTimeFormatter;
import java.util.Locale;
import java.util.concurrent.TimeUnit;

public class NavMenu
Expand Down Expand Up @@ -203,22 +208,46 @@ private void updateTimeEstimate(int seconds)
mTimeEstimate.setText(localTime.format(DateTimeFormatter.ofPattern(format)));
}

private void updateSpeedView(@NonNull RoutingInfo info)
private void updateSpeedView(@NonNull RoutingInfo info, int units)
{
final Location last = LocationHelper.from(mActivity).getSavedLocation();
if (last == null)
return;

// Log measurements for different Java implementations.
Speed.formatMeasurements(last.getSpeed(), units, mActivity.getApplicationContext());

// Speed formatting using native calls.
long start1 = System.nanoTime();
Pair<String, String> speedAndUnits = StringUtils.nativeFormatSpeedAndUnits(last.getSpeed());
long elapsed1 = System.nanoTime() - start1;

mSpeedUnits.setText(speedAndUnits.second);
mSpeedValue.setText(speedAndUnits.first);

// Speed formatting using Android calls.
long start2 = System.nanoTime();
speedAndUnits = Speed.format(last.getSpeed(), units, mActivity.getApplicationContext());
long elapsed2 = System.nanoTime() - start2;

mSpeedUnits.setText(speedAndUnits.second);
mSpeedValue.setText(speedAndUnits.first);

String text = String.format(Locale.US,
"Current / New / Diff (us): %4d / %4d / %4d (x%.1f)",
Math.round(0.001 * elapsed1),
Math.round(0.001 * elapsed2),
Math.round(0.001 * (elapsed2 - elapsed1)),
1.0 * elapsed2 / elapsed1);

Logger.i("LOCALE_MEASURE", text);

mSpeedViewContainer.setActivated(info.isSpeedLimitExceeded());
}

public void update(@NonNull RoutingInfo info)
public void update(@NonNull RoutingInfo info, int units)
{
updateSpeedView(info);
updateSpeedView(info, units);
updateTime(info.totalTimeInSeconds);
mDistanceValue.setText(info.distToTarget.mDistanceStr);
mDistanceUnits.setText(info.distToTarget.getUnitsStr(mActivity.getApplicationContext()));
Expand Down