-
Notifications
You must be signed in to change notification settings - Fork 25
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
Comments
Handling the ClosePath at all would necessitate some changes, but it would be possible (and with the old meaning). The questions here are:
More technically: We have to change the decode_linestring function to add this functionality. We could push some kind of checking function in the @flippmoke, @springmeyer any opinions? |
It's difficult to be sure about this, but here's what we know:
|
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. |
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. |
I do not think we should be handling the case in which we have a In mapnik-vector-tile currently, if it is a v1 tile we simply ignore the |
Options:
|
@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. |
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 |
This was a |
Alright, I finally managed to track down the history of the offending tile:
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. |
Thanks for tracking that down! |
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. |
Refs mapbox/mapbox-gl-native#11579
Fixture in mvt-fixtures: mapbox/mvt-fixtures#94
It looks like
mapbox/vector-tile
effectively just readsClosePath
asMoveTo(first_point_in_this_line)
. I suppose if we want mapbox-gl-native to behave exactly the same way after moving fromvector-tile
tovtzero
, we'd want to do the same thing here, but I'm not sure if that's necessary. If, for example, simply ignoring theClosePath
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.The text was updated successfully, but these errors were encountered: