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

[RFC] Make Tileset2D generic #6816

Closed
felixpalmer opened this issue Apr 12, 2022 · 9 comments
Closed

[RFC] Make Tileset2D generic #6816

felixpalmer opened this issue Apr 12, 2022 · 9 comments
Labels
Milestone

Comments

@felixpalmer
Copy link
Collaborator

felixpalmer commented Apr 12, 2022

Target Use Case

TileLayer relies on Tileset2D and associated classes to implement the loading tiles, using the OSM tiling scheme. This is restrictive in that only allows tiling where the tiles have the same shape as those in OSM. The Tileset2D is quite generic and with fairly minimal changes I've been able to build a PoC with uses H3 indices to define the tiles and use hexagonal tiles rather than square tiles.

Screen.Recording.2022-04-11.at.17.17.50.mov

In a similar manner a whole host of different tiling schemes could be supported by the TileLayer.

Proposal

New props & API

#5504 Expose Tileset2D instance already covered the first step, and this RFC assumes that it will be how the Tileset2D will be exposed to users of TileLayer.

class CustomTileset2D extends Tileset2D {};
new TileLayer({
   TilesetClass: CustomTileset2D,
   data: 'data/{q}.json'
   ...
});

The following classes are made publicly available from the geo-layers module: Tileset2D & Tile2DHeader.

Modifications to existing classes

TileHeader2D is modified to be generic:

  • index property is added. This uniquely describes the tile and allows it to be constructed. For OSM this is {x, y, z}.

Tileset2D is modified to better allow subclassing:

  • Rather than assuming an index of {x, y, z}, use TileHeader2D's index
  • getTileId method is added for subclasses to implement for converting the index into a unique string
  • getTileZoom method is added for subclasses to implement for converting the index into a zoom level
  • getURLFromTemplate is enhanced to support arbitrary keys in the index object. E.g. {h3: '82488ffffffffff'}

Usage

With the above changes the typical pattern of creating a new tiling scheme would consist of defining a new header and overriding the public API of Tileset2D to specify how to work with the tiles. The choice of the key name in the index matched the key(s) used for interpolation in the URL:

class QuadkeyTileset2D extends Tileset2D {
  getTileIndices(opts) {
    return super.getTileIndices(opts).map(({x, y, z}) => ({q: tile2Quadkey(x, y, z)}));
  }

  getTileId({q}) {
    return q;
  }

  getTileMetadata(index) {
    return {}; // Depending on use-case. OSM calculates bbox here, but TileLayer doesn't require it
  }

  getTileZoom({q}) {
    return q.length;
  }
  
  getParentIndex(index) {
    const q = index.q.slice(0, -1);
    return {q};
  }
}

Finally to use the custom Tileset2D with the TileLayer, the getURLFromTemplate method needs to be implemented to specify how to load the data:

new TileLayer({
  TilesetClass: QuadkeyTileset2D,
  data: 'data/{q}.json'
  renderSubLayers: props => {
      return new QuadkeyLayer(props, {
        getQuadkey: d => d.spatial_index,
        getFillColor: d => [(d.value - 12) * 25, d.value * 8, 79]
      });
    }
})

Discussion

  • Should Tileset2D be exposed as an abstract class and OSMTileset2D be provided as the default implementation to TileLayer?
  • Is zoom a good name? resolution may be more appropriate, but would mean changing the API
@ibgreen
Copy link
Collaborator

ibgreen commented Apr 12, 2022

Great proposal, making Tileset2D more generic and customizable/replaceable has been a goal for a long time.

  • There is a similar but more complex setup with the Tile3DLayer and the Tileset3D class in loaders.gl, where the Tileset2D class has subclasses for 3D Tiles and I3S.
  • There have also been discussions about having a single deck.gl TileLayer that can handle both Tileset2D and Tileset3D as overall the the operations the layer need to do are similar.
  • Generally I think it is ideal if the Tileset... classes know nothing about rendering and can be used by non-graphical applications to incrementally load data for an area at a specific resolution.
  • Along those lines, moving to Tileset2D to loaders.gl was discussed but bundle size concerns were raised. Creating a new @loaders.gl/tileset-2d module that only contains this class would presumably address that.

Finally, as a minor preference it would be nice to be able to pass an instance of a Tileset2D to the TileLayer rather than a constructor which I feel is less intuitive, but there are API tradeoffs.

@felixpalmer
Copy link
Collaborator Author

Good point regarding the link with Tile3DLayer. My feeling with Tile3DLayer is that it is pretty tightly coupled to the loaders.gl and assumes that it will be displaying 3D tiles / I3S, whereas TileLayer handles the separation of concerns more cleanly and defers to renderSubLayers to specify how the downloaded data is rendered.

Tile3DLayer is quite similar to MVTLayer in that it supports completely different data types, the display of which delegates to different sublayers. However MVTLayer follows the pattern of TileLayer, in that it encapsulates the rendering by deferring to GeoJsonLayer. I think it would be logical to move to a similar model with Tile3DLayer - namely by introducing a new Layer (perhaps internal), e.g. I3SLayer for rendering a single tile.

