mapbox / minjur

Osmium-based converter of OSM data to GeoJSON
ISC License
59 stars 14 forks source link

add filter to create closed ways only as polygon or linestring, dependent on tags #9

Closed mikelmaron closed 7 years ago

mikelmaron commented 8 years ago

Per discussion in https://github.com/mapbox/osm-qa-tiles/issues/43, add some kind of filter option to minjur that decides which closed ways to create as polygons and which to create as linestrings.

For OSMQA tiles, we'll start with the most common use case: closed ways, tagged as building, but not tagged as building=no

joto commented 8 years ago

Currently this is what happens:

I understand you want this to be changed to:

Is this correct?

Does this have to be an extra option or do we always want this?

Note that this logic has to be hardcoded in the C++ code. In theory we could of course make this somehow configurable on the command line, but that would either be a lot of work (writing some kind of filter language) or have extreme performance impact (making checks in some high level language).

aaronlidman commented 8 years ago

Does this have to be an extra option or do we always want this?

LineString or Polygon logic should be default.

In theory we could of course make this somehow configurable on the command line

Let's not for now and see if we need to later.

mikelmaron commented 8 years ago

LineString or Polygon logic should be default.

with an option to override the default back to the 'old' behavior. Not sure why you'd want to do this :). But does provide 'documentation' that this is happening.

Let's not for now and see if we need to later.

sounds good

mikelmaron commented 8 years ago

from chats between @aaronlidman and @bhousel

" https://gist.github.com/bhousel/90415106edea4e693f89 the keys are things that should be considered areas, and the values are the exceptions that should not be considered areas"

joto commented 8 years ago

I have changed minjur to only create Polygons (and not LineStrings) if -p is given and if the way is closed and has any of the keys in the list from the gist (but not the values from the list) (I also added building=no as exception). Note that what happens depends on the order of the tags in the input file if there are conflicting requirements, ie one tag says "Polgon", another says "LineString". (The order of the tags in a planet file is essentially random.)

The filter is here: https://github.com/mapbox/minjur/blob/master/minjur.cpp#L154-L250 . Just read from the top to understand this: Default is "false" (->LineString), aeroway=gate is "false" (->LineString), aeroway=taxiway is "false" (->LineString), aeroway=* is "true" (->Polygon), etc. Feel free to fiddle with this list yourself.

aaronlidman commented 8 years ago

Thank you @joto, we'll try this out and let you know how it goes.

rclark commented 8 years ago

For all map-building and rendering purposes we're going to want to either build a linestring or an area for each openstreetmap feature. Determining which to chose will be tag-based. Can we support this behavior in minjur and minjur-mp?

joto commented 8 years ago

@rclark I am not sure minjur ist the right place to add this kind of logic. Deciding whether something is a linestring or a polygon is a complex decision. This is especially true for QA tiles which (as I understand them) are specifically targeted at finding errors in the OSM data. So the more we pre-process this data before it gets into the QA tiles, the more information is lost and can't be used to detect problems.

I totally agree that it makes sense to do more pre-processing for tiles that are supposed to be used for rendering nice maps. But as far as I understood it, that isn't what minjur was to be used for. Do we want to extend its mission in this direction?

rclark commented 8 years ago

Yes, I am working on a project that targets map rendering and I'm hoping to use minjur for geojson there. I certainly can post-process away the things that I don't want, and this will be involved -- but it is worth noting that even from the qa-tiles perspective, output of either a line or a polygon (instead of both) sounds to be a desirable feature -- this ticket was in fact opened as a result of qa-tiles discussion.

mikelmaron commented 8 years ago

Hi, I'm a little confused here. The -p option in minjur already only creates Polygons when certain tag conditions are met. Is the question here about -p option in minjur-mp? @joto there an issue in changing the option in minjur-mp particularly?

Or another question, what about merging these two CLI, and making multipolygons an option?

joto commented 8 years ago

@mikelmaron yes, this is about matching against the special list of "polygon tags" not only in minjur but also in minjur-mp. If that's what y'all want, we can certainly do this. I was just warning that whatever we'll throw away, we can't get back later and we have to be very clear to any users about what they can expect from the data and what not.

For the second question: minjur and minjur-mp are sufficiently different that I think it could be more confusing to join them. See https://github.com/mapbox/minjur/issues/8#issuecomment-184732800

mikelmaron commented 8 years ago

@joto, if this is set up as documented options, then I can't see anything as more value. And definitely, on downstream products to be very clear with users about what to expect.

joto commented 7 years ago

The upcoming osmium export command (#28) supports setting a list of "polygon tags" and "linestring tags" in the config file. See its manual page for details.

Please have a look whether this does all it needs to do.

Because minjur is being phased out, this will not be changed in minjur any more.

springmeyer commented 7 years ago

Thanks @joto. Anyone interested in testing the new osmium-tool behavior and providing feedback please see https://github.com/mapbox/minjur/issues/28#issuecomment-298798206