mapbox-gl-js icon indicating copy to clipboard operation
mapbox-gl-js copied to clipboard

Denote the "missing" vs "empty" tile behavior in `CustomSource` using the `undefined` vs `null` return value

Open stepankuzmin opened this issue 1 year ago • 4 comments

This PR denotes the "missing" vs "empty" tile behavior in CustomSource by the undefined vs null value returned by the loadTile and prepareTile implementations.

Before the fix, we draw nothing (an empty tile) if loadTile throws or returns no data (blank space on the first screenshot). After the fix, if loadTile returns undefined, such tiles are marked as errored, and a map draws an overscaled parent tile if there is any (red tile on the second screenshot).

If prepareTile returns null, such tiles are marked as loading, and a map draws nothing. (blank space on the first screenshot). If prepareTile returns undefined, a map draws an overscaled parent tile if there is any (red tile on the second screenshot).

loadTile or prepareTile returns null

Screen Shot 2022-07-08 at 15 41 51

loadTile or prepareTile returns undefined

Screen Shot 2022-07-08 at 15 41 57

Related to #11608

Launch Checklist

  • [x] briefly describe the changes in this PR
  • [x] manually test the debug page
  • [x] apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

stepankuzmin avatar Jul 06 '22 13:07 stepankuzmin

Documenting conversation from elsewhere: Erroring the tile will cause the parent tile to be shown, if it is available. If we need this in some cases but not in other we'll need to support both a way to error the tile and a way to make it empty.

ansis avatar Jul 07 '22 19:07 ansis

After some feedback, it looks like we should support both ways — to render nothing and to show an overscaled parent tile in the tile’s space.

I see two possible solutions: we can either add a boolean flag to the CustomSource constructor or use the undefined vs null returned value to denote the behavior.

// If the implementation returned `undefined` as tile data,
// mark the tile as `errored` to indicate that we have no data for it.
// A map will render an overscaled parent tile in the tile’s space.
if (data === undefined) {
    tile.state = 'errored';
    return callback(null);
}

// If the implementation returned `null` as tile data,
// mark the tile as `loaded` and use an an empty image as tile data.
// A map will render nothing in the tile’s space.
if (data === null) {
    const emptyImage = {width: this.tileSize, height: this.tileSize, data: null};
    this.loadTileData(tile, (emptyImage: any));
    tile.state = 'loaded';
    return callback(null);
}

@ansis @mourner, what do you think of these options?

stepankuzmin avatar Jul 11 '22 09:07 stepankuzmin

Another option would be to catch rejected promises and treat those as errors. Error/rejection == missing tile. Undefined/null === blank tile.

ansis avatar Jul 11 '22 14:07 ansis

@ansis yes, that's possible too. It's probably a matter of taste, but since the use-case implies that the user might want to return a missing tile explicitly, I'd prefer not to use throw to denote the missing tile and go like this: Undefined/Error/rejection == missing tile. Null === blank tile. Is that okay?

stepankuzmin avatar Jul 11 '22 16:07 stepankuzmin

I'm not sure if it's related to this PR or not, but this is an odd behavior I observe on the custom-source debug page, in which high-zoom tiles continue to be displayed as you zoom out and until you zoom in elsewhere. I wonder if this is related to needing to manually implement your own unload function or something? I do not observe this behavior for regular tiles on the debug/debug.html page with tile debug turned on.

https://user-images.githubusercontent.com/572717/189183238-37122528-893e-4cc3-8714-944d28a6105f.mov

rreusser avatar Sep 08 '22 17:09 rreusser

A little comment about mapboxgl.CustomSourceInterface<T> in version "2.10.0" This T is expected to be HTMLImageElement | ImageData | ImageBitmap as it is declared in

export type AnySourceData =
        ......
        | CustomSourceInterface<HTMLImageElement | ImageData | ImageBitmap>;

but raster is checked as

function isRaster(data: any): boolean {
    return data instanceof window.ImageData ||
        data instanceof window.ImageBitmap ||
        data instanceof window.HTMLCanvasElement;
}

HTMLCanvasElement is what I am after, but I have to tweak return types as I use TS.

Thank you for the CustomSource in general :)

SergeiMelman avatar Sep 15 '22 05:09 SergeiMelman

Hey @rreusser,

…high-zoom tiles continue to be displayed as you zoom out and until you zoom in elsewhere.

I did some digging, and it's a bug related to the retaining raster tiles when raster-fade-duration is set to 0 https://github.com/mapbox/mapbox-gl-js/issues/12241

stepankuzmin avatar Sep 16 '22 08:09 stepankuzmin

Good catch, @SergeiMelman!

I've updated the JSDoc for loadTile.

stepankuzmin avatar Sep 16 '22 10:09 stepankuzmin