Enable property functions for {text,icon}-size #4455

anandthakker · 2017-03-20T15:56:34Z

TODO:

benchmark	master `a73a85b`	dds-text-size `326e42c`
map-load	233 ms	134 ms
style-load	117 ms	98 ms
buffer	1,065 ms	1,062 ms
fps	60 fps	60 fps
frame-duration	4.9 ms, 0% > 16ms	5 ms, 0% > 16ms
query-point	1.16 ms	1.18 ms
query-box	68.65 ms	70.20 ms
geojson-setdata-small	5 ms	2 ms
geojson-setdata-large	126 ms	166 ms

anandthakker · 2017-03-21T21:37:52Z

Capturing notes on this issue from the TODO above:

text-size/composite-function (symbol-placement: line, overzoomed tile - target edge case where shader's evaluated function value != layout-time function value)

The concern here is that we might run into a problem due to the shaders having to evaluate composite functions using a limited number of stops, because:

We evaluate text-size at layout time (with {zoom: tile.z + 1}) using the complete style function; among other things, this determines the valid zoom range for each of the several instances of a glyph produced when symbol-placement: line.
But then, in the shader, we calculate an adjusted version of those zoom ranges. To do so, we recover the value of text-size at the layout zoom level by evaluating the style function in the shader, which means we're using a limited number of stops (4 in gl js, 2 in native)

Cases to consider:
(u_zoom is the current (rendered) zoom level; "layout zoom" is tile.z + 1)

Normal case (`tile.z <= u_zoom < tile.z + 1`)

This case should be fine. If the function has zoom stops above u_zoom, ProgramConfiguration always includes them, and so the shader should get the "true" value of the style function at tile.z + 1.

update: Nope, this can go wrong too. If there are stops at, say, 2, 2.1, 2.2, 2.3, 3 -- then for a z2 tile, we would only get 2 through 2.3, so that she shader could not correctly evaluate the composite function at z3

Underzoomed tile (`u_zoom < tile.z`)

Test case:

[
  [{zoom: 0, value: 0}, 1],
  [{zoom: 1, value: 0}, 2],
  [{zoom: 2, value: 0}, 4],
  [{zoom: 3, value: 0}, 8],
  [{zoom: 4, value: 0}, 16]
]

If we render a z4 vector tile at zoom 1.8, then the shader would only have stop values up to [{zoom: 3, value: 0}, 8]. Thus, interpolating linearly, it would incorrectly calculate that the size used at layout time was 12.

Overzoomed tile (`u_zoom > tile.z + 1`)

Similar to underzoomed.

anandthakker · 2017-03-21T21:42:14Z

src/shaders/symbol_sdf.vertex.glsl

+    // Thus, we compensate for this difference by calculating an adjustment
+    // based on the scale of rendered text size relative to layout text size.
+    mediump float adjustedSize = u_is_size_composite
+        ?  evaluate_zoom_function_1(a_size, u_adjusted_size_t) / 10.0


It is unfortunate that this breaks the abstraction provided by the #pragma system, and it's also going to need reworking to be usable in native.

anandthakker · 2017-03-23T15:56:27Z

Okay, here's a draft based on treating text/icon-size as a "pseudo" paint property; the actual DDS part is in the last three commits (the previous ones are miscellaneous cleanup, docs, and a couple bug fixes).

A key difficulty is that, when text/icon-size is a composite function, we need per-vertex values for text-size at both the rendered zoom and layout zoom (the latter being adjustedSize described #4228 (comment)). In this draft, my approach is to use the four stop-values that are already being provided to the shader for the (pseudo-)paint property, along with an extra interpolation t-value that represents the layout zoom level. This allows us to only use one vertex attribute for text/icon-size, so that if we also pack together two of the existing layout attributes together we'd remain at 8--the max for native. However, it's got some problems: #4455 (comment) and #4455 (review).

I'm going to try an alternate approach next: instead of trying to make a pseudo paint property, I'll try doing it with a new layout attribute -- i.e. the second bullet here #4228 (comment)

anandthakker · 2017-03-24T12:22:05Z

src/render/draw_symbol.js

-        const iconScaled = size !== 1 || browser.devicePixelRatio !== painter.spriteAtlas.pixelRatio || iconsNeedLinear;
+        const iconSizeScaled = !layer.isLayoutValueFeatureConstant('text-size') ||
+            !layer.isLayoutValueZoomConstant('text-size') ||
+            layer.getLayoutValue('text-size', { zoom: tr.zoom }, {}) !== 1;


These should be icon-size

anandthakker · 2017-03-27T15:59:26Z

Replaced the problematic approach described in #4455 (comment) with using a new layout attribute 👌

jfirebaugh · 2017-03-27T22:43:42Z

src/data/bucket/symbol_bucket.js

-function addVertex(array, x, y, ox, oy, tx, ty, minzoom, maxzoom, labelminzoom, labelangle) {
-    array.emplaceBack(
+function addVertex(array, x, y, ox, oy, tx, ty, sizeData, minzoom, maxzoom, labelminzoom, labelangle) {
+    array.emplaceBack.apply(array,


I think this is a pretty hot path, so this change should receive some thorough benchmarking.

Yeah, buffer benchmark shows a 5% increase

benchmark master 7c4b9a1 dds-text-size ae60c28

buffer 906 ms 952 ms

Further profiling confirms that apply is a bad idea here.

Timing emplaceBack() ([min, 1st quartile, median, 3rd quartile, max]):

master (i.e. w/o the size data added in this PR): [ 0.001, 0.004, 0.005, 0.006, 0.778 ]

using apply: [ 0.002, 0.008, 0.01, 0.011, 12.945 ]

Changed to avoid using apply by always passing three (possibly undefined) args for the size data; timing for emplaceBack with that approach is basically identical to master.

jfirebaugh · 2017-03-27T22:50:44Z

src/render/draw_symbol.js

    }

    if (isSDF) {
        const haloWidthProperty = `${isText ? 'text' : 'icon'}-halo-width`;
        const hasHalo = !layer.isPaintValueFeatureConstant(haloWidthProperty) || layer.paint[haloWidthProperty];
-        const gammaScale = fontScale * (pitchWithMap ? Math.cos(tr._pitch) : 1) * tr.cameraToCenterDistance;
-        gl.uniform1f(program.u_font_scale, fontScale);
+        const gammaScale = (isText ? 1 / 24 : 1) * (pitchWithMap ? Math.cos(tr._pitch) : 1) * tr.cameraToCenterDistance;


Why does fontScale reduce to 1 elsewhere in this function but not here?

Good question. I think I made this change before I'd introduced u_is_text and the fontScale calculation into the shader. Now that those are in place, the (isText ? 1 / 24 : 1) can be removed here in favor of multiplying u_gamma_scale by fontScale instead of by v_size.

jfirebaugh · 2017-03-27T22:53:17Z

src/shaders/encode_attribute.js

+ *
+ * @private
+ */
+exports.packUint8ToFloat = function pack(a, b) {


Is this used somewhere? I can only find it in the tests.

Whoops. Leftover from previous attempt.

Oh wait, no -- I think we'll need this, it's just still TBD: we still need to pack a_data together with a_texture_pos so that there's no net increase in number of attributes, and this helper will be used for that.

It's now used (see 2d3b36a)

jfirebaugh · 2017-03-27T22:59:46Z

src/style-spec/function/index.js

@@ -98,21 +98,24 @@ function createFunction(parameters, propertySpec) {

        if (zoomAndFeatureDependent) {
            const featureFunctions = {};
-            const featureFunctionStops = [];
+            const zoomStops = [];


Can you split this into a separate fix/PR, with a test?

jfirebaugh · 2017-03-27T23:04:13Z

src/shaders/symbol_icon.vertex.glsl

@@ -3,11 +3,20 @@ attribute vec4 a_pos_offset;
 attribute vec2 a_texture_pos;
 attribute vec4 a_data;

+// icon-size data (see symbol_sdf.vertex.glsl for more)
+attribute vec3 a_size;


Are we combining two attributes somewhere else or does this put us over the eight attribute limit?

Yep we still need to combine #4455 (comment)

anandthakker · 2017-03-28T21:16:47Z

src/data/bucket/symbol_bucket.js

+        }
+        if (!layer.isLayoutValueZoomConstant('icon-size')) {
+            this.iconSizeCoveringZoomStops = this.layers[0].getLayoutValueCoveringZoomStops('icon-size', this.zoom, this.zoom + 1);
+        }


Hmm. This is overcomplicated. We don't really need to serialize adjusted{Text,Icon}Size, or {text,icon}SizeCoveringZoomStops on the bucket -- drawSymbols() can (re)compute these at render time, since they only depend on the tile's zoom level.

ChrisLoer

This is awesome! I'm still playing with the branch locally, but if we find some extra bits for labelangle, we should be able to get the tests to start passing.

ChrisLoer · 2017-03-28T21:17:05Z

src/shaders/symbol_sdf.vertex.glsl

+
+    mediump float zoomAdjust = log2(v_size / adjustedSize);
+    mediump float adjustedZoom = (u_zoom - zoomAdjust) * 10.0;
+    // result: z = 0 if a_minzoom <= adjustedZoom < a_maxzoom, and 1 otherwise


Conceptually, this means: if adjustedZoom is outside the min-max zoom range, shift the z value of this vertex by w -- i.e. push this vertex outside of clip space so it doesn't get rendered... right? This took several reads for me to understand.

Yeah, I think that's right. I wasn't 100% sure of the clip space part, though, and so didn't include it in the comment in case that was incorrect and thus misleading. But if that's your understanding, too, I think that's confirmation enough...

ChrisLoer · 2017-03-28T23:07:13Z

src/data/bucket/symbol_bucket.js

            (labelminzoom || 0) * 10, // labelminzoom
-            labelangle, // labelangle
+            labelangle // labelangle


Packing labelangle into 8 bits is really tight (less than 1 degree precision). Let's try to find some spare bits somewhere else.

ChrisLoer · 2017-03-29T07:17:29Z

src/data/bucket/symbol_bucket.js

    addSymbolInstance(anchor, line, shapedTextOrientations, shapedIcon, layer, addToBuffers, collisionBoxArray, featureIndex, sourceLayerIndex, bucketIndex,
-        textBoxScale, textPadding, textAlongLine,
-        iconBoxScale, iconPadding, iconAlongLine, globalProperties, featureProperties) {
+        layoutTextSize, textBoxScale, textPadding, textAlongLine,


Are layoutTextSize and layoutIconSize vestigial from some earlier version of this code? I don't see them being used.

Yep, good catch, thanks!

ChrisLoer · 2017-03-29T07:33:32Z

src/shaders/_prelude.vertex.glsl

+// packed like so:
+// packedValue = floor(input[0]) * 256 + input[1],
+vec2 unpack_float(const float packedValue) {
+    float v0 = floor(packedValue / 256.0);


I can't actually see a problem with the math/precision handling here, but for doing this type of math I would try to do the float -> int conversion as soon as possible (where we have to worry about weird precision stuff), and then do the rest of the math with ints instead of floats. Reasoning about things like .99999 + 1 - 1 == 0.9999899999999999 is tricky.

I'm assuming glsl 100 reads UNSIGNED_SHORT vertex attributes as 32-bit floats... which I think should be able to handle up to 24 bits of integers without precision loss. I can't actually find where the behavior is specified. But I think we'd see more breakage if it were using half-floats.

It's specified in OpenGL® ES Shading Language §4.5.2 (p33). For highp, 16 bits of precision, integer range (-2¹⁶, 2¹⁶).

anandthakker · 2017-03-30T04:27:23Z

@ChrisLoer I think I've addressed the review points. Still don't want to merge till -native port is done (finally making some progress there this evening), but could you check and see if this looks ready to you?

ChrisLoer · 2017-03-30T14:50:30Z

@anandthakker It's looking good to me, and I've been using the branch locally for work on https://github.com/mapbox/mapbox-gl-js/tree/cloer_text_pitch_scale without any problems. I'm still hoping to get some time to dive into how the zoom stop interpolation works (it was the sort of thing that I read with the mindset "this is complicated... I'm sure it's fine"). No 👶 🐒 today so I probably can't get to that until tomorrow.

ChrisLoer · 2017-03-31T01:16:40Z

@anandthakker These changes don't handle exponential interpolation consistently with other DDS styles. Here's the debug view I used to play with text-size vs circle-radius:

https://gist.github.com/ChrisLoer/a102c40ae5ad45b60c2d1b1d4bb738fc

With the base set to something high, you can see how the circles will change size rapidly as you approach the stop, while the text size doesn't. Also the 'interval' property function type doesn't work.

I don't really have a great solution in mind, or haven't wrapped my head around what the solution would be. If we can't make changes to support these modes, we should at least disable them and document accordingly?

anandthakker · 2017-03-31T01:29:48Z

Ah, dang. Nice catch @ChrisLoer. I suspect this is probably because I'm calculating the interpolation t-value naively here, whereas for other DDS properties we use StyleDeclaration#calculateInterpolationT.

We may be able to do better here... but it's going to be complicated by the fact that for text/icon-size, when we're rendering a tile at zoom = z, we're basically dropping any stops that fall between z and z+1.

Ports mapbox/mapbox-gl-js#4455

ChrisLoer · 2017-04-06T16:09:21Z

Playing around with this I noticed a kind of annoying problem even if you're on the "conservative" side of collision detection. Say you start at zoom 5.99, with the collision box very close to correct. As you zoom out, the label gets smaller (faster than the collision box). At zoom 5.5, the collision box gets triggered, and the label disappears. You keep zooming out, and then when you reach 4.99, the collision box gets recalculated with the new smaller size, and the label pops back in. It seems to me this behavior could cause a lot of label flicker on crossing integer zoom levels, even for very "reasonable" zoom functions (my example is exaggerated, but with a less exaggerated zoom function, the collision box flicker will just happen over a very small zoom range, which could actually look more jarring).

ChrisLoer

The collision issue with zooms and text-size is already something we're living with, I think this issue tracks it: #1196. Other than that, everything looks good to me. I'm suffering from attention exhaustion, but I did do one more quick run through the changes and tests. I'll leave to @jfirebaugh for final 🍏

…ction values

…namically

Fix bug in packing of labelangle value * Label angles passed to addVertex() are in units of 1/256 of a circle. Because packUint8ToFloat clamped input values to [0, 255], the value 256 was clamped to 255 (== 1.4 degrees off the horizontal), rather than being wrapped to 0.

- Consolidate it under bucket.{text,icon}SizeData objects - Include function base as part of said data - Use interpolationFactor to account for base != 1 for consistency with paint properties

mapbox/mapbox-gl-native#8654

Ports mapbox/mapbox-gl-js#4455

* Update gl-js and generate style code * Factor out packUint8Pair() helper function * Draft implementation of DDS for {text,icon}-size Ports mapbox/mapbox-gl-js#4455 * Fix text-size/composite-function-line-placement test * Refactor to PaintPropertyBinders-like strategy * Dedupe gl::Program construction * Use exponential function base for interpolation * Dedupe coveringZoomStops method * Fixup tests * Fix CI errors (hidden within #if block)

anandthakker added the under development label Mar 20, 2017

anandthakker force-pushed the dds-text-size branch from 68ad56a to 8f8e4b3 Compare March 20, 2017 16:04

jfirebaugh mentioned this pull request Mar 21, 2017

Master ticket for remaining data-driven style properties #4434

Open

anandthakker commented Mar 21, 2017

View reviewed changes

anandthakker force-pushed the dds-text-size branch 2 times, most recently from 233d2a7 to 23e2b51 Compare March 23, 2017 13:35

anandthakker commented Mar 24, 2017

View reviewed changes

anandthakker force-pushed the dds-text-size branch 2 times, most recently from eb8cd6c to 8e461c4 Compare March 27, 2017 14:57

anandthakker requested a review from jfirebaugh March 27, 2017 14:57

anandthakker force-pushed the dds-text-size branch from c798663 to ae60c28 Compare March 27, 2017 21:15

jfirebaugh reviewed Mar 27, 2017

View reviewed changes

anandthakker force-pushed the dds-text-size branch 2 times, most recently from 919306c to 2d3b36a Compare March 28, 2017 17:27

jfirebaugh requested a review from ChrisLoer March 28, 2017 18:14

anandthakker commented Mar 28, 2017

View reviewed changes

ChrisLoer reviewed Mar 28, 2017

View reviewed changes

ChrisLoer reviewed Mar 29, 2017

View reviewed changes

anandthakker pushed a commit to mapbox/mapbox-gl-native that referenced this pull request Mar 31, 2017

Draft implementation of DDS for {text,icon}-size

32f95a9

Ports mapbox/mapbox-gl-js#4455

ChrisLoer reviewed Apr 6, 2017

View reviewed changes

jfirebaugh approved these changes Apr 6, 2017

View reviewed changes

Anand Thakker added 18 commits April 6, 2017 14:57

Refactor byte pair unpacking into separate shader function

f2bfe76

Document / clarify StyleDeclaration#calculateInterpolationT

2957ca3

Document strategy for populating vertex attributes with composite fun…

b7b5b5f

…ction values

Clamp composite function interpolation to [0, 3], not [0, 4]

c52f636

Refactor {Array,Buffer}Group to construct layout vertex array type dy…

954dc5a

…namically

Enable property functions for text-size

fe0d376

Improve naming and symbol layout internal docs

fde0969

Enable property functions for icon-size

1b04c73

Add render tests for data-driven {text,icon}-size

9bfe5d7

Remove vestigial parameters

09683eb

Document and refactor exponential function interpolation

bec280e

Refactor {text,icon}-size-related bucket data

975641b

- Consolidate it under bucket.{text,icon}SizeData objects - Include function base as part of said data - Use interpolationFactor to account for base != 1 for consistency with paint properties

Fix handling of camera functions of 'interval' type

7a8e21d

Add t.equalWithPrecision() assertion and use it for function tests

e8e0485

Edit comment for accuracy

02ebd65

Ignore high-base tests for native

a85163e

mapbox/mapbox-gl-native#8654

Fixup render test: add missing feature properties

e3b2df2

anandthakker force-pushed the dds-text-size branch from 326e42c to e3b2df2 Compare April 6, 2017 18:57

anandthakker pushed a commit to mapbox/mapbox-gl-native that referenced this pull request Apr 6, 2017

Draft implementation of DDS for {text,icon}-size

7d8d2d8

Ports mapbox/mapbox-gl-js#4455

anandthakker merged commit 13363a2 into master Apr 6, 2017

ChrisLoer mentioned this pull request Apr 7, 2017

Collision detection doesn't fully account for label zoom functions #4558

Closed

jfirebaugh deleted the dds-text-size branch April 28, 2017 18:40

jfirebaugh mentioned this pull request Jul 6, 2017

Icon and text symbols disappearing on a specific text/icon-size #4942

Closed

This was referenced Feb 26, 2020

Older versions of chrome text rendering breaks (chrome 48) #8742

Closed

Use glUniform1i for boolean uniforms #9349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable property functions for {text,icon}-size #4455

Enable property functions for {text,icon}-size #4455

anandthakker commented Mar 20, 2017 •

edited

Loading

anandthakker commented Mar 21, 2017 •

edited

Loading

anandthakker Mar 21, 2017

anandthakker commented Mar 23, 2017 •

edited

Loading

anandthakker Mar 24, 2017

anandthakker commented Mar 27, 2017

jfirebaugh Mar 27, 2017

anandthakker Mar 28, 2017

anandthakker Mar 28, 2017

jfirebaugh Mar 27, 2017

anandthakker Mar 28, 2017

jfirebaugh Mar 27, 2017

anandthakker Mar 28, 2017

anandthakker Mar 28, 2017

anandthakker Mar 28, 2017

jfirebaugh Mar 27, 2017

anandthakker Mar 28, 2017

jfirebaugh Mar 27, 2017

anandthakker Mar 28, 2017

anandthakker Mar 28, 2017

ChrisLoer left a comment

ChrisLoer Mar 28, 2017

anandthakker Mar 29, 2017

ChrisLoer Mar 28, 2017

ChrisLoer Mar 29, 2017

anandthakker Mar 29, 2017

ChrisLoer Mar 29, 2017

jfirebaugh Mar 29, 2017

anandthakker commented Mar 30, 2017

ChrisLoer commented Mar 30, 2017

ChrisLoer commented Mar 31, 2017

anandthakker commented Mar 31, 2017

ChrisLoer commented Apr 6, 2017

ChrisLoer left a comment

Enable property functions for {text,icon}-size #4455

Enable property functions for {text,icon}-size #4455

Conversation

anandthakker commented Mar 20, 2017 • edited Loading

anandthakker commented Mar 21, 2017 • edited Loading

Normal case (tile.z <= u_zoom < tile.z + 1)

Underzoomed tile (u_zoom < tile.z)

Overzoomed tile (u_zoom > tile.z + 1)

Choose a reason for hiding this comment

anandthakker commented Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

anandthakker commented Mar 27, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisLoer left a comment

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

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 commented Mar 30, 2017

ChrisLoer commented Mar 30, 2017

ChrisLoer commented Mar 31, 2017

anandthakker commented Mar 31, 2017

ChrisLoer commented Apr 6, 2017

ChrisLoer left a comment

Choose a reason for hiding this comment

anandthakker commented Mar 20, 2017 •

edited

Loading

anandthakker commented Mar 21, 2017 •

edited

Loading

Normal case (`tile.z <= u_zoom < tile.z + 1`)

Underzoomed tile (`u_zoom < tile.z`)

Overzoomed tile (`u_zoom > tile.z + 1`)

anandthakker commented Mar 23, 2017 •

edited

Loading