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 single-tile index: GeoJSONVTTile #65

Merged
merged 1 commit into from
Sep 21, 2017
Merged

Add single-tile index: GeoJSONVTTile #65

merged 1 commit into from
Sep 21, 2017

Conversation

asheemmamoowala
Copy link
Contributor

For Custom Sources in GL-Native, a GeoJSONVT index is used to convert, clip, and tile geojson geometry from the client. For each high-zoom tile, this requires splitting the features for each tile, even though the features are already provided for only a specific tile ref

For a simple LineString feature, fetching just a single tile 100 times:
GeoJSONVT : 498.2ms (avg: 5ms)
GeoJSONVTTile: 148.6ms (avg: 1ms)

gl-native#9983

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe your design decisions here in more detail? In particular:

  • Why do we need to clip by parent tile?
  • Why do we need to world-wrap features?
  • If we only need a single converted tile and not any of the GeoJSON-VT features like indexing and tiling, why expose the conversion through this interface?

@asheemmamoowala
Copy link
Contributor Author

If we only need a single converted tile and not any of the GeoJSON-VT features like indexing and tiling, why expose the conversion through this interface?

This is a question I went back and forth on. I see this module as the logical place for conversion from GeoJSON data to Vector Tile data - even though it doesn't use a tile index or some of the other features.

Why do we need to world-wrap features?

I left this in case tiled features are provided in a wrapped world where features cross the anti-meridian. It may not be necessary specifically for this API, but the data may already be in that format.

@mourner
Copy link
Member

mourner commented Sep 18, 2017

Yeah, I think it makes sense to do this in GeoJSON-VT. I'm just not sure whether it should really clip/ wrap tiles — having the data tiled should already imply clipping/wrapping, so doing this the second time after the conversion feels redundant, unless we want to do it as a "fixing" stage for badly tiled data.

@asheemmamoowala
Copy link
Contributor Author

Would it be better if the clipping and wrapping steps were optional instead? It would allow someone to bring data that needs additional fixing and give them an efficient tool to do it, instead of requiring that the data be corrected at the platform level.

}

private:
std::experimental::optional<detail::InternalTile> internalTile;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing, does it have to be optional? I see that it's populated in constructor and then never removed dynamically, so perhaps we could do without optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't need to be optional. In fixing this, I realized that this class doesn't need to maintain state and is better used as a method that returns a Tile

@asheemmamoowala
Copy link
Contributor Author

asheemmamoowala commented Sep 19, 2017

Thinking about this a bit more, I feel that the GeoJSONVTTile class is ill-suited here. All it does is compute the Tile in the constructor and return it in getTile(). It has no other state or operations.

A static method makes more sense to me:

mapbox::geojsonvt::geoJSONToTile(geojson, z, x, y, options, bool wrap, bool clip);

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.

2 participants