-
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
oneway=-1 is broken? #108
Comments
i can't reproduce this, but i also found that there is something wrong. I did the following tests with the OSRM WebInterface and a minimalistic test file like emiltin's above. if the way is defined by points a->b->c and oneway=yes, routing is only possible from a to b. The segment b-c is not routable. The blue line (route) stops at point b. c to a, or b to a are not possible. If the way is defined by points a->b->c and oneway=-1: Routing is only possible from c to b. The segment b-a is not routable. If the way is defined by points a->b->c->d and oneway=yes: Routing is only possible from a to c. c-d is not routable. If the way is defined by points a->b->c->d and oneway=-1: Routing is only possible from d to c (sic!) |
some of this might be more of a bug in the map than the router: if you go into a oneway that's also a dead end, you can never get out again. in your example, if you go to c, you're stuck. to avoid these 'dead-end' issues, i changed the test to use four nodes for each way, and do the routing between the two middle nodes. it might still be important to test these cases, since such mapping could easily occur in real use. just need to decide what the desired result should be. |
btw, i'm curious how you test with the gui using custom data? |
Good point. But this would apply to the whole dead end way - not just for its last segment. Also note the strange difference on my last example (4 node way, oneway=-1, two segments not routable). Maybe these problems disappear if the oneway is embedded in between normal ways as you described. I can't test it at the moment. Anyway, the difference (1 segment not routable, 2 segments not routable) could be the outcome of a implementation bug, which could be more seriously in other situations. It's just a feeling...
In my opinion osrm should ignore the oneway attribute for such "stuck dead ends", as this seems to be very likely a mapping error. A warning message could help to fix such errors in osm. During my tests with real data i came also across situations like this (arrow describes the oneway direction): -a->b<-c- Also kind of a dead end.
I referenced my testing data on a clearly identifiable coordinate, not (0,0) or (1,1). The way segments also define a evenly spaced "zig zag", which helps to distinguish them. |
yes it seems something odd is going on. i hope to soon have a version of the cucumber test you can try if you want. being able to share failing tests might make it easier to debug |
please test this one again with the latest source. |
ok i will |
as far as i can tell, there's still a problem. but osrm is also returning empty routes, which messes with the cucumber tests. |
According to your pull request with the cucumber tests, you are missing the lines in your branch that revert the path in case of oneway=-1. Did you remove that on purpose? |
no, not on purpose. i suppose there's some confusion on my part due to the branch juggling. |
I reran the example from above and detected no error. Emil, if you have no objections, I'd like to close this issue. |
no problem. if i find any problems, i'll let you know. |
a2a485834 Update CHANGELOG 82f95612e Merge pull request #180 from mapbox/lightmare-move-out c0e7ac6fd Merge pull request #181 from lightmare/revive-result-type d13e61784 Revert "visitor - revive using explicit return type when provided" 0ebf09dea revive using visitor::result_type when available b1076bbee visitor - revive using explicit return type when provided 2e3947578 properly forward through variant::visit b09c7e121 travis: force compiling all tests 0ce49d837 add test case for issue #180 discovered by @artemp 8fc03c4d9 Remove clang 3.7 and gcc47 builds f6e57e9c2 Merge branch 'move-out' of https://github.com/lightmare/variant into lightmare-move-out 94c8ccf54 fix expected compilation error messages 6d21f7704 perfect forwarding in apply_visitor d960916fc implement match on rvalue 8b9eeb238 test matching unwrapped rvalue with lambda expression 77a24b9c0 Merge remote-tracking branch 'upstream/move-out' into move-out 4ce01e1c2 clang - self-assignment is a compile time error c94634bbd Merge pull request #172 from mapbox/self-assignment 3dcac646f updated move assignment as suggested in mapbox/variant#172 (comment) b36c78e12 Just use `assert(this!=&other)` in move assignment operator (https://stackoverflow.com/questions/9322174/move-assignment-operator-and-if-this-rhs) 767bc18f3 Improve self-assignment/move checks to have one return path. 94fc9377e Revert "disable -Wself-assign-overloaded (-Werror) in self-assignment test" da2b171b7 Revert "don't fail old compilers" 7918a4847 don't fail old compilers ad85832b8 disable -Wself-assign-overloaded (-Werror) in self-assignment test 4da455725 add self-assignment checks in copy and move assignment operator= (ref #164) cb02ad487 update CHANGELOG for variant v1.1.6 release [skip ci] a4f87dc69 fix version number ff14f222a update CHANGELOG 0305fdb2a Merge pull request #171 from mapbox/jrex-mute-clang-analyzer 2fef61f08 Moved to in-class initialization 52df2765e update CHANGELOG in preparation for v1.1.6 release 63854e5c9 Add explicit initialization of data to mute clang static analyzer warnings in Xcode (10.2). 0f734f01e Merge pull request #167 from mapbox/clang++4 5a5ecca5b Run ASAN builda in isolated VM via `sudo : required` c1a14e7d9 update mason + update clang++ to 4.0.1 fe0a0666f update mason 11a36a9f1 steady .. downgrade clang++ to 4.0.0 f31bcfb4b try fixing travis via upgrading clang++ from 3.9.1 -> 4.0.1 502e32b8b fix Makefile a64062576 use `ls -lah` as `du -h --apparent-size` is not universally supported. ef3856c85 report actual file size not allocated size. 256ddd555 Merge pull request #160 from mlogan/master 9c81bef8c Fix the noexcept specifications for move assignment and conversion. 5eee328d6 Merge pull request #165 from nick70/master 0888a8e92 Fix README.md issues 859a8c933 Merge pull request #163 from MaxRis/master 215d64585 Removes deprecated static_visitor to avoid msvc C4996 compiler warning 237f83cad Merge pull request #162 from mapbox/variant_alternative 835ebc193 add `variant_size` helper 30560e19e fix preprocessor logic 8b1de3147 add compile index in range check for __type_pack_element branch. ae1931413 add optimized 'variant_alternative' implementation usinh built-in `__type_pack_element` when available (clang++) 3ffef950b add `variant_alternative_t` 3449d00cf alternative implementation of `variant_alternative` 4b98c485c add lost test check + remove stderr 43357808c add intial `variant_alternative` implementation (#161 http://en.cppreference.com/w/cpp/utility/variant/variant_alternative) ba3085a5e use full sha1 75bb549d2 update CHANGELOG (git log <tag1>...<tag2> --pretty=format:'* %s [view commit](http://github.com/mapbox/variant/commit/%H)' --reverse) 6497bce68 add <sha1> to CHANGELOG entries. 555436f71 add CHANGELOG.md skeleton b78b51548 Merge pull request #154 from ricardocosme/forwarding_reference_make_visitor f0b50062b Add copy assignment and move assignment operators. 9f991da78 Use forwarding reference in make_visitor and visitor 266f68d9f Merge pull request #153 from ricardocosme/boost-build 04a6797a6 - Add auxiliar rule exe-test. bd0a2d559 - Use of the module 'os' to get BOOST_DIR. - Add macro SINGLE_THREADED to single threading mode. - Define single threading mode as default. - Add lambda_overload_test and hashable_test. 561a09dd0 - Remove the use of boost libraries. - Add default build. b2471ffc7 - Add a project mapbox_variant. - Use of the 'os' module to capture CXX_STD. - Common configs moved to project. - Built targets moved to 'out' directory. 624720759 add test for ref #147 + mapbox/variant#147 e01b7bf33 Merge branch 'BlueSolei-master' 195367cfc Merge branch 'master' of https://github.com/BlueSolei/variant into BlueSolei-master ea106db54 recursive_wrapper test - avoid constructing new functor in recursive calls, call itself via `this` pointer. 7a541ba10 recursive_wrapper fail to compile when used with 2 classes which are base and derived #146 291121f6a Merge pull request #144 from narizhny/Casts 51fccd755 Add static_variant_cast, dynamic_variant_cast 550ac2f15 Merge pull request #143 from tomhughes/catch a064940e2 REQUIRE_THROWS etc take an expression not a block f9c265d7e Update bundled Catch to v1.9.0 5778eede1 Fixes example: rvalue variant matching cdb9faf0f Simplifies result_of_* and let them handle rvalue refs c5dac859a Failing Test Case: std::move-ing out of variant 916139a2e Merge pull request #141 from mapbox/match-otherwise 3d807d316 Merge pull request #138 from mapbox/sizeof 9ac8978f5 Adds a test for polymorphic lambdas in match, resolves #140 c839c666c add missing <limits> 35487cd39 Make `type_index_t` configurable at compile time via `MAPBOX_VARIANT_MINIMIZE_SIZE` and `MAPBOX_VARIANT_OPTIMIZE_FOR_SPEED`. Default is `unsigned int`. (ref #138) 3f6fd131e Add compile time check to disallow array types as alternatives. fa8e124a2 Ensure internal index type is capable of holding all alternatives (ref #138) 05ee9aca1 use `mapbox::util::type_index_t` (#19) 9eec1fd48 make type used for `type_index` configurable via `type_index_t` typdef + use `unsigned int` by default. This addresses `sizeof` discrepancies between boost/std/mapbox variants (ref #19) d2588a8f1 Trivial missing comma in README example code 05b7612aa Merge pull request #135 from mapbox/llvm-3.9.1 61f8acea1 upgrade mason f5fb4661e upgrade to llvm 3.9.1 5baa948fa fix gyp build 4923eb527 osx: test that will support both latest (10.12) and oldest with c++11 support: 10.7 18a8055fe Merge pull request #134 from lightmare/warnings 5141d8d21 remove useless and/or dubious compiler flags a9707c3de Merge pull request #133 from mapbox/Werror a80beaafc disable -Wparentheses for older gcc c8ec829ff drop -Wstack-protector which gives unhelpful warnings 7b409402c upgrade libstdc++ for coverage build b43398619 Add -pthread 904dcaee6 limit some flags to clang++ 1023f2d9a try without pthreads 886377de0 fortification flags + -pthreads for linux where needed cf9a53499 build in both release and debug on travis 539d71274 fix conversion warnings 253047f53 enable -Werror, suppress warnings from non variant headers using isystem 18919174d Merge pull request #132 from lightmare/avoid-tuple-instantiation 4febf973c avoid expensive instantiation of tuple constructor in noexcept 6317a0b74 re-enable older compilers, trim excess 4fe5ced5d more sanitizer options d1bb6e546 -fsanitize=cfi and -fsanitize=safe-stack 20d693ed9 fix LDFLAGS 9b2de4546 test with clang++ sanitizers and flto 702826365 disable clang++ 3.9, will work on getting working in a branch e07a533a8 fix clang++ PATH 84eeb54c9 test clang++ via mason a760cea8d upgrade mason b9c58d631 upgrade boost to 1.62.0 c81b475b4 makefile improvements ce2eea644 travis: fix addons efa75df27 test with clang 3.9 and g++-6 cb5635ba2 add package.json for publishing to npm 02bd1ac4c Merge pull request #129 from daniel-j-h/docs ed84def12 Merge pull request #128 from daniel-j-h/match 3c17c37ae Merge pull request #126 from daniel-j-h/hashable d0266436b Adds Documentation for Readme, resolves #98 720c23736 Implements Pattern Matching for Sum Types via `.match` Member Function. 97d0379f0 Makes variant<Ts...> hashable iff Ts... are hashable, closes #125 9a115c5eb Merge branch 'daniel-j-h-lambda-visitor' 4d462f27b Adds C++14 SFINAE Test 2275a6197 Removes ::type Usage d09188640 Provides Convenient Lambda Overload Visitor Interface, resolves #113. a5a79a594 Fix #122 by adding an extra compile check in universal ctor (via @lightmare) + test case 9b46167f5 nicer stderr 84a426a31 Merge pull request #120 from mapbox/types 173a74579 add `struct adapted_variant_tag;` e5818212a expose `using types = std::tuple<Types...>;` - useful for adapting variant to `boost::spirit` (QI,Karma,X3) aaddee927 Update README 8e2f69641 Merge pull request #116 from lightmare/disjunction 2c7ddecdb use C++17 disjunction for no-references and one-convertible tests 388376ac9 Merge pull request #114 from mapbox/strict-conversions 075d9636f comment out code 8be6a2aa8 update tests 71ac8fdf9 Re-implement type matching logic to reject ambigious conversions c511b2f34 add test for b3a002d185afac295486e2ebd6b84c78a2267ba0 (ref #112) b3a002d18 fix value_traits to be able to match T, T& and T const& to the direct type stored in variant (ref #112) b5728ad76 update .mason pkgs eedafd31f use local HAS_EXCEPTIONS #define (__EXCEPTIONS is g++/clang specific macro) 372d7c88f c++ apply formatting 20e44accb Merge pull request #110 from mapbox/110-get_unchecked 37acc5a7c uncomment tests ref #82 adf0e02bc variant - yield return type of mapbox::util::get<T> automatically and make interface consistent (addresses #82) bb8c2d203 Merge branch '111-which-constexpr' dca3d967c Merge branch 'master' into 111-which-constexpr 74ce146d9 add static which<T>() function to get a contained types' which value 48d60445c remove unused internal metafunctions 434dab048 Add get_unchecked<T>() to enable use with exceptions disabled 2f8a4a381 Merge pull request #109 from mapbox/darwin-build-flags 33e27ec4c Update README.md 55579f03f Fix building with GCC (g++-5.2.0) on OS X (Darwin) (ref #108) 8bdad6b6d Update README.md 7f7470fee Jamroot - add missing include directory ./test/include for auto_cpu_timer.hpp 4368d7529 remove expected error string - current implementation emits compiler specific error message e.g c6ae1ea0a `is<T>()` - add specialisation for recursive_wrapper<T> + update tests (ref #102) 04dc3a46b Install boost with mason; eliminate boost::timer dependency 9b2fc858c Remove Xcode 6 from CI matrix 1bc46e525 Merge pull request #101 from mapbox/include 390229a59 fix compilation bfe0f19dd update remaining `<variant.hpp>` to `<mapbox/variant.hpp>` 343831611 ammend include dir a606e9024 fix typo 9bd902536 Merge branch 'master' into include 7e4a01189 Add include directory 13c631a62 Update README.md f00b24bf6 move headers into include/mapbox folder - closes #99 35ca16c74 issue warning `-Wweak-vtables` so this issue is not forgotten (mapbox/variant#95) 82bb901b6 run coverage with clang 3.5 - fix clang 3.8 build 5f6ed7149 remove invalid option for llvm-cov f034d5571 fix clang 3.8 compile, try 3.9 b0ee4729b fix coverage to avoid warning: unit.gcno:version '402*', prefer '406*' 3f025adbf remove erroneous `;` ref #96 git-subtree-dir: third_party/variant git-subtree-split: a2a4858345423a760eca300ec42acad1ad123aa3
handling of oneway=1 works.
but it seems handling of -1 is somehow not working, despite the code in ExtractorCallbacks.h?
Failed Scenario: Oneway street
Time: 2012-02-05T14:02:24Z
[test]
cycleway = 19
trunk = 19
trunk_link = 19
primary = 19
primary_link = 19
secondary = 17
secondary_link = 17
tertiary = 15
residential = 15
unclassified = 15
living_street = 13
service = 12
track = 12
path = 12
footway = 10
pedestrian = 5
pier = 5
steps = 3
ferry = 5
obeyOneways = yes
useRestrictions = yes
accessTag = bicycle
excludeFromGrid = ferry
defaultSpeed = 17
obeyBollards = no
Expected: {"from"=>"a", "to"=>"c", "route"=>""}
Got: {"from"=>"a", "to"=>"c", "route"=>"abc"}
Query: http://localhost:5000/viaroute&start=1.0,1.0&dest=1.0,1.002&output=json&geomformat=cmp
Response: {"version": 0.3,"status":0,"status_message": "Found route between points","route_summary": {"total_distance":110,"total_time":22,"start_point":"abc","end_point":"abc"},"route_geometry": "_ibE_ibE?gE","route_instructions": [["Head","abc",111,0,210,"0","NE",22.5]],"via_points":[],"transactionId": "OSRM Routing Engine JSON Descriptor (v0.3)"}
Parsed:
["version", 0.3]
["status", 0]
["status_message", "Found route between points"]
["route_summary", {"total_distance"=>110, "total_time"=>22, "start_point"=>"abc", "end_point"=>"abc"}]
["route_geometry", "_ibE_ibE?gE"]
["route_instructions", [["Head", "abc", 111, 0, 210, "0", "NE", 22.5]]]
["via_points", []]
["transactionId", "OSRM Routing Engine JSON Descriptor (v0.3)"]
Expected: {"from"=>"c", "to"=>"a", "route"=>"abc"}
Got: {"from"=>"c", "to"=>"a", "route"=>""}
Query: http://localhost:5000/viaroute&start=1.0,1.002&dest=1.0,1.0&output=json&geomformat=cmp
Response: {"version": 0.3,"status":207,"status_message": "Cannot find route between points","route_summary": {"total_distance":0,"total_time":214748365,"start_point":"unknown street","end_point":"unknown street"},"route_geometry": "","route_instructions": [],"via_points":[],"transactionId": "OSRM Routing Engine JSON Descriptor (v0.3)"}
Parsed:
["version", 0.3]
["status", 207]
["status_message", "Cannot find route between points"]
["route_summary", {"total_distance"=>0, "total_time"=>214748365, "start_point"=>"unknown street", "end_point"=>"unknown street"}]
["route_geometry", ""]
["route_instructions", []]
["via_points", []]
["transactionId", "OSRM Routing Engine JSON Descriptor (v0.3)"]
The text was updated successfully, but these errors were encountered: