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

Default icon-image value if not present in a sprite #8052

Closed
11sma opened this issue Mar 18, 2019 · 25 comments
Closed

Default icon-image value if not present in a sprite #8052

11sma opened this issue Mar 18, 2019 · 25 comments

Comments

@11sma
Copy link

11sma commented Mar 18, 2019

Motivation

I've searched in docs and in issues (quick search) and in google but I didn't find a possible solution. I think that simply didn't exists (sorry if yes...). so...
I'm trying to put in a map a serie of icon-image depending a property icon on tiles (or geojson). These icons are in sprite. I'm using:

"icon-image": "{icon}"

It works fine. The problem (and the motivation) is if "icon" in properties take a value that is not in the sprite, obviously, shows nothing. Is there some way to put a default icon if mapbox cannot use the icon of sprite because it doen't exists?

Design Alternatives

Nowadays I can create a new set of sprites faster I can to don't have this missmatch. But it's not automatic, nor always can do it so faster.
Another measure is to use match expression, to create some similar to default, but the inconvenient is that I need to specify to the style all the possible values of icon and, in fact, apply the changes faster as the option above.

I think this is not the correct way...

Mock-Up

I think it should be similar than match expression existent.

'icon-image': ['{icon}', {'default': 'default-icon-image'} ]
(sorry if the acuracy of code doesn't match)

Implementation

simply adding the "default" value if the value of a property is an array

@peterqliu
Copy link
Contributor

peterqliu commented Mar 18, 2019

🤔 this would be useful, but trying to think of how we can fit this in our existent types without a breaking change. So far, we don't have any properties that could take multiple types ("string or array"). Expressions and categorical DDS do support defaults, but as you said, users must list out the whole sprite list.

other ideas:

  • could add a default-icon-image prop
  • at the stylesheet's root level, could add a prop specifying one global fallback sprite for all layers using any sprite property (*-pattern and icon-image)
  • could expose the list of sprites as some variable name, for users to match against in an expression

@pathmapper
Copy link
Contributor

If I understood it right, another option is suggested in #5261 (Add expression operator to determine style image availability).

@ryanhamley ryanhamley self-assigned this Jun 28, 2019
@ryanhamley
Copy link
Contributor

ryanhamley commented Jul 11, 2019

Default icon-image options

A) icon-default symbol style spec property

    "layout": {
      "icon-default": "myDefault",
      "icon-image": "cat",
      "icon-size": 0.25
    }

Pros:

  • easy for users to understand
  • flexible enough to allow for different default icons depending on the layer

Cons:

  • requires a new style spec property
  • does not allow for data-driven or more complex, branching fallback checks

B) root-level default-sprite or default-icon style spec property

Pros:

  • users can quickly apply a single default icon to the entire map

Cons:

  • requires a new style spec property
  • could create confusion with the existing root-level sprite property
  • less flexible than a layout-level or expression option; prevents easily using different defaults for different types of icons

C) add has-image or image-exists expression operator

    ["case", ["has-image", "cat"], "cat", "dog"]

This is proposed in #5261

Pros:

  • the concept of “is my image in the style” is fairly easy to understand
  • makes use cases such as case expressions much simpler by avoiding the need to list every single icon in a sprite
  • hasImage is already an exposed method which could potentially be utilized

Cons:

  • requires a new expression operator
  • this approach would likely short circuit onstyleimagemissing because the missing image would be replaced by a fallback. it’s not clear how/when/if onstyleimagemissing should still be emitted in case of finding a fallback.

D) add new Image type and operator

This idea was proposed in #5261 (comment). The type would resolve to either an Image or Null where Image is a new type.

    ["match", ["image", id]...]

Pros:

  • Prepares the way for additional image related expressions that can manipulate images such as combine, recolor, lighten and darken

Cons:

  • Requires a new expression operator and a new type
  • existing image properties such as icon-image may need to move to the Image type which would be a breaking change
  • Significantly more work than the other options

Conclusions

B seems like it would not be flexible enough to meet the expectations of users. D is likely overkill and the other options wouldn’t preclude implementing something like D in the future.

I believe A and C are likely the simplest, most straightforward ways to achieve this. C is probably the most flexible option as it allows for data-driven fallbacks and complex expressions rather than a single fallback option.

The most significant consideration I am aware of with C is how to handle onstyleimagemissing. With the has-image operator, technically the image wouldn’t be missing since we’d just use the fallback. It’s easy to envision use cases where you’d want to dynamically load/generate an image in an onstyleimagemissing callback (e.g. a social media site that shows a generic person icon until it loads a user’s profile picture) but it’s also easy to think of cases where the fallback doesn’t represent some “missing” image (e.g. light vs dark themes). You can also consider the case in which you could build a complex expression from multiple case/has-image expressions; should each failed sub-expression emit onstyleimagemissing? Just the last failed sub-expression? None of them? We’ll have to carefully consider this dynamic when implementing a solution. @ansis do you have thoughts on this?

All things considered, option C, the has-image operator, seems like the best path to implementing default/fallback icons.

cc @mapbox/gl-js @mapbox/map-design-team @chloekraw

@asheemmamoowala
Copy link
Contributor

C is probably the most flexible option as it allows for data-driven fallbacks and complex expressions rather than a single fallback option.

Will has-image support data-driven styling? If so - it has the benefit of being able to use icon-image with a DDS expression and tailoring fallbacks differently for different cases.

@chloekraw
Copy link
Contributor

chloekraw commented Jul 11, 2019

@ryanhamley, thanks for this detailed write-up! I agree with you that C is the best option. You bring up a great point about the interaction with the styleimagemissing callback, but I think the limitation of the current behavior (not being able to use both features) is fine. That is, if you choose to use this feature to fallback to a default image, then you can’t also use the styleimagemissing callback, because your decision to use a default image means you don’t want your implementation to have any missing images. This seems to check out logically and intuitively to me. Is there a use case for using both features in the same layer that I’m overlooking?

EDIT: Changed my mind; I think there are valid use cases for using missing images with icon fallbacks: #5261 (comment)

@ryanhamley
Copy link
Contributor

ryanhamley commented Jul 11, 2019

@chloekraw I think so. I'm imagining something like a social media profile card. If a user's profile picture isn't available, then you could fallback to a generic icon but you'd still want to fetch the user's profile picture and replace the generic placeholder with it when it's available.

@asheemmamoowala data-driven styling seems appropriate so that users could implement separate fallback icons for different sources and layer types

@chloekraw
Copy link
Contributor

If a user's profile picture isn't available, then you could fallback to a generic icon but you'd still want to fetch the user's profile picture and replace the generic placeholder with it when it's available.

I think this is solved in the listener. Use map.addImage to add the generic placeholder in styleimagemissing; then, call updateImage to replace it with the profile photo when it's loaded. In fact, you have to do this if your profile photos are not already loaded at the time of the callback and need to be fetched, because styleimagemissing doesn't currently support adding images asynchronously.

@ryanhamley
Copy link
Contributor

ryanhamley commented Jul 12, 2019

After talking things over with @asheemmamoowala we've come up with a new option which is a variation on Option D and we think may be a better way forward than Option C.

E) introduce coercive imageref type along with image-list operator

The idea is that icon-image would take a new type imageref which would be coercive. icon-image: 'foo' would still be valid because a string would be coerced to the imageref type so this would not be a breaking change. a new operator image-list (or something similarly named) would take a list of imagerefs (basically an array of strings) and check each imageref in turn, returning the first imageref which resolves to a string that matches an available image in the style. this approach is similar to the formatted type that is used for text-font.

"icon-image": ["image-list", "foo", "bar", "baz", "bat"]

In the above example, if "foo" and "bar" don't exist, but "baz" does, then "baz" would be used.

Pros:

  • avoids a breaking change by coercing strings to the new imageref type
  • allows for easily defining multiple fallbacks
  • avoids needing to maintain a list of available images in the expression context's globalProperties
  • can be scoped to only properties that require images (*-pattern and icon-image) which cuts down on maintenance and implementation costs
  • each element in the image-list can also be an expression, as long as it resolves to a string, which allows for complex, data-driven fallbacks
  • may actually be a bit less work than the has-image operator

Cons:

  • adds a new operator and a new type

@chloekraw @mapbox/map-design-team @mapbox/gl-js

@tristen
Copy link
Member

tristen commented Jul 13, 2019

@ryanhamley 👍 Option D. Curious why a new operator is necessary. Couldn't this just be a new type and behave the same way as text-font?

@asheemmamoowala
Copy link
Contributor

Curious why a new operator is necessary. Couldn't this just be a new type and behave the same way as text-font?

@tristen the operator is needed to coerce the string id of an image into the corresponding image object. The idea in option D, is to allow operators and expressions to act on the underlying image data, but use string ids to fetch them from the spritesheet.

@tristen
Copy link
Member

tristen commented Jul 13, 2019

@asheemmamoowala I'm still not following why that can't be done by detecting type as array. Ergonomically, this would be much easier to remember/write and the value pattern mirrors the syntax of how text-font is written.

@ryanhamley
Copy link
Contributor

ryanhamley commented Jul 16, 2019

@tristen my understanding is that an operator will allow us to make this data-driven. if we made the type of icon-image an array, you couldn't really do something like ['foo', ['get', 'bar'], 'baz'] for example because GL JS would try to find an operator called foo. The operator allows for significantly more complex expressions.

@ryanhamley
Copy link
Contributor

Further exploration of Option D: ['image', id]

@asheemmamoowala and I revisited this option today and it seems like a more attractive option than we first recognized. to recap:

We would implement a new image operator and Image type. The image operator will resolve to either Image or Null and any style spec property that expects type Image would coerce plain strings to the Image type. This would allow us to avoid breaking changes since something like 'icon-image': 'cat' would still work. The Image type would be represented by a string.

Pros:

  • as mentioned previously, this would prepare the way for image manipulation expressions such as darken or rotate
  • this approach could be used anywhere, not just in icon-image or *-pattern allowing for flexible, complex use cases
  • avoids a breaking change
  • resolves the need for long image stacks as shown in “Image stacks” for image properties #4149 (comment)
  • can be data-driven and use sub-expressions

Cons:

  • needs a new operator and type
  • requires us to provide the list of available style images almost everywhere that expressions are evaluated since this operator could be used anywhere

Some possible uses of this operator:

Coalesce to find an available image with a default if no other image is available

"icon-image": ["coalesce",
    ["image", "{shield}-{reflen}-black"],
    ["image", "{generic}-{reflen}-black"],
    "generic-shield"]

Resolvable icon name

"icon-image": "{shield}-{reflen}-black"

Basic string name

"icon-image": "shield-black"

match expression

"icon-image": ["match",
    ["get", "name"],
    "poodle", ["image", "dog"],
    "tabby", ["image", "cat"]]

complex uses outside of icon-image or *-pattern

"text-field": [
    ["let", "icon-exists", ["image", ["get", "name"]]],
    ["case", [["var", "icon-exists"], "dog"], ["concat", ["get", "name"], "(image not found)"]]

Because of its flexibility and the fact that it lays some ground work for potential future expression work, this option is looking more and more like the correct decision. The major stumbling block I see is the need to provide the list of available images all over the code base. It's a significant amount of work. I'm also not certain that the list of images is always available when any arbitrary expression is evaluated. However, I'm leaning more towards this option after talking it through today.

@asheemmamoowala asheemmamoowala added this to the release-queso milestone Jul 19, 2019
@chloekraw chloekraw removed this from the release-queso milestone Aug 2, 2019
@chloekraw
Copy link
Contributor

The current proposal is great. I realized that with this proposal, we need to revisit the conversation about the interaction with the styleimagemissing callback. While I still believe that it mostly doesn't make sense to fire the callback with the "icon fallbacks" feature specifically, the decision we actually have to make is whether to fire the callback with the "image" operator, and this operator (#5261) could be used for many features, some of which may benefit from firing styleimagemissing.

@samanpwbb
Copy link
Contributor

@ryanhamley I really love this proposal I do have one question though. Your examples use tokens:

"icon-image": ["coalesce",
    ["image", "{shield}-{reflen}-black"],
    ["image", "{generic}-{reflen}-black"],
    "generic-shield"]

From Studio's POV, we'd probably prefer if we could deprecate the old token syntax, so instead this expression would be written like:

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

This would be more in line with how other expressions represent data values inside of strings.

@ryanhamley
Copy link
Contributor

ryanhamley commented Aug 2, 2019

Sorry, my example might have been inaccurate. This proposal isn't introducing token syntax into expressions. Your example would work as expected with the image operator @samanpwbb

Thinking about #5261 (comment) option A would work fine for most uses I think, but I can imagine a use case where you may want to do something in the onstyleimagemissing event listener callback in one case, but not in another. For example, if you request an image specifically with something like 'icon-image': 'foo', you might want to add it dynamically if it's not available, but in a coalesce operation, you may prefer to use the fallback image. It would probably be hard to distinguish the two uses in your event listener. This would be an argument for something like Option B that makes sending the event conditional. I'm not sure if that argument overcomes the downside of the extra complexity or not.

@ryanhamley
Copy link
Contributor

there's been a proposal from @chloekraw and @asheemmamoowala that we should disambiguate the type and operator rather than have both be named Image. @chloekraw said

it seems like it would be doubly confusing to use the same name for the type and expression when the image operator doesn't assert that the input value is of the image type, like array number object etc. do

and suggested keeping the type name image and making the operator get-image.

There is the precedent of the collator expression which returns the collator type so it could go either way based on existing expressions. I'm ok with naming the expression something like get-image. Does anyone have strong opinions on whether the operator should be renamed and what it should be?

@mapbox/gl-native @mapbox/studio @mapbox/map-design-team

@alexshalamov
Copy link
Contributor

alexshalamov commented Aug 28, 2019

@ryanhamley I like ["image", ... ] more. If new operator represents actual type, then 'get' prefix is superfluous. If Image type is retrieved from somewhere at evaluation time, like ["get", ... ] expression, then get prefix is a valid option.

"icon-image": ["image", ["get", "image_name"]] 👍
"icon-image": ["get-image", ["get", "image_name"]]

@chloekraw
Copy link
Contributor

tl;dr

I believe we should split our current conception of a single image operator into two:

  • a lookup operator has-image: true|false that would be used for this feature, icon fallbacks
  • a manipulation operator *-image that would be used in future features such as [RFC] New feature: Images in labels #8604

I think this approach would help resolve our questions around naming and handling styleimagemissing.

1. Naming

@ryanhamley and I voiced last week and we realized we had different conceptions of the expression operator that we are currently designing. For manipulating images, I thought the syntax would be:

"icon-image": ["format-image",
          ["concat", ["get-image", "image-name-1"], ["get-image", "image-name-2"]], { "scale"^: number, "blur": number }
          ]: image

where get-image is what we're building now and format-image is an operator we'd design in the future. Whereas Ryan was imagining a single operator that would perform both functions:

"icon-image": ["image",
        ["concat", ["image", "image-name-1", { "scale"^: number, "color": color } ],
                   ["image", "image-name-2", { "height": number, "width": number } ]
        ], { "blur": number }
        ]: image

The main reason I was under that impression is because I think to a user, "lookup" and "manipulate" sound like two distinct operations that would intuitively require different operators. If we're building the equivalent of format for images now, then I agree it wouldn't make much sense to name it get-image.

In general, I believe we've been struggling to name this operator because it's hard to name a single operator in a way that conveys it does both lookups and manipulations without being ambiguous.

I'm ok with an ambiguous name (e.g. image) if we believe a single operator exposed to users is best from an API design perspective.

2. Missing images

Splitting has-image off from *-image provides an opportunity for greater clarity around how to best handle missing images. A proposal:

  • has-image: return false if image is missing, do not fire the event
  • *-image: fire event if image is missing
    • all subsequent behavior to remain consistent with what we do for icons today

I haven't thought this through enough to have an opinion on whether this is the best approach, and I did point out that there could be a use case for using missing images with icon fallbacks.

In general, I think it's a benefit to have the freedom to independently evaluate the need for styleimagemissing with an image lookup operation vs. an image manipulation operation and the separate downstream features enabled by each.

Should we name one of these operators image?

I really like this idea when I'm just evaluating the operator itself.

When I step back and think more broadly, in my view, the strongest argument for avoiding image as a name for this proposed operator is that we're closing off our ability to create a future image operator that asserts the input value is an image type.

Per our docs, the array, boolean, number, object, string operators all perform this function when they return an eponymous type.

I'm not sure how to weigh the value of keeping this option open to us. On the one hand, there's a relatively high likelihood of convergence between images and text in Mapbox:

  • Images and text already share a layer type
  • We're thinking about shaders that can render icons and text
  • We're thinking about treating images as text

I can envision a future where we might want an operator that asserts a string of text are an image that work with icon-* properties.

On the other hand, is there really a use case for an operator like this that we couldn't solve another way? I'm not sure.

cc @kkaefer

@ryanhamley
Copy link
Contributor

ryanhamley commented Sep 5, 2019

TL;DR

It's difficult to enable both coalesce expressions and styleimagemissing events with the image operator and we need to come up with a solution/decision.

Problem

@asheemmamoowala and I talked for awhile yesterday about this operator and realized that there's a fundamental tradeoff at the heart of implementing this that we need to make a decision on before we can proceed.

The way that I have been designing the operator up to this point was that it would return either an Image (a new type which resolves to a string and which strings are automatically coerced to) or null. This is necessary to allow using the operator inside a coalesce expression since coalesce expects either a "truthy" value or null in order to make its decision about what to return. So basically, we've been working towards the ability to do something like "icon-image": ["coalesce", ["image", "foo"], ["image, "bar"], "fallback-image"].

The tradeoff is that this approach will disable the styleimagemissing event for anything using the image operator. We can see that by looking at a couple snippets of code. In

if (hasIcon) {
icon = layer.getValueAndResolveTokens('icon-image', feature);
}

if a feature has an icon, we resolve any tokens or expressions to get back the resolved icon id, which is set to the variable icon. Then in

if (icon) {
icons[icon] = true;
}

we check to see if icon exists and place it into the icons object, which represents the list of icons we need for a given tile. Eventually, this list of icons makes its way to the ImageManager and in

for (const id of ids) {
if (!this.images[id]) {
this.fire(new Event('styleimagemissing', {id}));
}

we check each icon against the list of available icons in the ImageManager and fire styleimagemissing for any icons not in the manager. However, if the image operator were to return null so that icon = null in the first step, then if (icon) will evaluate to false in the second step, the id won't be added to icons and the ImageManager will never know to fire styleimagemissing for the requested image.

It's possible to work around this by not returning null and instead always returning something (what that should be is TBD). However, that then breaks our ability to use coalesce statements with this operator.

Possible Solutions

  1. image could take an optional third argument called skipValidation or something similar

    • skipValidation: true would return the evaluated input argument, e.g. ["image", ["concat", "highway-shield-", ["get", "shield-type"]], {"skipValidation": true}] would always return something like highway-shield-california. We wouldn't check to see if it's available and would just evaluate the expression and return the output.
    • This would allow us to conditionally use both coalesce and styleimagemissing.
    • This could be very confusing to users because the output might not always be obvious since the same operator could be used in two different ways with different return types.
    • This could also be made private and not exposed to users so we can control when null is returned or not based on some heuristic about what behavior makes sense.
  2. Remove the image type so that icon-image and *-pattern properties still take a string. String inputs wouldn't be coerced to a new type.

    • image would only be used for "has image" functionality and fallback behavior
    • this doesn't lay the groundwork for more advanced image operations in the future but it is simpler
    • may obviate the need for image to be held back as a potential assertion operator since there wouldn't be an Image type to assert for (though we may still wish to add this type in the future if we move towards a separate format-image operator or something similar)
  3. If image returns null, we could add an additional evaluation step where we walk the expression tree and find the "top priority" missing image (presumably the first image requested in an expression) and use that id to trigger styleimagemissing

    • could allow us to use both coalesce and styleimagemissing without different behaviors based on input
    • it seems like a simple enough heuristic that in an expression is written like `["coalesce", ["image", "foo"], ["image", "bar"], ["image", "baz"], "generic-fallback"] that the first image in the list is the "desired" or "top priority". This is analogous to a font-stack where we proceed from left to right to attempt to render a font.

Some additional thoughts can be found in #5261 (comment)

Thoughts on #8052 (comment)

In general, I'm starting to really appreciate the simplicity and unambiguousness of the has-image/format-image split. I agree with the argument that lookup and modification are two different operations that make sense to split into different expressions.

That said, a has-image or get-image operator would still have all the problems outlined above. If it returned true/false, we would not be able to use the coalesce operator with it. You could probably work around this limitation with other operators such as case though. If we allow it to return an Image or null then we still need to implement a way to fire styleimagemissing.

After meeting again with @asheemmamoowala and talking this over, I think the conclusion I've come to is this:

Conclusion

We implement this operator as {has/get}-image which takes a single parameter (which could be an expression) and returns either an Image type or null. The Image type is a little ill-defined at the moment; it's not strictly necessary to implement a lookup operator, but since we'll need the type in the future for manipulation operators, it makes sense to just implement it now. If the operator returns null, then an additional evaluation step will occur which parses the expression tree and finds the "top priority" image as mentioned above and uses that image id to trigger styleimagemissing.

This achieves a few of our key objectives:

  • it splits the lookup and manipulation operations into separate expressions (the manipulation expression(s) will be left for a future PR)
  • it allows for fallback and default image behavior
  • it allows for use of coalesce operators
  • it allows for use of styleimagemissing to add an image if none exists
  • it lays some of the groundwork for more advanced expressions in the future
  • coercion to the Image type prevents a breaking change
  • makes the outcome of using this operator predictable: you will always get an Image or null and if null, styleimagemissing fires

@chloekraw

@alexshalamov
Copy link
Contributor

@ryanhamley Great summary! One question:

coercion to the Image type prevents a breaking change

This only provides forward compatibility, for older versions of sdk, this is a breaking change, right?

@ryanhamley
Copy link
Contributor

This only provides forward compatibility, for older versions of sdk, this is a breaking change, right?

It shouldn't be. Something like icon-image: "monument-15" will still work just fine. The implicit coercion is also how text-field works; it technically takes a Formatted type but strings work fine because they're coerced behind the scenes to Formatted.

@ryanhamley
Copy link
Contributor

Closed by #8684

@chloekraw
Copy link
Contributor

@ryanhamley thanks for the great summary in #8052 (comment) and all the hard work! Could you confirm which of the "possible solutions" you chose to implement in #8684 and #8793, and whether there are any other design or implementation decisions you made along the way that might impact how this is used?

If we don't have one yet, I think this is worth adding a GL-JS example (or some other tutorial/guide/etc) to demonstrate how to use it for default images.

@ryanhamley
Copy link
Contributor

@ryanhamley thanks for the great summary in #8052 (comment) and all the hard work! Could you confirm which of the "possible solutions" you chose to implement in #8684 and #8793

@chloekraw I ended up going with option 3. When a coalesce statement of images returns null, I "add[ed] an additional evaluation step where we walk the expression tree and find the "top priority" missing image and use that id to trigger styleimagemissing". We assume that the first image requested (reading from left to right) in the coalesce statement is the "top priority".

and whether there are any other design or implementation decisions you made along the way that might impact how this is used?

Ultimately, I decided to stick with calling the operator image because it does act in some ways like an assertion. For example, ["coalesce", ["image", "foo"], "bar", ["image", "baz"]] won't work because "bar" is not of the correct type. You have to assert that the string is an image. That's why I ultimately didn't go with has-image for this operator.

We also ended up renaming the type of the return value from Image to ResolvedImage because Image was accidentally shadowing the built-in Flow type for the Javascript Image class, creating some extremely confusing Flow errors.

If we don't have one yet, I think this is worth adding a GL-JS example (or some other tutorial/guide/etc) to demonstrate how to use it for default images.

Agreed! I added this to my to-do list

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

No branches or pull requests

10 participants