abrensch / brouter

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

No needless dropping of 'estimated_town_class' #602

Closed quaelnix closed 2 months ago

quaelnix commented 12 months ago

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 commented 11 months ago

@afischerdev, have you thought about it yet?

afischerdev commented 11 months ago

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

afischerdev commented 10 months ago

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

EssBee59 commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

@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 commented 9 months ago

@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

The arguments are understandable.

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

@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 commented 9 months ago

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 commented 9 months ago

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. :slightly_smiling_face:

quaelnix commented 9 months ago

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: https://github.com/abrensch/brouter/compare/master...quaelnix:brouter:improve-town-and-forest-tags

And there are a couple of comments that contain examples here: https://github.com/abrensch/brouter/issues/486#issuecomment-1656730847

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: https://github.com/abrensch/brouter/pull/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 commented 9 months ago

@devemux86

Usually some of the first things users expect from routers and they don't find in BRouter are street names (https://github.com/abrensch/brouter/issues/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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@quaelnix Yes, no comfortable way around.