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.

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

Coordinate Truncation #311

Merged
merged 4 commits into from
Nov 15, 2018
Merged

Coordinate Truncation #311

merged 4 commits into from
Nov 15, 2018

Conversation

JThramer
Copy link
Contributor

Fixes #309. Adds number formatter that truncates coordinates to relevant number of decimal places, depending on shapeFormat setting.

@JThramer JThramer self-assigned this Nov 13, 2018
@JThramer
Copy link
Contributor Author

cc @mapbox/navigation-ios

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Tying the requested waypoint coordinates’ precision to the requested shape format is a good idea, but there’s a simpler, more robust way to round numbers for machine consumption than to use a number formatter, which is meant for human consumption.

Here’s a suggestion for the changelog:

Fixed an error requesting a route with very many waypoints.


internal func jsonRepresentation(precision: CoordinatePrecision) -> String? {
let formatter = NumberFormatter()
formatter.maximumFractionDigits = precision.rawValue
Copy link
Contributor

@1ec5 1ec5 Nov 14, 2018

Choose a reason for hiding this comment

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

We’d get better performance by keeping just one number formatter around instead of recreating it twice for each waypoint. Given the large number of waypoints required to trigger the error being fixed, the difference might be measurable.

Additionally, the default NumberFormatter.locale depends on the application locale. So if the system language is set to Spanish and the application is localized into Spanish, all requests will produce errors, because Spanish uses a comma as the decimal separator.

Use the standard library math functions instead:

extension Double {
    func rounded(to precision: Double) -> Double {
        return (self * precision).rounded() / precision
    }
}

return "\(coordinate.longitude.rounded(to: 1e6)),\(coordinate.latitude.rounded(to: 1e6))"

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, NumberFormatters are known to be expensive to create (or perhaps they only used to be?)

this is worth profiling, and possibly statically memoizing an instance or using @1ec5’s suggestion

Copy link
Contributor

@1ec5 1ec5 Nov 14, 2018

Choose a reason for hiding this comment

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

Formatters are generally expensive to create because the actual formatting is done by ICU, which is generic enough to support lots of languages and configurations, based on locales defined by CLDR. As far as I know, NumberFormatter doesn’t do a whole lot of caching between instances, because each instance’s configuration is mutable and each configuration option can drastically affect output.

Anyways, formatters are only for output that’s meant for display to the user. This string is going into a URL query parameter that is interpreted as two standard POSIX doubles. So to fix the localization issue I pointed out, we’d need to set locale to en_US_POSIX. (It isn’t enough to set the decimal separator, because some languages like Arabic use non-Western numerals.) But at that point, we might as well use the standard library math functions, because we don’t benefit at all from NumberFormatter.

case six = 6
}

internal func jsonRepresentation(precision: CoordinatePrecision) -> String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

The return value goes directly into a URL query parameter, not JSON. What we’re implementing here is really String(describing:precision:) for a CLLocationCoordinate2D.

Jerrad Thramer and others added 2 commits November 15, 2018 13:48
@JThramer JThramer merged commit a5d8f99 into master Nov 15, 2018
@JThramer JThramer deleted the jerrad/coordinate-truncation branch November 15, 2018 21:15
@frederoni frederoni mentioned this pull request Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants