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

Adding symbol icon layer before adding icons causes icons not to render at initial zoom #6231

Open
bartcorremans opened this issue Feb 26, 2018 · 26 comments
Labels

Comments

@bartcorremans
Copy link

Apologies for the long title, this is a bit of a strange one and it took me a while to pin down and consistently reproduce.

Reproduction: https://jsbin.com/wigirelagu/edit?html,output (add your access token at the top of the script)

This is based on https://www.mapbox.com/mapbox-gl-js/example/add-image/ with a few changes.

mapbox-gl-js version: 0.44.1

Steps to Trigger Behavior

  • Create a symbol layer that uses image icons
  • Add this layer to your map before you add the images that that layer uses

Expected Behavior

Icons appear at any zoom level and regardless of when the layer was added.

Actual Behavior

Icons don't appear on the map unless you zoom away from the zoom level that the map initially loads with. This applies to the entire integer zoom level; since the reproduction has an initial zoom of 7, the icons will be hidden when your zoom level is anywhere between 7 and 8.

Adding the layer after the icons are added works around the issue (you can try this in the repro), but it seems like this should work either way.

@mollymerp
Copy link
Contributor

@bartcorremans thanks for the report and reproduction!

@mollymerp
Copy link
Contributor

I don't think that adding a layer with a symbol that has not been added to the map should work, but we can definitely make error reporting better here, and avoid the behavior you're seeing where it just skips rendering the symbol on the initial zoom level.

@bartcorremans
Copy link
Author

bartcorremans commented Mar 2, 2018

Thanks for the reply! I understand that it might indeed not make sense for this to actually work in the first place, though I've found it convenient to be able to just add layers at any point without having to take into account that the loadImage/addImage workflow is async, and know that the icons will be rendered as soon as the images load.

The more I think about it, the less sense it actually makes that this would work to begin with. 😅

@anandthakker
Copy link
Contributor

I don't think that adding a layer with a symbol that has not been added to the map should work

I'd have expected this to work: an icon layer might refer mostly to icons that exist in the sprite sheet, but to a few that have to be added at runtime for some reason.

@jfirebaugh
Copy link
Contributor

In general, the style specification and runtime styling APIs requires that ID-based references (source IDs in layers, icon-image and *-pattern image IDs in layer properties, and source and layer IDs passed to runtime styling APIs) are valid -- referring to objects that actually exist. This helps catch programming mistakes and avoids undue complexity in the implementation.

I'm not sure I'm convinced of the case of making an exception in this case. It's true that we don't currently validate icon-image and *-pattern image references. This is mostly a historical accident, due to the fact that the image list is not embedded in the style (as in retrospect we should have required: #4086), and therefore not guaranteed to be available at validation time.

If we were to support temporarily-invalid image references, then each symbol, background, fill, and line layer would need to track the set of image references used by any feature, passing that set back to the main thread after layout. Then on addImage (and for consistency, removeImage), the main thread would need to iterate through the layers and trigger re-layout of any tiles that could be affected (both currently visible and in the in-memory tile cache). It's not an overwhelming amount of complexity, but still I'd prefer to avoid it and remain consistent with the way we treat other references.

@hyperknot
Copy link

hyperknot commented Jun 15, 2018

I'm having problems because of the exact same issue, I've created a question about it:
#6824

My JSBin: https://output.jsbin.com/riwutuw

My biggest problem related to this issue is that the official example "add-image", is showing a very unrelalistic pattern, where the whole map data is only added in the callback of an image loader.

  1. For everything but the most simplified example, only adding a map data after every single icon has been loaded over HTTP would result in an empty map for a long time, especially over slow connections.

    Just imagine if your browser would always display a blank page until all the images have been completely loaded. Clearly no one would browse the internet like that.

  2. If there is any error with any loadImage request, the map would never display. And in real world this is happens surprisingly frequently, especially on mobile clients.

I believe the ideal solution would be to trigger a layout update after an addImage. As a workaround it'd be very important to be able to trigger a layout update manually.

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Jun 15, 2018

@hyperknot What about waiting to use addLayer until all the necessary images have been added?

Edit: oh, I think I understand. By "the whole map data" you don't mean the whole basemap, you mean that even waiting to display overlay layers until all icons are available is problematic. Hmm, will have to think about that.

@hyperknot
Copy link

hyperknot commented Jun 15, 2018

@jfirebaugh that's indeed a bit better, although both points 1. and 2. are still valid, if with fewer features.

In the meantime I've been able to solve this with some MobX magic:

  1. adding a .loaded property to each image
  2. making the icon-image key depend on the .loaded property, observing when it changes

Using this delicate MobX setup it seems to work well in my case. I say delicate because it all depends on never using icon-image before a given image has loaded.

I'll see how reliable this setup is, but it'd be nice if it was possible to manually trigger a layer/layout recalculation. Then everyone could just use it in loadImage's callback.

@hyperknot
Copy link

Here is a GIF about how the end results look. I'm really happy with this behaviour, I believe this is the nicest UX behaviour for this problem.

screenflow

@robinsummerhill
Copy link

robinsummerhill commented Aug 10, 2018

Just to confirm that this is a real problem and not an edge case. We are loading a complete map style that includes a base map and some overlay layers that are displaying a custom vector tile source. The overlay layers require some symbol images to be loaded by loadImage/addImage. We can't do it before the style is loaded because addImage requires a style. We can't do it after the style is loaded (in an on('load') handler) because it is too late and you see the disappearing icons bug above.

Having said that: hiding and then showing the affected layers fixes the problem.

@jacknkandy
Copy link

jacknkandy commented Aug 30, 2018

I also would like to report that I am experiencing the same issue. I do understand the claims that we should be waiting for the image to be loaded before adding the layer, but this is something that is quite difficult to manage in a highly dynamic application.

However, the fact that the icons are visible at the other zoom levels makes me think that this is more of a bug then intended functionality.

@jacknkandy
Copy link

I would also like to add that this issue only seems to be occurring in Chrome (66.0.3359.117). I have tested in Firefox (60.0.2) and Edge (41) and this issue is not present.

@jedgar1mx
Copy link

are there any updates on this issue?

@asheemmamoowala
Copy link
Contributor

#7987 and #7999 are potential ways to provide images after the layer is loaded.

@joewoodhouse
Copy link
Contributor

Not really sure why this issue was closed, the issue still remains. Using the new styleimagemissing event still results in the icon not being rendered at the initial zoom level. Can we get a proper fix for this?

@Akiyamka
Copy link

The problem is still relevant. Active zoom on which the image has not yet been uploaded remains empty even after the image has been uploaded.
Demo

@dimitrilahaye
Copy link

Here is a GIF about how the end results look. I'm really happy with this behaviour, I believe this is the nicest UX behaviour for this problem.

screenflow

Hi everyone, Have you got the code used to implement this behavior?

@xialvjun
Copy link

The problem is still relevant.

@Akiyamka
Copy link

look like need reopen this

@atyshka
Copy link

atyshka commented Dec 21, 2020

+1 on this. My use case is my images are known ahead of time, but that gives me 2 options. The first is to wait for all images to resolve before adding layers. This is obviously undesirable because it will add significant lag. The second is to add the layer in the callback after image load. I believe this is how it's done in the official mapbox docs. The problem with this is now the order in which layers are added is non-deterministic, which affects ordering.

@jfirebaugh makes a good point about how it would be a pain to implement this auto-reload. But what about a manual reload option? My proposed solution would be to add the image asynchronously, add in the layer, and then in the callback once the image loads, force reload images on the map via some new method on the map instance. Would that be a doable solution?

@BoJIbFbI4
Copy link

Is there some news about this issue?

@asheemmamoowala
Copy link
Contributor

Using the new styleimagemissing event still results in the icon not being rendered at the initial zoom level.

@joewoodhouse sorry about closing this pre-maturely.

@andrewharvey
Copy link
Collaborator

andrewharvey commented Sep 22, 2021

In my case, new data is added to the map on the fly from a remote service via a GeoJSON source with setData, but the icon image needs to be retrieved from a URL which comes with the dynamic data, which is then added via addImage, so the layer is already on the map by the time this happens. The only workaround I can think of is to call removeLayer then addLayer after each addImage.

A suitable workaround appears to be setLayoutProperty for any property value, eg icon-rotate = 0, even if the default forces it to refresh and show the newly added image.

@andrewharvey
Copy link
Collaborator

andrewharvey commented Oct 26, 2021

I've narrowed down a viable workaround when using a layers that use a GeoJSON source. You can either re-call setData with the same data, or if you're happy to cross into the private api call map.getSource('foo')._updateWorkerData() immediately after you've called addImage.

@Pictor13
Copy link

Pictor13 commented Sep 1, 2022

Seeing many are having troubles finding a good solution (myself included), I share a snippet similar to what I did to ensure everything is loaded AND added before adding a layer:

[⚠️ ❗️UNTESTED CODE: DO NOT COPY PASTE❗️ ⚠️ ]

// asynchronous loading & adding of map. wait for all to be terminated.
function loadImagesFrom(features, map) {
  const promisedImages = []
  for (var feature of features) {
    const imageUrl = feature.image
    const imageId = feature.id

    const promisedImage = new Promise((resolve, reject) => {
      map.loadImage(imageUrl, (error, image) => {
        // hint: for fallback logic use 'coalesce' (https://docs.mapbox.com/mapbox-gl-js/example/fallback-image/);
        // avoid 'styleimagemissing' or you'll have the same issue with addLayer used before addImage (mapbox-gl-js/issues/6231).
        if (error) return reject(error)
        if (!map.hasImage(imageId)) {
          map.addImage(imageId, image)
          return resolve(image)
        }
      })
    })
    promisedImages.push(promisedImage)
  }

  // wait for all images to be ready (either failing and successful)
  return Promise.allSettled(promisedImages)
}

function addLayerTo(map, geojsonData) {
  map.addSource('foobar', { type: 'geojson', data: geojsonData })
  /*
    ..... you own logic...
  */
  map.addLayer(/* layer specification */)
}

const map = new mapboxgl.Map(/* map config */)

$.getJSON(geojsonUrl)
  .done((geojson, textStatus, jqXHR) => {
    if (jqXHR.status !== 200 || !geojson)
      throw Error('no geojson')

    loadImagesFrom(geojson.features, map)
      .then(
        addedImages => addLayerTo(map, geojson)
      )
  })

Hopefully it saves some time to some one (and maybe a lot of time to a lot of ones 🙃).

@stevage
Copy link
Contributor

stevage commented Jan 17, 2024

I'm having an issue that may or may not be this one.

But I wanted to chime in and say: this is definitely a workflow that I expect to work. Just like a web page loads its various bits of content in any order, and the browser deals with it, it is much easier to write code that can fetch map data while also fetching icon images. And it should not matter which order the two events complete in: map data first or icon image added.

To require a fixed order means worse performance: we have to sit and wait for the icons to load before we can begin adding map data.

FWIW Andrew Harvey's workaround doesn't work in v3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests