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

TileLayer: add zoomOffset prop #5896

Merged
merged 7 commits into from
Jun 30, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions docs/api-reference/geo-layers/tile-layer.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ At each integer zoom level (`z`), the XY plane in the view space is divided into

When the `TileLayer` is used with a geospatial view such as the [MapView](/docs/api-reference/core/map-view.md), x, y, and z are determined from [the OSM tile index](https://wiki.openstreetmap.org/wiki/Slippy_map_tilenames).

When the `TileLayer` is used used with a non-geospatial view such as the [OrthographicView](/docs/api-reference/core/orthographic-view.md) or the [OrbitView](/docs/api-reference/core/orbit-view.md), `x` and `y` increment from the world origin, and each tile's width and height match that defined by the `tileSize` prop. For example, the tile `x: 0, y: 0` occupies the square between `[0, 0]` and `[tileSize, tileSize]`.
When the `TileLayer` is used used with a non-geospatial view such as the [OrthographicView](/docs/api-reference/core/orthographic-view.md) or the [OrbitView](/docs/api-reference/core/orbit-view.md), `x` and `y` increment from the world origin, and each tile's width and height match that defined by the `tileSize` prop. For example, the tile `x: 0, y: 0` occupies the square between `[0, 0]` and `[tileSize, tileSize]`. If you need to offset the `z` level at which the tiles are fetched, there is a `tileOffset` prop.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This actually seemed more reflective of the state of things now than before. Before both methods used the OSM index, from what I can tell, but now the infovis obeys the above.



## Properties
Expand Down Expand Up @@ -138,10 +138,17 @@ if (signal.aborted) {

The pixel dimension of the tiles, usually a power of 2.

The tile size represents the target pixel width and height of each tile when rendered. Smaller tile sizes display the content at higher resolution, while the layer needs to load more tiles to fill the same viewport.
For geospatial viewports, tile size represents the target pixel width and height of each tile when rendered. Smaller tile sizes display the content at higher resolution, while the layer needs to load more tiles to fill the same viewport.
Pessimistress marked this conversation as resolved.
Show resolved Hide resolved

For non-geospatial viewports, the tile size should correspond to the true pixel size of the tiles.

- Default: `512`

##### `tileOffset` (Number, optional)

For non-geospatial viewports, this offset changes the zoom level at which the tiles are fetched.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Additional changes:

  • Recommend using zoomOffset in upgrade guide
  • Update the map-tiles example (tileSize: 256, zoomOffset: devicePixelRatio === 1 ? -1 : 0)
  • Update the image-tiles example (data: '${ROOT_URL}/moon.image_files/{z}/{x}_{y}.jpeg', zoomOffset: 15)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The last one doesn't quite work that way, I think. I need to think about this a bit. I think the issue might be the extent prop. I think it assumes that the 0th zoom level is where the full resolution of your tiled image renders, but that wouldn't be the case here. So this might actually be a bug in what we have at the moment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On second thought, this isn't really how I intended this prop to be used. It is meant more for fetching tiles at a higher or lower resolution given the current zoom level. So changing this prop independent of other things just changes the resolution of the image but this + 15 that currently exists in the image-tiles examples is meant just to reconcile their inverted (compared to ours) system.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The map-tiles use case makes sense to me though, but maybe I am missing something. There, previously, we were actually intentionally affecting the resolution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should clamp z to minZoom, maxZoom before applying any adjustment (both from tileSize and zoomOffset)? Part of the confusion comes from mixing the terms z and zoom. Either way it would be good to explain how this works in the minZoom, maxZoom documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that is a good call. This affects the level of fetching, not the viewport to which maxZoom and minZoom should apply.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One issue this could cause would be if you zoom into zoom level 0 with maxZoom: 0 and then offset by some number. The maxZoom prop currently prevents fetching at that level but now you would fetch there even though at zoom level -1 with zoomOffset: 1 you might want to fetch at zoom level 0. I think it is good as it stands now, but we should update the docs to explain that this clips the fetching, which is what is currently done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re-requesting review. Added a note to the docs.


- Default: `0`
ilan-gold marked this conversation as resolved.
Show resolved Hide resolved

##### `maxZoom` (Number|Null, optional)

Expand Down
9 changes: 6 additions & 3 deletions modules/geo-layers/src/tile-layer/tile-layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ const defaultProps = {
maxCacheByteSize: null,
refinementStrategy: STRATEGY_DEFAULT,
zRange: null,
maxRequests: 6
maxRequests: 6,
tileOffset: 0
};

export default class TileLayer extends CompositeLayer {
Expand Down Expand Up @@ -89,7 +90,8 @@ export default class TileLayer extends CompositeLayer {
maxCacheByteSize,
refinementStrategy,
extent,
maxRequests
maxRequests,
tileOffset
} = props;

return {
Expand All @@ -100,7 +102,8 @@ export default class TileLayer extends CompositeLayer {
tileSize,
refinementStrategy,
extent,
maxRequests
maxRequests,
tileOffset
};
}

Expand Down
8 changes: 5 additions & 3 deletions modules/geo-layers/src/tile-layer/tileset-2d.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ export default class Tileset2D {

// Returns array of {x, y, z}
getTileIndices({viewport, maxZoom, minZoom, zRange, modelMatrix, modelMatrixInverse}) {
const {tileSize, extent} = this.opts;
const {tileSize, extent, tileOffset} = this.opts;
return getTileIndices({
viewport,
maxZoom,
Expand All @@ -168,13 +168,15 @@ export default class Tileset2D {
tileSize,
extent,
modelMatrix,
modelMatrixInverse
modelMatrixInverse,
tileOffset
});
}

// Add custom metadata to tiles
getTileMetadata({x, y, z}) {
return {bbox: tileToBoundingBox(this._viewport, x, y, z)};
const {tileSize} = this.opts;
return {bbox: tileToBoundingBox(this._viewport, x, y, z, tileSize)};
}

// Returns {x, y, z} of the parent tile
Expand Down
34 changes: 21 additions & 13 deletions modules/geo-layers/src/tile-layer/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ function getIndexingCoords(bbox, scale, modelMatrixInverse) {
return bbox.map(i => (i * scale) / TILE_SIZE);
}

function getScale(z) {
return Math.pow(2, z);
function getScale(z, tileSize = TILE_SIZE) {
return (Math.pow(2, z) * TILE_SIZE) / tileSize;
}

// https://wiki.openstreetmap.org/wiki/Slippy_map_tilenames#Lon..2Flat._to_tile_numbers_2
Expand All @@ -130,25 +130,24 @@ export function osmTile2lngLat(x, y, z) {
return [lng, lat];
}

function tile2XY(x, y, z) {
const scale = getScale(z);
function tile2XY(x, y, z, tileSize) {
const scale = getScale(z, tileSize);
return [(x / scale) * TILE_SIZE, (y / scale) * TILE_SIZE];
}

export function tileToBoundingBox(viewport, x, y, z) {
export function tileToBoundingBox(viewport, x, y, z, tileSize = TILE_SIZE) {
if (viewport.isGeospatial) {
const [west, north] = osmTile2lngLat(x, y, z);
const [east, south] = osmTile2lngLat(x + 1, y + 1, z);
return {west, north, east, south};
}
const [left, top] = tile2XY(x, y, z);
const [right, bottom] = tile2XY(x + 1, y + 1, z);
const [left, top] = tile2XY(x, y, z, tileSize);
const [right, bottom] = tile2XY(x + 1, y + 1, z, tileSize);
return {left, top, right, bottom};
}

function getIdentityTileIndices(viewport, z, extent, modelMatrixInverse) {
function getIdentityTileIndices(viewport, z, tileSize, extent, modelMatrixInverse) {
const bbox = getBoundingBox(viewport, null, extent);
const scale = getScale(z);
const scale = getScale(z, tileSize);
const [minX, minY, maxX, maxY] = getIndexingCoords(bbox, scale, modelMatrixInverse);
const indices = [];

Expand Down Expand Up @@ -178,9 +177,12 @@ export function getTileIndices({
extent,
tileSize = TILE_SIZE,
modelMatrix,
modelMatrixInverse
modelMatrixInverse,
tileOffset = 0
}) {
let z = Math.round(viewport.zoom + Math.log2(TILE_SIZE / tileSize));
let z = viewport.isGeospatial
? Math.round(viewport.zoom + Math.log2(TILE_SIZE / tileSize))
: Math.ceil(viewport.zoom) + tileOffset;
Copy link
Collaborator Author

@ilan-gold ilan-gold Jun 21, 2021

Choose a reason for hiding this comment

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

Another option here would be Math.ceil(viewport.zoom + zoomOffset) which is less error-prone for users (since it would also produce a valid z value) but would be less interpretable since we apply the ceil operation.

if (Number.isFinite(minZoom) && z < minZoom) {
if (!extent) {
return [];
Expand All @@ -196,7 +198,13 @@ export function getTileIndices({
}
return viewport.isGeospatial
? getOSMTileIndices(viewport, z, zRange, extent || DEFAULT_EXTENT)
: getIdentityTileIndices(viewport, z, transformedExtent || DEFAULT_EXTENT, modelMatrixInverse);
: getIdentityTileIndices(
viewport,
z,
tileSize,
transformedExtent || DEFAULT_EXTENT,
modelMatrixInverse
);
}

/**
Expand Down
14 changes: 13 additions & 1 deletion test/modules/geo-layers/tile-layer/utils.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ const TEST_CASES = [
}
}),
tileSize: 256,
output: ['1,2,4', '1,3,4', '2,2,4', '2,3,4', '3,2,4', '3,3,4', '4,2,4', '4,3,4']
output: ['1,2,3', '1,3,3', '2,2,3', '2,3,3', '3,2,3', '3,3,3', '4,2,3', '4,3,3']
},
{
title: 'non-geospatial modelMatrix identity',
Expand Down Expand Up @@ -396,6 +396,12 @@ test('tileToBoundingBox#Infovis', t => {
'0,0,1 Should match the results.'
);

t.deepEqual(
tileToBoundingBox(viewport, 0, 0, 0, 256),
{left: 0, top: 0, right: 256, bottom: 256},
'0,0,0 with custom tileSize Should match the results.'
);

t.deepEqual(
tileToBoundingBox(viewport, 4, -1, 2),
{left: 512, top: -128, right: 640, bottom: 0},
Expand All @@ -408,6 +414,12 @@ test('tileToBoundingBox#Infovis', t => {
'4,-1,3 Should match the results.'
);

t.deepEqual(
tileToBoundingBox(viewport, 4, -1, 2, 256),
{left: 256, top: -64, right: 320, bottom: 0},
'4,-1,2 with custom tileSize Should match the results.'
);

t.end();
});

Expand Down