Enable property functions for {text,icon}-{color,halo-color} #4186

anandthakker · 2017-02-02T15:09:07Z

Adds property function support for:

{text,icon}-color
{text,icon}-halo-color

Checklist

briefly describe the changes in this PR
write tests for all new functionality
post benchmark scores
manually test the debug page

Scarysize · 2017-02-02T15:45:24Z

Nice! We implemented this earlier this week in our fork (for sdf icons).

Just a thought going through the Symbol Bucket code: Would it make sense to split up text (glyph) and icon rendering completely? The bucket code contains a lot of if(isText/isIcon) statements, bucket methods receive a gazillion parameters because they handle both types and even draw_symbols needs to differentiate between the two types.

anandthakker · 2017-02-02T16:19:49Z

This is working for the simple text-color case, but not yet working for icon-color for some reason (in the added render test, both icons are coming up red) -- troubleshooting now.

Also, the existing approach to halo rendering causes a complication here. We currently re-use the same program to render the text/icon halo, and then the text/icon itself. Introducing property functions for paint props complicates this, because now we need a way to switch which property--e.g. text-color or text-halo-color--is being used for color. I'm really not sure where/how this ought to happen. @jfirebaugh @lucaswoj thoughts?

anandthakker · 2017-02-02T16:28:42Z

@Scarysize good question! I'm 💯 with you on those crazy long parameter lists. I definitely think this part of the codebase could use a solid refactor, and splitting up text/icon might be a good route to take.... I'm personally not very familiar w/ the history of the symbol stuff, though, so there might be lurking subtleties that I'm not aware of.

lucaswoj · 2017-02-02T18:51:04Z

I'm really not sure where/how this ought to happen.

The simplest design I can think of is adding a new parameter to VertexArrayObject#bind and Buffer#setVertexAttribPointers.

This method is responsible for setting up the connections between shader attributes and struct array members.

By default there is a 1:1 mapping of shader attributes to struct array members. We could solve this problem by making that relationship a little looser, allowing callers of Buffer#setVertexAttribPointers to define some kind of aliasing.

segment.vaos[layer.id].bind(gl, program, buffers.layoutVertexBuffer, buffers.elementBuffer, null, segment.vertexOffset, {
    attributeAliases: {
        'color': 'halo-color'
    }
});

Happy to voice on this if you have any follow-up questions. 😄

jfirebaugh · 2017-02-02T19:08:50Z

Other options:

Bind both colors, add a uniform boolean that selects between them and an if statement in the shader.
Create separate shaders for halo and fill. Factor the common parts into reusable GLSL functions.
Figure out how to draw halo and fill in a single draw call.

jfirebaugh · 2017-02-02T19:26:42Z

Figure out how to draw halo and fill in a single draw call.

Previously: #596. @lbud, how conclusive was the outcome there? Is there no way to overcome the overlap issue?

anandthakker · 2017-02-02T22:19:30Z

Update: ruled out consolidating the halo and fill in a single call (see #596 ). Because @jfirebaugh indicated in chat that the attributeAliases approach would likely be difficult to carry over to -native, going to try binding both sets of attributes, along with a uniform switch so the shader can decide.

anandthakker · 2017-02-02T23:04:31Z

benchmark	master `fac6358`	dds-symbol-paint `7872482`
map-load	215 ms	135 ms
style-load	115 ms	109 ms
buffer	1,163 ms	1,187 ms
fps	60 fps	60 fps
frame-duration	5.1 ms, 0% > 16ms	5.3 ms, 0% > 16ms
query-point	0.97 ms	1.05 ms
query-box	70.54 ms	64.17 ms
geojson-setdata-small	6 ms	5 ms
geojson-setdata-large	145 ms	174 ms

anandthakker · 2017-02-02T23:12:30Z

js/data/bucket/symbol_bucket.js

@@ -358,7 +366,7 @@ class SymbolBucket {
            // the buffers for both tiles and clipped to tile boundaries at draw time.
            const addToBuffers = inside || mayOverlap;
            this.addSymbolInstance(anchor, line, shapedTextOrientations, shapedIcon, this.layers[0],
-                addToBuffers, this.symbolInstancesArray.length, this.collisionBoxArray, feature.index, feature.sourceLayerIndex, this.index,


Per conversation w @mollymerp, I replaced the (unused) index field with a featureIndex, which provides a mapping back to from the symbol instance to the feature it came from. This allows us to retrieve the feature's properties down in place().

This will likely need a bit of reworking when we return to the cross-source symbol placement quest, but since we don't know exactly what that'll look like, I think we should cross that bridge when we come to it.

Any context on how the index field became unused?

@lucaswoj I believe it never was used, but rather was there on the hypothesis that it might be needed during cross-source placement. @mollymerp does that sound right to you?

anandthakker · 2017-02-02T23:25:30Z

@jfirebaugh @lucaswoj k -- this is still a subset of the symbol paint properties (list is here #2730 ), but it'd be good to go ahead and get some 👀 on it, esp. since this is my first foray into this area of the codebase.

lucaswoj · 2017-02-03T00:18:09Z

shaders/symbol_sdf.fragment.glsl

    lowp float dist = texture2D(u_texture, v_tex).a;
    lowp float fade_alpha = texture2D(u_fadetexture, v_fade_tex).a;
    lowp float gamma = u_gamma * v_gamma_scale;
    lowp float alpha = smoothstep(u_buffer - gamma, u_buffer + gamma, dist) * fade_alpha;

-    gl_FragColor = u_color * (alpha * u_opacity);
+    gl_FragColor = color_to_use * (alpha * u_opacity);


Instead of color, halo_color, and color_to_use how about fill_color, halo_color, and color?

Yeah. I'd foolishly assumed I needed to stick with the attribute-name <=> paint-property-name-sans-layer-type-prefix mapping, but doing #4186 (review) will free us up to improve the naming here.

lucaswoj · 2017-02-03T00:20:39Z

js/data/program_configuration.js

@@ -210,7 +210,8 @@ function getPaintAttributeValue(attribute, layer, globalProperties, featurePrope
 }

 function normalizePaintAttribute(attribute, layer) {
-    const name = attribute.property.replace(`${layer.type}-`, '').replace(/-/g, '_');
+    const layerTypePrefix = layer.type === 'symbol' ? /text-|icon-/ : `${layer.type}-`;
+    const name = attribute.property.replace(layerTypePrefix, '').replace(/-/g, '_');


Can you think of any cases where this magic behaviour might bite us later? I'd be interested to see if there was a way to specify this behaviour explicitly within SymbolBucket instead.

Can you think of any cases where this magic behaviour might bite us later?

@lucaswoj not offhand, but I'm 💯 with you in being uneasy / less than satisfied with this magic.

I'd be interested to see if there was a way to specify this behaviour explicitly within SymbolBucket instead.

👍 I'm thinking we can allow the bucket to explicitly define attribute.name, and then have normalizePaintAttribute just use that if it's there, instead of computing the attribute name from the paint property name.

anandthakker · 2017-02-03T14:00:27Z

@lucaswoj addressed your points re: bad magic and naming by explicitly setting the attribute names in paintAttributes.

When attribute.name isn't set explicitly, I'm personally okay with keeping the pre-existing, slightly-less-magical convenience where we construct attribute name based on the property name. It seems harmless, particularly since there's now a way to override it.

jfirebaugh · 2017-02-03T21:56:03Z

js/data/bucket/symbol_bucket.js

@@ -184,6 +192,12 @@ class SymbolBucket {
            // It's better to place labels on one long line than on many short segments.
            this.features = mergeLines(this.features);
        }
+
+        // set up this mapping to allow for constant-time lookups in place()
+        this.featuresByIndex = {};


Rather than create a separate index, can we attach a reference to the feature properties to the symbolInstance?

@jfirebaugh the symbolInstance is being stored (at populate() time) in a StructArray (this.symbolInstancesArray), so we can't attach a reference.

Granted, (a) we're not yet using symbolInstancesArray on a different thread, and (b) once we do (for cross-source placement), we'll have to find a different way to handle the paint attributes, since presumably whichever thread gets the array won't have the property data from the source.

Let's adjust/replace symbolInstancesArray based on the requirements here for data-driven properties. We can revisit its use in cross-source symbol placement when we're ready to work on that feature again.

Sure; if we're doing that I'd say we should go with 'replace' over 'adjust': adjusting it to these requirements would mean changing it from a StructArray, thus making it non-transferrable and no longer suitable for the cross-source placement project... if that's going to be the case, I don't see much reason to hold on to this state (symbolInstancesArray, symbolQuadsArray) at the WorkerTile level.

I'm good to go ahead with this, but I just want to flag that dropping symbolInstancesArray will make the changes in this PR a notch more invasive, removing some stuff that we may end up having to partially redo later when we tackle cross-source placement -- but it's probably true that even if we left it in place, making it work with data-driven styling would anyway have caused significant changes to what's there now.

The prep work for this is done in #4217. Tomorrow AM I'll rebase on that, which will allow us to just place a direct reference to featureProperties onto each symbolInstance as you suggested.

lucaswoj

I'm happy with this! Deferring to @jfirebaugh for the final 👍

anandthakker · 2017-02-07T15:21:12Z

@lucaswoj @jfirebaugh rebased onto the refactor in #4217, so that one should get merged first, but after that's in I think this is ready. Compare this PR's changes only at refactor-symbol-instances...dds-symbol-paint

* Enable property functions for {text,icon}-color * Update expected images for {icon,text}-color render tests * Add render tests for {icon,text}-halo-color property functions * Drop unneeded feature map

anandthakker added the under development label Feb 2, 2017

anandthakker mentioned this pull request Feb 2, 2017

Enable property functions for text-field, text-transform #4074

Merged

4 tasks

anandthakker mentioned this pull request Feb 2, 2017

draw text and halo in one call #596

Closed

anandthakker force-pushed the dds-symbol-paint branch from a8791b2 to 61d8a16 Compare February 2, 2017 22:37

anandthakker removed the under development label Feb 2, 2017

anandthakker commented Feb 2, 2017

View reviewed changes

lucaswoj reviewed Feb 3, 2017

View reviewed changes

anandthakker changed the title ~~Enable property functions for {text,icon}-color~~ Enable property functions for {text,icon}-{color,halo-color} Feb 3, 2017

jfirebaugh reviewed Feb 3, 2017

View reviewed changes

lucaswoj reviewed Feb 3, 2017

View reviewed changes

lucaswoj approved these changes Feb 6, 2017

View reviewed changes

anandthakker mentioned this pull request Feb 6, 2017

Refactor: remove SymbolInstancesArray, SymbolQuadsArray #4217

Merged

4 tasks

anandthakker force-pushed the dds-symbol-paint branch from 52653a5 to c7fef40 Compare February 7, 2017 15:15

anandthakker added 4 commits February 7, 2017 13:53

Enable property functions for {text,icon}-color

f865aea

Update expected images for {icon,text}-color render tests

3df3f7d

Add render tests for {icon,text}-halo-color property functions

d6d92b9

Drop unneeded feature map

b579604

anandthakker force-pushed the dds-symbol-paint branch from c20020e to b579604 Compare February 7, 2017 18:55

jfirebaugh approved these changes Feb 7, 2017

View reviewed changes

anandthakker merged commit 8f58879 into master Feb 7, 2017

anandthakker deleted the dds-symbol-paint branch February 7, 2017 20:21

anandthakker mentioned this pull request Feb 14, 2017

Enable property functions for symbol paint properties #2730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable property functions for {text,icon}-{color,halo-color} #4186

Enable property functions for {text,icon}-{color,halo-color} #4186

anandthakker commented Feb 2, 2017 •

edited

Loading

Scarysize commented Feb 2, 2017

anandthakker commented Feb 2, 2017

anandthakker commented Feb 2, 2017

lucaswoj commented Feb 2, 2017

jfirebaugh commented Feb 2, 2017

jfirebaugh commented Feb 2, 2017

anandthakker commented Feb 2, 2017

anandthakker commented Feb 2, 2017

anandthakker Feb 2, 2017

lucaswoj Feb 3, 2017

anandthakker Feb 3, 2017

anandthakker commented Feb 2, 2017

lucaswoj Feb 3, 2017

anandthakker Feb 3, 2017 •

edited

Loading

lucaswoj Feb 3, 2017

anandthakker Feb 3, 2017

anandthakker commented Feb 3, 2017

jfirebaugh Feb 3, 2017

anandthakker Feb 3, 2017

jfirebaugh Feb 6, 2017

anandthakker Feb 6, 2017

anandthakker Feb 7, 2017 •

edited

Loading

lucaswoj left a comment

anandthakker commented Feb 7, 2017

Enable property functions for {text,icon}-{color,halo-color} #4186

Enable property functions for {text,icon}-{color,halo-color} #4186

Conversation

anandthakker commented Feb 2, 2017 • edited Loading

Checklist

Scarysize commented Feb 2, 2017

anandthakker commented Feb 2, 2017

anandthakker commented Feb 2, 2017

lucaswoj commented Feb 2, 2017

jfirebaugh commented Feb 2, 2017

jfirebaugh commented Feb 2, 2017

anandthakker commented Feb 2, 2017

anandthakker commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker commented Feb 2, 2017

Choose a reason for hiding this comment

anandthakker Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

lucaswoj left a comment

Choose a reason for hiding this comment

anandthakker commented Feb 7, 2017

anandthakker commented Feb 2, 2017 •

edited

Loading

anandthakker Feb 3, 2017 •

edited

Loading

anandthakker Feb 7, 2017 •

edited

Loading