-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Comments
🤔 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:
|
If I understood it right, another option is suggested in #5261 (Add expression operator to determine style image availability). |
Default icon-image optionsA)
|
Will |
@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 EDIT: Changed my mind; I think there are valid use cases for using missing images with icon fallbacks: #5261 (comment) |
@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 |
I think this is solved in the listener. Use |
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
|
@ryanhamley 👍 Option D. Curious why a new operator is necessary. Couldn't this just be a new type and behave the same way as |
@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. |
@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 |
@tristen my understanding is that an operator will allow us to make this data-driven. if we made the type of |
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 Pros:
Cons:
Some possible uses of this operator: Coalesce to find an available image with a default if no other image is available
Resolvable icon name
Basic string name
match expression
complex uses outside of
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. |
The current proposal is great. I realized that with this proposal, we need to revisit the conversation about the interaction with the |
@ryanhamley I really love this proposal I do have one question though. Your examples use tokens:
From Studio's POV, we'd probably prefer if we could deprecate the old token syntax, so instead this expression would be written like:
This would be more in line with how other expressions represent data values inside of strings. |
Sorry, my example might have been inaccurate. This proposal isn't introducing token syntax into expressions. Your example would work as expected with the 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 |
there's been a proposal from @chloekraw and @asheemmamoowala that we should disambiguate the type and operator rather than have both be named
and suggested keeping the type name There is the precedent of the @mapbox/gl-native @mapbox/studio @mapbox/map-design-team |
@ryanhamley I like
|
tl;drI believe we should split our current conception of a single
I think this approach would help resolve our questions around naming and handling 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:
where
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 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. 2. Missing imagesSplitting
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 Should we name one of these operators
|
TL;DR It's difficult to enable both 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 The tradeoff is that this approach will disable the mapbox-gl-js/src/data/bucket/symbol_bucket.js Lines 411 to 413 in 87cac3b
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 mapbox-gl-js/src/data/bucket/symbol_bucket.js Lines 438 to 440 in 87cac3b
we check to see if mapbox-gl-js/src/render/image_manager.js Lines 141 to 144 in 87cac3b
we check each icon against the list of available icons in the It's possible to work around this by not returning Possible Solutions
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 That said, a 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 This achieves a few of our key objectives:
|
@ryanhamley Great summary! One question:
This only provides forward compatibility, for older versions of sdk, this is a breaking change, right? |
It shouldn't be. Something like |
Closed by #8684 |
@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. |
@chloekraw I ended up going with option 3. When a
Ultimately, I decided to stick with calling the operator We also ended up renaming the type of the return value from
Agreed! I added this to my to-do list |
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
The text was updated successfully, but these errors were encountered: