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

Support loading icons asynchronously on styleimagemissing

Open joewoodhouse opened this issue 5 years ago • 16 comments

Update: Design proposals that close this issue are in https://github.com/mapbox/mapbox-gl-js/issues/9018.


mapbox-gl-js version: 1.0.0

browser: Chrome 75.0.3770.80

Steps to Trigger Behavior

  1. Load a map with a style containing a layer that has a missing icon-image
  2. Attach to the styleimagemissing event and load an image for the missing icon-image

Link to Demonstration

This codepen demonstrates the behaviour: https://codepen.io/anon/pen/OeLYqW

It does the following:

  • Loads the streets-v11 style JSON (via the API)
  • Appends a symbol layer to the style's layers. This new layer re-uses an existing source layer (composite -> roads) but the icon-image is set to an non-existent value
  • It also attaches to the 'styleimagemissing' event and loads up a valid image when it fires.
  • Once the map has loaded, it waits 1.5 seconds then outputs how many features have been rendered from the test layer.

When you reload the codepen (reload the entire browser tab) you should see the number of features rendered is basically random. The correct answer (which you can see by changing the code-pen and setting the 'icon-image' property to e.g. beer-11 which is an image from the sprite) is 21, but I saw results like 0, 4, 23,24,25 (not sure how there is more!)

Expected Behavior

Features should render properly at initial zoom and all other zooms

Actual Behavior

On initial load, the features with the missing icon render seemingly at random.

joewoodhouse avatar Jun 10 '19 19:06 joewoodhouse

@mourner any updates on this one? I sort of built quite a large feature based off this new styleimagemissing functionality, and this bug has now made it unusable. I'd be happy to have a go at fixing it myself, if you could provide any sort of starting point as to where/what you think the issue might be.

joewoodhouse avatar Jun 21 '19 09:06 joewoodhouse

Even I am facing the same issue, any update on this

ghost avatar Jun 23 '19 12:06 ghost

Sorry for a late response! This seems to be working as designed — here's a relevant bit of the docs:

The missing image can be added with Map#addImage within this event listener callback to prevent the image from being skipped.

This means you have to synchronously call map.addImage within the styleimagemissing callback. So to fix your example, you would load the fallback image first, then add the layer that contains missing images (so that the image data is already there inside imagemissing).

mourner avatar Jun 24 '19 16:06 mourner

@mourner I don't agree with closing this issue. Surely your suggestion totally defeats the purpose of the feature itself? To quote the original PR/commit (https://github.com/mapbox/mapbox-gl-js/commit/fd32cc3f33909d225ad9da09623957387af85b13)

This event can be used to dynamically generate icons based on feature properties.

If you have to pre-add all potential images that might be missing before adding the layer, I wouldn't describe that as dynamically generating them. It also requires you to load images that potentially may never be used.

Also my codepen example I put together clearly shows something is broken, and it's an issue I'm facing in my production code. Please could you reconsider this?

joewoodhouse avatar Jun 24 '19 20:06 joewoodhouse

@joewoodhouse the codepen shows the API being misused so it's expected to be broken. The purpose of the feature was to allow generating icons on demand, which has to be synchronous (a technical limitation in the way the renderer handles icons), as opposed to loading icons on demand. You can see the intent demonstrated in the official example.

Perhaps we could add another, asynchronous API to handle your use case, but I'm not sure if that's technically viable — @ansis any thoughts on this? I'll reopen the issue to discuss.

mourner avatar Jun 24 '19 21:06 mourner

@joewoodhouse @rhushikeshSahaj, do you know the size of the images you want to add through styleimagemissing in advance? if so, you can synchronously add a blank image in the same size as your actual image, and then use the updateImage method to replace the blank image with your actual image after you've fetched it asynchronously.

chloekraw avatar Jul 02 '19 17:07 chloekraw

@chloekraw @mourner @ansis nice idea, but no I think the use-case I'm looking for here is that you don't know anything about the images (what size, how many, etc) before you load the map. Imagine a map where you're loading some 3rd party GeoJSON, and you want to style the icon-image based on a particular property of the features. You don't know in advance what values that property might have. Or you might want to create the icon-image based on some computation of a number of properties in the record. That's the use-case I'm imagining for this feature.

The feature itself is good, it's purely the restriction that images have to be loaded synchronously. I can't really imagine any real-world scenarios which would match the examples and tests, where an image is loaded by creating the image data manually in e.g. a Uint8Array. Surely 95% of real-world use-cases are going to be loading an image from a URL.

joewoodhouse avatar Jul 02 '19 18:07 joewoodhouse

Thanks for the feedback, @joewoodhouse! We will look into technical implementation paths for supporting synchronous image loading in our next iteration of the styleimagemissing feature, but this work isn't currently planned.

chloekraw avatar Jul 08 '19 18:07 chloekraw

We've encountered this issue. The workaround we've found is to hide and show the layer after the image has loaded which seems to trigger it to display. Would be great if there was a better solution though

joedjc avatar Nov 20 '19 18:11 joedjc

I was able to work around this issue by reloading the tile cache after calling addImage with the real data, although this does involve a bunch of hidden APIs.

/**
 * Reload tile cache of all non-raster sources in the map.
 * Used when adding new images, since Mapbox doesn't play well with image sizes changing.
 */
function reloadTileCache(map: mapboxgl.Map) {
  for (const cache of Object.values((map as any).style.sourceCaches)) {
    // Skip raster sources, like satellite tiles.
    if ((cache as any)._source.type === 'raster') {
      continue;
    }
    (cache as any).reload();
  }
}

zacharyliu avatar Nov 20 '19 20:11 zacharyliu

Hi Guys,

Any update of this? We have a scenario where we want to be able to create an image through the canvas and return the data, but there is no way around async. Even if we have Base64 data and want to use the Image/Canvas object, we have to wait for an onload on image.src = BASE64.

Any chance of a review of this priority? Even if we call "addImage" and we can identify which sources need refresh?

Thanks Cam

cammanderson avatar Feb 14 '20 04:02 cammanderson

@cammanderson we have provided design proposals for fixing this issue here: https://github.com/mapbox/mapbox-gl-js/issues/9018

If you all have feedback for the API design, it would be helpful to provide it on that ticket.

cc @zacharyliu @joedjc @joewoodhouse @rhushikeshSahaj

chloekraw avatar Feb 14 '20 10:02 chloekraw

@chloekraw Since we are also encountering the same limitation of not being able to offload any work in the onstyleimagemissing callback to a background thread, I created a little demo project (for the Android SDK) that demonstrates our use-case. https://github.com/florianPOLARSTEPS/mapbox-image-loading I hope that helps for understanding on how exactly this would be a very useful feature to have.

Thanks!

florianPOLARSTEPS avatar Aug 18 '20 16:08 florianPOLARSTEPS

Here is my workaround:

    this.map.on('styleimagemissing', (event) => {
        const id = event.id;
        this.map.addImage(id, {width: 0, height: 0, data: new Uint8Array()});
        this.map.loadImage(someUrl+id, (error, image) => {
          if (!error) {
            this.map.removeImage(id);
            this.map.addImage(id, image);
          } else {
            ...
          }
        });
      }
    });

So basically I add a 0x0 image with the id, and after asynchronously loading the actual image I remove the previous image and add the loaded one. It seems to work in my case with 1.13.1

stefanrybacki avatar Feb 22 '21 18:02 stefanrybacki

@stefanrybacki A repaint is needed in that case.

djibarian avatar May 04 '21 11:05 djibarian

I handle it using a "pending images" dictionary that blocks further calls for a specific image-id during the async load. (I.e similar to @stefanrybacki but using an external DS) Seems to work quite fine so far, no need to explicitly repaint. Is this approach correct or am I missing something subtle that could fail?

blq avatar Jul 07 '22 10:07 blq