organicmaps/organicmaps-tmp
Archived
13
Fork 1
Code Issues 2.7k Pull requests 182 Projects Releases 65 Wiki Activity Actions

Adjust bicycle routing weights and ETAs #9692

Merged
root merged 7 commits from pastk-routing-cycling into master 2025-02-07 14:14:13 +00:00
Conversation 0 Commits 7 Files changed 4 +294 -125
pastk commented 2024-11-19 20:27:28 +00:00
Member
Copy link

A big review of weights and ETAs.

I've reviewed all the current integration tests - seems like some of them lost their value over time due to map changes or too similar to other tests. @vng I want to proceed and delete such tests, but first please review my comments in case I've missed something.

Then I've added a bunch of new tests.

Out of scope: some cases are not fixable by weights alone

Cases solved:

Improves:

A big review of weights and ETAs. I've reviewed all the current integration tests - seems like some of them lost their value over time due to map changes or too similar to other tests. @vng I want to proceed and delete such tests, but first please review my comments in case I've missed something. Then I've added a bunch of new tests. Out of scope: some cases are not fixable by weights alone - https://git.omaps.dev/organicmaps/organicmaps/issues/9748 Cases solved: - closes https://git.omaps.dev/organicmaps/organicmaps/issues/7934 - closes https://git.omaps.dev/organicmaps/organicmaps/issues/7954 - closes https://git.omaps.dev/organicmaps/organicmaps/issues/1772 - closes https://git.omaps.dev/organicmaps/organicmaps/issues/4059 - closes https://git.omaps.dev/organicmaps/organicmaps/issues/6027 Improves: - https://git.omaps.dev/organicmaps/organicmaps/issues/3316#issuecomment-2452680022 - https://git.omaps.dev/organicmaps/organicmaps/issues/2253 - https://git.omaps.dev/organicmaps/organicmaps/issues/1201#issuecomment-946042937
👍 4 👎 1 ❤️ 4 🚀 4
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:20:42 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-20 06:20:41 +00:00

detects horrific regressions that may be missed by trickier tests?

detects horrific regressions that may be missed by trickier tests?
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:22:16 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-20 06:22:15 +00:00

From "CyclewayPriority" it sounds like it tries to confirm that cycleway is treated as not worse than alternatives? though maybe case where cycleway is a bit longer would be better? Can find some case for that (with use_sidepath on road)

From "CyclewayPriority" it sounds like it tries to confirm that cycleway is treated as not worse than alternatives? though maybe case where cycleway is a bit longer would be better? Can find some case for that (with use_sidepath on road)
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:25:03 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -157,4 +233,4 @@
mercator::FromLatLon(28.15865, -16.63704));
TEST_EQUAL(res.second, RouterResultCode::NoError, ());
TestRouteLength(*res.first, 26401);
matkoniecz (Migrated from github.com) commented 2024-11-20 06:25:03 +00:00

this is AFAIK typically allowed also elsewhere? At least on service which is not driveway

this is AFAIK typically allowed also elsewhere? At least on service which is not `driveway`
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:30:18 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -150,4 +225,3 @@
}
// Test on riding up from Adeje (sea level) to Vilaflor (altitude 1400 meters).
UNIT_TEST(SpainTenerifeAdejeVilaflor)
matkoniecz (Migrated from github.com) commented 2024-11-20 06:30:18 +00:00

I would say that https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.082166%2C20.033060%3B50.082567%2C20.031853 is not really worse

I actually prefer it if I would arrive at phase when Kocmyrzowska has green and Obrońców Krzyża is waiting so you can go onto road and start waiting for green (and if you have no tolerance to car traffic you would not go into this road but detour via parks)

It is caused by fact that cycleway has two separate traffic lights - and you may be caught between two traffic lights between carriageways, while if you are in the middle of the crossing on road you can leave it

  // Same as GraphHopper. Valhalla suggests going by the road (worse for some cyclists, but depending on light phase it may be preferable for at least some cyclists. With such route changing light will not force cyclist to wait between two carriageways and they would be able to leave crossing).
I would say that https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.082166%2C20.033060%3B50.082567%2C20.031853 is not really worse I actually prefer it if I would arrive at phase when Kocmyrzowska has green and Obrońców Krzyża is waiting so you can go onto road and start waiting for green (and if you have no tolerance to car traffic you would not go into this road but detour via parks) It is caused by fact that cycleway has two separate traffic lights - and you may be caught between two traffic lights between carriageways, while if you are in the middle of the crossing on road you can leave it ```suggestion // Same as GraphHopper. Valhalla suggests going by the road (worse for some cyclists, but depending on light phase it may be preferable for at least some cyclists. With such route changing light will not force cyclist to wait between two carriageways and they would be able to leave crossing). ```
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:33:34 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -150,4 +225,3 @@
}
// Test on riding up from Adeje (sea level) to Vilaflor (altitude 1400 meters).
UNIT_TEST(SpainTenerifeAdejeVilaflor)
matkoniecz (Migrated from github.com) commented 2024-11-20 06:33:34 +00:00 
  /// @todo(pastk): there are no detours here atm, the second route is just a tad longer
```suggestion /// @todo(pastk): there are no detours here atm, the second route is just a tad longer ```
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:35:19 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
matkoniecz (Migrated from github.com) commented 2024-11-20 06:35:18 +00:00 
/// @todo(pastk): find a good "avoid trunk" test case where trunk allows cycling.
```suggestion /// @todo(pastk): find a good "avoid trunk" test case where trunk allows cycling. ```
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:36:08 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-20 06:36:07 +00:00 
  // and it doesn't affect routing as there is no explicit bicycle=no.
  /// @todo(pastk): such barrier should have a small penalty in routing.
```suggestion // and it doesn't affect routing as there is no explicit bicycle=no. /// @todo(pastk): such barrier should have a small penalty in routing. ```
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:38:40 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
matkoniecz (Migrated from github.com) commented 2024-11-20 06:38:40 +00:00

maybe having synthetic tests would be better? Rather than specifying location, specify small routing graph/OSM data within test itself?

or specify date and use attic data to fetch OSM data frozen on some specific date? It would prevent tests rotting because someone changed OSM data (at cost of not spotting that preferred tagging changed for something, but this seems much less valuable and should be rare and deprecationists will usually create an issue to complain)

maybe having synthetic tests would be better? Rather than specifying location, specify small routing graph/OSM data within test itself? or specify date and use attic data to fetch OSM data frozen on some specific date? It would prevent tests rotting because someone changed OSM data (at cost of not spotting that preferred tagging changed for something, but this seems much less valuable and should be rare and deprecationists will usually create an issue to complain)
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:43:43 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-20 06:43:43 +00:00

I would dispute "not meant for passing through". And even ones built for car traffic and not designed for passing through can be still very useful for cyclists as cycleway substitute due to typically lower traffic.

this is based on extensive experience from Poland where https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.067527%2C20.003867%3B50.068148%2C20.004073 is normal and using service for bypasses - either intended by road design or not - is often a great idea

can list more examples

I would dispute "not meant for passing through". And even ones built for car traffic and not designed for passing through can be still very useful for cyclists as cycleway substitute due to typically lower traffic. this is based on extensive experience from Poland where https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.067527%2C20.003867%3B50.068148%2C20.004073 is normal and using service for bypasses - either intended by road design or not - is often a great idea can list more examples
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:45:37 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-20 06:45:37 +00:00

is there bonus for having a bicycle ramp? And maybe smaller for stroller/wheelchair one?

is there bonus for having a bicycle ramp? And maybe smaller for stroller/wheelchair one?
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:46:22 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-20 06:46:21 +00:00 
  // But why? If nocycleway is tagged explicitly means there is no cycling infra for sure.
```suggestion // But why? If nocycleway is tagged explicitly means there is no cycling infra for sure. ```
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:48:46 +00:00
routing_common/bicycle_model.cpp
@ -56,2 +58,4 @@
{HighwayType::HighwayCycleway, InOutCitySpeedKMpH(SpeedKMpH(21.0, 18.0), SpeedKMpH(23.0, 20.0))},
{HighwayType::HighwayLivingStreet, InOutCitySpeedKMpH(SpeedKMpH(12.0, 10.0), SpeedKMpH(14.0, 12.0))},
// Steps have obvious inconvenience of a bike in hands.
{HighwayType::HighwaySteps, InOutCitySpeedKMpH(SpeedKMpH(1.0, 1.0))},
matkoniecz (Migrated from github.com) commented 2024-11-20 06:48:46 +00:00

what are units for SpeedKMpH?

I tried looking at https://github.com/search?q=repo%3Aorganicmaps%2Forganicmaps+SpeedKMpH&type=code but managed to only learn that first is m_weight and second is m_eta (I guess to note that something may be faster but still less preferable - like cycling on trunk)

what are units for SpeedKMpH? I tried looking at https://github.com/search?q=repo%3Aorganicmaps%2Forganicmaps+SpeedKMpH&type=code but managed to only learn that first is m_weight and second is m_eta (I guess to note that something may be faster but still less preferable - like cycling on trunk)
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:50:00 +00:00
routing_common/bicycle_model.cpp
@ -56,2 +58,4 @@
{HighwayType::HighwayCycleway, InOutCitySpeedKMpH(SpeedKMpH(21.0, 18.0), SpeedKMpH(23.0, 20.0))},
{HighwayType::HighwayLivingStreet, InOutCitySpeedKMpH(SpeedKMpH(12.0, 10.0), SpeedKMpH(14.0, 12.0))},
// Steps have obvious inconvenience of a bike in hands.
{HighwayType::HighwaySteps, InOutCitySpeedKMpH(SpeedKMpH(1.0, 1.0))},
matkoniecz (Migrated from github.com) commented 2024-11-20 06:50:00 +00:00

f2dcb90b21/routing_common/vehicle_model.hpp (L108) notes "KMpH"

are both in kilometers per hour? and ETA has real km/h while weight has a bit fake one, to adjust for desirability?

https://github.com/organicmaps/organicmaps/blob/f2dcb90b21829bf75b19e8b83e3d5d307bf2b648/routing_common/vehicle_model.hpp#L108 notes "KMpH" are both in kilometers per hour? and ETA has real km/h while weight has a bit fake one, to adjust for desirability?
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:55:00 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-20 06:55:00 +00:00

one more cases where penalty for points is needed, though tiny as these are often on preferred cycling routes

one more cases where penalty for points is needed, though tiny as these are often on preferred cycling routes
matkoniecz (Migrated from github.com) reviewed 2024-11-20 06:56:38 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -150,4 +225,3 @@
}
// Test on riding up from Adeje (sea level) to Vilaflor (altitude 1400 meters).
UNIT_TEST(SpainTenerifeAdejeVilaflor)
matkoniecz (Migrated from github.com) commented 2024-11-20 06:56:38 +00:00

may be too much detail...

  // Same as GraphHopper. Valhalla suggests going by the road (worse for cyclists really preferring to avoid cars whenever possible, may be preferred for more aggressive ones depending on light phase to go through one traffic light only).
may be too much detail... ```suggestion // Same as GraphHopper. Valhalla suggests going by the road (worse for cyclists really preferring to avoid cars whenever possible, may be preferred for more aggressive ones depending on light phase to go through one traffic light only). ```
pastk reviewed 2024-11-20 15:23:54 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2024-11-20 15:23:54 +00:00
Author
Member

image

If something goes so horrific wrong to break this case, then I'm sure it'll break many other tests also :)

![image](https://github.com/user-attachments/assets/2744ee3e-ac5b-4bba-add4-d254f2dcede0) If something goes so horrific wrong to break this case, then I'm sure it'll break many other tests also :)
pastk reviewed 2024-11-20 15:27:24 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -157,4 +233,4 @@
mercator::FromLatLon(28.15865, -16.63704));
TEST_EQUAL(res.second, RouterResultCode::NoError, ());
TestRouteLength(*res.first, 26401);
pastk commented 2024-11-20 15:27:23 +00:00
Author
Member

@vng should know the context better here

@vng should know the context better here
pastk reviewed 2024-11-20 15:34:21 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -150,4 +225,3 @@
}
// Test on riding up from Adeje (sea level) to Vilaflor (altitude 1400 meters).
UNIT_TEST(SpainTenerifeAdejeVilaflor)
pastk commented 2024-11-20 15:34:21 +00:00
Author
Member

Actually the speed/distance difference is very minor here, reducing the cycleway weight by just 1 point makes OM take the road. (also the cycleway option has 1m altitude difference?)

The preferred OM route might change after we add support for turns, crossings and lights.

Actually the speed/distance difference is very minor here, reducing the cycleway weight by just 1 point makes OM take the road. (also the cycleway option has 1m altitude difference?) The preferred OM route might change after we add support for turns, crossings and lights.
pastk reviewed 2024-11-20 15:37:35 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2024-11-20 15:37:35 +00:00
Author
Member

good point!

good point!
pastk reviewed 2024-11-20 15:40:32 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
pastk commented 2024-11-20 15:40:32 +00:00
Author
Member

There are some tests with embedded map data, though maybe not routing tests.
I guess its just too time-consuming to set them up.
Right, @vng ?

There are some tests with embedded map data, though maybe not routing tests. I guess its just too time-consuming to set them up. Right, @vng ?
pastk reviewed 2024-11-20 15:43:24 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-20 15:43:24 +00:00
Author
Member

Yeap my comment is not clear enough, so I've just removed it.

Actually I was quite surprised to find that service roads seems to be commonly used to map in countryside in Germany, see the Germany_UseServiceCountrysideRoads case.
I would have expected either unclassifieds or grade1 tracks...

Yeap my comment is not clear enough, so I've just removed it. Actually I was quite surprised to find that service roads seems to be commonly used to map in countryside in Germany, see the Germany_UseServiceCountrysideRoads case. I would have expected either unclassifieds or grade1 tracks...
pastk reviewed 2024-11-20 15:45:03 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-20 15:45:02 +00:00
Author
Member

Nope its not processed yet.

Also the direction (up/down) of stairs should be taken in account. ATM its not, right @vng ?

Nope its not processed yet. Also the direction (up/down) of stairs should be taken in account. ATM its not, right @vng ?
pastk reviewed 2024-11-20 15:46:29 +00:00
routing_common/bicycle_model.cpp
@ -56,2 +58,4 @@
{HighwayType::HighwayCycleway, InOutCitySpeedKMpH(SpeedKMpH(21.0, 18.0), SpeedKMpH(23.0, 20.0))},
{HighwayType::HighwayLivingStreet, InOutCitySpeedKMpH(SpeedKMpH(12.0, 10.0), SpeedKMpH(14.0, 12.0))},
// Steps have obvious inconvenience of a bike in hands.
{HighwayType::HighwaySteps, InOutCitySpeedKMpH(SpeedKMpH(1.0, 1.0))},
pastk commented 2024-11-20 15:46:29 +00:00
Author
Member

Yeap, exactly!

Yeap, exactly!
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:13:43 +00:00
routing_common/bicycle_model.cpp
@ -56,2 +58,4 @@
{HighwayType::HighwayCycleway, InOutCitySpeedKMpH(SpeedKMpH(21.0, 18.0), SpeedKMpH(23.0, 20.0))},
{HighwayType::HighwayLivingStreet, InOutCitySpeedKMpH(SpeedKMpH(12.0, 10.0), SpeedKMpH(14.0, 12.0))},
// Steps have obvious inconvenience of a bike in hands.
{HighwayType::HighwaySteps, InOutCitySpeedKMpH(SpeedKMpH(1.0, 1.0))},
matkoniecz (Migrated from github.com) commented 2024-11-21 06:13:43 +00:00

I created #9708 to avoid such confusion for others

I created #9708 to avoid such confusion for others
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:14:17 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:14:17 +00:00

hmm, I would say that going up and down with bicycle is nearly as obnoxious

hmm, I would say that going up and down with bicycle is nearly as obnoxious
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:27:21 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:27:21 +00:00

Opened "for cyclists steps with ramps are much better than steps without ramps" #9709

Opened "for cyclists steps with ramps are much better than steps without ramps" #9709
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:27:54 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:27:54 +00:00

I would have expected either unclassifieds or grade1 tracks...

I expect that many are grade1 tracks tagged for renderer

> I would have expected either unclassifieds or grade1 tracks... I expect that many are grade1 tracks tagged for renderer
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:29:58 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
matkoniecz (Migrated from github.com) commented 2024-11-21 06:29:58 +00:00

I guess its just too time-consuming to set them up.

if it is possible than scripting extraction from OSM data should be fairly easy

or maybe even save bunch of .osm files

(StreetComplete has small synthetic tests as when it was using live OSM data tests were getting broken all the time resulting in them being ignored, if someone points me to format/example code I can prepare some for bicycle routing)

> I guess its just too time-consuming to set them up. if it is possible than scripting extraction from OSM data should be fairly easy or maybe even save bunch of .osm files (StreetComplete has small synthetic tests as when it was using live OSM data tests were getting broken all the time resulting in them being ignored, if someone points me to format/example code I can prepare some for bicycle routing)
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:31:21 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:31:21 +00:00

do you want a replacement for "a bit longer cycleway is better than road"?

do you want a replacement for "a bit longer cycleway is better than road"?
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:34:25 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:34:25 +00:00

maybe it was assumed that "no cycleway here" will be more often tagged in places which were expected to have cycleway like busy road?

but it is fairly bad assumption IMHO

(or maybe reason was different? is there any cycleway that is not getting bonus but would be not hit by this penalty? like sharrows? but getting bonus to sharrows more likely to be on major busy roads would be also silly)

maybe it was assumed that "no cycleway here" will be more often tagged in places which were expected to have cycleway like busy road? but it is fairly bad assumption IMHO (or maybe reason was different? is there any cycleway that is not getting bonus but would be not hit by this penalty? like sharrows? but getting bonus to sharrows more likely to be on major busy roads would be also silly)
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:36:30 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:36:30 +00:00

maybe it can be reduced further? primary road may be fairly small and calm, but if there is primary_link it indicates large intersection terrible for cyclists

we cannot easily extrapolate it on primary road itself but maybe penalising at least link section would be nice

maybe it can be reduced further? primary road may be fairly small and calm, but if there is `primary_link` it indicates large intersection terrible for cyclists we cannot easily extrapolate it on primary road itself but maybe penalising at least link section would be nice
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:40:17 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:40:17 +00:00

why living street is so heavily penalised? more than highway=primary ?

maybe real speed will be fairly low (but still greater than 8, I bet) but I would definitely take slower driving on highway=living_street over highway=primary.

And I am cyclist who on occasion uses overpasses on dual carriageway highway=primary (not anymore since proper cycleway exists now). And I sometimes cycle (legally) on roads with 5 lanes in each direction rather than on sidewalk (illegally).
I bet that 99%+ of people will prefer living_street over primary, 90%+ over secondary, 80% over tertiary.

