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

add validation to check against newly created old-style multipolygons #3933

Closed
tyrasd opened this issue Mar 28, 2017 · 19 comments
Closed

add validation to check against newly created old-style multipolygons #3933

tyrasd opened this issue Mar 28, 2017 · 19 comments
Labels
validation An issue with the validation or Q/A code

Comments

@tyrasd
Copy link
Member

tyrasd commented Mar 28, 2017

following up on #3908 and osmlab/fixing-polygons-in-osm#27 (comment), it would make sense to add a validation rule that checks whether newly created multipolygons follow the modern tagging scheme (i.e. tags on the relation, not outer ring) – e.g. similarly how we currently warn if a user tries to upload an untagged closed way (with area=yes).

@bhousel
Copy link
Member

bhousel commented Mar 28, 2017

AFAIK, iD only creates the new kind of multipolygon when merging line-line or line-area.

For iD to create an old style multipolygon, I think someone would need to manually add a tagged outer ring to a new multipolygon relation. In that case, rather than validate, maybe we should just silently move the tags from the outer to the multipolygon when the actionAddMember or actionChangeMember step happens? I think we can do this with actionChangePreset, so that only the important tags get changed.

I kind of doubt that this check makes sense as a validation because:

  • it's not likely that people are manually creating multipolygons
  • it's somewhat hard to fix - I fixed a few old legacy multipolygons in iD yesterday, but I don't think most people would know to select the multipolygon, assign the preset, select the outer, delete the relevant tags.

It's worth mentioning that we also don't show users the right way to do it. The walkthrough has been on my mind this week, and I'm thinking of maybe adding a second "Advanced Editing" walkthrough that could teach how to create multipolygons with the merge operation among other things.

@tyrasd
Copy link
Member Author

tyrasd commented Mar 28, 2017

For iD to create an old style multipolygon, I think someone would need to manually add a tagged outer ring to a new multipolygon relation.

That's also what I think is the case.

it's not likely that people are manually creating multipolygons

Well, a few people apparently do (see osmlab/fixing-polygons-in-osm#27 (comment)). Maybe intentionally, maybe following out-of-date instructions??

just silently move the tags from the outer to the multipolygon when the actionAddMember or actionChangeMember step happens?

Hmm. That could be convenient. Still, I think that there could/should be a validation error in case the created multipolygon still doesn't have tags (e.g. when created from a fresh area=yes polygon or an open ring segment): I think it'd be quite logical that if we warn when one saves one of these area=yes ways, to also warn in the equivalent multipolygon case.

[walkthrough]

👍

@slhh
Copy link
Contributor

slhh commented Mar 28, 2017

In that case, rather than validate, maybe we should just silently move the tags from the outer to the multipolygon when the actionAddMember or actionChangeMember step happens?

I think we can't do that. Multipolygon members can have their own properties. We don't know which tags the user intends to add later. An automatic decision to transfer tags based on incomplete data is not viable.

Valid use of area properties on outer members is rare, except of old style multipolygons. Therefore, iD might ask the user if tags should be transfered in case of adding an way with area tags as outer to an untagged multipolygon.

We can clean up automatically before saving to OSM, because at this stage the user should have added all intended information:
In case the multipolygon is untagged, and all outers do have the same tags, and the tags are not valid for lines, we can move the tags to the multipolygon.
We should also do another cleanup step here:
In case a multipolygon has no inner members, convert it into a closed way area, provided that the outer members meet the conditions:

  • are forming a single ring
  • aren't tagged
  • aren't member of any other relation
  • don't have too much nodes in total

Well, a few people apparently do (see osmlab/fixing-polygons-in-osm#27 (comment)).

Are these really valid old style multipolygons? Do their members have tags which are suitable for areas only? Do they have more than one member?

Maybe these are accidentially created multipolygons with a single member, or untagged features with line properties on its members.

@bhousel bhousel added the considering Not Actionable - still considering if this is something we want label Mar 29, 2017
@bhousel
Copy link
Member

bhousel commented Apr 2, 2017

Let's not do anything for this. We can reopen if there is data that shows a lot of users are creating old-style multipolygons with iD, but for now I don't think it's a common enough thing to spend time on.

@bhousel bhousel closed this as completed Apr 2, 2017
@sebastic
Copy link

sebastic commented May 4, 2017

The existing old-style multipolygons have been fixed. Pretty much all the newly creating old-style multipolygons that get added on a daily basis are by iD users.

Please reopen this issue and fix iD to do the right thing. It is a terrible decision not to handle multipolygons properly for the default editor on osm.org.

@bhousel
Copy link
Member

bhousel commented May 4, 2017

The existing old-style multipolygons have been fixed. Pretty much all the newly creating old-style multipolygons that get added on a daily basis are by iD users.

Sure, if this is true I would be happy to add a validation, otherwise I would prefer to spend my time working on improvements to iD that benefit more users. Do you have examples?

@sebastic
Copy link

sebastic commented May 4, 2017

Some examples:

@bhousel
Copy link
Member

bhousel commented May 4, 2017

Thanks @sebastic, it's helpful to know that people are actually doing this daily. I'll add the validation 👍

@bhousel bhousel reopened this May 4, 2017
@bhousel bhousel removed the considering Not Actionable - still considering if this is something we want label May 4, 2017
@sebastic
Copy link

sebastic commented May 4, 2017

And thank you for reconsidering this issue.

@bhousel
Copy link
Member

bhousel commented May 4, 2017

Ok I added this. It looks like this now, with a warning and a tooltip explaining how to fix the issue:

screenshot 2017-05-04 12 29 42

@sebastic
Copy link

sebastic commented May 4, 2017

Looks good. The validation may be too simple for cases where some tags on the outer way are appropriate like barrier=fence and the relation should have landuse=*.

@bhousel
Copy link
Member

bhousel commented May 4, 2017

Looks good. The validation may be too simple for cases where some tags on the outer way are appropriate like barrier=fence and the relation should have landuse=*.

This warning will appear if the relation has no interesting tags and the outer way has some interesting tags.

We can't give guidance to the user about which tags belong where, but I think it's fair to assume that anyone who creates multipolygons this way should be able to figure it out.

A validation like "fences belong on ways and landuses belong on multipolygons" is really out of scope for an editor, but would be a great thing to add to any of the dedicated OSM Q/A tools.

@sebastic
Copy link

sebastic commented May 4, 2017

I think you're overestimating iD users, experienced users who know what they're doing tend to use JOSM.

This warning is already bettter than nothing. And we can't expect iD to every become as full featured as JOSM. So I guess we should leave it at this.

@HolgerJeromin
Copy link
Contributor

@sebastic iD generates itself no old-style polygons. So what bhousel says: When someone was able to build an old-style polygon with iD it was on purpose and he should be able to fix it.

@sebastic
Copy link

sebastic commented May 4, 2017

iD users are generally not experienced mappers, and are not able to fix these issues without assistance.

It's the Potlatch story all over again. Inexperienced mappers create issues because the default OSM editor does not help them do the right thing.

@pnorman
Copy link
Contributor

pnorman commented May 5, 2017

iD does help them do the right thing - multipolygons it automatically creates when forming a geometry that requires them are new-style multipolygons. Just like with any editor which allows manually editing relations, it's possible to create an old-style MP.

If they're still creating old-style MPs, we need to know why to know the appropriate changes.

@sebastic
Copy link

sebastic commented May 5, 2017

But iD doesn't help them by providing an advanced validator like JOSM which does a much better job at helping users do the right thing.

I'm leaving this conversation now, as iD will never live up to my expectations of a decent editor. Thanks for 660511e, that is a very welcome change.

@fluous
Copy link

fluous commented May 7, 2017

@pnorman, I was called out by @sebastic for creating old-style multipolygons. I did it, among other reasons— because I didn't know any better, because iD never warned me about it, and because the multipolygons I had created appeared to work just fine.

Reading the comments here, the perception seems like a user must go out-of-their-way to create old-style multipolygons. Maybe. But for me, iD seems to make it easy. Here was my old process to create a large pool deck surrounding a swimming pool:

  1. Draw two areas: a smaller area nested in a larger area.
  2. Tag the areas, respectively, as swimming pool and pedestrian street.
  3. In the Edit Feature Panel for the pedestrian street, add a new multipolygon relation; tag the pedestrian street as "outer."
  4. Add the newly created multipolygon relation to the swimming pool; tag the swimming pool as "inner."

A few points:

  • The problem, for me at least, was the option to create a new multipolygon relation on an area. That a multipolygon requires a "line" as outer is not intuitive. When I think "multipolygon," I think "multiple areas." I think, "small area nested inside big area." Or some kind of "mainland/ exclave" relationship.
  • The process for creating new-style multipolygons is not made clear in iD.
  • It appears that I had "manually" created these old-style multipolygon relations. Was there a more "automatic" process in iD that I missed?

@bhousel
Copy link
Member

bhousel commented May 7, 2017

@fluous The more automatic way to create multipolygons in iD is to draw the two areas, (or an outer area and inner line for a hole) - then select them both and use the "merge" command.

We know this isn't intuitive yet, see #3134. We also plan to train users on this technique eventually by offering a more advanced walkthrough, see #4012

pool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validation An issue with the validation or Q/A code
Projects
None yet
Development

No branches or pull requests

7 participants