Fix profile regressions

[quaelnix]

https://github.com/abrensch/brouter/pull/579 broke the logic that handled highway=path trying to add the missing avoid_path logic.

Example: http://brouter.de/brouter-web/#map=15/51.3734/7.0430/...&profile=fastbike

See: https://github.com/nrenner/brouter-web/issues/756

---

Both avoid_unsafe and avoid_path are dysfunctional. We should remove these options or implement them properly.

[rkflx]

Thanks! I was just about to submit a similar PR last week, but run out of time.

Could you look into the other profiles touched too, e.g. whether anything breaks (trekking.brf)? I'd also revert the addition of UI-facing profile options when they are a no-op and thus could be confusing for users (hiking-mountain.brf, fastbike.brf). This could be done in this PR, and reintroducing them is always possible in conjunction with a proper implementation.

Besides, I fail to see how the original change relates to "Update profiles for new db tags", it seems to have slipped in. It has been mentioned by others before, but we really should stop adding unrelated changes to atomic commits, since this is not the first time it led to breakage (and I already identified more regressions with a similar pattern I simply have not had the time to report here yet).

[quaelnix]

Could you look into the other profiles touched too, e.g. whether anything breaks (trekking.brf)?

The changes in the trekking.brf probably did not break something, but they also do not work as one would expect, because the else branch where the avoid_path penalty was added is rarely reached on way segments that are of type highway=path.

[quaelnix]

@afischerdev, here is an example route that shows that the new avoid_path logic in the trekking.brf basically does the opposite of what it is supposed to do: https://brouter.de/brouter-web/#map=12/50.8610/6.9499/standard,route-quality&lonlats=6.957874,50.942837;7.096699,50.732365

I'd also revert the addition of UI-facing profile options when they are a no-op

Done.

[afischerdev]

Nice to see that we have new maintainers for the profiles. Because I really don't feel like the supervisor for it.

As I mentioned in #561 I'm a friend of normalization. fastbike-verylowtraffic already use a avoid_path. And that is why I added it to the others as well. The input wasn't perfect at all. But it was good enough to start discussion.

And so I would like to see it holded in the profiles and optimized. And not thrown away.

BTW there was the constant expressions optimization #566 (PR #573). The fastbike profile had the consider_traffic logic before. So this idea doesn't work.

[quaelnix]

As I mentioned in https://github.com/abrensch/brouter/issues/561 I'm a friend of normalization.

Me too, but we should first define what the goal is then discuss different alternatives and then apply them. There is no time pressure with this issue and the rushed avoid_path patch sends people across way segments with surface=ground, smoothness=impassable for no reason.

so I would like to see it holded in the profiles and optimized.

The problem is that avoid_unsafe (e.g. avoid major roads) makes no sense on a profile like fastbike which actively searches for major roads, because you can drive fast there.

Same with avoid_path on the trekking profile. highway=path is usually the most used type of way on this profile.

fastbike-verylowtraffic already use a avoid_path

Yes, but it has a completely different cost structure and is apparently also not included in the profile list on brouter.de

The fastbike profile had the consider_traffic logic before. So this idea doesn't work.

What do you mean by that?

[afischerdev]

The fastbike profile had the consider_traffic logic before. So this idea doesn't work.

What do you mean by that?

The idea is not to have too much overhead in the routing result. When a constant is not used because of e.g. consider_traffic = false it is not need in message block when processUnusedTags=false. This does not work for fastbike.

Same with avoid_path on the trekking profile. highway=path is usually the most used type of way on this profile.

May be, but what when I like to drive side by side (e.g. with kids), my trekking profile parameter are very optimized to my favours - only today no smaller ways.

There is no time pressure

Yes, no time pressure, but there should be some pressure otherwise nothing happens. Some likes on an idea is not enough. Changes should follow. Changing profiles is a thankless task. Everyone has different ideas. The goal here is finding the best way all can live with. And a switch more can add more comfort without changing profiles manually.

[quaelnix]

This does not work for fastbike.

Yes, but all we need to do to make it work is to also wrap the contents of the assign trafficpenalty0 = block into an if consider_traffic then ... else ... statement.

but what when I like to drive side by side (e.g. with kids)

The most likely effect of avoid_path in the trekking profile is that you will instead be routed via highway=track or highway=footway, both of which are most likely worse than any paved highway=path in this case.

Just look at the example I posted above. Activation of avoid_path replaces a segment of:

highway=path surface=asphalt foot=designated bicycle=designated smoothness=excellent

with a segment of:

highway=track tracktype=grade2 surface=fine_gravel

Some likes on an idea is not enough. Changes should follow.

It's not that I didn't try to find a way to optimize the default profiles according to https://github.com/abrensch/brouter/issues/561, I just failed.

And a switch more can add more comfort without changing profiles manually.

I agree with that, but the switch must do what it is supposed to do, it must work hand in hand with all the other switches, and it must also not change the profile behavior when the switch is not active. The code I want to revert fails in all three categories.

[rkflx]

Fix regression in trekking profile Remove unused profile options

LGTM.

Both test cases presented above are clearly showing regressions. Also, showing non-working UI checkboxes to the user is bad, developer TODOs should be tracked elsewhere.

As such, I'd suggest to merge this PR in its current form.

Any further discussion not relating to what's stated in the title of this PR is best done in (likely multiple) separate PRs or issues. With Git it is trivial to get back the original changes and apply fixes on top (git revert --no-commit <commit>) anyway.

[afischerdev]

Ok, it's up to you.