search

abrensch / brouter

configurable OSM offline router with elevation awareness, Java + Android
MIT License
473 stars 113 forks source link

Update the trekking profile #641

Open afischerdev opened 8 months ago

afischerdev commented 8 months ago

Here a new version for the trekking profile suggested by @EssBee59

zod commented 8 months ago

It seems like the updated profile does no longer have a avoid_unsafe parameter. The fix for the overrideParam testcase should be a valid parameter (which produces a different result) instead of changing the file.

afischerdev commented 8 months ago

@zod thanks for your review. The trekking profile has an avoid_unsafe, but it is located a little bit down.

  1. override param in 'RoutingEngineTest' paramTrack comes out as paramTrack0 (first alternative, can't say why the first test was against paramTrack1, don't see a second call)
  2. override param in 'RouteServerTest' also works for me ('avoid_unsafe=0' brings a different results)
afischerdev commented 8 months ago

@zod I made new tests to get this. First I found no paramTrack0.gpx in my local repo. Must have dropped it But it is needed to build the paramTrack1.gpx Then I generated the route with old and new profile.

test_alt test_neu

First one is the old situation - blue the unsafe route, red the safe result and green the test route from repo Second one is new - blue unsafe, red the safe route. As you can see the new safe route is near to the test route. That seems to be the reason that there is no paramTrack1.gpx as result

What to do: I would like to replace the old test route0 with the unsafe route. Both profiles produce it in the same way. What do you think?

quaelnix commented 8 months ago

This PR adds the following without comment:

removes the following without comment:

changes the following without comment:

And introduces a new logic for subtracting costfactors that can result in the costfactor becoming less than 1, which not only has the potential to completely destroy routing, but also contradicts what is currently being discussed here: https://github.com/abrensch/brouter/issues/610.

If this is merged, I will end the collaboration.

rkflx commented 8 months ago

This PR [ adds | removes | changes ] the following without comment

It's hardly the first time this has been criticized, yet nothing changed.

If this is merged, I will end the collaboration.

FWIW, I already decided some time ago to stop caring about and commenting on such PRs, so do not mistake silence for agreement with any such PR. I suspect other contributors quit or reduced their involvement out of frustration how things are run here, too.

EssBee59 commented 8 months ago

Hello ! Yes, I suggested this profile version to replace the current "trekking", the PR is a kind of a misunderstanding with afischerdev. Anyway, (perhaps better so!) we have now first reactions to the proposed changes, so that a real discussion on the parameters could start?

My first goal was to change the default value for "ignore_cycleroute": see https://github.com/abrensch/brouter/issues/622
the further changes are not so critical, but as example not considering surface=compacted in the same way as fine_gravel is for me a severe bug. Yes, I accept the critic that no documentation was provided, thank Quelnix for your list!

I suggest to start (or better to continue) the discussion with https://github.com/abrensch/brouter/issues/622 Till now I am missing arguments to not change the default value?

I hope we can prepare together a solution to this first parameter and go on with the others.
Regards

afischerdev commented 8 months ago

@zod Added a new paramTrack file and went back to old test. But for me it is still more a test for the generation of a 2nd route then a test for a param change.

afischerdev commented 8 months ago

@quaelnix You're right, a detailed explanation would have been good. When I started I was still unsure whether I should create an issue or a PR. But I decided to do a PR because we already have enough issues about it that end without a result. But I didn't ask for any further information about it. Sorry.

From my point of view, it would be good if we have a person responsible for the profiles who would bring the issues together. That's why I had the feeling that no one felt responsible for it and nothing was happening. I think it was good that @EssBee59 had the courage to create a new version of the trekking profile.

With a maintainer we could also go back to the normal way, create an issue and a PR for a result.

If this is merged, I will end the collaboration.

Hard words, but understandable. I would miss you. We need a corrective, someone who will point out the critical points and encourage changes. But I know that is not always easy going.

EssBee59 commented 8 months ago

With a maintainer we could also go back to the normal way, create an issue and a PR for a result.

Yes, with a maintainer defect #622 would probably not hang for weeks!!!

It had be better to start with that, but now I found time to document the changes suggested in this PR profile:

1-cycle_route

Not every part of a cycle-route is a "safe harbor": Constrains exist to create a cycle-route, as example

Statistics from Hesse-Germany show as example, that a big part of the OSM primary (10%) and secondary (20%) ways are used in these cycle-routes, most of them without any cycleway=lane!

In the trekking profile (with the default parameters) these ways get a lower cost as any other ways (track, cycleway etc...), so that the resulting routing is not safe and do not reflect what the Brouter can do!

2-surface=compacted

Fine_gravel ==> cost 1 compacted ==> cost 2

But compacted (quality just below asphalt!) is better than fine_gravel

The profile make a very limited usage of the surface tag ... assign ispaved = surface=paved|asphalt|concrete|paving_stones|sett ... assign isunpaved = not or surface= or ispaved surface=fine_gravel|cobblestone

3-uphillcost penalty

The current implementation in the trekking:

assign downhillcost = 60 # %downhillcost% | Cost for going downhill | number assign downhillcutoff = 1.5 # %downhillcutoff% | Gradients below this value in percents are not counted. | number assign uphillcost = 0 # %uphillcost% | Cost for going uphill | number assign uphillcutoff = 1.5 # %uphillcutoff% | Gradients below this value in percents are not counted. | number

assign downhillcost = if consider_elevation then downhillcost else 0 assign uphillcost = if consider_elevation then uphillcost else 0

With the default value "consider_elevation=true", uphillcost remains =0, and this is a very poor usage of the "elevation" fonction as supported by the Brouter.

example 2 routes are possible to the same destination:

Route A: 20 km with uphill factor of 7.5

Alternativ Route B: 125 Km with uphill=downhill=0

The user would certainly prefer "Route B" as he choosed "consider_elevation".... but Brouter will suggest "Route A" because no elevation costs are added during his calculation. (because uphillcost=0 and donwhillcutoff = 1.5)

4- downhillcutoff and upillcutoff logic that avoids steep inclines

Also when "consider_elevation" is changed to "false", bikers are not able to use ways with very steep inclines. To avoid such ways, a minor change is possible:

assign downhillcost = 60 # %downhillcost% | Cost for going downhill | number assign downhillcutoff = switch considerelevation 1.5 10 assign uphillcost = 80 # %uphillcost_% | Cost for going uphill | number assign uphillcutoff = switch consider_elevation 1.5 10

5- consider_traffic

Default value is false, so that the traffic do not impact the routing. The pseudo-tag "estimatd_traffic_class" is now available world-wide for primary, secondary and tertiary highways, it should be considered for biker routing. (impact can be changed with the parameter, current setting "very low traffic" can be discussed)

6- smoothnesspenalty

The surface tag is important, but the surface quality can very different between 2 highways with the same type and surface:

When available, the smoothness is a good help!

Remark: When the tag is considered in the profile, then the users have the posibility by need to add or change its value in OSM!!! So, it impacts positively not only the own routing, but it is also a benefit for all OSM users.

7- railway and barrier penalties

OSM delivers detailed tags about railways and barriers, why not to add penalties when the cyclist have to pass them?

assign railwaypenalty switch railway=level_crossing 155 0

Note: each rail-track is tagged, so the penalty depends on the number of rail-tracks to be crossed-and probably on the number of trains crossing that place per hour/day.

assign barrierpenalty switch barrier= 0 switch barrier=block|bollard 140

Note: Barriers not only cost time, the risk of accidents increases.

Here a new version containing these changes: trekking_v11.brf.txt

But weaknesses remain (a.e. surface tag), and the maintenance remains difficult. Better would be a new, well structured, easy to maintain profile!

Regards

zod commented 8 months ago

@zod Added a new paramTrack file and went back to old test. But for me it is still more a test for the generation of a 2nd route then a test for a param change.

I agree that the tests is far from ideal, but unfortunately I didn't find a better way to test if param overriding works. BRouter only creates the 2nd GPX file when the track differs from the existing track. If you would pass avoid_unsafe=0 in the overrideParam test it would fail.

afischerdev commented 8 months ago

@zod You don't have a better idea either. And if we test both aspects, it's ok too.

quaelnix commented 8 months ago

Yes, with a maintainer defect https://github.com/abrensch/brouter/issues/622 would probably not hang for weeks!!!

Your assumption of being smarter than the person who created the trekking profile leads to ill-conceived changes and I am simply not going to accept them. The logic you are trying to remove does exactly the right thing despite map errors.

I have already proven that the alleged routing error in the first example you gave in this issue ticket is caused by incorrect map data, and here is the proof that it is no different in the second example: b486 Source: https://www.google.de/maps/@49.9787262,8.5809106,3a,60.3y,274.65h,88.89t/data=!3m6!1e1!3m4!1s79wZe-swSnTwfujOZnYEbw!2e0!7i16384!8i8192?entry=ttu

EssBee59 commented 8 months ago

@quaelnix,

thanks for the kind words! It takes too much time for me to read comments that are confusing or completely out of place. From now on I will simply ignore your posts.

@ other participants of this PR:

in the first example Quelnix suspects a mapping error: the cycle_route would not use the secondary hw as mapped, but the parallel path! (mapping error)

No proof is delivered for this! I rather think, the route was planed for groups of cyclists biking together "sunday mornings" where the traffic remains low. And the path is this case probably not prefered to the secondary hw. (avoid path for groups)

in the second example Quaelnix delivers a picture taken nearly 1 km outside the route, strange for a proof! Within the critical route part, no path exists parallel to the primary hw.

Is it possible, that 10 % of the primary and 20% of the secondary hw in Hess are related BY ERRORS to cycle_route?

afischerdev commented 7 months ago

Let us stay by facts. Please.

Routing is very individual and many answers are correct.

Change that appear to be critical:

- assign   ignore_cycleroutes       = false  # %ignore_cycleroutes% | Set true for better elevation results | boolean
+ assign   ignore_cycleroutes       = true   # %ignore_cycleroutes% | Set to false to favor the segments on cycleroutes | boolean

At first glance I would say it doesn't matter what the starting value is. But cycle routes can present a few problems for routing:

May be a feature 'follow cycleroute' could help for trekking profile that follows the direction of a route but uses the saver way. When I use the original profile with ignore_cycleroutes=false and avoid_unsafe=true, route doesn't change.

quaelnix commented 7 months ago

From now on I will simply ignore your posts.

We can reopen this pull request once you change your mind.

No proof is delivered for this!

radroutenplaner-hessen osm-relation

Within the critical route part, no path exists parallel to the primary hw.

Within the "critical route part" the speed limit of primary highway is 50 km/h or less and your alternative looks like this: alternative-route Source: https://www.google.de/maps/@49.9880073,8.5751062,3a,75y,311.2h,80.22t/data=!3m6!1e1!3m4!1seaC8bC_-mAKskLhcHiBTNQ!2e0!7i16384!8i8192?entry=ttu

afischerdev commented 7 months ago

Here we are again.

@quaelnix I don't think it's a good action to just close. As your pictures show, action is needed.

We can

Or maybe someone else would like to contribute a completely new trekking profile from their pool.

quaelnix commented 7 months ago

I don't think it's a good action to just close.

None of the proposed changes is acceptable in its current form and as long as @EssBee59 is neither willing to properly test his changes nor to discuss them, this pull request is not going to happen. His behavior is completely unacceptable. Period.

enable avoid_unsave to work in the original profile

Yes, of course we can discuss if there are ways to improve the avoid_unsafe switch. But this has nothing to do with this pull request. And by the way a highway=primary section with maxspeed=40, right of way, asphalt in pristine condition and basically no hgv traffic is not unsafe.

As your pictures show, action is needed.

Yes, someone needs to put the route relation were it actually belongs and properly tag the path that has not been touched in 10 years and make this path the combined foot and cycleway it actually is.

EssBee59 commented 7 months ago

Hello afficherdev,

  1. Please tell us how did you find the errors on Hessen data?

The statistics were created using overpass turbo http://overpass-turbo.eu/

here some examples: overpass.txt

A further example...

http://brouter.de/brouter-web/#map=9/51.0552/13.7219/osm-mapnik-german_style&lonlats=8.76663,50.017366;13.725973,51.044692

I tested with the current profile, the current profile + ignore_Cycleway=true and with the trekking_v11 including the changes suggested above

image

quaelnix commented 7 months ago

Even with this relaxed query: https://overpass-turbo.eu/s/1zDU, the rate of false positives is extremly high:

-> Map is wrong.

-> Map is incomplete. Route relation again on the wrong highway ...

I guess we don't have to talk about how the situation will be in other countries, where the density of mappers is much much lower than in Germany and where the trekking profile is also supposed to produce artefact free routes that will not get you stuck on paths where you have to carry your bike ...

quaelnix commented 7 months ago 
assign smoothnesspenalty =
  switch smoothness=intermediate            0
  switch smoothness=bad                     0.5
  switch smoothness=very_bad                1
  switch smoothness=horrible            3
                                            0

The result of this will be that you downgrade ways which are tagged as smoothness=bad (of which almost all are perfectly fine for trekking bikes) and instead favor any of the ~ 97 % alternative ways that do not carry any smoothness tag.

This is plain stupid and results in a near 100 % failure rate:

quaelnix commented 7 months ago

but as example not considering surface=compacted in the same way as fine_gravel is for me a severe bug.

Another example of a gross misunderstanding of how the trekking profile works. Adding compacted to this exclusion list actually removes such ways from the list of unpaved ways, which throws off the tracktype cost logic and results in chosing:

afischerdev commented 7 months ago

@quaelnix

...as long as @EssBee59 is neither willing to properly test his changes nor to discuss them...

I think it's sentences like this that make @EssBee59 a little more reserved:

Your assumption of being smarter than the person who created the trekking profile leads to ill-conceived changes...

Anyway, thanks for your samples, good basis for a discussion.

quaelnix commented 7 months ago

I think it's sentences like this that make @EssBee59 a little more reserved:

His complete inability to follow arguments is not new and now it is enough. Not to mention this .txt mess, broken markdown formatting in basically every post and the aparrent refusal to open his own pull requests. Again, I am not going to accept this anymore.

Yes, we don't have to talk about that.

We need to talk about this because the design goal of the trekking profile was (and still is, in my opinion) to make it resistant to map errors.

In the first example I didn't find any route networks

What do you mean by that? The relation in question does carry a network=lcn tag.

But also cobblestone as unpaved is not really understandable

cobblestone is excluded from the list of unpaved roads!

afischerdev commented 7 months ago

@quaelnix

Yes, we don't have to talk about that.

We need to talk about this ...

This quote has other context, Original it was:

  • Map errors Yes, we don't have to talk about that.

And first sample of your map error post is:

B3: https://www.openstreetmap.org/way/364301602

And I've found: it is member of the route Radnetz Stadt Darmstadt

cobblestone is excluded from the list of unpaved roads!

Yes, my fail - short reading only. But then remark is the other way round, why cobblestone and fine_gravel are not in ispaved collection. Historical I guess. But I'm digressing from the topic.

And a last remark on compacted. I found this

Best sort of ways below paving with asphalt, concrete, paving stones.

Seems to be better in surface hierarchy then fine gravel.

His complete inability to follow arguments is not new and now it is enough. Not to mention this .txt mess, broken markdown formatting in basically every post and the aparrent refusal to open his own pull requests. Again, I am not going to accept this anymore.

Not everyone is born with developer skills. And I think we have to accept collaborators who are not that equipped. And not to push them out. They often come from an interested user community and contribute here the discussion of the scene. Our discussions are not primarily user driven. That's okay, we can do important things of our own. But it also makes us forget the user needs we have. E.g. if you take the example route Frankfurt - Dresden, around 500 km, it takes a lot of time to calculate. And I am willing to make the technical transfer for @EssBee59 if needed. I haven't been able to do this so well in the past.

quaelnix commented 7 months ago

This quote has other context

Yes, sorry, my bad.

Historical I guess.

No, the (almost exclusive) purpose of these two variables is to feed the probablyGood logic, that can be considered a whitelist of ways that are likely kind of decent.

If you start feeding this whitelist with things that might not be good, you're taking it ad absurdum and end up with the routing artefacts I showed above.

You simply cannot look at one line of a profile and say this looks incorrect, lets change it. Doing it like that will almost always fail horribly.

And I think we have to accept collaborators who are not that equipped.

To be honest, I don't know what to say anymore. If by now @EssBee59 is still not able to grasp the idea behind the cycleroute logic, then yes, maybe this is the end of the discussion. One last example (from: https://github.com/abrensch/brouter/issues/479):

E.g. if you take the example route Frankfurt - Dresden, around 500 km, it takes a lot of time to calculate.

I agree that we should try our best to keep the performance of the trekking profile as high as possible.

afischerdev commented 7 months ago

I added a little test for the variables inside a profile. And a minimal profile with the logic on our discussion: probablyGood is defined as true also when we have no surface definition. Because some said this is a cycle way. But if we have an undefined value (e.g bricks, is added in the next generation lookups.dat) isunpaved becomes true. You could use this for short results:

$ gradlew brouter-expressions:test