Skip to content

Add MD Three Inch Aerial Imagery #206

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

Merged
merged 1 commit into from
Nov 13, 2016

Conversation

simon04
Copy link
Contributor

@simon04 simon04 commented Nov 11, 2016

For #171.

This is the first imagery where the geometry is specified as MultiPolygon. This requires adapting convert_xml.py.

@tyrasd
Copy link
Member

tyrasd commented Nov 11, 2016

Ack! 😱

This is the first imagery where the geometry is specified as MultiPolygon.

Well, actually, there are already a couple of "multipolygon" sources in the repo (for example the South Tyrol 2014, or the Tiris * imagery), but which we forgot about in the geojsonification process in #191. Instead, they continued to use the slightly odd (and incompatible to GeoJSON) convention where the rings in the coordinates array are all outer rings and inner rings are not allowed.

It's probably best to fix those all in one go and to apply an additional fix (very similar to yours also to fix the legacy json output).

For this particular layer this means that we can either wait until this is all fixed in the data processing before adding the source, or that we merge it using the old&incorrect coordinate scheme (because the PR as-is will probably break things for users of our legacy-json output).

@bhousel
Copy link
Member

bhousel commented Nov 11, 2016

Well, actually, there are already a couple of "multipolygon" sources in the repo (for example the South Tyrol 2014, or the Tiris * imagery), but which we forgot about in the geojsonification process in #191. Instead, they continued to use the slightly odd (and incompatible to GeoJSON) convention where the rings in the coordinates array are all outer rings and inner rings are not allowed.

Yes, I just realized this as well.. @tyrasd should I do one last sync of the imagery as-is into iD before we change this?

I do think going forward we should use the correct geojson conventions for rings and holes..

@tyrasd
Copy link
Member

tyrasd commented Nov 11, 2016

should I do one last sync of the imagery as-is into iD before we change this?

ok 👍

Imho, it would make the most sense for iD to consume the imagery.geojson output, and for this repo to keep around the old scheme in the imagery.xml and imagery.json output (since we don't version this repo it could mean to be problematic for existing users if we change anything that's not backwards compatible – e.g. for someone who's running iD v1 for whatever reason).

@simon04 simon04 force-pushed the MD_ThreeInchImagery branch from b7dca24 to 19dc98e Compare November 11, 2016 20:57
@simonpoole
Copy link
Contributor

Rather a weird place to discuss this, but I consider the issue that @tyrasd touched on, that we are not versioning at least the output the most annoying thing about the current setup. Besides that having the unversioned output files in the repo itself is simply messy, it further makes it difficult to consume the output automatically without having to fear breakage (as we had with the transition to GeoJSON).

Is there any fundamental problem with say generating a (github) release per week if something has changed and uploading the generated files to the release section of github?

@iandees
Copy link
Member

iandees commented Nov 11, 2016

Versioning is a good idea. Can you open up a new ticket for that, @simonpoole?

@simon04 simon04 force-pushed the MD_ThreeInchImagery branch from 19dc98e to 50d3ddb Compare November 13, 2016 11:46
@simon04
Copy link
Contributor Author

simon04 commented Nov 13, 2016

Let's use the "slightly odd (and incompatible to GeoJSON) convention" for this source as well and discuss the MultiPolygon change in #217.

@simon04 simon04 merged commit 2396a4d into osmlab:gh-pages Nov 13, 2016
@simon04 simon04 deleted the MD_ThreeInchImagery branch November 16, 2016 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants