abrensch / brouter

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

Use cycleway:surface for routing #623

Open pkubowicz opened 1 year ago

pkubowicz commented 1 year ago

In https://github.com/nrenner/brouter-web/issues/438 I reported that a way with:

surface=paved
cycleway:surface=asphalt
footway:surface=paving_stones

is reported as 100% paved (instead of 100% asphalt).

The response I got was:

should be fixed (not only for asphalt, obviously) in either BRouter's profiles or in BRouter's tag aliasing. We want to not only count the tag in our statistics, but use it for routing too.

So I'm reporting here.

quaelnix commented 1 year ago

As suggested by rkflx, a fix for the trekking profile could look like this:

- assign ispaved = surface=paved|asphalt|concrete|paving_stones|sett
+ assign ispaved = or surface=paved|asphalt|concrete|paving_stones|sett \
+                     cycleway:surface=paved|asphalt|concrete|paving_stones|sett

the same code could be used for the fastbike profile.

afischerdev commented 1 year ago

I added the footway:surface to the next generation lookups.dat

rkflx commented 1 year ago

As suggested by rkflx, a fix for the trekking profile could look like this

FTR, that's not what I said. I said that this "triggers the correct behaviour" for that particular example, and that "not considering cycleway:surface [...] should be fixed". I did in no way describe how the actual fix would look like, so don't make it look like I "suggested a fix" that "could look like this".

Also, either properly @-mention my name, or link to the actual comment. Linking my name without actually notifying me makes people question your intentions.

in either BRouter's profiles or in BRouter's tag aliasing

I might be talking nonsense and should probably read the code, but is there a way in BRouter to automatically alias surface to surface|cycleway:surface for profiles setting validForBikes (and surface|footway:surface for validForFoot)?

This seems a bit more elegant and could ease profile maintenance compared to having to repeat the whole shebang everywhere each time.

(Ideally such aliasing should be reflected in the emitted messages too, i.e. when a profile has validForBikes and evaluates surface, the way's message should say cycleway:surface if that's what was used for routing.)

If that does not work yet or is too hard to implement, changing the profile as mentioned is probably the second best option.

quaelnix commented 1 year ago

is there a way in BRouter to automatically alias surface to surface|cycleway:surface

I also have not looked at that part of the code yet, but maybe @afischerdev knows more.

This seems a bit more elegant and could ease profile maintenance

I prefer as little preprocessing as possible, but maybe in this case it indeed would make sense to make an exception?

changing the profile as mentioned is probably the second best option

Yes, it certainly looks tempting, but I wouldn't change it without regression testing.

rkflx commented 1 year ago

surface|cycleway:surface