some highway=living_street will be extremely busy with pedestrians, but most are great for cycling (and ones most crowded will often be bicycle=no anyway)

    {HighwayType::HighwayLivingStreet, InOutCitySpeedKMpH(SpeedKMpH(13.0, 10.0))},

this matches highway=secondary in desirability and path/bridleway in speed

why living street is so heavily penalised? more than `highway=primary` ? maybe real speed will be fairly low (but still greater than 8, I bet) but I would definitely take slower driving on `highway=living_street` over `highway=primary`. And I am cyclist who on occasion uses overpasses on dual carriageway `highway=primary` (not anymore since proper cycleway exists now). And I sometimes cycle (legally) on roads with 5 lanes in each direction rather than on sidewalk (illegally). I bet that 99%+ of people will prefer `living_street` over `primary`, 90%+ over secondary, 80% over tertiary. some `highway=living_street` will be extremely busy with pedestrians, but most are great for cycling (and ones most crowded will often be `bicycle=no` anyway) ```suggestion {HighwayType::HighwayLivingStreet, InOutCitySpeedKMpH(SpeedKMpH(13.0, 10.0))}, ``` this matches `highway=secondary` in desirability and path/bridleway in speed
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:40:54 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:40:54 +00:00 
    {HighwayType::HighwayLivingStreet, InOutCitySpeedKMpH(SpeedKMpH(14.0, 10.0))},

this matches highway=tertiary in desirability and path/bridleway in speed

```suggestion {HighwayType::HighwayLivingStreet, InOutCitySpeedKMpH(SpeedKMpH(14.0, 10.0))}, ``` this matches highway=tertiary in desirability and path/bridleway in speed
matkoniecz (Migrated from github.com) reviewed 2024-11-21 06:52:34 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 06:52:34 +00:00

or maybe give larger boost

I would say that after cycleway, good track, good service road, living street with asphalt surface is the next in order of preferred road types.

or maybe give larger boost I would say that after cycleway, good track, good service road, living street with asphalt surface is the next in order of preferred road types.
pastk reviewed 2024-11-21 08:12:30 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2024-11-21 08:12:30 +00:00
Author
Member

Yes please!

Yes please!
pastk reviewed 2024-11-21 08:20:43 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
pastk commented 2024-11-21 08:20:43 +00:00
Author
Member

There are many *.osm samples in data/test_data/osm/. Check their usage in the code.

If we'll use such excerpts for routing then how would we visualize them and build routes and reason about?
Right now check the test in the desktop app - its more or less easy to reason which way is better, try to put intermediate points to check alternatives, check output of alternative routers on osm.org...

Maybe synt tests will be suitable for strict yes/no tests only like - a constructed road is a no go, cycleway=opposite should allow going in any direction, etc.

There are many *.osm samples in `data/test_data/osm/`. Check their usage in the code. If we'll use such excerpts for routing then how would we visualize them and build routes and reason about? Right now check the test in the desktop app - its more or less easy to reason which way is better, try to put intermediate points to check alternatives, check output of alternative routers on osm.org... Maybe synt tests will be suitable for strict yes/no tests only like - a constructed road is a no go, cycleway=opposite should allow going in any direction, etc.
matkoniecz (Migrated from github.com) reviewed 2024-11-21 08:33:09 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 08:33:09 +00:00

https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.076924%2C19.917848%3B50.074458%2C19.910502#map=17/50.075451/19.914844&layers=N (this one misses even North-South shared sidewalk!)

screen06

https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.076924%2C19.917848%3B50.074480%2C19.910488#map=18/50.075936/19.914098&layers=N

screen07

https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.076924%2C19.917848%3B50.074458%2C19.910502#map=17/50.075451/19.914844&layers=N (this one misses even North-South shared sidewalk!) ![screen06](https://github.com/user-attachments/assets/c39e14ea-b4e6-4dfc-979d-bd883d144cef) https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.076924%2C19.917848%3B50.074480%2C19.910488#map=18/50.075936/19.914098&layers=N ![screen07](https://github.com/user-attachments/assets/ef7f3999-b43b-4b1f-8698-b7ab1d639eb5)
matkoniecz (Migrated from github.com) reviewed 2024-11-21 08:37:31 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 08:37:31 +00:00

more bordeline

https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.043813%2C20.016456%3B50.047522%2C20.029986

screen08

https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.043813%2C20.016456%3B50.047522%2C20.029986#map=17/50.046302/20.023109&layers=N

screen09

more bordeline https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.043813%2C20.016456%3B50.047522%2C20.029986 ![screen08](https://github.com/user-attachments/assets/8d1bd8bc-0164-406d-8490-32e88f1993c5) https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.043813%2C20.016456%3B50.047522%2C20.029986#map=17/50.046302/20.023109&layers=N ![screen09](https://github.com/user-attachments/assets/4ac85516-e608-43ac-8941-731702609f6b)
matkoniecz (Migrated from github.com) reviewed 2024-11-21 08:39:20 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 08:39:20 +00:00

this one has contraflow on service road as cycleway substitute

https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.036180%2C19.982480%3B50.040088%2C19.983348#map=17/50.038544/19.985375&layers=N

screen10

https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.036180%2C19.982480%3B50.040088%2C19.983348#map=17/50.038172/19.982790&layers=N

screen11

this one has contraflow on service road as cycleway substitute https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.036180%2C19.982480%3B50.040088%2C19.983348#map=17/50.038544/19.985375&layers=N ![screen10](https://github.com/user-attachments/assets/efefe4b3-cf0b-4302-90e6-04df2ebe1269) https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.036180%2C19.982480%3B50.040088%2C19.983348#map=17/50.038172/19.982790&layers=N ![screen11](https://github.com/user-attachments/assets/31a6e9e9-09ec-4507-b914-82a8f9e54ef4)
matkoniecz (Migrated from github.com) reviewed 2024-11-21 08:43:17 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 08:43:17 +00:00

https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.031478%2C19.948912%3B50.036289%2C19.941198#map=17/50.033927/19.945056&layers=N

screen14

https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.031478%2C19.948912%3B50.036289%2C19.941198#map=17/50.033927/19.945164&layers=N
screen15

https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.031478%2C19.948912%3B50.036289%2C19.941198#map=17/50.033927/19.945056&layers=N ![screen14](https://github.com/user-attachments/assets/4093d659-722a-45d4-a0f0-eb480a71b905) https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.031478%2C19.948912%3B50.036289%2C19.941198#map=17/50.033927/19.945164&layers=N ![screen15](https://github.com/user-attachments/assets/8a8d5561-b74f-4a7c-9f23-cf880fadd146)
matkoniecz (Migrated from github.com) reviewed 2024-11-21 08:43:45 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 08:43:45 +00:00

should be enough, I think :)

I can give more examples of specific thing from Kraków

in all cases fixed routing is below.

I see that Valhalla really hates cycleways :)

should be enough, I think :) I can give more examples of specific thing from Kraków in all cases fixed routing is below. I see that Valhalla really hates cycleways :)
matkoniecz (Migrated from github.com) reviewed 2024-11-21 08:46:09 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
matkoniecz (Migrated from github.com) commented 2024-11-21 08:46:09 +00:00

I thought about bunch of small obvious tests like "point A, point B, 100m long highway=living_street, 80m long highway=primary, living street should be selected" (can be done with 4 nodes and two ways in .osm format)

that in comment could link to issue/real A to B routing

I thought about bunch of small obvious tests like "point A, point B, 100m long highway=living_street, 80m long highway=primary, living street should be selected" (can be done with 4 nodes and two ways in .osm format) that in comment could link to issue/real A to B routing
pastk reviewed 2024-11-21 09:49:02 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-21 09:49:02 +00:00
Author
Member

There is some context in organicmaps/organicmaps#5192 and https://github.com/orgs/organicmaps/discussions/5158#discussioncomment-5938807

I found several cases where the 0.8 factor is contributing to weird detours.

I'm not 100% sure its better to remove the adjustment completely though. Maybe a small factor like 0.95 is beneficial.

Its like assuming that e.g. an explicit smoothness=good surface=asphalt footway is a better bet than a footway with no additional tags. WDYT?

There is some context in https://git.omaps.dev/organicmaps/organicmaps/pulls/5192#discussion_r1826000716 and https://github.com/orgs/organicmaps/discussions/5158#discussioncomment-5938807 I found several cases where the 0.8 factor is contributing to weird detours. I'm not 100% sure its better to remove the adjustment completely though. Maybe a small factor like 0.95 is beneficial. Its like assuming that e.g. an explicit `smoothness=good surface=asphalt` footway is a better bet than a footway with no additional tags. WDYT?
pastk reviewed 2024-11-21 09:52:39 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-21 09:52:38 +00:00
Author
Member

it sounds reasonable, it'd be great to have a test case 🙏

in my experience link roads (esp. on/off ramps) are also narrower and no shoulder

it sounds reasonable, it'd be great to have a test case :pray: in my experience link roads (esp. on/off ramps) are also narrower and no shoulder
matkoniecz (Migrated from github.com) reviewed 2024-11-21 09:57:38 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 09:57:38 +00:00

Is there any known case at all where this extra complexity helps? And this 0.8 is a massive penalty.

(61.5445696, 23.9394003) -> (61.6153965, 23.876755) was mentioned, I guess it is https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=61.54%2C23.94%3B61.62%2C23.88#map=13/61.57544/23.89835&layers=N

but it surely can be achieved by looking at highway=secondary maxspeed=80 and highway=tertiary maxspeed=50, not at effectively random "where missing cycleway was mapped"

Is there any known case at all where this extra complexity helps? And this 0.8 is a massive penalty. (61.5445696, 23.9394003) -> (61.6153965, 23.876755) was mentioned, I guess it is https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=61.54%2C23.94%3B61.62%2C23.88#map=13/61.57544/23.89835&layers=N but it surely can be achieved by looking at `highway=secondary maxspeed=80` and `highway=tertiary maxspeed=50`, not at effectively random "where missing cycleway was mapped"
pastk reviewed 2024-11-21 10:00:26 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-21 10:00:26 +00:00
Author
Member

Sounds reasonable, I'll try to adjust and see if the tests are good. Would be great also to add specific tests for living_street preference/avoidance cases.

IDK the reason it was penalized before, I can only guess it stems from experience of living_streets in ex-USSR countries used predominantly for inside-the-block small and winding access roads.

@vng might know more.

Sounds reasonable, I'll try to adjust and see if the tests are good. Would be great also to add specific tests for living_street preference/avoidance cases. IDK the reason it was penalized before, I can only guess it stems from experience of living_streets in ex-USSR countries used predominantly for inside-the-block small and winding access roads. @vng might know more.
pastk reviewed 2024-11-21 10:03:43 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
pastk commented 2024-11-21 10:03:43 +00:00
Author
Member

yeap sounds good!

yeap sounds good!
matkoniecz (Migrated from github.com) reviewed 2024-11-21 10:08:39 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 10:08:38 +00:00

I have nothing obvious, on Poland for whatever reason primary is always used instead of primary_link

I have nothing obvious, on Poland for whatever reason `primary` is always used instead of `primary_link`
matkoniecz (Migrated from github.com) reviewed 2024-11-21 10:21:20 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 10:21:20 +00:00

meandering roads should be solved by itself due to increased length...

bad: https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.053595%2C19.948463%3B50.050640%2C19.950151#map=17/50.052076/19.949938&layers=N
good: https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.053595%2C19.948463%3B50.050640%2C19.950151#map=17/50.052062/19.949316&layers=N
(going together with trams, busses and heavy traffic is clearly inferior)

https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.006085%2C19.962158%3B50.006664%2C19.957639#map=18/50.006384/19.959900&layers=N
currently OM wants to do detour via south (warning: not checked this area in person, looks like a good test based on mapped data)

https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.020758%2C20.049156%3B50.022031%2C20.048289 (OM wants illegally drive on sidewalk, GraphHopper wants to go on route that is of lower quality but balanced by being longer :) )

looks like good test case, never was there:

https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.096027%2C19.904330%3B50.099875%2C19.889867
should be
https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.096008%2C19.904225%3B50.099847%2C19.889835

(let me know if screens are also needed)

meandering roads should be solved by itself due to increased length... bad: https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.053595%2C19.948463%3B50.050640%2C19.950151#map=17/50.052076/19.949938&layers=N good: https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.053595%2C19.948463%3B50.050640%2C19.950151#map=17/50.052062/19.949316&layers=N (going together with trams, busses and heavy traffic is clearly inferior) https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.006085%2C19.962158%3B50.006664%2C19.957639#map=18/50.006384/19.959900&layers=N currently OM wants to do detour via south (warning: not checked this area in person, looks like a good test based on mapped data) https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.020758%2C20.049156%3B50.022031%2C20.048289 (OM wants illegally drive on sidewalk, GraphHopper wants to go on route that is of lower quality but balanced by being longer :) ) looks like good test case, never was there: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.096027%2C19.904330%3B50.099875%2C19.889867 should be https://www.openstreetmap.org/directions?engine=fossgis_osrm_bike&route=50.096008%2C19.904225%3B50.099847%2C19.889835 (let me know if screens are also needed)
matkoniecz (Migrated from github.com) reviewed 2024-11-21 10:22:48 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
matkoniecz (Migrated from github.com) commented 2024-11-21 10:22:48 +00:00

great! is there any such test already? Can you point me to one? (I can try to dig into data/test_data/osm/ but I think that for tiny sythethic tests data would be defined it tests itself? unless it would not exercise OM code properly, and entire pipeline should be run starting from .osm file to test things well?)

great! is there any such test already? Can you point me to one? (I can try to dig into `data/test_data/osm/` but I think that for tiny sythethic tests data would be defined it tests itself? unless it would not exercise OM code properly, and entire pipeline should be run starting from .osm file to test things well?)
pastk reviewed 2024-11-21 10:25:06 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-21 10:25:06 +00:00
Author
Member

Actually this kind of highway=service use (rather long countryside connections which are not to e.g. a specific property) looks like mismapping according to https://wiki.openstreetmap.org/wiki/Tag:highway=service

Or are there local (German, Poland, etc.) conventions which support this kind of usage?

Actually this kind of `highway=service` use (rather long countryside connections which are not to e.g. a specific property) looks like mismapping according to https://wiki.openstreetmap.org/wiki/Tag:highway=service Or are there local (German, Poland, etc.) conventions which support this kind of usage?
matkoniecz (Migrated from github.com) reviewed 2024-11-21 10:38:33 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 10:38:33 +00:00

Actually this kind of highway=service use (rather long countryside connections which are not to e.g. a specific property) looks like mismapping according to https://wiki.openstreetmap.org/wiki/Tag:highway=service

yes, those are highway=track - no idea why Germans tagged it those way. If you point me to some I will open notes (or you can open notes yourself, I guess)

(Poland has opposite problem, misuse of highway=track for unpaved driveways but there is ongoing cleanup)

I can give examples of some service that are validly mapped and are useful shortcuts/bypasses if it would be useful.

> Actually this kind of highway=service use (rather long countryside connections which are not to e.g. a specific property) looks like mismapping according to https://wiki.openstreetmap.org/wiki/Tag:highway=service yes, those are `highway=track` - no idea why Germans tagged it those way. If you point me to some I will open notes (or you can open notes yourself, I guess) (Poland has opposite problem, misuse of `highway=track` for unpaved driveways but there is ongoing cleanup) I can give examples of some `service` that are validly mapped and are useful shortcuts/bypasses if it would be useful.
pastk reviewed 2024-11-21 10:54:56 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-21 10:54:56 +00:00
Author
Member

yes, those are highway=track - no idea why Germans tagged it those way. If you point me to some I will open notes (or you can open notes yourself, I guess)

Please check the Germany_UseServiceCountrysideRoads case, ref organicmaps/organicmaps#6027

Maybe we should remove/disable this case until the tagging is fixed.

I can give examples of some service that are validly mapped and are useful shortcuts/bypasses if it would be useful.

Yeap it'd be helpful! Both shortcuts and detours via service.

> > yes, those are `highway=track` - no idea why Germans tagged it those way. If you point me to some I will open notes (or you can open notes yourself, I guess) Please check the `Germany_UseServiceCountrysideRoads` case, ref https://git.omaps.dev/organicmaps/organicmaps/issues/6027 Maybe we should remove/disable this case until the tagging is fixed. > > I can give examples of some `service` that are validly mapped and are useful shortcuts/bypasses if it would be useful. Yeap it'd be helpful! Both shortcuts and detours via service.
matkoniecz (Migrated from github.com) reviewed 2024-11-21 11:09:13 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 11:09:13 +00:00

cycleway interruptor, but as low traffic service it works OK:
https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.067485%2C20.003859%3B50.068198%2C20.004041

shortcut unavailable to vehicles in general: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.067845%2C19.900210%3B50.068379%2C19.897437

avoids megatraffic, then continues as living street: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.098161%2C19.890951%3B50.101826%2C19.888762

should be enough, I guess? Can find more

cycleway interruptor, but as low traffic service it works OK: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.067485%2C20.003859%3B50.068198%2C20.004041 shortcut unavailable to vehicles in general: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.067845%2C19.900210%3B50.068379%2C19.897437 avoids megatraffic, then continues as living street: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.098161%2C19.890951%3B50.101826%2C19.888762 should be enough, I guess? Can find more
matkoniecz (Migrated from github.com) reviewed 2024-11-21 12:32:51 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-21 12:32:51 +00:00

For avoiding: I have no good examples, cases where it should not be used is happening when it is a dead end. Which seem not worth testing.

For avoiding: I have no good examples, cases where it should not be used is happening when it is a dead end. Which seem not worth testing.
pastk reviewed 2024-11-22 20:11:29 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2024-11-22 20:11:29 +00:00
Author
Member

Many thanks! I've picked 2nd and 4th cases

Many thanks! I've picked 2nd and 4th cases
pastk reviewed 2024-11-22 20:44:26 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
@ -216,0 +326,4 @@
}
UNIT_TEST(UK_UseNocyclewayTertiary)
{
pastk commented 2024-11-22 20:44:26 +00:00
Author
Member

I don't know if there are such routing tests already. @vng?

There are some other tests which seem to setup mock mwms, e.g. search/search_integration_tests/smoke_test.cpp. But probably setting up for the routing to work will need to be more elaborate.

I don't know if there are such routing tests already. @vng? There are some other tests which seem to setup mock mwms, e.g. `search/search_integration_tests/smoke_test.cpp`. But probably setting up for the routing to work will need to be more elaborate.
pastk reviewed 2024-11-22 21:07:13 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-22 21:07:13 +00:00
Author
Member

All these tests seems way too obvious (only something horrific could break them).. All those service roads have some kind of bike infra (lanes...) or bicycle=yes which makes it very easy for OM to prefer them.

All these tests seems way too obvious (only something horrific could break them).. All those service roads have some kind of bike infra (lanes...) or bicycle=yes which makes it very easy for OM to prefer them.
pastk reviewed 2024-11-22 21:59:39 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-22 21:59:39 +00:00
Author
Member

I've added a small penalty to all link roads.
Making the penalty higher breaks e.g. the RussiaMoscowKashirskoe16ToCapLongRoute test - to avoid a long secondary_link OM prefers a detour involving a secondary of a similar length :)

