-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
wrong way around a triangle #105
Comments
Can I have the input data and exact query? |
data: <?xml version="1.0" encoding="UTF-8"?>
<osm generator="osrm-test" version="0.6">
<node id="-1" version="1" uid="1" user="osrm" timestamp="2012-01-01T00:00:00Z" lon="1.0" lat="1.0">
<tag k="name" v="a"/>
</node>
<node id="-2" version="1" uid="1" user="osrm" timestamp="2012-01-01T00:00:00Z" lon="1.002" lat="1.0">
<tag k="name" v="b"/>
</node>
<node id="-3" version="1" uid="1" user="osrm" timestamp="2012-01-01T00:00:00Z" lon="1.001" lat="0.998">
<tag k="name" v="c"/>
</node>
<way id="-4" version="1" uid="1" user="osrm" timestamp="2012-01-01T00:00:00Z">
<nd ref="-1"/>
<nd ref="-2"/>
<tag k="highway" v="primary"/>
<tag k="name" v="ab"/>
</way>
<way id="-5" version="1" uid="1" user="osrm" timestamp="2012-01-01T00:00:00Z">
<nd ref="-2"/>
<nd ref="-3"/>
<tag k="highway" v="primary"/>
<tag k="name" v="bc"/>
</way>
<way id="-6" version="1" uid="1" user="osrm" timestamp="2012-01-01T00:00:00Z">
<nd ref="-3"/>
<nd ref="-1"/>
<tag k="highway" v="primary"/>
<tag k="name" v="ca"/>
</way>
</osm> speedprofile: [test] queries and results: http://localhost:5000/viaroute&start=1.0,1.002&dest=1.0,1.0&output=json&geomformat=cmp http://localhost:5000/viaroute&start=0.998,1.001&dest=1.0,1.002&output=json&geomformat=cmp hopefully you'll soon be able to check easily via test cases. |
cucumber tests now write failed scenarios to a log file, for easier debugging. example: Failed Scenario: Circular ways <?xml version="1.0" encoding="UTF-8"?>
<osm generator="osrm-test" version="0.6">
<node id="-21" version="1" uid="1" user="osrm" timestamp="2012-02-04T21:02:12Z" lon="1.0" lat="1.0">
<tag k="name" v="a"/>
</node>
<node id="-22" version="1" uid="1" user="osrm" timestamp="2012-02-04T21:02:12Z" lon="1.002" lat="1.0">
<tag k="name" v="b"/>
</node>
<node id="-23" version="1" uid="1" user="osrm" timestamp="2012-02-04T21:02:12Z" lon="1.001" lat="0.998">
<tag k="name" v="c"/>
</node>
<way id="-24" version="1" uid="1" user="osrm" timestamp="2012-02-04T21:02:12Z">
<nd ref="-21"/>
<nd ref="-22"/>
<tag k="highway" v="primary"/>
<tag k="name" v="ab"/>
</way>
<way id="-25" version="1" uid="1" user="osrm" timestamp="2012-02-04T21:02:12Z">
<nd ref="-22"/>
<nd ref="-23"/>
<tag k="highway" v="primary"/>
<tag k="name" v="bc"/>
</way>
<way id="-26" version="1" uid="1" user="osrm" timestamp="2012-02-04T21:02:12Z">
<nd ref="-23"/>
<nd ref="-21"/>
<tag k="highway" v="primary"/>
<tag k="name" v="ca"/>
</way>
</osm> [test] Expected: {"from"=>"b", "to"=>"a", "route"=>"ab"} Expected: {"from"=>"c", "to"=>"b", "route"=>"bc"} |
Does it work with positive IDs? Also, the difference in edge lengths is rather small and prone to rounding errors. |
sorry, i still see the same result. once tests are ready for you to try, it might be easier for you to investigate. regarding length: ways are in a triangle a,b,c, and instead of simply going b-a it's going b-c-a. actual is 500, while expected is just 220 (the triangle is not quite eqilateral). isn't that quite a big difference? |
Good catch, Emil. I figured this one out. Outgoing edges from the node with the highest ID were lost in translation. So when this node was queried it did not connect to anything. On large graphs, it is quite unlikely to hit this node. As always, fix will be pushed to repo later today. |
thank cucumber :-) nice you found out what was going on. |
It may also very well be possible that this bug caused random crashes reported in the other issue. |
Another thing is, that OSRM does not locate anything on nodes. So, the expected output is not just one edge here. |
Formatted output looks like this now: {
"version": 0.3,
"status": 0,
"status_message": "Found route between points",
"route_summary": {
"total_distance": 220,
"total_time": 12,
"start_point": "bc",
"end_point": "ca"
},
"route_geometry": "_ibEoubE???nK??",
"route_instructions": [
[
"Start",
"bc",
0,
0,
0,
"0m",
"N",
0
],
[
"Turn sharp left",
"ab",
222,
1,
114,
"222m",
"E",
89.95
],
[
"Turn sharp left",
"ca",
0,
2,
0,
"0m",
"N",
0
]
],
"via_points": [],
"transactionId": "OSRM Routing Engine JSON Descriptor (v0.3)"
} |
So, a couple lines of code later, the output now is: {
"version": 0.3,
"status": 0,
"status_message": "Found route between points",
"route_summary": {
"total_distance": 220,
"total_time": 12,
"start_point": "bc",
"end_point": "ca"
},
"route_geometry": "_ibEoubE?nK",
"route_instructions": [
[
"Head",
"ab",
222,
0,
114,
"222m",
"E",
89.95
]
],
"via_points": [],
"transactionId": "OSRM Routing Engine JSON Descriptor (v0.3)"
} This looks about right, doesn't it? |
i suppose start and end point actually refer to start and end way? |
and iif it's a start way, not start point, perhaps it would be better to rename the field? |
True, this makes sense. |
Alright, now it is: {
"version": 0.3,
"status": 0,
"status_message": "Found route between points",
"route_summary": {
"total_distance": 220,
"total_time": 12,
"start_point": "ab",
"end_point": "ab"
},
"route_geometry": "_ibEoubE?nK",
"route_instructions": [
[
"Head",
"ab",
222,
0,
114,
"222m",
"E",
89.95
]
],
"via_points": [],
"transactionId": "OSRM Routing Engine JSON Descriptor (v0.3)"
} Will test it now, and push it in the coming hours. |
given a-b-c, and going from b to c, it still returns instructions ab,bc, with ab having length 1. is there some unfortunate rounding involved? Failed scenario: Two ways connected in a straight line <?xml version="1.0" encoding="UTF-8"?>
<osm generator="osrm-test" version="0.6">
<node id="1" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z" lon="1.0" lat="1.0">
<tag k="name" v="a"/>
</node>
<node id="2" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z" lon="1.001" lat="1.0">
<tag k="name" v="b"/>
</node>
<node id="3" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z" lon="1.002" lat="1.0">
<tag k="name" v="c"/>
</node>
<way id="4" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z">
<nd ref="1"/>
<nd ref="2"/>
<tag k="highway" v="primary"/>
<tag k="name" v="ab"/>
</way>
<way id="5" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z">
<nd ref="2"/>
<nd ref="3"/>
<tag k="highway" v="primary"/>
<tag k="name" v="bc"/>
</way>
</osm> [test] Expected: {"from"=>"b", "to"=>"c", "route"=>"bc"} Expected: {"from"=>"c", "to"=>"b", "route"=>"bc"} |
This one also looks better now for both directions: {
"version": 0.3,
"status": 0,
"status_message": "Found route between points",
"route_summary": {
"total_distance": 110,
"total_time": 27,
"start_point": "bc",
"end_point": "bc"
},
"route_geometry": "_ibEeobE?iE",
"route_instructions": [
[
"Head",
"bc",
112,
0,
269,
"112m",
"W",
270.02
]
],
"via_points": [],
"transactionId": "OSRM Routing Engine JSON Descriptor (v0.3)"
} and {
"version": 0.3,
"status": 0,
"status_message": "Found route between points",
"route_summary": {
"total_distance": 110,
"total_time": 27,
"start_point": "bc",
"end_point": "bc"
},
"route_geometry": "_ibEoubE?hE",
"route_instructions": [
[
"Head",
"bc",
112,
0,
269,
"112m",
"E",
89.97
]
],
"via_points": [],
"transactionId": "OSRM Routing Engine JSON Descriptor (v0.3)"
} |
sweet |
looking forward to rerunning test after these change have been pushed |
Please report back, as soon as you have any results. |
much better. Failed scenario: 3 ways connected in a triangle <?xml version="1.0" encoding="UTF-8"?>
<osm generator="osrm-test" version="0.6">
<node id="1" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z" lon="1.0" lat="1.0">
<tag k="name" v="a"/>
</node>
<node id="2" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z" lon="1.0018018018018018" lat="1.0">
<tag k="name" v="b"/>
</node>
<node id="3" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z" lon="1.000900900900901" lat="0.9981981981981982">
<tag k="name" v="c"/>
</node>
<way id="4" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z">
<nd ref="1"/>
<nd ref="2"/>
<tag k="highway" v="primary"/>
<tag k="name" v="ab"/>
</way>
<way id="5" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z">
<nd ref="2"/>
<nd ref="3"/>
<tag k="highway" v="primary"/>
<tag k="name" v="bc"/>
</way>
<way id="6" version="1" uid="1" user="osrm" timestamp="2000-00-00T00:00:00Z">
<nd ref="3"/>
<nd ref="1"/>
<tag k="highway" v="primary"/>
<tag k="name" v="ca"/>
</way>
</osm> [Scenario: 3 ways connected in a triangle] Expected: {"from"=>"b", "to"=>"c", "route"=>"bc"} Attempt: Backward |
i hope you will consider getting the cucumber tests running yourself. i moved the ini files to the root folder as you suggested. |
f379578a3 Release 1.7.1 040947ba2 CMake: Add language C to project() c60e87879 Update include catch.hpp to 2.13.8 6599d4828 Update change log ebd2e4f40 Allow setting C++ version to compile with in CMake config bbb2a95d0 Github actions: Only install x64 version of vcpkgs in windows build 546edf929 Avoid narrowing conversion by being explicit 91adaecd6 Fix various issues reported by clang-tidy and disable some checks 68f30a1f7 Add Github actions CI build for Linux/macOS/Windows c13886b78 Update some links bd429c52f Include tools subdirectory *after* testing is enabled 3133dc52f Use std::memcpy instead of just memcpy 7ee29422a Merge pull request #106 from daniel-j-h/fix-byteswap-ub aba0800aa CMake config: clang-tidy target needs writer_tests which need protobuf 74516e8df Modernize CMake config a little bit b4486ca7a Disable some clang-tidy warnings 19f4b75f6 Fix appveyor build 58b1a19a4 Modernize Doxygen config file 046c07d0e Update included catch2 framework to current version v2.13.7 49acea746 fix some doxygen warnings by removing obsolete config entries 0c5426df3 fix cmake dep warning: 'Compatibility with CMake < 2.8.12 will be removed from a future version of CMake' dadf7bd51 Fixes float and double byteswap undefined behavior 85db94025 Merge pull request #105 from ffontaine/master d3a35791d Merge pull request #104 from joto/master 03daae49c CMakeLists.txt: respect BUILD_TESTING 67133e362 Add missing includes 9f85f3a5e Update README.md 010ffcf69 Release 1.7.0 6ad492994 Fixed docs adde4dedf Travis: Add non-Intel architectures 37c3d0e1d Add typedefs and functions to make buffer adaptor work as container 83563acdb Remove delegating constructor because clang-tidy doesn't like it b4afc06db Use #include "" for library-internal includes d1929788e Add missing includes fae5247f0 Update change log 2abb1b5cd Travis: Test with std::string_view, not std::experimental::string_view 697bd812d Use forwarding constructor in fixed_size_buffer_adaptor c8fd2e819 Move fixed_size_buffer_adaptor into buffer_fixed.hpp abb856ecc Remove semicolon where it doesn't belong 6243855bc Change the way the customization for special buffer classes work d6a8ed098 Remove useless post-increment 184046cb0 Remove need for push_back() on custom buffer types 0a974e067 Remove templated buffer adaptor wrappers ed6ba5097 Add buffer implementation based on std::vector<char> cec309c3c Use more descriptive names for buffer test types a7b99da6f Use TEMPLATE_TEST_CASE to test different buffer implementations a0abc493c Use explicit for constructor 72850abc9 Remove broken doxygen link 1e347c620 Add more convenient fixed_size_buffer_adaptor constructor fb575e0ea Make members private 50e953b71 Make older compilers happy c850ef150 Extend tests of static buffer use f6d8394c0 Rename fixed_size_buffer to fixed_size_buffer_adaptor 3b18162e3 Make the buffer backend used by the pbf writer configurable. 6fd19c58d "Modernize" travis config 981aba084 Use explicit cast to avoid undefined behaviour 550974d5d Travis: Do not test GCC 4.7 any more 866e024fc Revert "Workaround in catch code so it compiles with older compilers" 65dfad056 Disable a clang-tidy test triggered by Catch. 02bf73df5 Workaround in catch code so it compiles with older compilers f98792a15 Travis: Do not update homebrew for faster builds 2d87da7ec Switch tests to Catch2 5dc45ac3b Avoid signed/unsigned comparison 3a93f19ba Add missing includes b49c077ac Disable clang-tidy for files where we don't have a compile command 34396fc7d Travis: Fix gcc8 build c3060101c Handle clang-tidy warnings 64ef96ff0 Revert "Initialize test messages" a0828d538 Travis: Also build with GCC 8 171c5c446 Update travis xcode versions f5a223aa7 Use "auto*" instead of just "auto" for pointer types e4fa23616 Initialize test messages e3a59454a Simpler code and avoid shadowing of external function 830f049b4 Use STL algorithms insted of raw loops faa7e6e8a Disable config settings not used in newer Doxygen versions 3b2e11438 Remove unnecessary enum name 7487f8109 Release 1.6.8 6dcaf8fde Travis config: Use "official" way to pull homebrew package. c61eb29c3 Revert "Disable warnings from clang-tidy about a missing file." 28d05a0a8 Disable warnings from clang-tidy about a missing file. 329920a3c Pesky aliases of clang-tidy warnings strike again. 79fd87922 User plain assert() instead of our own so compare() can be noexcept. 473e6ec13 Update change log. 393e279b7 Make pbf_writer destructor noexcept. 48a38b3f2 Disable clang-tidy misc-non-private-member-variables-in-classes. e9c148c8a Use no-argument version of main(). 29ba04123 Disable clang tidy checks for C arrays. 2fcfb56e2 More places to use std::array instead of a C style array. 7321761a3 Disable a clang-tidy warning. 4d9d8fff4 Make data_view::compare() noexcept. 3325364cf User uppercase integer literal suffix. df0a23c5e Use std::array instead of C arrays in some places. 8247ed76b Make clang-tidy include order check happy. f1b504e16 Update travis config to user newer compilers and operating systems. ccf692d47 Disable some clang-tidy warnings. 095abd259 CMake config: Also look for newer clang-tidy versions. 2c1f6f9c8 Use uppercase integer literal suffixes. fadd024d4 Release 1.6.7 8c6acbff7 Fix signed-unsigned comparison. b36774ccb Release 1.6.6 5a92b744f Remove useless asserts, simplify condition. 06bafb56c Fix several possible UBs. b7b290b1a Release 1.6.5 51753d514 Merge pull request #95 from tomhughes/subscript b90faaf03 Avoid out of bounds array subscript 7d418492e Merge pull request #94 from nigels-com/proto2 015f9cc5e Specify proto2 syntax to appease protoc 23d48fd2a Use universal initialization syntax in constructors. 0f610fad5 Update travis config: Use xenial for most builds. d71da0b04 Update appveyor config: Simpler builds, current MSVC, 32bit build 3ef46ba78 Release 1.6.4 3a1ef0138 Tighten some tests. 18eebb8c3 Remove unused code from tests. 29ef3e4e7 More casts to remove undefined behaviour. 6108e6480 No more bitwise operations on signed integers in zigzag encoder/decoder. 6e0d34985 Remove bitwise operations on signed integers in varint decoder. 4af65f262 Update change log. 2f82182fe Add some tips to test/create_pbf_test_data.sh. c55f4ed55 Fix some doxygen warnings. afa362a03 Add static_asserts to check movability of some classes. efeb45e0c Disable readability-implicit-bool-conversion clang-tidy warning. 78febda5b Explicit conversion and tests for new pbf_reader::data() function. 0d5492c9c Revert "Explicit conversion and tests for new pbf_reader::data() function." 43cf8fa5a Fix travis config. bd2ae4682 Explicit conversion and tests for new pbf_reader::data() function. 28cd406bd Update travis with newer compiler versions. 0555e6a1f Add function to get the not yet read data from a pbf_reader. bf4284bee Disable docker builds on travis. They are being phased out by travis. 5ffe45b71 New add_packed_fixed template function. e54cd858d Add helper function that computes the length a varint would have. 72d7e143a More consistent implementation of operators. 3a41880c2 Do not download protobuf library, it isn't found by cmake anyway. 3c662ce3c Remove comment that doesn't apply (any more). 45da6dd4d Update zigzag tests. 4ad573dbf Extra cast so we do the xor with unsigned ints. 509aec5ab Update appveyor build to current Visual Studio compiler. 67b24e1a3 Remove unnecessary workaround in Appveyor config. c559af682 Remove xcode6.4 build soon to be removed from travis. 0662dcecc Release 1.6.3 da5bfc019 Move byteswap_inplace functions from detail into protozero namespace. a44efc34e Travis: Ignore install problems on OSX. 5775b2b23 Travis update to newer OSX image. 032aa037c Special case the distance between default initialized iterators. 0ca02161e Make dereferencing operator of fixed_iterator noexcept. a0095f603 Test code must call functions that it wants to test. 6791b0bc3 Add unit tests. 191eb4004 Add some paranoia asserts. 99ca512f5 Use TEST_CASEs instead of SECTIOs in some tests. 040e2bc14 Add some asserts and tests. git-subtree-dir: third_party/protozero git-subtree-split: f379578a3f7c8162aac0ac31c2696de09a5b5f93
expected routes marked with stars
so it seems i'm getting routed the long way around on some of the trips?
when going from b to a, i get bc,ca, instead of just ab
when going from c to b, i get ca,ab, instead of just bc
(note: i'm temporarily filtering out instructions with length<=1, that why the last line shows 2 steps under route, but 3 under details...)
The text was updated successfully, but these errors were encountered: