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 expression operator to determine style image availability #5261

Closed
1ec5 opened this issue Sep 8, 2017 · 15 comments
Closed

Add expression operator to determine style image availability #5261

1ec5 opened this issue Sep 8, 2017 · 15 comments
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏

Comments

@1ec5
Copy link
Contributor

1ec5 commented Sep 8, 2017

The style specification should have an image-exists or has-image operator for use in expressions. It would accept one argument, the name of an image, and return a Boolean indicating whether the image is part of the style (e.g., via addImage()). Typically, this operator would be used inside a case operator.

This operator would allow style designers to significantly condense any symbol layers that display route shields using the Mapbox Streets source by no longer hard-coding a list of known shield values: #4149 (comment). It would also future-proof those layers for any additional shield values that we would add to the source down the line; currently such changes are breaking changes because the hard-coded list has no fallback mechanism.

An image-exists operator would be more flexible than the “image stacks” proposed in #4149: it could be used on any property, not only an image-typed property. The existence of an image with a given name could determine not only the icon to display but also the color of the text to superimpose. A designer would still need to employ the -white hack described at the bottom of #4149 (comment) until we get a substring expression operator (#4113), but at least the long and fragile list would go away and we’d be able to use a single layer for both black- and white-lettered shields.

For context, a shield fallback was a key use case that led to the implementation of expressions in the first place: mapbox/mapbox-gl-style-spec#104 (comment). In #4715 (comment), we decided to punt on this use case for the initial PR that implemented expressions.

/cc @anandthakker @jfirebaugh @mapbox/cartography

@1ec5 1ec5 added cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏 labels Sep 8, 2017
@jfirebaugh
Copy link
Contributor

With an eye towards future introduction of expression forms that combine, lighten, darken, recolor, or otherwise manipulate images, I would design this slightly differently, providing an ["image", id] expression form whose result type is Image | Null, where Image is a new type. Then coalesce is convenient for finding the first available image in a stack, and case can still be used as well.

@1ec5
Copy link
Contributor Author

1ec5 commented Sep 8, 2017

Would properties like icon-image continue to take a string, or would they migrate over to the new Image type?

@1ec5
Copy link
Contributor Author

1ec5 commented Oct 30, 2017

Moving icon-image over to a new Image type seems like a pretty long-range proposal. In the meantime, it wouldn’t be possible to use coalesce with the image operator for icon-image. Instead, you’d have to write ["match", ["image", "us-oh-sci-3"], null, "us-oh-sci-3"].

If we were to introduce a has-image operator, that wouldn’t preclude a future image property, would it? has-image would also be a potential opportunity for optimization, since the expression wouldn’t need the actual value of the image.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 1, 2017

Tangentially, #5775 exposes a hasImage method to avoid committing to a specific image format in the GL JS API.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 26, 2018

Since my original work on conditional images in baf963b and 471f003, SymbolBucket.populate() has changed dramatically, such that it no longer has access to the list of image names in the style, but this method is still responsible for evaluating the icon-image expression.

@ChrisLoer came up with two suggestions for plumbing this information through to SymbolBucket:

  1. Return a list of images in every getImages callback. This has the advantage of being up-to-date and pretty simple, but the disadvantage (maybe?) that you may be sending back just one or two images, but then you have to serialize/transfer a long list of image names to the background on every tile load.
  2. Have the workers maintain a shared image list the same way they do with layers. When you mutate the set of images in the Style, update the workers with an analog of Style#updateWorkerLayers. This has the advantage of minimizing the amount of data transfer, and I think there’s a fair argument that since it’s a style update, it should follow the same path as layer updates. The downside is (a) it’s probably a little bit (?) more complicated to implement, (b) the logic on GL JS is a little bit different from GL Native (although the basic idea is the same).

/cc @nickidlugash

@nickidlugash
Copy link

@1ec5 @ChrisLoer @jfirebaugh @anandthakker @lucaswoj This feature is still something that the cartography team would find valuable to have implemented and available in our APIs and SDKs by mid Q2 2018 (which I'm assuming will be far in advance of any larger style spec image refactors). The most critical gain for us would be flexibility in when we can add additional values to maki (POI), highway shield, and rail station fields in the Mapbox Streets vector tiles. Without the ability to look up image existence in sprites, all icon value additions are considered a breaking change and can only be added at major version bumps. If this feature is not available by the time we release Streets v8, we will not be able to support any additions for at least a year after. This could be potentially concerning if there are critical regional markets that customers need added support for.

From a style spec perspective, does it makes sense to add this feature now (before other image refactors)? Please let me know if I can help further scope how this feature might be used, either to help decide whether we can more forward with this now or to help determine the best method for implementation.

/cc @ajashton @eschow

@1ec5
Copy link
Contributor Author

1ec5 commented May 11, 2018

Return a list of images in every getImages callback. This has the advantage of being up-to-date and pretty simple, but the disadvantage (maybe?) that you may be sending back just one or two images, but then you have to serialize/transfer a long list of image names to the background on every tile load.

By the time getImages is sent, the icon dependencies have already been computed. But with this operator, the list of icon dependencies depends on knowing all the possible outputs of the expression.

actor.send('getImages', {icons}, (err, result) => {
if (!error) {
error = err;
imageMap = result;
maybePrepare.call(this);
}
});

@chloekraw
Copy link
Contributor

chloekraw commented Aug 2, 2019

This (#5261 (comment)) is a valuable addition that could have broad applicability. We've discussed its utility in icon fallbacks in this ticket and #8052 (comment), and it could also be used to support composing multiple images together as a single icon-image (mapbox/mapbox-gl-native#14764).

We should consider how to handle the interaction between an image operator and the styleimagemissing callback for adding missing images on-demand (#7987, #7587).

In a chat with @ansis, we discussed a few options:

  • a) Always fire the callback with the image operator if it evaluates to the name of an image that's not on the spritesheet, and accept any potential performance tradeoff for features in which we don't think we need the callback to fire
  • b) Allow the developer to choose: Add a boolean object to the image operator that controls whether the callback fires: [“image”, name, missing]
    • By default, missing would be set to false.
    • Developers who are aware of styleimagemissing and know how to use it can turn it on if they have a need for it in a particular use case.
  • c) Create two operators, one where the callback fires and the other one where it does not

I'm comfortable with option A as I think firing the event is fairly cheap and don't foresee huge issues with it, though it would be nice if it were possible to gather data on the question. If we're nervous about always firing the event, I prefer option B over C, which introduces unnecessary complexity.

The main con to option B is a general hesitation around introducing additional objects to an operator with broad applicability. We don't want to get into the habit of bloating image with unnecessary objects, as it could complicate its usage in future capabilities. In this particular case, we always fire styleimagemissing when icon-image can't be found, so I think it would be fair to add this option to the operator, as every feature that touches icons is going to have to consider whether/how to handle missing images.

cc @1ec5 @alexshalamov @mapbox/gl-js @mapbox/gl-native @mapbox/maps-ios @mapbox/maps-android

@chloekraw
Copy link
Contributor

also cc @samanpwbb @tristen @mapbox/map-design-team

@samanpwbb
Copy link
Contributor

A or B both sound ok but if we can get away with A, definitely prefer that for the reasons you mentioned above. I believe that, with #8052 (comment), many use cases for styleimagemissing will be able to be handled at design time, so this callback will be less relied upon at runtime.

@asheemmamoowala
Copy link
Contributor

we should consider how to handle the interaction between an "image" operator and the styleimagemissing callback for adding missing images on-demand (#7987, #7587).

I believe that, with #8052 (comment), many use cases for styleimagemissing will be able to be handled at design time, so this callback will be less relied upon at runtime.

To add to what @samanpwbb said, I think the expression-based fallback should be treated separately from the "styleimagemissing" callback to a certain degree. That is to say that - an image is missing and emits the event if it is the final image resolved from an expression and is still not found (not if it resolves to null)

Applications that rely on styleimagemissing will likely have crafted their style to provide special image ids, and will have code to resolve those ids. In this case it is not much harder to resolve the id to a fallback image, and use updateImage when the required image is loaded.

Always fire the callback with the "image" operator if it evaluates to null, and accept any potential performance tradeoff for features in which we don't think we need the callback to fire

As far as option A is concerned - always firing the event can be misleading. We have documented that calling addImage in the event handler will use the newly provided image. Will this still be possible with the image operator, when used within a collate expression? Will it be possible in native to provide the same guaranteed behavior?

cc @ansis @alexshalamov @pozdnyakov

@chloekraw
Copy link
Contributor

chloekraw commented Aug 6, 2019

I think the expression-based fallback should be treated separately from the "styleimagemissing" callback to a certain degree. That is to say that - an image is missing and emits the event if it is the final image resolved from an expression and is still not found (not if it resolves to null)

@asheemmamoowala, am I interpreting your comment correctly: you're saying you prefer a separate option

  • d) Never fire the styleimagemissing callback with the image operator

because icon-image can still fire the styleimagemissing callback if the proposal in #8052 (comment) has a fallback image that's missing, e.g.,

"icon-image": ["coalesce",
    ["image", ["concat", ["get", "shield"], "-", ["get", "reflen"], "-black"]],
    ["image", ["concat", ["get", "generic"], "-", ["get", "reflen"], "-black"]],
    "missing-shield"]

so we don't need to fire it with each image operator individually if the image is missing?

I have two responses to that:

The image operator itself transcends any one feature or property. In addition to being used for icon fallbacks, image could be used in icon-image to combine multiple images to form a composite icon (mapbox/mapbox-gl-native#14764) or used inside of a formatted expression in text-field to add images (e.g., emojis) in text labels. John also mentioned possible future expression forms that "combine, lighten, darken, recolor, or otherwise manipulate images."

It's safe to say that we will have new icon features for which firing the callback on image is desirable, and others in which it's less needed. So I think never firing is too restrictive. For composited images and images in text labels, there are many use cases for missing/on-demand images.

Moreover, I thought of a use case for using styleimagemissing with image in icon fallbacks: branded POI icons. Imagine you want to use a branded icon for a POI if you have a partnership with the company, and fallback to the maki icon otherwise:

"icon-image": ["coalesce",
    ["image", ["concat", "branded-icon-", ["get", "name"]]],
    ["image", ["get", "maki"]],
    "generic-icon"]

You'd want to be able to provide the branded icon to the map on-demand so that you don't need to change your map/sprite implementation at all as your partnerships change. Further, it would be impractical to add all branded icons on the spritesheet, as you may eventually run up against our sprite limit.

As far as option A is concerned - always firing the event can be misleading. We have documented that calling addImage in the event handler will use the newly provided image. Will this still be possible with the image operator, when used within a collate expression? Will it be possible in native to provide the same guaranteed behavior?

Great questions, we should investigate the technical feasibility of these designs on JS and Native. The intention is that the developer should be able to provide a missing image via addImage so that coalesce evaluates to true at that argument if a missing image is added. I think we should fire styleimagemissing after image evaluates to the image name and it's not found on the spritesheet, so image doesn't evaluate to null if a missing image is added in the callback and coalesce can evaluate to true.

@chloekraw
Copy link
Contributor

an image is missing and emits the event if it is the final image resolved from an expression and is still not found (not if it resolves to null)

ah, earlier tonight I realized it doesn't make sense to fire styleimagemissing if image evaluates to null; we should fire it when image resolves to the name of a missing image, and I edited my comments to reflect that. Now that I re-read this, I realize that's also the point you made here so I misinterpreted this sentence / which expression you were referring to with "an expression."

Still not sure whether you're overall for firing the styleimagemissing with image; however, I think my points stand and the example I provided wouldn't be solved by adding a fallback synchronously in the listener.

@chloekraw
Copy link
Contributor

Originally I wanted to keep this ticket open to house discussion about an image operator separate from one of the downstream features it enables, “icon fallbacks” (#8052), but with the direction that thread is going, I think it’s becoming increasingly untenable to split the conversation and we should close this ticket in favor of consolidating all discussion on #8052.

@ryanhamley
Copy link
Contributor

This operator was implemented in #8684

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏
Projects
None yet
Development

No branches or pull requests

9 participants