openstreetmap / iD

🆔 The easy-to-use OpenStreetMap editor in JavaScript.
https://www.openstreetmap.org/edit?editor=id
ISC License
3.37k stars 1.21k forks source link

Double-tagged features can display an unexpected preset name in validation messages #6162

Closed Thorwynn closed 5 years ago

Thorwynn commented 5 years ago

When selecting this way at the northeast corner of York Maine: https://www.openstreetmap.org/way/435075515 iD throws this issue "Unmaintained Track Road crosses Administrative Boundary" It suggests these fixes: "Connect the features Use a bridge or tunnel Reposition the features"

This is obviously a wrong suggestion but other nearby tracks that cross admin boundaries do not throw this error, so I don't know what is happening. image I am using Chrome browser

kymckay commented 5 years ago

If you click the warning it takes you to map=19/43.26380/-70.65054

Looks like this is actually a valid error, because the track crosses a stream at this position which is also tagged as an admin boundary. The error message could probably be improved for such situations.

bhousel commented 5 years ago

Good catch- thanks @SilentSpike for checking. I agree we should improve the message that the validator shows.

Thorwynn commented 5 years ago

Nice I didn't know hovering on warning highlighted the error. I never would have caught that since the stream is not rendered as an admin boundary. OSMcarto properly renders it as an admin boundary but the iD editor does not do so while editing. Should this be opened as a separate issue?

bhousel commented 5 years ago

OSMcarto properly renders it as an admin boundary but the iD editor does not do so while editing. Should this be opened as a separate issue?

I don't understand the issue.. A thing that is tagged as both an admin boundary and a stream can really be rendered either way.

Thorwynn commented 5 years ago

I thought it would be obvious how dangerous or annoying it is to effectively make an admin boundary invisible. People take more liberties moving rivers around, but should be more cautious moving admin boundaries. Boundary relations could easily end up broken as well. In the area linked above, the admin/stream could be interpreted as crossing another nearby admin boundary in the imagery, so if someone attempted to "fix" the stream route, they would inadvertently mess up the boundary without knowing it. Hiding an admin boundary is not quite as bad as obscuring the coastline, but should never be done.

bhousel commented 5 years ago

Hiding an admin boundary is not quite as bad as obscuring the coastline, but should never be done.

Oh, iD hides the administrative boundaries by default. But when they are hidden, users can't edit them.

Thorwynn commented 5 years ago

I was easily able to drag the admin-boundary-tagged-stream across the other town line. Of course I didn't try saving it. I've never had any trouble editing town or county lines, such as aligning them to L3 parcel maps in Massachusetts. When I said "hiding" I meant obscuring the admin nature of the stream by rendering it only as a regular stream, providing no visual clue that it was also an admin boundary.

DujaOSM commented 5 years ago

In fact, I would rather warn against tagging physical objects with boundary=* tags, in line with recommendations on OSM wiki:

Avoid connecting boundaries to physical features like woods or rivers. Sooner or later these features change in reality and get updated in OSM – but usually the shape of the boundary remains. An exception is if the boundary is legally defined to be the physical feature.

I was bitten just recently with this: someone had moved a national park boundary to the riverbank, and when I started defining forest multipolygons along it, I broke the boundary relation. Granted, iD renderer gives priority to the boundary tag and thus does indicate that there's more than just a riverbank there, but it's just too easy to overlook.

bhousel commented 5 years ago

In fact, I would rather warn against tagging physical objects with boundary=* tags, in line with recommendations on OSM wiki:

When we detect boundaries mapped along physical features, what do you think iD should help our users to do? Split the boundary/physical feature into 2 (colinear) features? This seems worse.

I don't see a really great solution for these, and they are super common in OSM. The wiki page you quoted doesn't mention roads as physical features, and I think is probably the most common case.

Also want to remind everyone: iD hides boundaries by default, because there isn't much that casual mappers can do to improve them. The screenshot the OP posted is not how most users see things.

DujaOSM commented 5 years ago

Yes, split it into two (near-) colinear features. It isn't worse, it's how things are supposed to be done. Two real-world features -- two OSM objects.

I've drawn a LOT of administrative boundaries recently (they were missing from my area), all of them as separate layer, and I'm a happy man; nobody is going to mess with them unless they know what they're doing. Thus, I approve the iD's new (?) default.

For a counterexample, these guys reused physical objects for boundaries, and they didn't get it right.

Thorwynn commented 5 years ago

How about rendering river/stream admin-boundaries with white arrows instead of black? This illustrates what I was trying to say above: image

When you say iD hides boundaries by default, do you mean they are unchecked in the filter panel for new users the first time they edit?

bhousel commented 5 years ago

How about rendering river/stream admin-boundaries with white arrows instead of black?

I'd really prefer not to introduce special rendering for "rivers that are also boundaries" as opposed to "rivers that are not also boundaries".

do you mean they are unchecked in the filter panel for new users the first time they edit?

Yes..

tordans commented 5 years ago

Yes, split it into two (near-) colinear features. It isn't worse, it's how things are supposed to be done. Two real-world features -- two OSM objects.

I agree, the cleanest way would be to have boundaries separate from other features. It's also, how I read the various docs on the wiki; however, quite a few of them are only available in German (Example).

But, I think the iD live-validation-feature is not the right tool for the job. It's IMO not a one-click-action to "duplicate the way correctly and split the tags". Too much can go wrong. And even then, having two lines right above one another is a pain, so that would not help.

The only think iD live-validation could do IMO is tell the user "you are editing a river/coastline/street that is also a boundary which is not ideal. If you know how, please separate the boundary. Otherwise its better not to edit this element until someone split the tags. You could add a node here so other can continue the work." Maybe adding a note actually could be the one click action.

To solve the issue, IMO other tooling is better suited, like a post-commit-validation like KeepRight. This could then be used inside iD and also for a maproulette.org Challenge to cleanup borders.


Bryan, about the "border is hidden" feature. I could not reproduce it for the edge case, where the border is also a street (or other). Would the street be hidden, too (so the border-tag hides the street-part of the line as well)? In this case, the hiding-feature also needs those double-taggings to be cleaned up in order to work properly, right?

DujaOSM commented 5 years ago

But, I think the iD live-validation-feature is not the right tool for the job. It's IMO not a one-click-action to "duplicate the way correctly and split the tags". Too much can go wrong.

I sort-of-agree on one hand, but on the other, it's much easier for software to clone a long way automatically than do it manually: iD's copy+paste of a large object makes it quite distant from the original, and it's rather hard to align them back. But yes, a lot of things can go wrong.

But then, even issuing a warning would be a step forward, to make mappers aware of the issue. For example, I was blissfully ignorant about "Road crosses X" and similar issues until the latest iD release started warning about them, so at least now I tend to fix the most blatant ones of my own making.

And even then, having two lines right above one another is a pain, so that would not help.

Not really, as long as you can hide one of those from the "Features" panel. I hide and show them routinely: e.g. I don't want roads stand in the way when I'm defining landuse, or anything else when I work on boundaries. Snapping those features to one another is a sin, and makes any subsequent editing a real pain.

bhousel commented 5 years ago

Bryan, about the "border is hidden" feature. I could not reproduce it for the edge case, where the border is also a street (or other). Would the street be hidden, too (so the border-tag hides the street-part of the line as well)? In this case, the hiding-feature also needs those double-taggings to be cleaned up in order to work properly, right?

No, because the feature hiding code is written in a way that it considers these as streets instead of borders:

https://github.com/openstreetmap/iD/blob/891de117f4a970899bd22eed6a93cc519044dc7e/modules/renderer/features.js#L135-L143

I did some tests by making a shape that is a road and also an administrative boundary, and iD considers the road part as a road (for feature hiding and rendering)

Screenshot 2019-04-11 09 54 50

And if the road is also part of an administrative boundary relation, iD will prevent users from disconnecting it:

Screenshot 2019-04-11 09 54 41
bhousel commented 5 years ago

But, I think the iD live-validation-feature is not the right tool for the job. It's IMO not a one-click-action to "duplicate the way correctly and split the tags". Too much can go wrong. And even then, having two lines right above one another is a pain, so that would not help.

I agree with this.. I don't think there is anything we should do right now in iD to help people clean them up because it's easy to break things worse. And although having boundaries on other items is discouraged, it's not strictly wrong.

For this issue, I'd really like to just limit the scope to 1. improve the message and 2. consider adjusting the filtering rules so that features with a boundary and a physical tag are always treated like the physical feature (like roads in my example above).

DujaOSM commented 5 years ago

I don't think there is anything we should do right now in iD to help people clean them up because it's easy to break things worse. And although having boundaries on other items is discouraged, it's not strictly wrong.

I beg to differ, but the call is yours. It's documented as "strongly discouraged" enough to warrant at least a warning (which is, by the way, rather simple to implement). As much as multipolygons and admin boundaries are fragile, having them overlap with other types of ways makes them doubleplusfragile (I can think of several ways where a seemingly innocuous change breaks the relation and consequently geocoding). And, just like any other warning, one is free to turn it off if they find it annoying.

bhousel commented 5 years ago

I beg to differ, but the call is yours. It's documented as "strongly discouraged" enough to warrant at least a warning (which is, by the way, rather simple to implement).

I don't want to warn people about stuff they can't fix.

DujaOSM commented 5 years ago

Why can't they? Copy+paste the way approximately over the same place, and modify it, or just redraw a boundary from scratch (not that admin boundaries typically require surgical precision).

I've drawn, split and merged hundreds of boundaries with iD, it's not exactly rocket science. It tends to involve tedious working with very long features, but that's the nature of the beast.

kymckay commented 5 years ago

@DujaOSM It's technically possible, but if a validation was added to warn when boundaries are also tagged as physical features, new users are likely to not understand the significance of boundary features and could end up making things worse in an attempt to fix it.

However, @bhousel what if a validation was added but disabled by default? The same as the boundary data layer.

I do think it's tangential to this particular issue and your scope points 1 & 2 both sound like reasonable solutions to this.

bhousel commented 5 years ago

It's a lot more than copy and paste..

If we had an "unmerge tags" action it would need to do this:

DujaOSM commented 5 years ago

"Perfect is the enemy of the good."

I'm not hard-pressing that iD should do everything (or even anything) automatically; but identify the issue and give me tools to fix that myself. Let's consider the mentioned "road crosses ditch" warning, which I see dozens of times daily, as it is triggered for all roads and ditches I touch, even if I didn't author them myself. Depending on my mood, I can fix it (4-5 clicks per issue), ignore it, or turn off the warning. Same thing here: let the iD identify the issue but do not force the user to do anything. As always with user interface, a good design is a compromise on the scale between "let the software always do the right thing" (on the expense on complexity and bugs) and "let the user do all the work by themselves" (on the expense of user annoyance and attrition).