search

abrensch / brouter

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

Fix bug in exclusion of small water bodies #601

Closed quaelnix closed 1 year ago

quaelnix commented 1 year ago

This little pond was causing estimated_river_class=2 on the Hackländerstraße:

estimated_river_class

It would probably also be worth discussing whether it would not make sense to raise the threshold back to something near the original value of around 8000 square meter: https://github.com/abrensch/brouter/blob/e66468b091506833eb5890656a0ef3a538397ab2/misc/scripts/mapcreation/brouter.sql#L475

afischerdev commented 1 year ago

Thanks for your fix. But please hang on a moment, @EssBee59 will not comment at the moment. And I have the task of checking in the latest changes.

quaelnix commented 1 year ago

@afischerdev, it's been a month now with zero input on this.

Why don't we fix this bug and what prevevents us from going back to a sane threshold of around 8000 square meters?

afischerdev commented 1 year ago

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

EssBee59 commented 1 year ago

It's ok to have different opinions, but I don't understand to create an issue with the label "bug" directly out of it!

I prefer to keep 1000 qm, else a lot of nice water places would not be considered, and this would probably lead to the report of new bugs. Examples:

case1 case2

quaelnix commented 1 year ago

I prefer to keep 1000 qm

This PR fixes the bug that the threshold is currently ~ 1000 square feet instead of 1000 square meters.

quaelnix commented 1 year ago

Another example. This little basin (less than 300 m² in size) is causing estimated_river_class=4 on Wiesenstraße:

afischerdev commented 1 year ago

This PR fixes the bug that the threshold is currently ~ 1000 square feet instead of 1000 square meters.

I'm not sure about this. A sql like this select p.osm_id, p.water, st_area(p.way), st_area(st_transform (p.way, 4326)::geography) area_4326, st_area(st_transform (p.way, 4326)::geography) / 0.3048 ^ 2 sqft_4326 from polygons p where p.water is not null limit 10; brings out e.g this pont - controlled by this - switch to satellite view

osm_id water st_area area_4326 sqft_4326
833830724 pond 395.8 154.1 1659.6

Seems the area_4326 is the correct value for sqr meter.

quaelnix commented 1 year ago

@afischerdev, yes, I got the square feet wrong, but that doesn't change the fact that the current code never gives a threshold of 1000 square meter unless you are at the equator.

Essbee59 wants a 1000 square meter threshold, I'm fine with a 1000 square meter threshold and this pull request does get us a 1000 square meter threshold, so please let us merge it.

EssBee59 commented 1 year ago

Yes, 1000 is at the equator! Historically the first calculation were done for Europe only... Yes I can / will next change that in the same way as I did by nature_reserve (in Greenland, there was a real need for that)

It seems extremly important for you to be "exact" by a parameter that can just be set with feeling / testing... I would prefer to concentrate my energy on really critical points (as https://github.com/abrensch/brouter/issues/622) Regards

quaelnix commented 1 year ago

that can just be set with feeling / testing...

Yes, but the intention was 1000 square meters and not < 300 square meters. That's the whole point of this pull request.

I would prefer to concentrate my energy on really critical points

Me too, see: https://github.com/abrensch/brouter/pull/614