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

MVT Tile Layer #3935

Merged
merged 10 commits into from
Feb 28, 2020
Merged

MVT Tile Layer #3935

merged 10 commits into from
Feb 28, 2020

Conversation

jesusbotella
Copy link
Contributor

Following #3695 and #3704

Background

Following what we talked about in the previous PR about polygons/lines merging, we thought of implementing that functionality in another layer called MVTTileLayer, so that we can consume MVTs coming from a backend service and represent them within the map.

We followed TileLayer principals to create this layer as there are a lot of similarities between the two of them. Nevertheless, some differences make this layer work with MVTs without implementing a custom getTileData method and avoiding border in represented geometries when clipped by tile limits.

The first topic is solved by getting and decoding tile data from an MVT backend URL with a worker to avoid blocking the UI thread.

The second topic is related to our previous PR, because we are joining all tiles' data into a single tile, and then merging geometries by a common property that contains a unique id within GeoJSON properties using turf for polygons and other strategies for other kinds of features. We know that might not be the most efficient approach, but our other approach was to create derived tiles merging split features into one of the tiles that contained it and remove that feature from the other ones.

Other improvements that we had in mind were to execute that geometry merge in a WebWorker or caching composite tiles' (that's how we call our new tile object) render, but we wanted to get your feedback before continuing that path that might not fit everyone's needs.

So it would be great if you could take a look at this and give us your thoughts about it.

We need to improve workers' integration because they are not inlined yet and a new bundle is generated within dist files, but that'll come later.

Change List

  • Create MVTTileLayer extending TileLayer
  • Create CompositeLayer to be able to merge features using different strategies

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jesusbotella
Copy link
Contributor Author

jesusbotella commented Dec 19, 2019

Hello @Pessimistress!

Sorry for disturbing, but as you gave us insightful feedback in our previous PR I’d like to ask if you or any other members/contributors within the team could tell us their/your thoughts about our current approach and our MVTTileLayer.

Thank you!

@Pessimistress
Copy link
Collaborator

Pessimistress commented Jan 3, 2020

@jesusbotella Thank you for the thoughtful PR. Now that we are done with the 8.0 release, I'm happy to resume this conversation.

There are several things I feel worth discussing:

Should we add a MVTTileLayer to the layer catalog?

When we originally released the TileLayer, the answer was no -- deck.gl is not in the market of competing with a base map provider. We didn't want to publish something that depends on a proprietary format/service. There also are many map-specific features that deck simply does not implement, e.g. rendering text labels along a path, repeating worlds at low zoom levels, etc. Eventually, mapbox-gl would always be the most accurate and most efficient in handling their own format.

With that said, I believe you have good reasons to render Mapbox tiles with deck.gl. I think there are a lot of value in sharing this code so that people who have the same needs don't have to jump through the same hoops over and over again. We just have to make it very clear the limitations of this approach.

Coordinate system

There are roughly 3 coordinate systems we deal with in geospatial: world space (degrees in lng-lat), common space ("pixels" on the Web Mercator plane), screen space. When you load a GeoJSON, Mapbox projects world space to common space on the CPU, then common space to screen space on the GPU. deck.gl does both projections on the GPU.

As an optimization, Mapbox tiles are encoded in mercator pixel offsets aka common space. Using @mapbox/vector-tiles to decode it to GeoJSON means unprojecting all points on the CPU, which has a significant perf cost. We should investigate if we can use the raw coordinates with deck's IDENTITY mode.

Decoding

Web worker is definitely going to help performance. We should look at decoding the coordinates directly into flat binary arrays -- another issue of @mapbox/vector-tiles is that it creates a lot of small JS arrays to conform with GeoJSON spec which is very slow. We can also skip normalization assuming the tile data is already validated.

Merging Features

This would be the most controversial one. I have a strong opinion against this approach, as we have previously discussed. I proposed a different solution in another thread -- it would be cheaper to use GPU-based clipping on each tile to remove the overlapping. If autoHighlighting is needed, the MVTTileLayer can implement some internal hover handling to set the highlightedObjectIndex of each tile.


So things we need to make this production ready:

  • Support Web Mercator coordinates in deck
  • Implement GPU-based mask/clip feature (v8.1)
  • May need to fork or contribute to @mapbox/vector-tiles to get better perf

@alasarr
Copy link
Contributor

alasarr commented Jan 9, 2020

@Pessimistress thanks a lot for all the comments. I can't agree more with the performance improvements you're suggesting! Our original idea was to have something faster with our main requirements without going down to the internals of the library, so it's great to get your guidance for the next steps to bring this for the community.

Should we add a MVTTileLayer to the layer catalog?

Regarding this point, I'd like to add that MVT layers should be a very useful feature to other users because it solves the issue of rendering big datasets in Deck.gl. For example, if you want to visualize census blocks (22M records ~ 8GB). Another interesting use case is time-series datasets, we've time series datasets of 4TB that we're serving via TileLayers, that would be great if we could visualize it via Deck.gl (and integrate them in Kepler).

Absolutely agree with you that competing with a base map provider is not the goal.

MVT has become the standard in the industry for vector tiles and there are different solutions (commercial and OpenSource) out there for that: CARTO (commercial and Open Source, Mapbox, Tegola, DIY by Paul Ramsey

@jesusbotella
Copy link
Contributor Author

jesusbotella commented Jan 9, 2020

Thank you for your review @Pessimistress, we share your concerns about performance as well, so I think we can discuss it :)

Should we add a MVTTileLayer to the layer catalog?

Our point for rendering MVT tiles with Deck.gl is to be able to use layers with elevation features, aggregations and some other unique features that Deck.gl has.

We are not especially interested in basemaps, but in our own thematic data which are all kinds of points, lines and polygons of diverse domains. MVT provides us a good mechanism to serve the data in our CARTO Platform, with a better performance over other formats for big datasets, so we would be pleased to achieve that using MVTTileLayer integrated in the core library. We are ready to get involved in those tasks with your collaboration.

Which are the limitations that this approach has for you?

Coordinate system

Yes, that’s something that we should take a look at.

I made a little test to get an object with a flat array filled with positions without unprojecting coordinates and another flat array with hole indices from Mapbox’s vector tile library, but I haven’t tried if it would work with IDENTITY coordinate system (COORDINATE_SYSTEM.CARTESIAN). I guess that Deck.gl’s MapView will know how to work with that kind of coordinates.

Decoding

We improved our performance a lot by implementing WebWorkers to decode MVT tiles.

Using flat binary arrays would be much better because creating GeoJSON spec objects is slow. But I am concerned about two topics:

  • What about multiple ring features? Should we use positions and holeIndices object instead?
  • We rely on GeoJSON properties for styling features within a map, so if we use flat binary arrays we should implement a mechanism in our MVTTileLayer class to be able to continue using them, right? As GeoJSON properties will not be included within that array.

About this last specific topic, how do you think we should approach that properties store? Having a store in JS for properties and then matching properties with features by a certain property?

Merging Features

We agree with you that merging features is costly, so it would be faster to implement GPU clipping to tiles. We left it because that was a low-level feature and we decided to go for merging to see how it performs, but it would be great to see a better performance.

It is very relevant for us to get the feature properties, for interaction operations with event handlers (like click, hover, etc.) but also for any other kind of advanced styling and calculations that could be derived from those data, so we saw that the merging option was easier to address than the other one upfront, building on the top of an already working layer, the GeoJSON one.

About the clipping, I saw that you added a v8.1 between parenthesis, is it something that you have in your roadmap?

Thank you very much!

@Pessimistress
Copy link
Collaborator

@alasarr @jesusbotella Thank you for the additional context. I think this layer will be a nice addition to the deck.gl layer catalog. There are several technical details that need work on, either in this PR or later as a follow up:

Decoding

Unfortunately we cannot add a webpack-specific dependency (webworkify-webpack). We have many users relying on other bundlers. @ibgreen suggests that we can add a MVTLoader to loaders.gl, which already has a built-in worker solution (basically publish a pre-built worker to npm).

What about multiple ring features? Should we use positions and holeIndices object instead?

I think it's worth a try. I suspect it will be visibly faster. You won't be able to use the GeoJsonLayer though - flat coordinates will cause an error in that layer. The MVTLayer will have to sort its features and render ScatterplotLayer, PathLayer and PolygonLayer directly.

We rely on GeoJSON properties for styling features within a map

You could put the properties back into a protobuf message. Only the coordinates require heavy processing. My understanding is that re-decoding a message on the main thread is very cheap. And you won't be updating the color/radius/etc. attributes constantly.

Coordinate system

I ran some experiments and deck does not seem to handle the combination of COORDINATE_SYSTEM.CARTESIAN and WebMercatorViewport correctly. I will get this fixed.

Merging

Feature clipping is on our 8.1 roadmap (@1chandu). Would you be ok with holding off the merging feature till we compare the performance?

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 10, 2020

@ibgreen suggests that we can add a MVTLoader to loaders.gl, which already has a built-in worker solution (basically publish a pre-built worker to npm).

Yes, an @loaders.gl/mapbox-vector-tile module would fit well into the loaders.gl catalog of pluggable loaders, and take care of all the worker overhead. We can just copy an existing loader, such as wkt loader.

It should be a fairly simple exercise, happy to assist e.g. if you want to put up an initial PR.

@jesusbotella
Copy link
Contributor Author

For sure, we think that as well. We would be very pleased seeing these MVTTileLayer changes coming forward.

Decoding
Including an MVTLoader into @loaders.gl would make sense so that we can decouple that functionality from our layer and anyone can use it separately.

I would be happy to start working on that and open up a pull request when I have something to share. You can review the code once I open the pull request, and we can work together on merging those changes into master source code. What do you think?

Styling features via properties
My current approach to property styling is that we can provide feature properties decoded from MVT within each data array element.

So, for example, when using a Polygon Layer, each data item could have this shape:

    {
    	geometry: {
    	  positions: TypedArray,
    	  holeIndices: [ ... ]
    	},
    	properties: { ... }
    }

And then, return geometry property (shaped like one of the current Polygon Layer data formats) within getPolygon data accessor.

That way we would have the same versatility as we had with GeoJSON, and we could retrieve properties easily to be used within getFillColor or any other data accessor to modify each feature style.

Does it make sense?

Coordinate system
Cool. Thanks!

Merging
Yes, sure! We are ok with it. Let us know if you need something, or you want us to test some alpha version of that :)

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 10, 2020

I would be happy to start working on that and open up a pull request when I have something to share. You can review the code once I open the pull request, and we can work together on merging those changes into master source code. What do you think?

@jesusbotella Great! If you put up an initial PR I will take over, wire up the worker loader and get it merged. It should be fairly obvious what to do, but here are some bullets to get you started.

  • Copy modules/wkt to modules/mapbox-vector-tile or modules/mvt (I have a slight preference for more readable names to help guide new users through the growing "loader soup", we could call the loader MVTLoader though load(url, MapboxVectorTileLoader) would make for very clear application code).
  • Add the mapbox mvt module to dependencies.
  • Search replace WKT in docs/src/test/package.json
  • Add a couple of MVT test tiles and specify what license those test tiles are under in ../test/data/README.md.
  • Wire up src and test folders.

Scaffolding all of this should not take you more than 1-2 hours (it may not build/pass tests at that stage but that is OK), then you can hand over to me.

@jesusbotella
Copy link
Contributor Author

Hello again!

Just an update from our side.

Cartesian Coordinates

We’ve been making progress with the coordinate system and created a fork in our organization for @mapbox/vector-tile-js repository so that we can make changes and try them with your modifications for cartesian coordinates.

At this point, I think we are in the right way but we’ll need some help/guidance from you!

I have two major doubts on how to get those coordinates:

  • One is the tile offset that we should add to points within tiles
  • The other one is the point coordinates themselves.

For me, a point in cartesian coordinates can be generated with this formula:

x = horizontalOffset + XpointWithinMVT
y = verticalOffset + YpointWithinMVT

Regarding the offset, if we consider pixel offsets as the index of the tile on screen (vertical or horizontal) * tile size in px, I need the index of the tile within the screen which I don’t know if it is something I can have.

Reading Mapbox Vector Tiles specification, they say that geometry data in a Vector Tile is defined in a screen coordinate system. So, given that our tile extent is 2048, and our tile size on screen is 256px, should we convert all points so that they fit in our tile size of 256? I think so but I am not sure if I am right or if it’s better to change our tile extent or not. That conversion operation would give decimal numbers in some cases and I think it’s not ideal.

I hope the whole story makes sense.

About the PR for the repository containing these changes, we’re not sure if it will get traction from the official maintainers given that the repository is not very active nowadays. If it doesn’t, we could for sure maintain a fork with this improvement so that we can use it for our @loaders.gl MVT parser.

Clipping

I’ve checked the latest pull requests and I haven’t found any open PR or work in progress about that. Given that you have that in your roadmap for 8.1, when do you plan on releasing Deck.gl 8.1 (just an ETA)?

@ibgreen
Copy link
Collaborator

ibgreen commented Feb 5, 2020

@jesusbotella I don't have a clear answer since I don't fully understand what you are trying to do, but here are some observations that might be helpful:

  • Many loaders.gl modules include a fork of an existing loader module, rather than just importing it. So replacing the import of the mapbox module with forked src/test directories is certainly an option.
  • But to do that, we should first agree that your changes to that module make sense for the general case (see below).
  • Alternatively we could support "dependency injection" - i.e. allow the user to pass in the imported module symbol(s) as an option to the loader, and your app could import the custom version and inject it. (The draco module already has experimental support for this.)

Back to the reason you are making changes...

When you say "cartesian coordinates", are you just referring to correctly handle the local coordinates in mapbox tiles, or are you talking about e.g. supporting WGS84 cartesian coordinates? As you can infer I am not at all clear what problem you are trying to solve.

Also, I remember mapbox had a good blog post a few years ago explaining the 4096x4096 coordinate system they use in their tiles, but I couldn't find it right now. Suspect it would be worth your while digging it up.

@jesusbotella
Copy link
Contributor Author

jesusbotella commented Feb 7, 2020

Yes, sorry, I think my previous message lacked some context 😁

I am talking about what @Pessimistress told in a message above about using Mapbox Vector tile raw coordinates offset with Deck.gl's IDENTITY mode, so that we don't convert them into WGS84 and then back to "mercator pixel" space.

I used @loaders.gl/mvt parser to get all geometry coordinates in relative tile coordinates (with 2048x2048 grid) and then convert them into a 256x256 grid so that I can render them with Deck.gl IDENTITY coordinate system. But I need to apply an offset to those positions to place them correctly within the viewport, because coordinate origin is the same for all tiles right now.

I hope that it makes more sense :)

@Pessimistress
Copy link
Collaborator

Pessimistress commented Feb 7, 2020

We encountered the same issue when working on #3984. A reasonable approach would be:

  • @loaders.gl/mvt always parses to relative coordinates, i.e. pixels from the tile origin.
  • The deck.gl MVTTileLayer renders the parsed data with the CARTESIAN coordinate system and a modelMatrix that converts the tile coordinates to deck.gl's common space.

Here's what the props will look like:

import {Matrix4} from 'math.gl';

new GeoJsonLayer({
  ...
  coordinateSystem: COORDINATE_SYSTEM.CARTESIAN,
  modelMatrix: getModelMatrix(tile)
});

function getModelMatrix(tile) {
  const extent = tile.extent; // 2048? 4096?
  const WORLD_SIZE = 512; // deck.gl constant
  const worldScale = Math.pow(2, tile.z);

  const xScale = WORLD_SIZE / extent / worldScale;
  const yScale = -xScale;

  const xOffset = WORLD_SIZE * tile.x / worldScale;
  const yOffset = WORLD_SIZE * (1 - tile.y / worldScale);

  return modelMatrix = new Matrix4()
      .translate([xOffset, yOffset, 0])
      .scale([xScale, yScale, 1]);
}

Edit: It might be easier if @loaders.gl/mvt always fit the tile into a 512*512 bounding box, as you suggested, since extent is per MVT layer.

@jesusbotella
Copy link
Contributor Author

Thank you @Pessimistress! That was SUPER helpful!

I have already implemented the changes and it works good! I have opened a PR for @loaders.gl/mvt changes and I'll push my MVTTileLayer changes using the cartesian coordinate system whenever that PR is ready.

@chrisgervang chrisgervang mentioned this pull request Feb 12, 2020
7 tasks
@coveralls
Copy link

coveralls commented Feb 17, 2020

Coverage Status

Coverage increased (+0.06%) to 82.522% when pulling 4c64857 on CartoDB:mvt-layer into 3e5a561 on uber:master.

return new Matrix4().translate([xOffset, yOffset, 0]).scale([xScale, yScale, 1]);
}

function getTileURLIndex({x, y}, templatesLength) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a standard schema? If yes, can you add a link to its specs? Otherwise, should we allow users to customize this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's used to bypass Domain Sharding when HTTP 2 is not available. The user can provide N tiles endpoint and this function does the splitting to each one.

The tilejson spec defines a tiles attributes for that:

// REQUIRED. An array of tile endpoints. {z}, {x} and {y}, if present,
    // are replaced with the corresponding integers. If multiple endpoints are specified, clients
    // may use any combination of endpoints. All endpoints MUST return the same
    // content for the same URL. The array MUST contain at least one endpoint.
    "tiles": [
        "http://localhost:8888/admin/1.0.0/world-light,broadband/{z}/{x}/{y}.png"
    ]

this.state = {
...this.state,
urlTemplates,
getTileData: this.getTileData.bind(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you do getTileData.bind(this, urlTemplates) then getTileData can be a pure function.


const defaultProps = Object.assign({}, TileLayer.defaultProps, {
renderSubLayers: {type: 'function', value: renderSubLayers, compare: false},
urlTemplates: []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Start a documentation page in docs/layers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point!

@@ -53,7 +53,7 @@ export default class TileLayer extends CompositeLayer {
tileset.finalize();
}
tileset = new Tileset2D({
getTileData,
getTileData: this.state.getTileData || getTileData,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, we can create a getTileData method that subclasses can override. By default, it just returns this.props.getTileData()

Copy link
Contributor

Choose a reason for hiding this comment

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

Done. Let me know if it makes sense for you.

@jesusbotella
Copy link
Contributor Author

I've just merged master with our branch and resolved the conflicts.

I think the code is OK (if you think it doesn't need any changes), but we need to coordinate the merging of this PR (visgl/loaders.gl#646).
Mainly because we'd need to update @loaders.gl/mvt dependency version so that the current MVT Tile Layer uses the proper loader code.

@xintongxia
Copy link
Contributor

@jesusbotella

I landed your loaders.gl PRs and published loaders.gl v2.1.0-alpha.5 with your changes included.

@Pessimistress Pessimistress merged commit 99603db into visgl:master Feb 28, 2020
@jfrankl
Copy link

jfrankl commented Feb 28, 2020

I've been following this issue for a while and am excited to check it out. Congrats everyone who was involved!

@jesusbotella
Copy link
Contributor Author

Thank you very much for the merge! 🎉

@kylebarron
Copy link
Collaborator

I was just reading through this thread again. Originally @Pessimistress mentioned loading data into flat TypedArrays.

We should look at decoding the coordinates directly into flat binary arrays -- another issue of @mapbox/vector-tiles is that it creates a lot of small JS arrays to conform with GeoJSON spec which is very slow. We can also skip normalization assuming the tile data is already validated.

It seems like this got passed over. Was there a conscious decision not to implement this? If not, is there still interest towards this?

@ibgreen
Copy link
Collaborator

ibgreen commented Mar 13, 2020

Yes decoding features into flat binary arrays is very interesting.

I think this is primarily a loaders.gl issue and we should open an issue there that references this issue. visgl/loaders.gl#685.

Converting general geojson (with different feature types) to a flat array leads to a rather complex flat array (many different coordinate array structures). Generating a bunch of different flat arrays optimization for the different feature types could also be an option.

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.

9 participants