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

feat(layers): Trim Top of Pyramids #633

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- Use correct deck.gl 8.8 `getTileData` args.
- Replace `postversion` script with `version` script for CI release.
- Remove `package-lock.json` from root (since we use pnpm)
- Clip low resolution tiles that might be padded by a loader.

## 0.13.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,50 @@ import ImageLayer from '../image-layer';
import { getImageSize, isInterleaved, SIGNAL_ABORTED } from '@vivjs/loaders';
import { ColorPaletteExtension } from '@vivjs/extensions';

/**
* Here we create an object for checking clipping the black border of incoming tiles
* at low resolutions i.e for zarr tiles which are padded by zarr.
* We need to check a few things before trimming:
* 1. The height/width of the full image at the current resolution
* produces an image smaller than the current tileSize
* 2. The incoming image is padded to the tile size i.e one of its dimensions matches the tile size
* Once these have been confirmed, we trim the tile by going over it in row major order,
* keeping only the data that is not out of the clipped bounds.
* @param {{
* loader: PixelSource[],
* resolution: number,
* tileSize: number,
* }}
* @return {{ clip: function, height: number, width: number }}
*/
const createTileClipper = ({ loader, resolution, tileSize }) => {
Copy link
Member

Choose a reason for hiding this comment

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

tileSize is a property of loader[number].

const planarSize = Object.values(getImageSize(loader[0]));
const [clippedHeight, clippedWidth] = planarSize.map(size =>
Math.floor(size / 2 ** resolution)
);
Copy link
Member

Choose a reason for hiding this comment

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

This relies on the ordering of key names in getIamgeSize, if those were to change this could break.

Suggested change
const planarSize = Object.values(getImageSize(loader[0]));
const [clippedHeight, clippedWidth] = planarSize.map(size =>
Math.floor(size / 2 ** resolution)
);
const planarSize = getImageSize(loader[0]);
const [clippedHeight, clippedWidth] = [planarSize.height, planarSize.width].map(size =>
Math.floor(size / 2 ** resolution)
);

const isHeightUnderTileSize = clippedHeight < tileSize;
const isWidthUnderTileSize = clippedWidth < tileSize;
return {
clip: ({ data, height, width }) => {
if (
(isHeightUnderTileSize && height === tileSize) ||
(width === tileSize && isWidthUnderTileSize)
) {
return data.filter((d, ind) => {
return !(
(ind % tileSize >= clippedWidth && isWidthUnderTileSize) ||
(isHeightUnderTileSize &&
Math.floor(ind / tileSize) >= clippedHeight)
);
});
}
return data;
},
height: clippedHeight,
width: clippedWidth
};
};

const defaultProps = {
pickable: { type: 'boolean', value: true, compare: true },
onHover: { type: 'function', value: null, compare: false },
Expand Down Expand Up @@ -87,7 +131,7 @@ const MultiscaleImageLayer = class extends CompositeLayer {
const config = { x, y, selection, signal };
return loader[resolution].getTile(config);
};

const clipper = createTileClipper({ loader, resolution, tileSize });
try {
/*
* Try to request the tile data. The pixels sources can throw
Expand All @@ -98,11 +142,22 @@ const MultiscaleImageLayer = class extends CompositeLayer {
* return type, and optional throw for performance.
*/
const tiles = await Promise.all(selections.map(getTile));

const tile = {
data: tiles.map(d => d.data),
width: tiles[0].width,
height: tiles[0].height
data: tiles.map(d =>
clipper.clip({
data: d.data,
width: tiles[0].width,
height: tiles[0].height
})
Copy link
Member

@manzt manzt Sep 12, 2022

Choose a reason for hiding this comment

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

Is there a reason why we need to use the first tile over and over? The with and height should be consistent.

Suggested change
data: tiles.map(d =>
clipper.clip({
data: d.data,
width: tiles[0].width,
height: tiles[0].height
})
data: tiles.map(clipper.clip),

),
width:
clipper.width < tileSize && tiles[0].width === tileSize
? clipper.width
: tiles[0].width,
height:
clipper.height < tileSize && tiles[0].height === tileSize
? clipper.height
: tiles[0].height
Copy link
Member

Choose a reason for hiding this comment

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

I generally am just having a hard time understanding the logic of this bit of code. Now the logic for clipping the data and clipping the width and height are separated, which makes it difficult to reason through.

};

if (isInterleaved(loader[resolution].shape)) {
Expand Down