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

Accept linestring geometries containing a ClosePath command #39

Closed
anandthakker opened this issue Apr 3, 2018 · 12 comments
Closed

Accept linestring geometries containing a ClosePath command #39

anandthakker opened this issue Apr 3, 2018 · 12 comments

Comments

@anandthakker
Copy link
Contributor

anandthakker commented Apr 3, 2018

Refs mapbox/mapbox-gl-native#11579
Fixture in mvt-fixtures: mapbox/mvt-fixtures#94

It looks like mapbox/vector-tile effectively just reads ClosePath as MoveTo(first_point_in_this_line). I suppose if we want mapbox-gl-native to behave exactly the same way after moving from vector-tile to vtzero, we'd want to do the same thing here, but I'm not sure if that's necessary. If, for example, simply ignoring the ClosePath command made more sense from vtzero's point of view, mapbox-gl-native would be able to be backwards compatible with a slight degradation that, IMO, would be acceptable.

@joto
Copy link
Collaborator

joto commented Apr 9, 2018

Handling the ClosePath at all would necessitate some changes, but it would be possible (and with the old meaning). The questions here are:

  • Do we really want this? Being compatible with old mapbox-gl-native code is only needed if there is actually some data out there that generates this format. Or at least some software that generated this format. Has this been established?
  • If we want to be compatible, do we still want to get some kind of warning out of vtzero, so that software that wants to be strict, can be?

More technically: We have to change the decode_linestring function to add this functionality. We could push some kind of checking function in the TGeomHandler class, which is already something the user code has to provide. We could call a static function on that class (if available) that notifies the user code that something was not quite right here which gives the user code a chance to react in some way.

@flippmoke, @springmeyer any opinions?

@anandthakker
Copy link
Contributor Author

Being compatible with old mapbox-gl-native code is only needed if there is actually some data out there that generates this format. Or at least some software that generated this format. Has this been established?

It's difficult to be sure about this, but here's what we know:

  • Per Replace outdated vector tile fixtures mapbox-gl-native#11579 (comment), there is a tile in this format in the gl-native codebase as a test fixture.
  • The tile's exact origin is unknown, but looking at the contents, it looks "mapboxy".
  • That tile was added after the offline cache feature had already shipped. More generally, the offline feature shipped before VT v2, so it's possible that there exist offline databases in production that contain v1 tiles, and specifically tiles with this particular issue.

@joto
Copy link
Collaborator

joto commented Apr 9, 2018

The important question is: Was this tile added as an example of a "usual tile" or was it added as an example for a "slightly malformed tile" that should still be accepted. Only the first case it is likely that there are actual tiles like that in the wild.

@anandthakker
Copy link
Contributor Author

Right. Looking at its history, I have seen no reason to think that it was intended to cover an edge case (if it were, I would have expected to see something to suggest that: a code or commit comment, it originally being added in an obvious edge-case unit test, etc.). That, and the fact that it looks "mapboxy", makes me think that it very well could have been a typical tile at the time.

@flippmoke
Copy link
Member

I do not think we should be handling the case in which we have a ClosePath in a linestring or point with out error. The v2 version of the specification is very clear what is allowed in a linestring.

In mapnik-vector-tile currently, if it is a v1 tile we simply ignore the closepath as well -- but instead we just break decoding the entire geom at that point. However, I think at some point we need to stop designing around such edge cases. Despite V1 being very vague, I do not think the intent was ever to support ClosePath in linestrings and I am not sure if any encoder ever had this as a design decision (other then an encoding bug).

@anandthakker
Copy link
Contributor Author

Options:

  1. DON'T support this anomaly in vtzero, and DON'T port mapbox-gl-native to vtzero. Ensures backwards compatibility, but deprives vtzero of a client and leaves gl-native with tech debt.
  2. DON'T support this anomaly in vtzero, but DO port mabpox-gl-native to vtzero. Avoids tech debt, but potentially breaks backwards compatilibity: any users with an old offline database containing features with this anomaly will no longer see the offending feature in their maps.
  3. DO support this anomaly in vtzero and DO port mapbox-gl-native to vtzero. Ensures backwards compatibility, but leaves vtzero with tech debt.

@flippmoke
Copy link
Member

@anandthakker Do we actually know of any tiles that do this? We ported the vast majority of our own tilesets to v2 a very long time ago, even before that we were not creating V1 tiles with closepath. Old customer data might contain such a situation, but I very much doubt that it is many of them. Additionally, this would only be a problem for individuals who updated their GL client and were still using very old data.

Overall, I feel that we are pretty safe here. However, I think this continues to pop up that we do not have any tooling to check situations such this across all of our tiles that have been uploaded. I know that long ago @lbud had some long running scripts to test for issues such as this when we were updating to v2. I have no idea if they would still work or could be modified to test for something such as this.

@anandthakker
Copy link
Contributor Author

The only tile I know of that does this is the test fixture I referred to above. I just have no idea how to estimate the likelihood that such a tile is also sitting in either (a) a non-Mapbox server or (b) on an end-user's phone in their offline cache. cc @jfirebaugh @kkaefer

@lbud
Copy link

lbud commented Apr 9, 2018

I know that long ago @lbud had some long running scripts to test for issues such as this when we were updating to v2. I have no idea if they would still work or could be modified to test for something such as this.

This was a checker stack, but looking at it it relies on a lot of outdated Mapbox platform infrastructure and I doubt it would work now 😕

@anandthakker
Copy link
Contributor Author

Alright, I finally managed to track down the history of the offending tile:

  • Confirmed that it's a Mapbox Streets v6 vector tile, went into production on April 2, 2015.
  • Also confirmed that the closepath issue is not present in the subsequent version of Mapbox Streets v6 that was deployed on May 14, 2015.
  • Furthermore, [wip] Mapnik geometry mapnik-vector-tile#94, an extensive PR improving mapnik-vector-tile's geometry encoding, was merged on April 29, 2015, just before the above deploy.

Therefore, it seems fairly likely that this issue was due to an encoding issue that was fixed in mapnik . in mid-2015. Since offline didn't ship in native until 2016, I think it's reasonably safe to assume that we can punt on supporting this issue.

@jfirebaugh
Copy link

Thanks for tracking that down!

@flippmoke
Copy link
Member

flippmoke commented Apr 10, 2018

It should also be noted that in April of 2016 we re-rendered Mapbox Streets v5,v6, and v7 - This was so that they would all be using v2 tiles.

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

No branches or pull requests

5 participants