To clarify what I mean by that (in case it wasn't obvious):

Essentially provide what is called "tag normalization" in nrenner/brouter-web#460 (also note fixes in nrenner/brouter-web#550 and nrenner/brouter-web#553).

not only for asphalt, obviously

And probably not only for surface, but for smoothness too.


That being said, I just discovered another complication with the idea of automatic aliasing:

We'd not only have to look at validForBikes, but would need to differentiate between a profile wanting to route over the (car) lane part of the highway or the cycleway part in case of non-mandatory cycleways. I guess we should leave that to profiles, after all, or only apply the aliasing if cycleway:surface is unambiguous, e.g. if cycleway=* is unset or for highway=path|track|cycleway. Sigh.


changing the profile

I wouldn't change it without regression testing.

What particular regressions do you have in mind?

Apart from that I don't see any room for trade-offs or subjective preferences here, as it would be merely about evaluating more accurately what has been mapped.

quaelnix commented 1 year ago

What particular regressions do you have in mind?

Basically the ones you mentioned above:

Plus the ones we probably did not think about it yet.

I just discovered another complication with the idea of automatic aliasing

We would also have to properly consider the oneway logic.

afischerdev commented 1 year ago

I also have not looked at that part of the code yet, but maybe @afischerdev knows more.

I'm sorry, no. I'll do have a look later on - but will take a few weeks. And we will need the lookups update #458. Seeing the weight of the surface value, I could think of ordering the surface values in the lookups.dat file.

rkflx commented 1 year ago

cycleway:surface=paved shouldn't overwrite surface=asphalt

I disagree. When the cycleway is mandatory and a patchwork of different paving styles, paved would be much more accurate than asphalt, even though the latter might seem more specific at first glance.


Thinking more about this, perhaps we could define aliasing a bit differently (| meaning "fallback if not existing" as detailed previously, and _ illustrating the tag being an alias):

That way profiles could simply choose what they require (e.g. it would be explicit whether the car lane or a cycleway is meant), while still being less verbose (no repetitions and no extra conditions necessary). Maybe we could even move aliasing to rd5 creation time that way, instead of having to process it dynamically during routing.

Note that it might make sense to be even more generic here, e.g. define a whole class of aliases like A_X for A:X|X with A ∈ {cycleway, footway, …} and X ∈ {surface, smoothness, …}. Are there any further tags for you could think of?

AFAICT from (finally) reading the code, in lookups.dat we currently only support aliasing like this: tag;001 value1 value2 value3 (where value1 will become an alias for value{1,2,3} in the expression tag=value1). This is different from aliasing tags themselves as discussed here. However, there is also generatePseudoTags which we might be able to extend.

OTOH, it might be too much work for too little gain…

afischerdev commented 1 year ago

@rkflx

Does this mean all ways have a new cycleway_surface or footway_surface tag at the end - when they have a surface tag? Could be blow up the data source.

The more I look at it, the more I think: A simple surface has no place in cycleway:surface and/or footway:surface combination. I followed some ways that are visible in GMap Streetview like this. Visible is the traffic seperation but where is the asphalt?

There are more tags around:

cycleway:both:surface
cycleway:right:surface
cycleway:left:surface

sidewalk:surface
sidewalk:both:surface
sidewalk:right:surface
sidewalk:left:surface
rkflx commented 1 year ago

Does this mean all ways have a new cycleway_surface or footway_surface tag at the end - when they have a surface tag?

We could coalesce all specialized tags into a single tag per use case, so while there would be some duplication (which might be possible to avoid by deferring selected parts of the fallback algorithm to runtime), we could also drop other tags not needed anymore in such a scenario.

Could be blow up the data source.

Whether or not rd5 sizes will be affected is yet to be determined, it's not even clear where/when to do the aliasing yet. Also note that evaluating more complex expressions in profiles comes with quite a sizable performance penalty too, so there is always a space/time trade-off.


In summary, it does not seem trivial to answer a question like "Assuming a cyclist using mandatory cycleways and riding in direction X on way Y, what will the surface there look like?", since only looking at surface can be incomplete or even misleading, as we've seen.

On the contrary, it can be quite complicated. This brings us back to the original dilemma: Is it reasonable to expect each profile author (which includes advanced users too) to know about and evaluate all of these tag combinations and special conditions, or should we pre-process the complexity into an alias once? Or maybe choose a different approach and extend profile syntax to allow for | and/or * in the left side of tag=value expressions too? Perhaps we should just ask profile authors what they prefer.

Anyway, I'll stop my investigations here for now. Nevertheless I still believe we should probably somehow consider cycleway:surface for routing. I'll be curious though to watch with which constructive solutions you'll come up with.

quaelnix commented 1 year ago

I spent an hour trying to find an example (using this overpass query: https://overpass-turbo.eu/s/1zIa) were:

- assign ispaved = surface=paved|asphalt|concrete|paving_stones|sett
+ assign ispaved = or surface=paved|asphalt|concrete|paving_stones|sett \
+                     cycleway:surface=paved|asphalt|concrete|paving_stones|sett

would affect the trekking profile routing and only found this one:

Querying cycleway:surface in the trekking profile increases the calculation time by about 0.65%.


As a side note, the above overpass query has a near 100% success rate in finding map errors.

quaelnix commented 1 year ago

I'll be curious though to watch with which constructive solutions you'll come up with.

Probably an unpopular opinion, but I would suggest to do nothing. No tag aliasing and no profile change.

EssBee59 commented 11 months ago

See discussion in https://github.com/abrensch/brouter/pull/641

rkflx commented 11 months ago

See discussion in #641

Could you expand on how the linked PR relates to the issue at hand, since the tag mentioned in the issue's title (cycleway:surface) is nowhere to be found in that PR?

EssBee59 commented 11 months ago

Tank for the feedback, Sorry! I confused "cycleway" and "cycleroute"... ( https://github.com/abrensch/brouter/issues/622 in my mind)