-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix profile regressions #593
Fix profile regressions #593
Conversation
abrensch@e66468b broke the logic that handled highway=path. This patch reverts the problematic change in behavior. See: nrenner/brouter-web#756
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 ( 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). |
The changes in the |
The 'avoid_path' logic which was added in abrensch@89b71c2 ignores the cycleroute logic and makes no sense.
@afischerdev, here is an example route that shows that the new
Done. |
Nice to see that we have new maintainers for the profiles. As I mentioned in #561 I'm a friend of normalization. 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 |
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
The problem is that Same with
Yes, but it has a completely different cost structure and is apparently also not included in the profile list on brouter.de
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.
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.
Yes, no time pressure, but there should be some pressure otherwise nothing happens. Some likes on an idea is not enough. Changes should follow. |
Yes, but all we need to do to make it work is to also wrap the contents of the
The most likely effect of Just look at the example I posted above. Activation of
with a segment of:
It's not that I didn't try to find a way to optimize the default profiles according to #561, I just failed.
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. |
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 ( |
Ok, it's up to you. |
#579 broke the logic that handled
highway=path
trying to add the missingavoid_path
logic.Example: http://brouter.de/brouter-web/#map=15/51.3734/7.0430/...&profile=fastbike
See: nrenner/brouter-web#756
Both
avoid_unsafe
andavoid_path
are dysfunctional. We should remove these options or implement them properly.