Fix rendering for icon-text-fit + data-driven text-size #5632
Conversation
| }, | ||
| "icon-image": "label", | ||
| "icon-text-fit": "both", | ||
| "icon-ignore-placement": true, |
There was a problem hiding this comment.
I'm not sure why icon-ignore-placement and icon-allow-overlap were needed here, because the two symbols don't appear to collide. But without them, only "A" shows up. Might be another bug lurking here.
There was a problem hiding this comment.
Yeah, it looks like there is a bug -- the icons have collision boxes that are much bigger than the text:
I'll look into that tomorrow.
There was a problem hiding this comment.
Ticketed that issue as #5641.
There was a problem hiding this comment.
This looks good to me, but it might be a good idea to get @anandthakker to glance at it too?
| @@ -71,7 +71,7 @@ function getIconQuads(anchor: Anchor, | |||
| if (layout['icon-text-fit'] !== 'none' && shapedText) { | |||
| const iconWidth = (right - left), | |||
| iconHeight = (bottom - top), | |||
| size = layout['text-size'] / 24, | |||
| size = layer.getLayoutValue('text-size', globalProperties, feature) / 24, | |||
There was a problem hiding this comment.
Why isn't this using {zoom: bucket.zoom + 1} like we do in symbol_layout#addFeature? That seems like the best match. @anandthakker understands that logic better than I do, though, I think.
There was a problem hiding this comment.
Woah, good point @ChrisLoer. This raises an interesting issue. It's not just that we need to evaluate text-size with {zoom: bucket.zoom + 1} during layout (though I think you're right about that) -- if text-size is zoom depentent, we also need to scale the icon at render time according to the current text-size value. And if text-size is a composite function, that's going to be problematic -- we could try to use the text-size attributes for the icon rendering pass in that case, but that would only help for icon-text-size: both.
In any case, I think this goes well beyond the scope of this PR. I'll ticket it separately, 👍 for now!
| "text-field": { | ||
| "type": "identity", | ||
| "property": "name" | ||
| }, |
| "text-size": { | ||
| "type": "identity", | ||
| "property": "size" | ||
| }, |
Fixes #5631.