I will work on a PR that focuses on the 2D use case for now, basically implementing my proposal above. We can then revisit the question on whether it makes sense to move the class to loaders.gl

@ibgreen
Copy link
Collaborator

ibgreen commented Apr 21, 2022

Without necessarily wanting to influence your conclusions, let me just try to make sure you have a fair picture of the 3D tile system

The idea with the tiles loader architecture in loaders.gl is that that the various tile loaders load into a normalized Tileset3D / Tile3D format defined by the @loaders.gl/tiles module. This normalized format is completely generic and optimized for display by any 3D framework, including deck.gl.

namely by introducing a new Layer (perhaps internal), e.g. I3SLayer for rendering a single tile.

No, to the contrary, the Tile3DLayer shouldn't need to know anything about the details of e.g. 3D Tiles or I3S formats. In practice, over years of development, some small dependency might have accidentally crept in but if so it should be easily fixable and the basic architecture should still be clean and abstract.

The idea here is of course that we should be able to add support for e.g. potree and COPC loader modules in loaders.gl without even changing a single line in the deck.gl Tile3DLayer.

The normalized Tileset3D class has a generic traversal model that requests new tiles for a viewport, and I don't think it should be any problems to extend it to handle 2D.

Getting up to speed on that code base may not be worth it but the downside is of course that we may end up with two parallel, partly overlapping, complex systems.

image

@felixpalmer
Copy link
Collaborator Author

Thanks for the diagram, that is useful. My point regarding the new Layer, which I badly named I3SLayer was to decouple the rendering of the scenegraph/pointcloud/mesh from Tile3DLayer so that (as you write) Tile3DLayer doesn't know how to render the data it is receiving from loaders.gl. Currently Tile3DLayer makes assumptions about what it is going to render, with no way for the user to override this using a prop (as TileLayer supports via renderSubLayers):

_getSubLayer(tileHeader, oldLayer) {
if (!tileHeader.content) {
return null;
}
switch (tileHeader.type) {
case TILE_TYPE.POINTCLOUD:
return this._makePointCloudLayer(tileHeader, oldLayer);
case TILE_TYPE.SCENEGRAPH:
return this._make3DModelLayer(tileHeader, oldLayer);
case TILE_TYPE.MESH:
return this._makeSimpleMeshLayer(tileHeader, oldLayer);
default:
throw new Error(`Tile3DLayer: Failed to render layer of type ${tileHeader.content.type}`);
}
}

I agree that it is a great goal to have loaders.gl be written such that new 3D tile formats can be added.

However to allow other tiles with data formats to be added, e.g. bitmaps, quadkey/H3 data as JSON, Tile3DLayer needs to have a way to know how to render these and throwing them at a scenegraph/pointcloud/mesh isn't going to work. Hence I think introducing renderSubLayers in Tile3DLayer makes sense if we want it to be able to render arbitrary data, and not just 3D-tile like formats.

@felixpalmer
Copy link
Collaborator Author

Anyway, to get back to the original proposal, I had already got most of the way there with a draft PR on making Tileset2D generic. I've made a test app which implements loading for Quadkeys and H3 indices. In doing so I came across a few things which made me change my mind about the original proposal:

  • cacheKey needs to be calculated purely from the tile index and thus doesn't make sense as a property on the tile header
  • In order to simplify the API, I opted to keep TileHeader2D generic, and not export it. Instead, to create a custom tileset the user only needs to subclass Tileset2D and implement the required methods

@Pessimistress
Copy link
Collaborator

The two TileLayer examples under examples/website need to be updated to the new API (indices in the tooltip showing undefined).

@Pessimistress Pessimistress added this to the 8.8 milestone Jul 1, 2022
@kopp
Copy link
Contributor

kopp commented May 17, 2023

Hi @felixpalmer and @Pessimistress and @ibgreen , I used a custom TilesetClass to implement a tiling scheme that is different from the "OSM slippy tiles" scheme. It works in that I can display my data. What stopped to work, though, is the onHover and getTooltip functionality. Those callbacks do not get called any more. Interestingly, it still works as long as the geometry of the tiles is the same as in osm (i.e. only the index name changes, as for Quadkeys), and breaks if the geometry is different. Do you have any pointers how I could solve/debug that issue?

If I set a breakpoint into deck.ts in the _pickAndCallback method, the this._pick will always return an empty result list for my custom tiling scheme and the object I hover over when I use the Tileset2D.

Interestingly, even If I make my points in the PointCloudLayer so large, that they overlap (i.e. there is no way to miss one of the points and onHover should trigger permanently), I do not get an onHover callback called.

@felixpalmer
Copy link
Collaborator Author

@kopp I can't really say without seeing the code. Here is an example of a H3 based tile layer where picking is working: https://github.com/visgl/deck.gl/blob/master/modules/carto/src/layers/h3-tile-layer.ts

@kopp
Copy link
Contributor

kopp commented May 19, 2023

Hi @felixpalmer thanks a lot for getting back to me about this and pointing out the example. My understanding is now, that you have to implement the getTileMetadata function and return the bounding box bbox: ..., otherwise picking will not work. I'll do a PR to clarify that in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants