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

add styleimagemissing event for on-demand images #7987

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Mar 4, 2019

This is the first of several dynamic icon prs.

The event gets fired when a layer needs an image that the map doesn't have. The developer has a chance to listen for this event and add an image within the callback in order to have it included.

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

fix #7587

map.on('styleimageneeded', function(e) {
    var image = generateImage(e.id);
    map.addImage(e.id, image);
});

See the committed add-image-missing-generated example for a full version.

Naming

I'm pretty ok with styleimageneeded. Other alternatives were:

  • styleimagemissing
  • imageneeded
  • imagemissing

Do you prefer any of these?

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • [n/a] post benchmark scores
  • manually test the debug page

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

I think this looks great. Just a couple nits on language.

I don't have a strong preference on the name of the event. styleimageneeded and styleimagemissing both work for me. If you want to include a warning as well, that's probably not a bad idea, but otherwise 👍

src/ui/events.js Outdated
@@ -778,6 +778,20 @@ export type MapEvent =
*/
| 'sourcedataloading'

/**
* Fired when an icon or pattern needed by the style is missing. The missing image can
* be added with {@link Map#addImage} within this callback to prevent the image from
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to be explicit here -> within the event listener callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, good idea!

@@ -0,0 +1,11 @@
/*---
title: Generate and add a missing icon to the map
description: Add a missing icon to the map that was generated at runtime.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: in the map that was generated at runtime, "that" appears to reference "map". Maybe something like Dynamically generate a missing icon at runtime and add it to the map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that's better. Thanks!

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

The example in this PR is 💯!

I think the styleimage prefix is good for distinguishing from source images (for image or raster sources). I like missing vs needed because it describes the event better. styleimagenotfound is one more for the naming 🎩.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

👍 No strong preference but styleimagemissing sounds good to me.

The event gets fired when a layer needs an image that the map doesn't
have. The developer has a chance to listen for this event and add an
image within the callback in order to have it included.

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

fix #7587
@ansis
Copy link
Contributor Author

ansis commented Mar 6, 2019

Thanks for the suggestions and feedback. I'll merge this as styleimagemissing

@joewoodhouse
Copy link
Contributor

#6231 for me renders this feature unusable (though I love the idea of it). Currently that bug still exists for me in my use-case - the icons never properly render at the initial zoom level when I add them via the styleimagemissing event. As the initial reports say, they seem to mostly render fine at all other zoom levels, which makes me think this must surely just be a bug that needs fixing? Even if I heavily delay adding the layers that have the missing icons, they still don't render properly.

@mourner
Copy link
Member

mourner commented Jun 10, 2019

@joewoodhouse can you submit a new bug report with a minimal reproducible test case for this (e.g. using JSFiddle)?

@joewoodhouse
Copy link
Contributor

@mourner I've created #8335 which includes a codepen to recreate the issue.

@mourner mourner deleted the styleimageneeded branch August 1, 2019 11:46
madwort added a commit to WoodlandsCoUk/components that referenced this pull request Feb 26, 2021
* reproducible issues were observed where marker icons don't display at initial zoom, but visible at other zoom levels
  * reproducible on Android browsers
  * also in Firefox desktop with connection throttling set to "Regular 3G"
* caused by marker image not loaded in time
* in theory we can use ['styleimagemissing'](mapbox/mapbox-gl-js#7987) to handle this, but this does not work with image files loaded via callback, only for inline generated images
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: a way to conditionally provide icon images
5 participants