Allow custom done callback for RasterTileSource.loadTile

[timiyay]

I have a use case that requires me to make a custom SourceType, for which I was hoping to inherit from RasterTileSource.

I'm integrating an external WMS into our mapbox-gl-js project, as they provide high-quality aerial imagery for my area of interest.

The issue is that the WMS will always return a 200. When tiles are missing for high zoom levels, the WMS return a 200 and a blank white tile.

I need a custom SourceType so that I can programatically-detect these blank tiles, and prevent them being rendered.

I realise there are current issues and PRs looking at custom SourceTypes. Nonetheless, for my use case, I would be greatly aided if RasterTileSource.loadTile was refactored into separate methods: https://github.com/mapbox/mapbox-gl-js/blob/master/js/source/raster_tile_source.js#L49

If I was able to override the done() function and then, on success, pass my img object to a separate function that called the GL rendering code from line 59 (https://github.com/mapbox/mapbox-gl-js/blob/master/js/source/raster_tile_source.js#L59), then it'd be an easy switch.

As it stands, I'll have to cut'n'paste all of code from loadTile into my own version of the function.

Would there be any support for this change within the master mapbox-gl-js repo, or do you see this use case as being better-solved by existing/upcoming capabilities of custom SourceTypes?

I'm happy to whip up a PR, if there's support for this change.

[anandthakker]

@timiyay thanks for raising this. Ergonomic extensibility of RasterTileSource is definitely something that wasn't really addressed in the current custom source type setup.

I'm thinking that a possible variation to your proposal might be to extend the RasterSource constructor to accept a fourth, optional loadTileImage function with a signature something like (TileCoordinate, callback) => void, which, if provided, is responsible for fetching the raw image data (and i guess maybe the content type?) for a given tile and calling the callback with it.

Then a slightly reworked version of these lines would essentially be the default implementation of loadTileImage.

Once this was in place, then implementing a custom raster source could look like:

var RasterSource = require('mapbox-gl/js/source/raster_source')
function MyRasterSource (id, options, dispatcher) {
   var rasterSource = new RasterSource(id, options, dispatcher, myLoadTileImage);
   // delegate Source interface methods to the RasterSource instance:
   this.loadTile = rasterSource.loadTile.bind(rasterSource);
   this.reloadTile = rasterSource.reloadTile.bind(rasterSource);
   this.unloadTile = rasterSource.unloadTile.bind(rasterSource);
   this.abortTile = rasterSource.abortTile.bind(rasterSource);
   this.serialize = function () {
      return Object.assign(rasterSource.serialize(), { type: 'my-source-type' })
   }
}

function myLoadTileImage (coord, callback) { /* do custom stuff */ }

[anandthakker]

^ cc @lucaswoj

[timiyay]

@anandthakker Yes this sounds like it'd work for my use case.

It seems like my issue is related to this PR (#2918), in the sense that we need to customise the way a given Source interacts with a remote server. In that PR, the customisation is needed for the AJAX request, whereas I need customisation for the AJAX response.

Perhaps there's scope to add a couple of hooks into the design of Source, such as tileWillBeRequested and tileWasRequested. These would receive request / response objects, and would be allowed to modify and return them, at which point they'd be handed to the next function in the request chain.

As you say, a reworked version of the AJAX request line (https://github.com/mapbox/mapbox-gl-js/blob/master/js/source/raster_tile_source.js#L45-L47) would achieve this. From what I've seen in other issues, my minimal spec would be:

• allow setting custom headers in the request
• allow inspection of response
• allow prevention of tile being rendered, based on custom processing of response

[lucaswoj]

The intent behind using composition to create custom source types from core source types is that custom implementations can be supplied for any method.

What would it look like to accommodate this use case by writing a custom implementation for MyRasterSource#loadTile? How can we make that workflow easier?

var RasterSource = require('mapbox-gl/js/source/raster_source')
function MyRasterSource (id, options, dispatcher) {
   var rasterSource = new RasterSource(id, options, dispatcher);
   // delegate Source interface methods to the RasterSource instance:
   this.loadTile = function(tile, callback) {
        ...
   }
   this.reloadTile = rasterSource.reloadTile.bind(rasterSource);
   this.unloadTile = rasterSource.unloadTile.bind(rasterSource);
   this.abortTile = rasterSource.abortTile.bind(rasterSource);
   this.serialize = function () {
      return Object.assign(rasterSource.serialize(), { type: 'my-source-type' })
   }
}

[anandthakker]

@lucaswoj I think the present difficulty with this approach is that loadTile is doing two different things: 1) loading the source raster data from the tile service and 2) setting up the GL textures for the painter.

@timiyay's use case involves providing a custom implementation for 1), but not for 2). Supporting this requires some way of separating the two. One way to do this is to accept a custom implementation for 1) as an optional constructor parameter -- that's what I proposed above, and is consistent with how VectorTileSource currently works. But, @lucaswoj, it seems like you're not quite sold on that...

[timiyay]

@lucaswoj, as @anandthakker describes, the issue is that loadTile is doing multiple things. For my use case, loadTile is performing 3 sequential actions, and I need to override the 2nd one.

Let's call these actions:

1. sendRequest
2. handleResponse
3. setUpGLTextures

Here's how they break down in the code:

    loadTile: function(tile, callback) {
        // Part 1: sendRequest
        var url = normalizeURL(tile.coord.url(this.tiles, null, this.scheme), this.url, this.tileSize);

        tile.request = ajax.getImage(url, done.bind(this));

        function done(err, img) {
            // Part 2: handleResponse
            // This is the part I want to customise, so that I can apply custom rules
            // to decide whether a WMS raster tile should be considered 404 Not Found or aborted,
            // based on the unchangeable constraints of the source WMS server.
            delete tile.request;

            if (tile.aborted)
                return;

            if (err) {
                return callback(err);
            }

            // Part 3: setUpGLTextures
            // I'm a GL n00b, so this is all voodoo to me. I'd like to have a composable chain
            // of functions that ends up with me handing a result to this GL-setup code, and not
            // having to know anything about it
            var gl = this.map.painter.gl;
            tile.texture = this.map.painter.getTexture(img.width);
            if (tile.texture) {
                gl.bindTexture(gl.TEXTURE_2D, tile.texture);
                gl.texSubImage2D(gl.TEXTURE_2D, 0, 0, 0, gl.RGBA, gl.UNSIGNED_BYTE, img);
            } else {
                tile.texture = gl.createTexture();
                gl.bindTexture(gl.TEXTURE_2D, tile.texture);
                gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR_MIPMAP_NEAREST);
                gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.LINEAR);
                gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S, gl.CLAMP_TO_EDGE);
                gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_T, gl.CLAMP_TO_EDGE);
                gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, true);
                gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, img);
                tile.texture.size = img.width;
            }
            gl.generateMipmap(gl.TEXTURE_2D);

            this.map.animationLoop.set(this.map.style.rasterFadeDuration);

            tile.state = 'loaded';

            callback(null);
        }
    },

The composability shown in #3123 (comment) - overriding this.loadTile, this.reloadTile, etc - is perfect. All I need is for loadTile to be broken into 3 separate functions, so I can override the handleResponse portion.

Beyond my use case, I noted that another PR (#2918) has asked for similar extensibility in the sendRequest phase, in order to customise headers. This may or may not be relevant to my issue - I don't have my head fully around the architecture of custom sources just yet.

[timiyay]

I'm spending some of my downtime looking at this, attempting to find a graceful way to add this change to the Source interface, so that it propagates to all built-in Source types.

I see that ImageSource and VideoSource each have a prepare method which, if present, is called in Painter.renderPass. Would it be appropriate to add this prepare method to RasterTileSource, to encapsulate the code I described as setUpGLTextures above?

[lucaswoj]

My ideal architecture would factor the loadTile method into

• getTileURL(tile) - get a tile's URL according to TileJSON declarations
• populateTile(tile, imageData) - populate a tile with a GL texture created from an ImageData object (returned by ajax.getImage)
• loadTile(tile, callback) - glue together getTileURL, ajax.getImage, and populateTile, ideal point for overriding default behaviour

[lucaswoj]

prepare is a little different: prepare is called every time the source's tile is drawn. populateTile would only be called once when the tile was being loaded.

[timiyay]

Giggity, thanks for the tip. I'll get a PR together by the end of the weekend.

Update: PR is delayed, as I've been dragged down a JS modules / browserify rabbit-hole.

I want to be able to import js/sources/raster_tile_source.js into my EmberJS app code. Due to the fragmented nature of JS module imports, this is harder than it sounds. I'm using ember-browserify to handle the imports, which works well in most cases.

In this case, however, I'm hitting issues when:

• my EmberJS static files are built and deployed via CI, rather than locally
• mapbox-gl-js tries to create web workers

For each web worker, I'm getting a console error saying Uncaught TypeError: Cannot read property '0' of undefined blob:http://my-staging-site.com.au/ye87fhe9r9hf9e90d0fje

The ye87fhe9r9hf9e90d0fje hash is different per-worker, and is a link to my vendor.js file. Something fishy is happening when a web worker is trying to load its code from my browserified vendor.js, perhaps?

Once I solve this issue of being able to import a file from js/sources, then I'll be back on this PR.

[lucaswoj]

Merging this into #3186