Skip to content
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

No needless dropping of 'estimated_town_class' #602

Closed
wants to merge 1 commit into from

Conversation

quaelnix
Copy link
Collaborator

@quaelnix quaelnix commented Jul 27, 2023

To enable the crossing of a city along rivers, only one line of code in the profile text needs to be adjusted:

switch estimated_town_class= 0

->

switch or estimated_town_class= ( not estimated_river_class= )  0

This significantly improves the usability of the estimated_town_class pseudo tag.

@quaelnix quaelnix temporarily deployed to BRouter July 27, 2023 16:53 — with GitHub Actions Inactive
@quaelnix quaelnix temporarily deployed to BRouter July 27, 2023 17:03 — with GitHub Actions Inactive
To enable the crossing of a city along rivers, only one line of code in the profile text needs to be adjusted:

switch estimated_town_class= 0

->

switch or estimated_town_class= ( not estimated_river_class= )  0
@quaelnix quaelnix temporarily deployed to BRouter July 27, 2023 17:04 — with GitHub Actions Inactive
@quaelnix
Copy link
Collaborator Author

@afischerdev, have you thought about it yet?

@afischerdev
Copy link
Collaborator

As before, I don't mind to do that. But I thought maybe @EssBee59 would like to add something else to that.

@afischerdev
Copy link
Collaborator

@EssBee59
What do you think about this?
Or just merge?

@EssBee59
Copy link
Collaborator

Hello,
As every body is responsable for his profile, I will not comment this.
I am very satistfied with my profiles (without the proposed change, that means river_class=1 is not clearring the town penalty).
With the new tags a lost of parameters had to be set by "feeling", it would cost months of work to discuss each one.
As allready said, I would like to concentrate/ work on real bugs, not on own desires.

@quaelnix
Copy link
Collaborator Author

the proposed change, that means river_class=1 is not clearring the town penalty

The proposed change does not "not clear the town penalty". It only shifts the place where the mingling between the two tags is happening from the preprocessing into to the profile, which increases the usability of the new tags because we no longer enforce one preference (namely your preference) upon users.

As every body is responsable for his profile

Yes, but without this change users simply cannot decide if they want to intermix the river and town class or not!

I am very satistfied with my profiles

So what is the problem if you can simply replace estimated_town_class= with or estimated_town_class= estimated_river_class=2|3|4|5|6?

I would like to concentrate/ work on real bugs, not on own desires.

You completely ignore that the proposed change increases the freedom of choice for profile developers.

@quaelnix
Copy link
Collaborator Author

quaelnix commented Oct 3, 2023

@afischerdev, I'm not going to accept @EssBee59 answer.

We currently spend a lot of computational time figuring out which way is within a city, only to later discard some of that information.

This pull request is not about "own desires" but about reasonable design decisions and throwing this valuable information away for no good reason is unreasonable.

Especially when avoiding cities is likely going to be on if not the biggest use-case of the new tags.

@afischerdev
Copy link
Collaborator

@quaelnix
At the end I guess it could be you have to accept. @EssBee59 is the maintainer of the database scripts and I would not commit without an OK.

But back to the facts:
What I understand is

  • you want to keep the town data in database and do the exclusion river/town in profile
  • the other way is to drop town tag from database when river is available

The arguments are understandable.

  • users should have more options
  • database should be as small as possible
    We are living always in this conflict.

The sample issue #608 is not perfect for this discussion:

  • it uses car profile which have no new 'green' tags at the moment
  • it works in an area where the towns are small and so they don't have a town tag
  • but yes, when we break the 50000 population rule - is not the topic at the moment -, the town tag for this villages will be gone

@quaelnix
But the #608 shows you have other ideas on town tag generation. Please tell us more about 'building density' for town class.
The idea using maxspeed and town class is interesting. But I didn't understand it. I know lots of streets with maxspeed 50 and lots of building, but no one slows down. And wouldn't effect it the time calculation?

@quaelnix
Copy link
Collaborator Author

quaelnix commented Oct 4, 2023

What I understand is

  • you want to keep the town data in database and do the exclusion river/town in profile
  • the other way is to drop town tag from database when river is available

Exactly.

database should be as small as possible

Again, it makes no sense to drop valuable information because of space concern.

The arguments are understandable.

Which arguments?

I will not comment this

is not an argument.

@devemux86
Copy link
Contributor

database should be as small as possible

Again, it makes no sense to drop valuable information because of space concern.

Usually some of the first things users expect from routers and they don't find in BRouter
are street names (#55), speed limits, maybe street information in response (e.g. tunnels).

So additional data in the database that will make users' lives easier,
it would be good to consider. 🙂

@quaelnix
Copy link
Collaborator Author

quaelnix commented Oct 4, 2023

Please tell us more about 'building density' for town class.

There is a branch on my BRouter fork that generates town tags based on building density: master...quaelnix:brouter:improve-town-and-forest-tags

And there are a couple of comments that contain examples here: #486 (comment)

But aside from the considerable performance impact this will also not work properly in countries with poor OSM data, which is why I did not follow through on that concept. My best concept for the town tag so far can be found here: #614

I would love to discuss the pros and cons of the different approaches and find the best solution as a team, but there doesn't seem to be much interest in taking the new pseudo tags to the next level. Which is a bit sad to be honest.

@afischerdev
Copy link
Collaborator

@devemux86

Usually some of the first things users expect from routers and they don't find in BRouter
are street names (#55), speed limits, maybe street information in response (e.g. tunnels).

I see the street name problem, but I doesn't see the others.
Take this sample and make all tags visible. You will get maxspeed and tunnel in the json output.

@afischerdev
Copy link
Collaborator

@quaelnix
Thanks for the hint. I tried it and it really takes a long time. I still think it's a good idea to do it over the buildings. Maybe the special idea of speeding things up will come along.

I also tried #614, it runs much faster. But it's broken down to 1000 residents per village. I can't say what that means to database. May be @EssBee59 have more experiences here. May be a whole world test could be done.

@quaelnix
Copy link
Collaborator Author

quaelnix commented Oct 9, 2023

I still think it's a good idea to do it over the buildings.

But take a look at cities like Sydney, Australia, where the majority of buildings are still missing.

Maybe the special idea of speeding things up will come along.

I'm pretty sure that a SQL expert would be able to speed up our sql code by a factor of 2 or more.

@quaelnix
Copy link
Collaborator Author

quaelnix commented Oct 9, 2023

Try this route with the Fastbike (low traffic) profile: https://brouter.de/brouter-web/#map=11/50.1237/8.7139/standard&lonlats=8.582151,50.202148;8.863721,50.077662&profile=fastbike-lowtraffic

Then toggle consider_town and compare the results, then enable consider_noise and once again toggle consider_town and compare the results and you will immediately understand why the needless dropping of the town class is plain stupid.

@afischerdev
Copy link
Collaborator

@quaelnix
Yes, no comfortable way around.

@quaelnix quaelnix closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants