TileLayer API audit #4246
TileLayer API audit #4246
Conversation
docs/layers/tile-layer.md
Outdated
| Renders one or an array of Layer instances with all the `TileLayer` props and the following props: | ||
|
|
||
| * `id`: An unique id for this sublayer | ||
| * `data`: Resolved from `getTileData` |
There was a problem hiding this comment.
Nit: Not something that bothers me, just calling out (and I think you already know) that in 3d tiles, the naming is "reversed":
datais calledtile(akatile content)tileis calledtile header
I personally think the 3d tiles naming ultimately is less confusing as various tile loaders return the actual "tile".
There was a problem hiding this comment.
We cannot make a breaking API change in 8.x.
docs/layers/tile-layer.md
Outdated
|
|
||
| ## Tileset2D | ||
|
|
||
| > This section describes advanced customization of the TileLayer. |
There was a problem hiding this comment.
Maybe some simpler usage exposition of the non-subclassed Tileset2D class before jumping into the advanced customization case?
There was a problem hiding this comment.
Going off that, is there a strong argument against baking this into deck.gl in some way? Using the non-geospatial coordinate system is something deck.gl supports but this layer as it stands is married to geo-spatial coordinates (and specifically does not have to be). Also, is there a difference between getTileIndicesInViewport and getTileIndices?
There was a problem hiding this comment.
It should be just getTileIndices.
I can try integrating the non-geospatial handling. My main concern is that we don’t really have any test case for this.
There was a problem hiding this comment.
Do you mean unit testing or some sort of visual testing? I could create an image pyramid for this if you'd like. I'd also be happy to write unit tests.
There was a problem hiding this comment.
That would be great! I'll add some unit tests before landing, but a sample set of tiles would be very helpful.
docs/layers/tile-layer.md
Outdated
|
|
||
| ##### `updateTileStates` | ||
|
|
||
| `updateTileStates()` |
There was a problem hiding this comment.
Nit: For the next release; I think this API (my understanding it requires calling getTileIndicesInViewport/updateTiles/selectedTiles) is a bit cumbersome.
Also to support multi-viewport scenarios without thrashing I believe both this class and the Tileset3D should be able to accept a list of viewports when selecting tiles to be loaded and then return a list of tiles per viewport.
| @@ -1,10 +1,11 @@ | |||
| import {log} from '@deck.gl/core'; | |||
|
|
|||
| export default class Tile2DHeader { | |||
There was a problem hiding this comment.
Here it is called header, not tile, so maybe a reason to align naming above?
There was a problem hiding this comment.
Cannot make a breaking change in 8.x.
|
|
||
| const defaultProps = { | ||
| renderSubLayers: {type: 'function', value: props => new GeoJsonLayer(props), compare: false}, | ||
| getTileData: {type: 'function', value: ({x, y, z}) => null, compare: false}, | ||
| tileToBoundingBox: {type: 'function', value: defaultTile2Bbox, compare: false}, | ||
| getTileIndices: {type: 'function', value: defaultGetIndices, compare: false}, | ||
| Tileset: Tileset2D, |
There was a problem hiding this comment.
Nit: Don't we want to we accept a tileset instance, rather than a constructor? That seems a bit limiting?
Of course if none is provided we instantiate Tileset2D
There was a problem hiding this comment.
Removing for now, will re-visit when Tileset2D is moved to loaders.gl.
For #4230
Follow up of #4139 and #4232
@ilan-gold
Change List
strategytorefinementStrategy, add a modenevermaxCacheByteSizeprop, resize cache based on byte size if suppliedTilesetpropgetTileIndices,tileToBoundingBox; subclassTileset2Dby overriding thegetTileIndicesandgetTileMetadatamethods insteadTileset2Dclass, documentation