Fix distance calculation consistency. by SiarheiFedartsou · Pull Request #6315 · Project-OSRM/osrm-backend

SiarheiFedartsou · 2022-08-18T20:56:52Z

Issue

closes #5316
closes #5151

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-08-18T20:58:05Z

features/testbot/distance_matrix.feature

+            | 2    | 3  | fghij,fghij        | 400m       |
+

-        # TODO: this is "correct", but inconsistent with viaroute


This test now ensures that we fixed #5151 (also checked that manually via requests similar to what was provided in the issue)

SiarheiFedartsou · 2022-08-18T20:58:59Z

features/support/data.js

        // see ApproximateDistance() in ExtractorStructs.h
        // it's only accurate when measuring along the equator, or going exactly north-south
-        this.zoom = parseFloat(meters) * 0.8990679362704610899694577444566908445396483347536032203503E-5;
+        this.zoom = this.gridSize * 0.8990679362704610899694577444566908445396483347536032203503E-5;


It is now only used in findNodeByLocation and we probably could use just this.gridSize there, but it is easier to keep it as is.

SiarheiFedartsou · 2022-08-18T21:00:19Z

include/engine/api/base_api.hpp

                phantom.location,
-                util::coordinate_calculation::fccApproximateDistance(phantom.location,
-                                                                     phantom.input_location),
+                util::coordinate_calculation::greatCircleDistance(phantom.location,


Renamed it to more generic greatCircleDistance, but have no strong opinion here - we can leave old naming(or introduce new one, may be just distance?).

And btw it cheap-ruler is no longer based on FCC formula https://twitter.com/mourner/status/1560003819211309056?s=20&t=6zvluLg1gn3HYB0zM1znOw

Sounds like the right name to me - agnostic to the underlying algorithm.

SiarheiFedartsou · 2022-08-18T21:05:02Z

CMakeLists.txt

 set(MBXGEOM_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/third_party/geometry.hpp-0.9.2/include")
 include_directories(SYSTEM ${MBXGEOM_INCLUDE_DIR})
-set(CHEAPRULER_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/third_party/cheap-ruler-cpp-2.5.4/include")
+set(CHEAPRULER_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/third_party/cheap-ruler-cpp-2778eb8/include")


Decided to update it to newer version. Mapbox doesn't set tags since 2017, but there was significant changes since that time.

SiarheiFedartsou · 2022-08-18T21:09:33Z

features/testbot/planetary.feature


        When I route I should get
-            | from | to | route | distance       |
-            | a    | b  | ab,ab | 8905559m ~0.1% |


Btw I quickly compared old and new values against Vincenty formula(I used https://www.npmjs.com/package/node-vincenty) and new values are more precise(I lost the exact values, but it is easy to repeat the experiment).

SiarheiFedartsou · 2022-08-18T21:10:45Z

src/util/coordinate_calculation.cpp

 }

-double bearing(const Coordinate first_coordinate, const Coordinate second_coordinate)
+double bearing(const Coordinate coordinate_1, const Coordinate coordinate_2)


Bearing is also migrated to cheap-ruler.

SiarheiFedartsou · 2022-08-18T21:11:40Z

@mjjbell please take a look when you have a chance

SiarheiFedartsou · 2022-08-19T10:28:55Z

src/util/coordinate_calculation.cpp

+    auto result = ruler.bearing({lon1, lat1}, {lon2, lat2});
+    if (result < 0.0)
    {
        result += 360.0;


cheap-ruler returns bearing in [-180;180] format, just like turf.js
then we need to convert it to [0;360]

See how it is done in turf.js https://github.com/Turfjs/turf/blob/6bc5fbe927b85b2be8e38600b5ffb29525a6f925/packages/turf-helpers/index.ts#L657

mjjbell

👍

mjjbell · 2022-08-19T11:29:53Z

src/util/coordinate_calculation.cpp

        .distance({lon1, lat1}, {lon2, lat2});
 }

-double haversineDistance(const Coordinate coordinate_1, const Coordinate coordinate_2)


Yes I noticed this a while ago that there were almost duplicate methods. Nice to clean them up 👍

mjjbell · 2022-08-19T11:32:58Z

include/engine/api/base_api.hpp

                phantom.location,
-                util::coordinate_calculation::fccApproximateDistance(phantom.location,
-                                                                     phantom.input_location),
+                util::coordinate_calculation::greatCircleDistance(phantom.location,


Sounds like the right name to me - agnostic to the underlying algorithm.

mjjbell · 2022-08-19T11:45:25Z

features/testbot/projection.feature

+            | from | to | route   | bearing       | distance     |
+            | b    | a  | abc,abc | 0->225,225->0 | 1002.9m      |
+            | b    | c  | abc,abc | 0->45,45->0   | 1005m +- 3   |
+            | a    | d  | abc,abc | 0->45,45->0   | 1002.9m      |


It looks like this test was round-tripping the haversine calculation.
Is this worth keeping, now that the algorithm implementation is in the third-party library?

I think it is okay to leave, it is not a big deal to support, but may potentially catch something unexpected.

mjjbell · 2022-08-19T12:06:11Z

features/guidance/anticipate-lanes.feature

+               | c,g       | cb,bdehi,eg,eg | depart,turn left,turn left,arrive          | ,left:true left:true left:true left:true,left:true left:true straight:false straight:false straight:false straight:false right:false right:false,                                                 |             |
+               | c,f       | cb,bdehi,ef,ef | depart,turn left,turn right,arrive         | ,left:true left:true left:true left:true,left:false left:false straight:false straight:false straight:false straight:false right:true right:true,                                                 |             |
+               | c,l       | cb,bdehi,il,il | depart,turn left,end of road left,arrive   | ,left:true left:true left:true left:true;left:false left:false straight:true straight:true straight:false straight:false right:false right:false,left:true left:true right:false right:false,     |             |
+               | c,j       | cb,bdehi,ij,ij | depart,turn left,end of road right,arrive  | ,left:true left:true left:true left:true;left:false left:false straight:false straight:false straight:true straight:true right:false right:false,left:false left:false right:true right:true,     | not perfect |


I'm going to assume these make sense given I don't understand the lane guidance logic.
Looks like it makes more lanes viable as part of the turn maneuvers 🤷‍♂️

mjjbell · 2022-08-19T12:09:33Z

features/guidance/dedicated-turn-roads.feature

       When I route I should get
            | waypoints | route             | turns                                      | locations |
-            | s,f       | sabc,ae,dbef,dbef | depart,fork slight right,turn right,arrive | s,a,e,f   |
+            | s,f       | sabc,ae,dbef,dbef | depart,turn straight,turn right,arrive     | s,a,e,f   |


Does the input map need tweaking to use the dedicated turn road?

Do you mean we should modify test input in order to have exact the same result as it used to be? To be honest I am not familiar with feature we test here, just changed it assuming that it is caused by slight differences in distance calculation - if you can elaborate I would really appreciate.

Yes the guidance message to stay straight no longer matches the route path that takes the right-hand road.
Perhaps this reflects on the brittleness of the guidance code that minor distance changes are breaking it.

v5.27.0 - Changes from 5.26.0 - API: - ADDED: Add Flatbuffers support to NodeJS bindings. [Project-OSRM#6338](Project-OSRM#6338) - CHANGED: Add `data_version` field to responses of all services. [Project-OSRM#5387](Project-OSRM#5387) - FIXED: Use Boost.Beast to parse HTTP request. [Project-OSRM#6294](Project-OSRM#6294) - FIXED: Fix inefficient osrm-routed connection handling [Project-OSRM#6113](https://gihub.com/Project-OSRM/osrm-backend/pull/6113) - FIXED: Fix HTTP compression precedence [Project-OSRM#6113](Project-OSRM#6113) - NodeJS: - FIXED: Support `skip_waypoints` in Node bindings [Project-OSRM#6060](Project-OSRM#6060) - Misc: - ADDED: conanbuildinfo.json for easy reading of dependencies [Project-OSRM#6388](Project-OSRM#6388) - CHANGED: Improve performance of JSON rendering. Fix undefined behaviour in JSON numbers formatting. [Project-OSRM#6380](Project-OSRM#6380) - ADDED: Add timestamps for logs. [Project-OSRM#6375](Project-OSRM#6375) - CHANGED: Improve performance of map matching via getPathDistance optimization. [Project-OSRM#6378](Project-OSRM#6378) - CHANGED: Optimize RestrictionParser performance. [Project-OSRM#6344](Project-OSRM#6344) - ADDED: Support floats for speed value in traffic updates CSV. [Project-OSRM#6327](Project-OSRM#6327) - CHANGED: Use Lua 5.4 in Docker image. [Project-OSRM#6346](Project-OSRM#6346) - CHANGED: Remove redundant nullptr check. [Project-OSRM#6326](Project-OSRM#6326) - CHANGED: missing files list is included in exception message. [Project-OSRM#5360](Project-OSRM#5360) - CHANGED: Do not use deprecated Callback::Call overload in Node bindings. [Project-OSRM#6318](Project-OSRM#6318) - FIXED: Fix distance calculation consistency. [Project-OSRM#6315](Project-OSRM#6315) - FIXED: Fix performance issue after migration to sol2 3.3.0. [Project-OSRM#6304](Project-OSRM#6304) - CHANGED: Pass osm_node_ids by reference in osrm::updater::Updater class. [Project-OSRM#6298](Project-OSRM#6298) - FIXED: Fix bug with reading Set values from Lua scripts. [Project-OSRM#6285](Project-OSRM#6285) - FIXED: Bug in bicycle profile that caused exceptions if there is a highway=bicycle in the data. [Project-OSRM#6296](Project-OSRM#6296) - FIXED: Internal refactoring of identifier types used in data facade [Project-OSRM#6044](Project-OSRM#6044) - CHANGED: Update docs to reflect recent build and dependency changes [Project-OSRM#6383](Project-OSRM#6383) - Build: - REMOVED: Get rid of Mason. [Project-OSRM#6387](Project-OSRM#6387) - CHANGED: Use clang-format from CI base image. [Project-OSRM#6391](Project-OSRM#6391) - ADDED: Build Node bindings on Windows. [Project-OSRM#6334](Project-OSRM#6334) - ADDED: Configure cross-compilation for Apple Silicon. [Project-OSRM#6360](Project-OSRM#6360) - CHANGED: Use apt-get to install Clang on CI. [Project-OSRM#6345](Project-OSRM#6345) - CHANGED: Fix TBB in case of Conan + NodeJS build. [Project-OSRM#6333](Project-OSRM#6333) - CHANGED: Migrate to modern TBB version. [Project-OSRM#6300](Project-OSRM#6300) - CHANGED: Enable performance-move-const-arg clang-tidy check. [Project-OSRM#6319](Project-OSRM#6319) - CHANGED: Use the latest node on CI. [Project-OSRM#6317](Project-OSRM#6317) - CHANGED: Migrate Windows CI to GitHub Actions. [Project-OSRM#6312](Project-OSRM#6312) - ADDED: Add smoke test for Docker image. [Project-OSRM#6313](Project-OSRM#6313) - CHANGED: Update libosmium to version 2.18.0. [Project-OSRM#6303](Project-OSRM#6303) - CHANGED: Remove EXACT from find_package if using Conan. [Project-OSRM#6299](Project-OSRM#6299) - CHANGED: Configure Undefined Behaviour Sanitizer. [Project-OSRM#6290](Project-OSRM#6290) - CHANGED: Use Conan instead of Mason to install code dependencies. [Project-OSRM#6284](Project-OSRM#6284) - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [Project-OSRM#6279](Project-OSRM#6279) - CHANGED: Update macOS CI image to macos-11. [Project-OSRM#6286](Project-OSRM#6286) - CHANGED: Enable even more clang-tidy checks. [Project-OSRM#6273](Project-OSRM#6273) - CHANGED: Configure CMake to not build flatbuffers tests and samples. [Project-OSRM#6274](Project-OSRM#6274) - CHANGED: Enable more clang-tidy checks. [Project-OSRM#6270](Project-OSRM#6270) - CHANGED: Configure clang-tidy job on CI. [Project-OSRM#6261](Project-OSRM#6261) - CHANGED: Use Github Actions for building container images [Project-OSRM#6138](Project-OSRM#6138) - CHANGED: Upgrade Boost dependency to 1.70 [Project-OSRM#6113](Project-OSRM#6113) - CHANGED: Upgrade Ubuntu CI builds to 20.04 [Project-OSRM#6119](Project-OSRM#6119) - CHANGED: Make building osrm-routed optional [Project-OSRM#6144](Project-OSRM#6144) - FIXED: Run all unit tests in CI [Project-OSRM#5248](Project-OSRM#5248) - FIXED: Fix installation of Mason CMake and 32 bit CI build [Project-OSRM#6170](Project-OSRM#6170) - FIXED: Fixed Node docs generation check in CI. [Project-OSRM#6058](Project-OSRM#6058) - CHANGED: Docker build, enabled arm64 build layer [Project-OSRM#6172](Project-OSRM#6172) - CHANGED: Docker build, enabled apt-get update/install caching in separate layer for build phase [Project-OSRM#6175](Project-OSRM#6175) - FIXED: Bump CI complete meta job to ubuntu-20.04 [Project-OSRM#6323](Project-OSRM#6323) - CHANGED: Node packages are now scoped by @Project-OSRM [Project-OSRM#6386](Project-OSRM#6386) - Routing: - CHANGED: Lazily generate optional route path data [Project-OSRM#6045](Project-OSRM#6045) - FIXED: Completed support for no_entry and no_exit turn restrictions. [Project-OSRM#5988](Project-OSRM#5988) - ADDED: Add support for non-round-trips with a single fixed endpoint. [Project-OSRM#6050](Project-OSRM#6050) - FIXED: Improvements to maneuver override processing [Project-OSRM#6125](Project-OSRM#6125) - ADDED: Support snapping to multiple ways at an input location. [Project-OSRM#5953](Project-OSRM#5953) - FIXED: Fix snapping target locations to ways used in turn restrictions. [Project-OSRM#6339](Project-OSRM#6339) - ADDED: Support OSM traffic signal directions. [Project-OSRM#6153](Project-OSRM#6153) - FIXED: Ensure u-turn exists in intersection view. [Project-OSRM#6376](Project-OSRM#6376) - FIXED: Gracefully handle no-turn intersections in guidance processing. [Project-OSRM#6382](Project-OSRM#6382) - Profile: - CHANGED: Bicycle surface speeds [Project-OSRM#6212](Project-OSRM#6212) - Tools: - CHANGED: Do not generate intermediate .osrm file in osrm-extract. [Project-OSRM#6354](Project-OSRM#6354)

Consolidate great circle distance calculations to use cheap ruler library.

SiarheiFedartsou commented Aug 18, 2022

View reviewed changes

SiarheiFedartsou force-pushed the sf-haversine3 branch from 0857485 to 1800abf Compare August 18, 2022 21:02

Fix distance calculation consistency.

ec134ae

SiarheiFedartsou force-pushed the sf-haversine3 branch from 1800abf to ec134ae Compare August 18, 2022 21:04

SiarheiFedartsou commented Aug 18, 2022

View reviewed changes

Fix distance calculation consistency.

772bac7

SiarheiFedartsou commented Aug 18, 2022

View reviewed changes

SiarheiFedartsou marked this pull request as ready for review August 18, 2022 21:11

SiarheiFedartsou commented Aug 19, 2022

View reviewed changes

mjjbell approved these changes Aug 19, 2022

View reviewed changes

mjjbell reviewed Aug 19, 2022

View reviewed changes

Merge branch 'master' into sf-haversine3

9eba4e4

mjjbell merged commit aadc088 into Project-OSRM:master Aug 19, 2022

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Fix distance calculation consistency. (Project-OSRM#6315)

c030d7f

Consolidate great circle distance calculations to use cheap ruler library.

mjjbell mentioned this pull request Mar 30, 2024

negative values in distances matrix (v5.22.0) #5855

Open

1ec5 mentioned this pull request Jul 9, 2024

Bad value of EARTH_RADIUS #5051

Closed

		\| 2 \| 3 \| fghij,fghij \| 400m \|


		# TODO: this is "correct", but inconsistent with viaroute

Uh oh!

Conversation

SiarheiFedartsou commented Aug 18, 2022

Issue

Tasklist

Requirements / Relations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou commented Aug 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjjbell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SiarheiFedartsou Aug 19, 2022 •

edited

Loading