I've added a small penalty to all link roads. Making the penalty higher breaks e.g. the RussiaMoscowKashirskoe16ToCapLongRoute test - to avoid a long secondary_link OM prefers a detour involving a secondary of a similar length :)
matkoniecz (Migrated from github.com) reviewed 2024-11-23 06:32:18 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
matkoniecz (Migrated from github.com) commented 2024-11-23 06:32:18 +00:00

Have you pushed the new code? I see older in commits? (or maybe you selected it without turning it into code, sorry for noise then)

Have you pushed the new code? I see older in commits? (or maybe you selected it without turning it into code, sorry for noise then)
pastk reviewed 2024-11-23 12:54:29 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-23 12:54:29 +00:00
Author
Member

What is your opinion on position of highway=residential, unclassified, tertiary amongst those?

What is your opinion on position of highway=residential, unclassified, tertiary amongst those?
pastk reviewed 2024-11-23 12:59:49 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2024-11-23 12:59:49 +00:00
Author
Member

I didn't manage to finish adjustments yesterday, I'll push an update later today.

I didn't manage to finish adjustments yesterday, I'll push an update later today.
pastk reviewed 2024-11-23 15:31:06 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-23 15:31:06 +00:00
Author
Member

I've picked two, thanks!

I've picked two, thanks!
pastk reviewed 2024-11-23 15:52:31 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-23 15:52:31 +00:00
Author
Member

I've raised weights for living streets and service. But not as much. Logically a living_street isn't more desirable than a service road and we can't raise service much at the moment as it leads to many small useless detours "to visit a gas station" :)

We need some special logic to filter such small and pointless deviations from the main route.

I've raised weights for living streets and service. But not as much. Logically a living_street isn't more desirable than a service road and we can't raise service much at the moment as it leads to many small useless detours "to visit a gas station" :) We need some special logic to filter such small and pointless deviations from the main route.
pastk reviewed 2024-11-23 15:55:17 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-23 15:55:17 +00:00
Author
Member

E.g. if the route has to go via a primary for several kms then there is no point to turn into gas stations driveways as 100m further one is back to the primary anyway.

Actually maybe introduction of turns/intersections penalties will help remedy this behavior.

E.g. if the route has to go via a primary for several kms then there is no point to turn into gas stations driveways as 100m further one is back to the primary anyway. Actually maybe introduction of turns/intersections penalties will help remedy this behavior.
matkoniecz (Migrated from github.com) reviewed 2024-11-23 16:06:03 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-23 16:06:03 +00:00

Logically a living_street isn't more desirable than a service road and we can't raise service much at the moment as it leads to many small useless detours "to visit a gas station" :)

even if living_street is not better than service it may be still possible to achieve better routing by giving it cost lower than service

in general these are routing weigths, not intended to be used to compare cycling quality in consistent, transitive etc. fashion

What is your opinion on position of highway=residential, unclassified, tertiary amongst those?

tertiary? definitely worse than residential and living_street, even for tertiary with bicycle lane due to higher traffic

though, tertiary with good asphalt may be better than service/residential/etc with terrible surface such as sand, mud, grass paver, true cobblestone and worse kinds of set

residential/unclassified is all over the place, but I would consider it (in my area) to be overall a tiny bit worse than living_street

> Logically a living_street isn't more desirable than a service road and we can't raise service much at the moment as it leads to many small useless detours "to visit a gas station" :) even if living_street is not better than service it may be still possible to achieve better routing by giving it cost lower than service in general these are routing weigths, not intended to be used to compare cycling quality in consistent, transitive etc. fashion > What is your opinion on position of highway=residential, unclassified, tertiary amongst those? tertiary? definitely worse than residential and living_street, even for tertiary with bicycle lane due to higher traffic though, tertiary with good asphalt may be better than service/residential/etc with terrible surface such as sand, mud, grass paver, true cobblestone and worse kinds of set residential/unclassified is all over the place, but I would consider it (in my area) to be overall a tiny bit worse than living_street
pastk reviewed 2024-11-23 18:22:05 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-23 18:22:05 +00:00
Author
Member

Forgot to add that OM with latest changes passes 3 cases you've suggested.

Forgot to add that OM with latest changes passes 3 cases you've suggested.
pastk reviewed 2024-11-23 18:24:40 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-23 18:24:40 +00:00
Author
Member

https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.020758%2C20.049156%3B50.022031%2C20.048289 (OM wants illegally drive on sidewalk, GraphHopper wants to go on route that is of lower quality but balanced by being longer :) )

OM goes via a sidewalk still because it has bicycle=yes. Actually it might be a good test case for #9593

> https://www.openstreetmap.org/directions?engine=fossgis_valhalla_bicycle&route=50.020758%2C20.049156%3B50.022031%2C20.048289 (OM wants illegally drive on sidewalk, GraphHopper wants to go on route that is of lower quality but balanced by being longer :) ) OM goes via a sidewalk still because it has `bicycle=yes`. Actually it might be a good test case for #9593
matkoniecz (Migrated from github.com) reviewed 2024-11-23 18:33:15 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-23 18:33:15 +00:00

OM goes via a sidewalk still because it has bicycle=yes

Interesting, I opened https://www.openstreetmap.org/changeset/151976870

> OM goes via a sidewalk still because it has bicycle=yes Interesting, I opened https://www.openstreetmap.org/changeset/151976870
matkoniecz (Migrated from github.com) reviewed 2024-11-23 18:34:14 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-23 18:34:14 +00:00

Forgot to add that OM with latest changes passes 3 cases you've suggested.

congratulations!

> Forgot to add that OM with latest changes passes 3 cases you've suggested. congratulations!
pastk reviewed 2024-11-23 21:58:00 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-23 21:58:00 +00:00
Author
Member

For some reason residentials had quite little weight before, maybe because is some areas its usual to have a lot of parked cars along? IDK.

I've made it higher but lower than tertiary still.

We can review it later again, but first would be good to find some test cases.

(here I talk about roads of similar quality only; OM applies a factor depending on the quality, see e.g. paved_bad in the file with weights)

For some reason residentials had quite little weight before, maybe because is some areas its usual to have a lot of parked cars along? IDK. I've made it higher but lower than tertiary still. We can review it later again, but first would be good to find some test cases. (here I talk about roads of similar quality only; OM applies a factor depending on the quality, see e.g. `paved_bad` in the file with weights)
matkoniecz (Migrated from github.com) reviewed 2024-11-24 03:42:01 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-24 03:42:01 +00:00

maybe because is some areas its usual to have a lot of parked cars along?

static parked cars are little to no problem (just do not cycle within doorzone, glued to side of the road)

maybe it was a performance optimization to prefer compute inferior route but faster? or attempt to prefer simpler instructions over busy roads?

We can review it later again, but first would be good to find some test cases.

I will find some cases. I expect that no amount of weight tweaking will get match for all at once. That is OK.

prefer residential over tertiary (tertiary has high traffic and nasty curve with very poor visibility, and some local drivers taking shortcut over a wrong lane, I protested against using this hint too much but there is bicycle route going over residential. Residential has good surface and almost zero traffic.)

https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.093257%2C19.814578%3B50.093415%2C19.807746#map=18/50.093343/19.811364&layers=N

screen03

preferred route: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.095435%2C19.980408%3B50.098643%2C19.994554#map=16/50.09866/19.98585&layers=N

screen05

worse route: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.09480%2C19.97874%3B50.09864%2C19.99455 (no osm.org router passes, Google routes cyclists there)

screen06

though you need to dismount on a tiny bridge and has gravel section

but this tertiary has really heavy traffic

bad https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.078763%2C19.960291%3B50.078825%2C19.971396#map=17/50.077131/19.965656&layers=N

screen07

good http://brouter.de/brouter-web/#map=17/50.07864/19.96423/standard&lonlats=19.971321,50.078749;19.962094,50.079217;19.960356,50.0789

screen08
though literally no router wanted to go there on its own

but https://www.openstreetmap.org/way/237026822 is nasty enough (lanes=4, cycleway=no, I added shoulder=no now) that going up/down on steps without ramp is still better

https://www.google.pl/maps/@50.0766128,19.9688628,3a,42.5y,216.52h,85.61t/data=!3m7!1e1!3m5!1sI9LitafSvcq_vcxbd4k1FA!2e0!6shttps:%2F%2Fstreetviewpixels-pa.googleapis.com%2Fv1%2Fthumbnail%3Fcb_client%3Dmaps_sv.tactile%26w%3D900%26h%3D600%26pitch%3D4.391837655803542%26panoid%3DI9LitafSvcq_vcxbd4k1FA%26yaw%3D216.5195172870467!7i16384!8i8192?entry=ttu&g_ep=EgoyMDI0MTExOS4yIKXMDSoASAFQAw%3D%3D

screen09
have fun cycling here

especially that uphill part after going under railway bridge

this altitude change may be not seen in OM data :(

http://brouter.de/brouter-web/#map=16/50.0756/19.9222/standard&lonlats=19.926238,50.07722;19.917955,50.078223

cycle for longer to avoid busy tertiary - prefer to escape going slightly downhill (to East) than to go on it (to West)

again, this altitude may be not seen in OM data :(

screen10

prefers residential over tertiary + unsegregated footway allowing cyclists http://brouter.de/brouter-web/#map=16/50.0748/19.9204/standard&lonlats=19.921668,50.079108;19.916038,50.07401

screen12

> maybe because is some areas its usual to have a lot of parked cars along? static parked cars are little to no problem (just do not cycle within doorzone, glued to side of the road) maybe it was a performance optimization to prefer compute inferior route but faster? or attempt to prefer simpler instructions over busy roads? > We can review it later again, but first would be good to find some test cases. I will find some cases. I expect that no amount of weight tweaking will get match for all at once. That is OK. prefer residential over tertiary (tertiary has high traffic and nasty curve with very poor visibility, and some local drivers taking shortcut over a wrong lane, I protested against using this hint too much but there is bicycle route going over residential. Residential has good surface and almost zero traffic.) https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.093257%2C19.814578%3B50.093415%2C19.807746#map=18/50.093343/19.811364&layers=N ![screen03](https://github.com/user-attachments/assets/7c61bbec-5adb-46a5-969f-3cec3d7a44e0) ---------------------------------- preferred route: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.095435%2C19.980408%3B50.098643%2C19.994554#map=16/50.09866/19.98585&layers=N ![screen05](https://github.com/user-attachments/assets/fc2dc35b-d983-42d4-a4ee-705b38ba61fc) worse route: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.09480%2C19.97874%3B50.09864%2C19.99455 (no osm.org router passes, Google routes cyclists there) ![screen06](https://github.com/user-attachments/assets/f5987cf3-68b7-4afc-8100-1f508ca9acef) though you need to dismount on a tiny bridge and has gravel section but this tertiary has really heavy traffic ----------------------------------------- bad https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.078763%2C19.960291%3B50.078825%2C19.971396#map=17/50.077131/19.965656&layers=N ![screen07](https://github.com/user-attachments/assets/1091e340-687b-4733-8cfe-652e986d7b04) good http://brouter.de/brouter-web/#map=17/50.07864/19.96423/standard&lonlats=19.971321,50.078749;19.962094,50.079217;19.960356,50.0789 ![screen08](https://github.com/user-attachments/assets/c07f907e-0011-4798-84a7-a197ca8c266e) though literally no router wanted to go there on its own but https://www.openstreetmap.org/way/237026822 is nasty enough (lanes=4, cycleway=no, I added shoulder=no now) that going up/down on steps without ramp is still better https://www.google.pl/maps/@50.0766128,19.9688628,3a,42.5y,216.52h,85.61t/data=!3m7!1e1!3m5!1sI9LitafSvcq_vcxbd4k1FA!2e0!6shttps:%2F%2Fstreetviewpixels-pa.googleapis.com%2Fv1%2Fthumbnail%3Fcb_client%3Dmaps_sv.tactile%26w%3D900%26h%3D600%26pitch%3D4.391837655803542%26panoid%3DI9LitafSvcq_vcxbd4k1FA%26yaw%3D216.5195172870467!7i16384!8i8192?entry=ttu&g_ep=EgoyMDI0MTExOS4yIKXMDSoASAFQAw%3D%3D ![screen09](https://github.com/user-attachments/assets/e7f781a8-59dc-4916-8fba-186e240c50f7) have fun cycling here especially that uphill part after going under railway bridge this altitude change may be not seen in OM data :( ------------------------------------------------- http://brouter.de/brouter-web/#map=16/50.0756/19.9222/standard&lonlats=19.926238,50.07722;19.917955,50.078223 cycle for longer to avoid busy tertiary - prefer to escape going slightly downhill (to East) than to go on it (to West) again, this altitude may be not seen in OM data :( ![screen10](https://github.com/user-attachments/assets/3d36aebd-e9b7-4cda-92f8-aa8d4e8138a5) ------------------------------------------ prefers residential over tertiary + unsegregated footway allowing cyclists http://brouter.de/brouter-web/#map=16/50.0748/19.9204/standard&lonlats=19.921668,50.079108;19.916038,50.07401 ![screen12](https://github.com/user-attachments/assets/f66b9e60-c74f-4127-ba60-d019896e8e0f)
matkoniecz (Migrated from github.com) reviewed 2024-11-24 03:45:11 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-24 03:45:11 +00:00

http://brouter.de/brouter-web/#map=15/50.0381/19.9428/standard&lonlats=19.945207,50.045255;19.94033,50.036663

prefers minor streets over busy tertiaries

screen13

Can add more, what kind of characteristics are not satisfied by tests so far?

http://brouter.de/brouter-web/#map=15/50.0381/19.9428/standard&lonlats=19.945207,50.045255;19.94033,50.036663 prefers minor streets over busy tertiaries ![screen13](https://github.com/user-attachments/assets/32f09d05-9769-44cb-b392-29621cbec9d6) Can add more, what kind of characteristics are not satisfied by tests so far?
pastk reviewed 2024-11-24 12:00:06 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-24 12:00:06 +00:00
Author
Member

prefer residential over tertiary (tertiary has high traffic and nasty curve with very poor visibility, and some local drivers taking shortcut over a wrong lane, I protested against using this hint too much but there is bicycle route going over residential. Residential has good surface and almost zero traffic.)

https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.093257%2C19.814578%3B50.093415%2C19.807746

OM passes. The residential has yesbicycle (prob inherited from route relation), so OM prefers it.

preferred route: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.095435%2C19.980408%3B50.098643%2C19.994554

Passed.

worse route: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.09480%2C19.97874%3B50.09864%2C19.99455 (no osm.org router passes, Google routes cyclists there)

though you need to dismount on a tiny bridge and has gravel section

but this tertiary has really heavy traffic

Not passed.

bad https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.078763%2C19.960291%3B50.078825%2C19.971396

good http://brouter.de/brouter-web/#map=17/50.07864/19.96423/standard&lonlats=19.971321,50.078749;19.962094,50.079217;19.960356,50.0789

but https://www.openstreetmap.org/way/237026822 is nasty enough (lanes=4, cycleway=no, I added shoulder=no now) that going up/down on steps without ramp is still better
especially that uphill part after going under railway bridge

Not passed.

http://brouter.de/brouter-web/#map=16/50.0756/19.9222/standard&lonlats=19.926238,50.07722;19.917955,50.078223

cycle for longer to avoid busy tertiary - prefer to escape going slightly downhill (to East) than to go on it (to West)

again, this altitude may be not seen in OM data :(

Not passed.

prefers residential over tertiary + unsegregated footway allowing cyclists http://brouter.de/brouter-web/#map=16/50.0748/19.9204/standard&lonlats=19.921668,50.079108;19.916038,50.07401

OM avoid the tertiary too, but in a different way

image

> prefer residential over tertiary (tertiary has high traffic and nasty curve with very poor visibility, and some local drivers taking shortcut over a wrong lane, I protested against using this hint too much but there is bicycle route going over residential. Residential has good surface and almost zero traffic.) > > https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.093257%2C19.814578%3B50.093415%2C19.807746 > OM passes. The residential has yesbicycle (prob inherited from route relation), so OM prefers it. > > preferred route: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.095435%2C19.980408%3B50.098643%2C19.994554 > Passed. > > worse route: https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.09480%2C19.97874%3B50.09864%2C19.99455 (no osm.org router passes, Google routes cyclists there) > > though you need to dismount on a tiny bridge and has gravel section > > but this tertiary has really heavy traffic > Not passed. > bad https://www.openstreetmap.org/directions?engine=graphhopper_bicycle&route=50.078763%2C19.960291%3B50.078825%2C19.971396 > > > good http://brouter.de/brouter-web/#map=17/50.07864/19.96423/standard&lonlats=19.971321,50.078749;19.962094,50.079217;19.960356,50.0789 > > but https://www.openstreetmap.org/way/237026822 is nasty enough (lanes=4, cycleway=no, I added shoulder=no now) that going up/down on steps without ramp is still better > especially that uphill part after going under railway bridge > Not passed. > > http://brouter.de/brouter-web/#map=16/50.0756/19.9222/standard&lonlats=19.926238,50.07722;19.917955,50.078223 > > cycle for longer to avoid busy tertiary - prefer to escape going slightly downhill (to East) than to go on it (to West) > > again, this altitude may be not seen in OM data :( > Not passed. > > prefers residential over tertiary + unsegregated footway allowing cyclists http://brouter.de/brouter-web/#map=16/50.0748/19.9204/standard&lonlats=19.921668,50.079108;19.916038,50.07401 OM avoid the tertiary too, but in a different way ![image](https://github.com/user-attachments/assets/58b9af21-9917-46a0-bde7-3f1b9392bf2e)
pastk reviewed 2024-11-24 12:00:46 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-24 12:00:46 +00:00
Author
Member

So let's leave for the next iteration.

So let's leave for the next iteration.
matkoniecz (Migrated from github.com) reviewed 2024-11-24 13:13:40 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-24 13:13:40 +00:00

OM avoid the tertiary too, but in a different way

this is not terrible and at this point it starts to get to "do you prefer to weave around pedestrians on crowded sidewalk or be on road with some cars" point, with preferred routes depending on person.

OM passes

I would consider adding also some that pass now - this will protect against regressions

> OM avoid the tertiary too, but in a different way this is not terrible and at this point it starts to get to "do you prefer to weave around pedestrians on crowded sidewalk or be on road with some cars" point, with preferred routes depending on person. > OM passes I would consider adding also some that pass now - this will protect against regressions
matkoniecz (Migrated from github.com) reviewed 2024-11-24 18:59:29 +00:00
routing_common/bicycle_model.cpp
matkoniecz (Migrated from github.com) commented 2024-11-24 18:59:29 +00:00

OM goes via a sidewalk still because it has bicycle=yes

contacted the mapper in https://www.openstreetmap.org/changeset/151976870 - they commented that it was a mistake and removed it

> OM goes via a sidewalk still because it has bicycle=yes contacted the mapper in https://www.openstreetmap.org/changeset/151976870 - they commented that it was a mistake and removed it
pastk reviewed 2024-11-25 20:38:58 +00:00
routing_common/bicycle_model.cpp
pastk commented 2024-11-25 20:38:58 +00:00
Author
Member
- https://git.omaps.dev/organicmaps/organicmaps/issues/9749
matkoniecz (Migrated from github.com) approved these changes 2025-02-01 07:52:13 +00:00
matkoniecz (Migrated from github.com) left a comment

looks great to me, though I have not run code myself

existing routing is really silly and I hope that it will get better with this changes

looks great to me, though I have not run code myself existing routing is really silly and I hope that it will get better with this changes
vng (Migrated from github.com) reviewed 2025-02-01 16:33:09 +00:00
vng (Migrated from github.com) left a comment

I don't mind moving forward here. Just don't delete some tests and leave (todo) comments if needed.

I don't mind moving forward here. Just don't delete some tests and leave (todo) comments if needed.
routing/routing_integration_tests/bicycle_route_test.cpp
vng (Migrated from github.com) commented 2025-02-01 16:08:43 +00:00

AFAIR (a long time ago), this test is based on a real case from the EMail of some bicycle rental app. There was a much bigger detour because of previous bugs in altitude calculations.

AFAIR (a long time ago), this test is based on a real case from the EMail of some bicycle rental app. There was a much bigger detour because of previous bugs in altitude calculations.
routing/routing_integration_tests/bicycle_route_test.cpp
vng (Migrated from github.com) commented 2025-02-01 16:19:11 +00:00

AFAIR, this test is from a user claim in EMail. Like "All cyclists in our town use this particular footway" :)

AFAIR, this test is from a user claim in EMail. Like "All cyclists in our town use this particular footway" :)
routing_common/bicycle_model.cpp
vng (Migrated from github.com) commented 2025-02-01 16:22:18 +00:00

countries

countries
routing_common/bicycle_model.cpp
vng (Migrated from github.com) commented 2025-02-01 16:28:46 +00:00

Well, I don't mind with the update here if you checked and updated "Finland_Use_Tertiary_LowTraffic" test.

Well, I don't mind with the update here if you checked and updated "Finland_Use_Tertiary_LowTraffic" test.
routing_common/routing_common_tests/vehicle_model_test.cpp
vng (Migrated from github.com) commented 2025-02-01 16:30:48 +00:00

Why did you remove case with {"hwtag", "nobicycle"} ?

Why did you remove case with {"hwtag", "nobicycle"} ?
pastk reviewed 2025-02-02 03:37:38 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2025-02-02 03:37:38 +00:00
Author
Member

Ok, but do I understand right you want to keep this test? Why?
I understand the test was useful before, but now its not. If fixes to altitude calculations need to be tested then it should be some altitude calculations unit test, not an integration bicycle routing test, right?

Ok, but do I understand right you want to keep this test? Why? I understand the test was useful before, but now its not. If fixes to altitude calculations need to be tested then it should be some altitude calculations unit test, not an integration bicycle routing test, right?
pastk reviewed 2025-02-02 03:48:26 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2025-02-02 03:48:26 +00:00
Author
Member

Did this user tell whether the cyclists cycle through this footway or they dismount and push their bikes indeed?

Did this user tell whether the cyclists cycle through this footway or they dismount and push their bikes indeed?
pastk reviewed 2025-02-02 04:39:35 +00:00
routing_common/routing_common_tests/vehicle_model_test.cpp
pastk commented 2025-02-02 04:39:35 +00:00
Author
Member

Its a bit controversial.
The test is for AllLimitsInstance which means that cycling is allowed on footways.
The speed calculation doesn't take the hwtag-nobicycle into account (I guess its processed elsewhere and just forbids bici routing there), hence a nobicycle footway would appear in this test with the same speed as a regular footway, which doesn't make sense.

Though logically an explicit bicycle=no should result in a dismount speed:

This PR doesn't change the current behavior, but it increased the footway speed for countries where its allowed.

Its a bit controversial. The test is for `AllLimitsInstance` which means that cycling is allowed on footways. The speed calculation doesn't take the `hwtag-nobicycle` into account (I guess its processed elsewhere and just forbids bici routing there), hence a `nobicycle footway` would appear in this test with the same speed as a regular `footway`, which doesn't make sense. Though logically an explicit `bicycle=no` should result in a dismount speed: - https://git.omaps.dev/organicmaps/organicmaps/issues/9784#issuecomment-2508229612 This PR doesn't change the current behavior, but it increased the footway speed for countries where its allowed.
pastk reviewed 2025-02-02 04:40:01 +00:00
routing_common/routing_common_tests/vehicle_model_test.cpp
pastk commented 2025-02-02 04:40:01 +00:00
Author
Member

I'll add a todo comment to the test.

I'll add a todo comment to the test.
pastk reviewed 2025-02-04 09:10:01 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2025-02-04 09:10:01 +00:00
Author
Member

I.e. if they (illegally) cycle through then we shouldn't adjust to route them there and it'd be better to treat this as an AvoidDismount case indeed (as per my changes in this PR).

But if they dismount indeed and prefer it over making a pedalling detour then its a legit UseDismount case and we can look into fulfilling it (e.g. increase dismount speed/weight).

I.e. if they (illegally) cycle through then we shouldn't adjust to route them there and it'd be better to treat this as an AvoidDismount case indeed (as per my changes in this PR). But if they dismount indeed and prefer it over making a pedalling detour then its a legit UseDismount case and we can look into fulfilling it (e.g. increase dismount speed/weight).
pastk reviewed 2025-02-04 09:10:36 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2025-02-04 09:10:35 +00:00
Author
Member

So WDYT? Keep it or drop it?

So WDYT? Keep it or drop it?
vng (Migrated from github.com) reviewed 2025-02-05 17:03:57 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
vng (Migrated from github.com) commented 2025-02-05 17:03:56 +00:00

I've checked this test. Yes, now it can be replaced with simple:

  // Check that OM uses dedicated "Carril bici del Paseo de la Castellana".
  CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle),
      mercator::FromLatLon(40.459616, -3.690031), {0.0, 0.0},
      mercator::FromLatLon(40.4403523, -3.69267444), 2283.89 /* expectedRouteMeters */);

Before data or elevation fix the r2 route was completely different from r1 route (despite of equal start point and a small offset of the end point). That's why it was introduced.

I've checked this test. Yes, now it can be replaced with simple: ``` // Check that OM uses dedicated "Carril bici del Paseo de la Castellana". CalculateRouteAndTestRouteLength(GetVehicleComponents(VehicleType::Bicycle), mercator::FromLatLon(40.459616, -3.690031), {0.0, 0.0}, mercator::FromLatLon(40.4403523, -3.69267444), 2283.89 /* expectedRouteMeters */); ``` Before data or elevation fix the r2 route was _completely_ different from r1 route (despite of equal start point and a small offset of the end point). That's why it was introduced.
pastk reviewed 2025-02-06 06:36:24 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2025-02-06 06:36:24 +00:00
Author
Member

Thanks! Updated.

Thanks! Updated.
pastk reviewed 2025-02-06 06:47:17 +00:00
routing/routing_integration_tests/bicycle_route_test.cpp
pastk commented 2025-02-06 06:47:17 +00:00
Author
Member

For now I've just added a comment about this controversial situation..

For now I've just added a comment about this controversial situation..
vng (Migrated from github.com) approved these changes 2025-02-07 10:10:35 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
matkoniecz
vng
Labels
Clear labels   
Accessibility

issues affecting users with specific needs

  
Accessibility

issues affecting users with specific needs

  
Address

Issues with addresses, post codes, city/town/village borders

  
Address

Issues with addresses, post codes, city/town/village borders

  
Android

Android development

  
Android

Android development

  
Android Auto

Android Auto

  
Android Auto

Android Auto

  
Android Automotive (AAOS)

   
Android Automotive (AAOS)

   
API

Short Links, deep links and url schemes, including ge0 and om://

  
API

Short Links, deep links and url schemes, including ge0 and om://

  
AppGallery

Huawei AppGallery

  
AppGallery

Huawei AppGallery

  
AppStore

Apple AppStore

  
AppStore

Apple AppStore

  
Battery and Performance

Performance and battery consumption-related issues and optimizations

  
Battery and Performance

Performance and battery consumption-related issues and optimizations

  
Blocker

An issue blocking the release

  
Blocker

An issue blocking the release

  
Bookmarks and Tracks

Bookmarks, imported tracks, and KML, KMZ, KMB, GPX, GPZ import or export

  
Bookmarks and Tracks

Bookmarks, imported tracks, and KML, KMZ, KMB, GPX, GPZ import or export

  
Borders

Country borders and polygons

  
Borders

Country borders and polygons

  
Bug

Something isn't working

  
Bug

Something isn't working

  
Build

Compilation issue

  
Build

Compilation issue

  
CarPlay

CarPlay Integration

  
CarPlay

CarPlay Integration

  
Classificator

Classify object by types

  
Classificator

Classify object by types

  
Community

Community relations

  
Community

Community relations

  
Core

Cross-platform C++ libraries with a core functionality

  
Core

Cross-platform C++ libraries with a core functionality

  
CrashReports

Crash Reporting

  
CrashReports

Crash Reporting

  
Cycling

Bike, cycle, biking and related

  
Cycling

Bike, cycle, biking and related

  
Desktop

Desktop application

  
Desktop

Desktop application

  
DevEx

Developer Experience

  
DevEx

Developer Experience

  
DevOps

Automation and pipelines

  
DevOps

Automation and pipelines

  
dev_sandbox

A desktop app to debug rendering

  
dev_sandbox

A desktop app to debug rendering

  
Directions

Issues with next turns, labels, streets, voice instructions

  
Directions

Issues with next turns, labels, streets, voice instructions

  
Documentation

Improvements or additions to documentation

  
Documentation

Improvements or additions to documentation

  
Downloader

Map dowloader

  
Downloader

Map dowloader

  
Drape

Drape OpenGL, Vulkan and Metal graphics rendering engine

  
Drape

Drape OpenGL, Vulkan and Metal graphics rendering engine

  
Driving

Drive a car related issues

  
Driving

Drive a car related issues

  
Duplicate

This issue or pull request already exists

  
Duplicate

This issue or pull request already exists

  
Editor

OSM Editor

  
Editor

OSM Editor

  
Elevation

Isolines and altitude

  
Elevation

Isolines and altitude

  
Enhancement

New feature or request, an improvement of some existing feature

  
Enhancement

New feature or request, an improvement of some existing feature

  
Epic

A large body work

  
Epic

A large body work

  
External Map Datasets

TIGER, Ordnance Survey, ASTER, SRTM, etc.

  
External Map Datasets

TIGER, Ordnance Survey, ASTER, SRTM, etc.

  
F-Droid

F-Droid build and distribution

  
F-Droid

F-Droid build and distribution

  
Fonts

Font and text rendering

  
Fonts

Font and text rendering

  
Frequently User Reported

Most annoying UX and other issues for users reported by many people

  
Frequently User Reported

Most annoying UX and other issues for users reported by many people

  
Fund

   
Fund

   
Generator

Related to generator tool.

  
Generator

Related to generator tool.

  
Good first issue

Good for newcomers

  
Good first issue

Good for newcomers

  
Google Play

Google Play

  
Google Play

Google Play

  
GPS

Location and positioning issues

  
GPS

Location and positioning issues

  
GSoC

Ideas for Google Summer of Code

  
GSoC

Ideas for Google Summer of Code

  
iCloud

   
iCloud

   
Icons

Map and app icons

  
Icons

Map and app icons

  
iOS

iOS development

  
iOS

iOS development

  
Legal

Legal aspects

  
Legal

Legal aspects

  
Linux Desktop

Ubuntu, Gentoo, Red Hat, Debian, Cent OS, Fedora, etc.

  
Linux Desktop

Ubuntu, Gentoo, Red Hat, Debian, Cent OS, Fedora, etc.

  
Linux packaging

   
Linux packaging

   
Linux Phone

LibreM, PinePhone, Wayland and other mobile Linux versions

  
Linux Phone

LibreM, PinePhone, Wayland and other mobile Linux versions

  
Mac OS

Desktop issues related to Mac OS X, OM running on M1, M2, M3 Apple silicon, or Qt version

  
Mac OS

Desktop issues related to Mac OS X, OM running on M1, M2, M3 Apple silicon, or Qt version

  
Map Data

OpenStreetMap data related issues

  
Map Data

OpenStreetMap data related issues

  
Metro

Subway, Tube, light-rail public transport issues

  
Metro

Subway, Tube, light-rail public transport issues

  
Navigation

Issues related to the active Navigator / Navigation mode

  
Navigation

Issues related to the active Navigator / Navigation mode

  
Need Feedback

Further information is requested

  
Need Feedback

Further information is requested

  
Night Mode

Night or dark mode feature

  
Night Mode

Night or dark mode feature

  
NLnet 2024-06-281

Tasks related to NLnet 2024-06-281 project.

  
NLnet 2024-06-281

Tasks related to NLnet 2024-06-281 project.

  
No Feature Parity

A platform needs this to be implemented to have feature parity with other platforms

  
No Feature Parity

A platform needs this to be implemented to have feature parity with other platforms

  
Opening Hours

Business Hours, off time, holidays, seasonal

  
Opening Hours

Business Hours, off time, holidays, seasonal

  
Outdoors

Hiking, mtb, 4x4...

  
Outdoors

Hiking, mtb, 4x4...

  
POI Info

Feature metadata, OSM tags that are displayed in Place Page

  
POI Info

Feature metadata, OSM tags that are displayed in Place Page

  
Privacy

Privacy

  
Privacy

Privacy

  
Public Transport

Bus, trolleybus, train, tram, metro, subway, light rail

  
Public Transport

Bus, trolleybus, train, tram, metro, subway, light rail

  
Raw Idea

An idea that needs to be elaborated

  
Raw Idea

An idea that needs to be elaborated

  
Refactoring

   
Refactoring

   
Regional

This issue is different depending on the country or region

  
Regional

This issue is different depending on the country or region

  
Regression

A regression bug

  
Regression

A regression bug

  
Releases

Meta-tickets for tracking the release procedure

  
Releases

Meta-tickets for tracking the release procedure

  
RoboTest

Robo / Monkey Auto Tests

  
RoboTest

Robo / Monkey Auto Tests

  
Route Planning

Preview and plan your track

  
Route Planning

Preview and plan your track

  
Routing

Route building issues, e.g. valid route, valid ETA

  
Routing

Route building issues, e.g. valid route, valid ETA

  
Ruler

Helicopter routing, "as a crow flies"

  
Ruler

Helicopter routing, "as a crow flies"

  
Search

Search

  
Search

Search

  
Security

Security improvements

  
Security

Security improvements

  
Styles

Map drawing styles

  
Styles

Map drawing styles

  
Tests

   
Tests

   
Track Recording

Track Recording

  
Track Recording

Track Recording

  
Translations

Localization and translations issues

  
Translations

Localization and translations issues

  
TTS

Text-to-Speech

  
TTS

Text-to-Speech

  
UI

User interface issues

  
UI

User interface issues

  
UX

User eXperience, an issue with usability

  
UX

User eXperience, an issue with usability

  
Walk Navigation

Foot pedestrian mode

  
Walk Navigation

Foot pedestrian mode

  
Watches

Android Watches or Apple Watches support

  
Watches

Android Watches or Apple Watches support

  
Web

https://organicmaps.app/

  
Web

https://organicmaps.app/

  
Wikipedia

Issues related to Wiki articles embedded in OM data files.

  
Wikipedia

Issues related to Wiki articles embedded in OM data files.

  
Windows

Windows

  
Windows

Windows

  
Won't fix

Unfeasible or impossible to resolve

  
Won't fix

Unfeasible or impossible to resolve

  
World Map

Issues about improving the base map of the whole world that is bundled with the app

  
World Map

Issues about improving the base map of the whole world that is bundled with the app

No labels
Accessibility
Accessibility
Address
Address
Android
Android
Android Auto
Android Auto
Android Automotive (AAOS)
Android Automotive (AAOS)
API
API
AppGallery
AppGallery
AppStore
AppStore
Battery and Performance
Battery and Performance
Blocker
Blocker
Bookmarks and Tracks
Bookmarks and Tracks
Borders
Borders
Bug
Bug
Build
Build
CarPlay
CarPlay
Classificator
Classificator
Community
Community
Core
Core
CrashReports
CrashReports
Cycling
Cycling
Desktop
Desktop
DevEx
DevEx
DevOps
DevOps
dev_sandbox
dev_sandbox
Directions
Directions
Documentation
Documentation
Downloader
Downloader
Drape
Drape
Driving
Driving
Duplicate
Duplicate
Editor
Editor
Elevation
Elevation
Enhancement
Enhancement
Epic
Epic
External Map Datasets
External Map Datasets
F-Droid
F-Droid
Fonts
Fonts
Frequently User Reported
Frequently User Reported
Fund
Fund
Generator
Generator
Good first issue
Good first issue
Google Play
Google Play
GPS
GPS
GSoC
GSoC
iCloud
iCloud
Icons
Icons
iOS
iOS
Legal
Legal
Linux Desktop
Linux Desktop
Linux packaging
Linux packaging
Linux Phone
Linux Phone
Mac OS
Mac OS
Map Data
Map Data
Metro
Metro
Navigation
Navigation
Need Feedback
Need Feedback
Night Mode
Night Mode
NLnet 2024-06-281
NLnet 2024-06-281
No Feature Parity
No Feature Parity
Opening Hours
Opening Hours
Outdoors
Outdoors
POI Info
POI Info
Privacy
Privacy
Public Transport
Public Transport
Raw Idea
Raw Idea
Refactoring
Refactoring
Regional
Regional
Regression
Regression
Releases
Releases
RoboTest
RoboTest
Route Planning
Route Planning
Routing
Routing
Ruler
Ruler
Search
Search
Security
Security
Styles
Styles
Tests
Tests
Track Recording
Track Recording
Translations
Translations
TTS
TTS
UI
UI
UX
UX
Walk Navigation
Walk Navigation
Watches
Watches
Web
Web
Wikipedia
Wikipedia
Windows
Windows
Won't fix
Won't fix
World Map
World Map
Milestone
Clear milestone
No items
No milestone
Ready for alpha testing
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#9692
No description provided.
Delete branch "pastk-routing-cycling"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?