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

Way with complicated surface tag shows as without surface tag #5330

Closed
rhhsm opened this issue Oct 23, 2023 · 28 comments · Fixed by #5984
Closed

Way with complicated surface tag shows as without surface tag #5330

rhhsm opened this issue Oct 23, 2023 · 28 comments · Fixed by #5984
Assignees

Comments

@rhhsm
Copy link

rhhsm commented Oct 23, 2023

This way https://www.openstreetmap.org/way/787847733 and the oneway ways the the northeast have a rather complicated surface that is different for different lanes. However, on SC its surface is quested and in the surface overlay it shows red, i.e. as if its surface has not been tagged yet.
Screenshot (23 okt

This creates the risk that an unsuspecting user will decide that it's "mostly asphalt" or "mostly sett" and will answer the quest like this, which would be wrong.
Probably a very rare occasion so not very urgent...

Expected Behavior
Ways that have a more complicated surface tag (for instance surface:lanes:*=*|*; there may be others) should not be considered as being without surface tag. No surface quest should appear, and they should not appear as red in the surface overlay.

Versions affected
54.1

@rhhsm rhhsm added the bug label Oct 23, 2023
@matkoniecz
Copy link
Member

This creates the risk that an unsuspecting user will decide that it's "mostly asphalt" or "mostly sett" and will answer the quest like this, which would be wrong.

Expected answer is surface=paved which even has as example concrete and asphalt on one surface area.

@matkoniecz matkoniecz removed the bug label Oct 23, 2023
@rhhsm
Copy link
Author

rhhsm commented Oct 23, 2023

If the surface was indeed unknown, that would be the correct answer, or even better if a note was created. But here the surface is already described, so it would be useless to add another description of the surface in words. I'm afraid, however, that many users would be tempted to enter a wrong answer.

@matkoniecz
Copy link
Member

Is there any case of this problem actually happening anywhere?

@matkoniecz matkoniecz added the feedback required more info is needed, issue will be likely closed if it is not provided label Oct 23, 2023
@rhhsm
Copy link
Author

rhhsm commented Oct 24, 2023

It's probably very rare, so solving it has low priority.

@HolgerJeromin
Copy link
Contributor

But here the surface is already described, so it would be useless to add another description of the surface in words.

I am not sure. Perhaps not in SC, but in the data it would be useful for programs which are not able to parse this information.

@Helium314
Copy link
Collaborator

In most cases I found, surface was tagged before someone added surface:lanes[:*]. But there are some cases where StreetComplete users only tagged one of the surfaces, changed paved to one of the surfaces, or even a added a surface different to any of the lane surfaces, e.g. https://www.openstreetmap.org/way/1004372340/history, https://www.openstreetmap.org/way/1030195917, https://www.openstreetmap.org/way/241497124/history

(and I noticed that in northeastern Germany, surface:lanes seems to be used for single lane tracks)

@mnalis
Copy link
Member

mnalis commented Oct 24, 2023

I agree with @rhhsm about not coloring in red such already detail-mapped cases; as it basically invites users to lower the OSM data quality (and also needlessly wastes mapper time, but that is much lower-priority issue in this case)

@matkoniecz matkoniecz removed the feedback required more info is needed, issue will be likely closed if it is not provided label Oct 24, 2023
@matkoniecz
Copy link
Member

I plan to look how many such places exist worldwide, maybe manual handling will take less time than extra filtering.

For now I commented on https://www.openstreetmap.org/changeset/122193302 https://www.openstreetmap.org/changeset/142930527 https://www.openstreetmap.org/changeset/136056524

@rhhsm
Copy link
Author

rhhsm commented Oct 25, 2023

But here the surface is already described, so it would be useless to add another description of the surface in words.

I am not sure. Perhaps not in SC, but in the data it would be useful for programs which are not able to parse this information.

A written description would be even more difficult to parse for programs.

The best answer in the cases described above would be to reply with surface=paved, but that cannot be done in SC without also adding a description.
I added surface=paved to the above ways with iD, but that doesn't help much because SC still shows them as red in the overlay and quests them for surface.

@matkoniecz
Copy link
Member

matkoniecz commented Oct 25, 2023

surface:note=different lanes have different surfaces or other surface:note tag will change display (with surface=paved).

For example, see https://www.openstreetmap.org/way/4708393/history

@matkoniecz
Copy link
Member

@rhhsm What you think about tagging such as https://www.openstreetmap.org/way/4708393/history ? Would you consider it as preferable to omitting surface ?

(I do not want to apply weird tagging as workaround, but I think that it is genuinely better than no surface tag AND it solves problem raised here)

@matkoniecz matkoniecz added the feedback required more info is needed, issue will be likely closed if it is not provided label Oct 26, 2023
@rhhsm
Copy link
Author

rhhsm commented Oct 26, 2023

If we should not tag for the renderer, we should certainly not tag for the OSM editor app :) So I think we should add surface info as we like and see fit, even if it needs to be done with a surface:lanes tag. That such a tag is ignored by an app (and probably also by many humans because it's usually not very visible) is someone else's problem. And I'm afraid a surface:note tag will likely be ignored as well (though it might work for SC).

@matkoniecz
Copy link
Member

matkoniecz commented Oct 26, 2023

If we should not tag for the renderer, we should certainly not tag for the OSM editor app :)

Just because it is supported by app it does not mean that using this tagging scheme is incorrect.

I agree, that is why asked

Would you consider it [adding surface=paved surface:note=different lanes have different surfaces] as preferable to omitting surface ?

and commented

I do not want to apply weird tagging as workaround, but I think that it is genuinely better than no surface tag AND it solves problem raised here

@mnalis
Copy link
Member

mnalis commented Oct 26, 2023

Just because it is supported by app it does not mean that using this tagging scheme is incorrect.

I agree; in hindsight it was bad idea to name this principle as "Do not tag for the renderer", as the actual meaning of the phrase is "Do not mistag for the renderer"; and its popular phrasing makes it frequently misunderstood.

Tagging (correctly) for the renderer (and for any other data consumer) is not only accepted, it is in fact the whole purpose of OSM existence 😃

I plan to look how many such places exist worldwide, maybe manual handling will take less time than extra filtering.

You mean human time to implement extra filtering functionality (i.e. adding extra tag surface:lanes to ignore-coloring-as-red; which seems easy to me, although I have not looked at the code yet) or CPU-time (i.e. cumulative slowdown affecting UI, which I would think should also not being the issue)?

@matkoniecz
Copy link
Member

matkoniecz commented Oct 26, 2023

You mean human time to implement extra filtering functionality (i.e. adding extra tag surface:lanes to ignore-coloring-as-red

and surface:lanes:backward and surface:lanes:forward and skipping in surface quest and tests and maintaining even more complex code (it is already on edge of what I can understand)

it is also about precedent - there are many cases where such similar detail may appear, and could be solved by tagging surface and surface:note

and future confused issues "why this is colouring in this way" or "why quest is not asked here"


Note that I am right now processing fixme=surface and preparing bot edit for weird surface values, see say https://forum.openstreetmap.fr/t/review-requested-before-proposing-bot-edit-for-automated-fixing-of-surface-values/18419

doing all that in StreetComplete would not work well

@mnalis
Copy link
Member

mnalis commented Oct 26, 2023

I'm not sure if we're talking about same thing, I was thinking more about just not coloring in red such already-complexly-mapped surfaces in Surface overlay; not about fixing strange values or attempting to consolidate them, nor changing quests etc.

IOW It would not prevent people who decide on changing the surface; but it would not invite them to do so (which it currently does by waving that red cloth in front of mapper eyes 😸).

E.g. adding extra check to this:

// not set but indoor, private or just a "virtual" link -> do not highlight as missing
if (isIndoor(element.tags) || isPrivateOnFoot(element) || isLink(element.tags)) {
Color.INVISIBLE

via something like this (I have not checked syntax / tried to compile, just an example):

- if (isIndoor(element.tags) || isPrivateOnFoot(element) || isLink(element.tags)) { 
+ if (isIndoor(element.tags) || isPrivateOnFoot(element) || isLink(element.tags) || isComplexSurface(element.tags)) { 

[...]

+ private fun isComplexSurface(tags: Map<String, String>): Boolean =
+    tags["surface:lanes"] || tags["surface:lanes:forward"] || tags["surface:lanes:backward"] || tags["surface:lanes:both_lanes"]

And adding a test or two for this case. It does not seem to me to raise the complexity significantly? Or would that not work (I have not tried writing overlays yet)? Or are we simply talking about different things?

I would be willing to write and test that code to reduce load on main devs, if such a fix is deemed acceptable?

it is also about precedent - there are many cases where such similar detail may appear, and could be solved by tagging surface and surface:note

Sure, but I'm not talking here about having StreetComplete use something else - surface:note usage is just fine. But convincing every mapper on planet to use that method (instead of surface:lanes etc what they use currently) is surely going to be way more complex and time consuming than those few lines of code change above?

Note that I am right now processing fixme=surface and preparing bot edit for weird surface values, see say https://forum.openstreetmap.fr/t/review-requested-before-proposing-bot-edit-for-automated-fixing-of-surface-values/18419

Nice effort, thanks for that!

doing all that in StreetComplete would not work well

Which is why I'm not proposing anything like that 😺

@matkoniecz
Copy link
Member

instead of surface:lanes etc what they use currently

I am not proposing to do it instead of perfectly valid surface:lanes but in addition to it

I would be willing to write and test that code to reduce load on main devs, if such a fix is deemed acceptable?

I think that such surface should be rather shown as complex (in black, like surface=paved with surface:note), rather than not at all but in principle it would work. Note that also SurfaceQuest would need to be modified.

I am not enthusiastic as from own experience I expect that there will be an endless parade of similar ideas. I was dealing with it in some places.

@mnalis
Copy link
Member

mnalis commented Nov 6, 2023

I am not proposing to do it instead of perfectly valid surface:lanes but in addition to it

So (if I'm understanding it correctly), e.g. taking the example from the top of the issue, if the way was tagged:

surface:lanes:forward=asphalt|sett
surface:lanes:backward=asphalt|sett

you would leave those existing tags in place but also add surface:note=* (with text akin to in each direction, outermost (right) lanes are sett, and innermost (left) lanes are asphalt or similar)?

Am I understanding that correctly? If so, what would be the advantage of doing that human-readable transcription of machine-readable tags? (and if there are clear advantages, should we be doing such human-readable tags transcription for other tags too?)

Dunno 🤷, to me it would seem to have more problems then it solves (adding useless duplication is lesser of the problems; breaking 2NF in that way would require endless effort to keep those tags in sync should any of them change, which might not be possible to automate in 100% of the cases, thus requiring possible separate database for storing exceptions, human time for reviews, bot upgrades & maintenance for new cases etc.)

I think that such surface should be rather shown as complex (in black, like surface=paved with surface:note), rather than not at all but in principle it would work.

Makes sense, thanks.

Note that also SurfaceQuest would need to be modified.

Yes, good point, the quest also should not be asked on such already-detailed-surfaces.


If nobody objects (or prefers to do it themselves!), I could give it a try.

@westnordost
Copy link
Member

westnordost commented Nov 14, 2023

The following PR would be accepted:

(In a nutshell: surface:lanes (etc.) is treated as if a surface:note was present. In detail:)

  • change surface quests so that when specifying paved or another general value on a way that has surface:lanes (etc.) a surface:note is not be required when selecting such a general value. Conversely, a general value plus surface:lanes (etc.) but without surface:note shall not trigger the creation of the quest
  • + Tests
  • change surface overlay in the same way. For coloring: Red if surface is missing, the same color as when both surface and surface:note are present if surface and surface:lanes (etc.) are present.

@westnordost westnordost added enhancement needs PR accepted suggestion, but only if a contributor implements it and removed feedback required more info is needed, issue will be likely closed if it is not provided labels Nov 14, 2023
@westnordost
Copy link
Member

Do you intend to implement it, @mnalis ?

@mnalis
Copy link
Member

mnalis commented Jan 20, 2024

OK, I'll give it a go

@mnalis mnalis self-assigned this Jan 20, 2024
@Dimitar5555
Copy link
Contributor

Expected answer is surface=paved which even has as example concrete and asphalt on one surface area.

After some time SC asks the user what is the surface again (or the user uses the surface overlay) and we are back at square one. See version 35 of https://www.openstreetmap.org/way/275480468/history.

@westnordost
Copy link
Member

Are you sure? I think it should ask the surface again immediately, because when a general surface like paved is selected, it always expects a note.

@westnordost
Copy link
Member

westnordost commented Oct 16, 2024

I have been rummaging through @mnalis draft PR with the intention to finish it.

However, with heavy heart, I have now reached a point where I'd actually rather close this issue as will-not-fix with the advice to always add a surface:note in cases where a generic surface has to be tagged because different lanes have different surfaces, regardless of the presence of surface:lanes.

The reason is, in a nutshell, that it would add too much complexity to the code.

In particular, it adds too many asterisks to the code, in the sense that having to handle surface:lanes as sufficient replacement for surface:note in case a generic surface was specified breaks a lots of abstractions made in the code:
surface can be applied to many things - roads, paths, pitches, sidewalks (left and right), cycleways (left and right) - , so the code has been written to accommodate that. But only for roads, surface:lanes is relevant.
So basically, we would end up with "but for roads that have surface:lanes tagged..."-exceptions basically everywhere where anything with surface is done. Or refactor it all to be even more generic in order to accommodate this somehow in a clean manner, but this is even more effort.

(By the way, if there only was the surface quests, it wouldn't be such an issue, but to accommodate it in the overlay is a real PITA.)

So, anyway, sorry that this is not going to be implemented and also sorry to @mnalis that he poured some work into this already.


But, there is something that came to my mind when reviewing the whole "generic surfaces plus surface:note thing".

Do we really need to require a surface:note when a generic surface is set? If I remember correctly, the following things changed since the decision was made to require a note when a surface is under-specified:

  1. surfaces are no longer grouped by paved, unpaved, ground and these general values are displayed at the very bottom of the list, making them much less prominent

  2. the surface quest is asked again every X years. SC always gives a warning when the surface is under-specified, so it is very likely that on re-survey, users will specify a specific surface.¹

  3. we have the surface overlay with which it is somewhat easy to spot and correct under-specified surfaces

¹ However, surface quests are rarely re-asked because they are only re-asked if the road hasn't been touched in X years. Roads are generally touched more often, so this mechanic isn't working well and never did, TBH.

So the original reason to ask again if surface is a generic value (and use surface:note as a marker tag that indeed it was checked and is the most specific that is possible) was to reduce the huge number of generic and thus relatively useless surface tags.
But it could be solved differently, too. Maybe some other tag could be used as a marker. For example, always re-ask for surface if it was a generic before and didn't have a check_date:surface yet?

@westnordost westnordost closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2024
@westnordost
Copy link
Member

@matkoniecz what do you think about this?

@matkoniecz
Copy link
Member

In general, personally I am not really liking when editors are pushing some tagging in one direction or another with primary justification as "it would add too much complexity to the code"

But in this case I think that I would use surface:note or note also as a human editor, so I consider it as fine here.

@westnordost
Copy link
Member

I rather meant, what do you think about the latter part of my previous post. Think about why we require surface:note for in the first place: We want (SC) users to resurvey places where the surface type has been under-specified. And if this is our motivation to do this, there is another solution for it. E.g., something like this in the quest filter:

- or surface ~ paved|unpaved and !surface:note
+ or surface ~ paved|unpaved and !check_date:surface

This forces SC users to re-survey any road with a generic surface once.

@matkoniecz
Copy link
Member

I agree with this idea and I like it.

@westnordost westnordost reopened this Oct 22, 2024
@westnordost westnordost assigned westnordost and unassigned mnalis Oct 22, 2024
@westnordost westnordost removed the needs PR accepted suggestion, but only if a contributor implements it label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment