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

Open
rhhsm opened this issue Oct 23, 2023 · 21 comments · May be fixed by #5453
Open

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

rhhsm opened this issue Oct 23, 2023 · 21 comments · May be fixed by #5453
Assignees
Labels
enhancement needs PR accepted suggestion, but only if a contributor implements it

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs PR accepted suggestion, but only if a contributor implements it
Projects
None yet
6 participants