Attenuate label size scaling with distance #4547

ChrisLoer · 2017-04-05T22:07:08Z

The motivation for these changes is that we want to improve our rendering support for the "navigation" use case, in which:

Tilting the map is important, because in turn by turn directions you want to be able to see ahead
Legibility becomes more important relative to information density or aesthetics, because users of the map need to be able to read labels with a quick glance.

This PR improves support for this case by:

Changing label scaling behavior to halve the effect of perspective on label size. This preserves the "3d" feel of pitched maps while preventing near labels from becoming comically large and preventing distant labels from becoming impossibly small.
Giving style designers the ability to control how much labels scale down with distance from the camera. This addresses two of the remaining items in Complete set of *-rotation-alignment, *-pitch-alignment, and *-pitch-scale properties #4120 (text-pitch-scale and icon-pitch-scale).

Changes we make to placement logic to support collision detection over a wider range of zooms may make it easier to address #4376.

ChrisLoer · 2017-04-13T00:40:11Z

@nickidlugash @anandthakker helped me draft up some math that could help us avoid uneven spacing on highly curved labels while still doing all the glyph position calculations in the shader:

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

The math depends on adding an(other) approximation that will have the biggest effect on labels that are close to horizontal. I attached some screenshots to a comment on the gist to show the expected difference -- doesn't look like a big deal to me, but could use your input.

ChrisLoer · 2017-04-13T19:37:12Z

@mourner @kkaefer I'm going to increasingly elaborate lengths to do complicated glyph positioning in the vertex shader. Tangram looks like it does glyph positioning in the CPU at render time and seems to get acceptable performance. Doing positioning that way also allows them to send smaller vertex arrays to the GPU, because they don't have to send copies of glyphs for each possible line segment they could be on.

Do either of you have any advice on how to think about the performance trade-offs of the two architectures here? I don't have strong enough intuitions to guess at the differences very well, and since the architectural approaches are so different, it's not easy to just find out through experiment...

nickidlugash · 2017-04-13T21:45:05Z

@ChrisLoer Awesome to see the improvements and to do's on this PR!

The math depends on adding an(other) approximation that will have the biggest effect on labels that are close to horizontal. I attached some screenshots to a comment on the gist to show the expected difference -- doesn't look like a big deal to me, but could use your input.

It's hard to say without being able to interact with it and experiencing the rate of change, but the sin^2(angle) version actually looks more correct to me...it seems slightly too condensed, but the "correct" sin(angle) version actually looks noticeably too spaced out. I noticed this earlier while playing around with this current branch – perfectly horizontal and vertical labels look correctly spaced, but all angles in between seem to be getting too much expansion. The more exaggerated easing for legibility expansion with the sin^2(angle) version seems to be improving this 🤔 Is it possible that it's compensating for an inaccuracy somewhere else in the legibility expansion calculation?

ChrisLoer · 2017-04-13T21:52:26Z

Is it possible that it's compensating for an inaccuracy somewhere else in the legibility expansion calculation?

Yes, the same thought occurred to me! It's definitely possible considering all the different approximations that are involved, although right now if that's actually what's happening it's just dumb luck. I don't have a theoretical explanation -- but with enough time staring at this, it should be possible to construct one. 😅

ChrisLoer · 2017-04-13T21:56:16Z

BTW, if you want to play with the behavior of sin^2(angle) while looking at straight line labels, it's easy to swap into the vertex shader. Just replace legibility_expansion += abs(sin(projected_label_angle)) * (incidence_stretch - 1.0) with legibility_expansion += pow(sin(projected_label_angle), 2.0) * (incidence_stretch - 1.0).

nickidlugash · 2017-04-13T21:59:26Z

@ChrisLoer Another unrelated issue I noticed: the angle of the rendered glyphs for vertical labels seems off, the further away they are from vertical center:

They look like they are on tilted planes. Is this related to how the incident angle does not take into account gl_Position.x?

(This is an issue too in the current released implementation – it's just not as obvious with the current foreshortening behavior).

ChrisLoer · 2017-04-13T23:06:36Z

I think that's our brain applying perspective to the labels based on what's around them, and interpreting the input as "tilted plane":

If we zoom in on the lower left corner, look at Wilshire Blvd, then use photoshop to rotate the street back to vertical, we can see the glyphs are lined up parallel to the road and "flat".

I think the effect is kind of cool because it makes the map feel a little more three-dimensional. Other mapping products^™️ seem to do the same thing.

ChrisLoer · 2017-04-14T05:26:31Z

I got an initial implementation of the fancy trigonometry for label expansion... kind of working. It looks like it's doing a better job of keeping spacing even on curved labels, but there are two big problems:

There's lots of glyph flicker/duplication around segment junctions. This might be caused by the placement-time calculations working with higher precision angles than the part of the calculation done in the shader (the positioning calculations only have to disagree by a tiny amount between two segments to cause artifacts). It looks bad enough it might just be a matter of me messing up the math.
This approach ties the minZoom and maxZoom for a glyph to the "legibility expansion" calculated for that glyph... which breaks the labelMinScale trick we use at https://github.com/mapbox/mapbox-gl-js/blob/master/src/symbol/quads.js#L218 to make all the glyphs disappear if one of the glyphs has to disappear. To fix this, we'd need to add even more vertex attributes. 😞

Overall, it's feeling like too complicated a solution, but I'll keep poking at it a bit longer to see if I can come up with anything compelling.

Here's a shot showing the algorithm actually doing its thing. This is the kind of label that does poorly with the simple algorithm: the center of the label is at about 45 degrees, while the left side is near vertical and the right side is near horizontal. As a result, the simple algorithm (on the left) squishes the left side and stretches out the right side. The fancy algorithm appears to do a reasonable job of spacing across the length of the label.

kkaefer · 2017-04-18T10:40:44Z

Do either of you have any advice on how to think about the performance trade-offs of the two architectures here? I don't have strong enough intuitions to guess at the differences very well, and since the architectural approaches are so different, it's not easy to just find out through experiment...

We have never tried this out, but as a data point: We moved label placement on rotation from pre-calculcated to on-demand (CPU) and haven't run into any performance issues.

ChrisLoer · 2017-04-24T21:57:09Z

We moved label placement on rotation from pre-calculated to on-demand (CPU) and haven't run into any performance issues.

Are you talking about how we re-do collision tiles on rotation? Although we do it on demand with the CPU, we also do it asynchronously and coalesce multiple frames into one calculation. That works for collision detection which is just on/off, but for glyph placement to be smooth we would need to synchronously calculate positions on every frame.

ChrisLoer · 2017-04-25T22:09:52Z

Per discussion with @nickidlugash, we think the navigation use case can benefit from the text-pitch-scale changes alone, so I'm shelving the curved label legibility work for now. This will leave us in a bit of a weird place: pitch-alignment: viewport curved labels are already broken for labels in the distance, but the breakage will be exacerbated if combined with text-pitch-scale < 1.0.

kkaefer · 2017-04-26T16:05:19Z

That works for collision detection which is just on/off, but for glyph placement to be smooth we would need to synchronously calculate positions on every frame.

Yeah, but we do it asynchronously because the placement is the slow part, not the uploading part. If we can calculate the placement fast enough, a synchronous upload might be possible as well, but, as all things GL, we'll have to try them out to know how they perform.

ChrisLoer · 2017-04-26T18:42:51Z

This will leave us in a bit of a weird place: pitch-alignment: viewport curved labels are already broken for labels in the distance, but the breakage will be exacerbated if combined with text-pitch-scale < 1.0.

Here's an example of how broken a curved label can get when it's in the far distance (with text-pitch-scale: 0):

Maybe a hardwired max-distance property for these labels could work as a stopgap.

ChrisLoer · 2017-04-26T22:49:33Z

I implemented the logic that re-calculates collision tiles as a tile's distance from the camera changes, so that we can avoid the performance cost of calculating collision tiles over giant zoom ranges.

Playing with the changes, one concern I have is that re-doing the collision logic frequently makes labels feel jumpy: a relatively small change in position can cause a cascade of collision changes. Throttling how often we re-do placement (e.g. only after the map has panned at least half a tile) might reduce this effect a little, but a lot of it is just the result of the way we're adjusting collision boxes based on perspective at render time.

(watch "Oakwood Knolls" pop in and out during the pan operation)

ChrisLoer · 2017-04-27T18:00:44Z

I implemented a max-camera-distance property that allows style designers selective control over which labels show in the distance. The unit for the property is "multiples of the distance from the camera to the center of the map"... which is kind of tricky to wrap your head around, but gives clearly defined behavior across different zooms and pitches.

I imagine that if we were to make a navigation style using just the functionality from this PR, we would use the max-camera-distance to hide symbol-placement: line labels whenever they're very far from the camera (maybe give them a max-camera-distance around 1.3-1.5..)

mb12 · 2017-04-27T18:56:17Z

@ChrisLoer Is it possible to explain the distance whose multiple is taken in "max-camera-distance" via a diagram? (Whiteboard photo etc should be good enough. Thanks for fixing the issues with symbol/label placement in pitched state.).

ChrisLoer · 2017-04-27T19:29:37Z

I'm not great at hand-drawing 3d stuff, but hopefully this diagram captures the gist of it:

ChrisLoer · 2017-04-27T22:43:05Z

I moved to only re-doing collision detection every 300 ms while the map is panning. I think it helps with the feeling that labels are flickering all over the place, but overall label placement still doesn't feel super smooth or predictable.

There's a subtle problem with the approach I'm using. On a flat map, we calculate collisions for a tile between a range of minScale: 0.5 and maxScale: 2. Now, if we have a distant tilted tile with text-pitch-scaling: 0, we might have labels be 4 times bigger, so we calculate collisions with the range minScale: 0.125 and maxScale: .5.

Now consider a city label and a a highway shield placed near each other. The city label's minimum placement scale is 1.0. The highway shield has lower precedence, so it can only show when the map has higher scale: say it shows at scale 1.25. The problem occurs when our maxScale goes below 1.0 -- all of a sudden the city label is removed from the collision grid (to constrain the cost of collision detection), but since the highway shield is small and takes a lot of scaling o collide with whatever knocked the city label out, it now gets a much lower minimum placement scale (say .25).

The result of all this is that as you pan a titled map tile into the distance, you could see:

First: highway shield + city label
Second: city label
Third: highway shield

In terms of avoiding collisions and getting good label density, this is fine. But it breaks the desideratum that once a label appears it should stick around as scale increases -- and when it happens across a whole bunch of small labels and highway shields, it contributes to that "flickery" feel.

Note that we already have the same problem when we cross integer zoom levels, it's just much less noticeable.

ChrisLoer · 2017-04-29T01:46:31Z

Some problems to tack onto the list:

Labels with low pitch scaling that are also near tile boundaries can collide in the distance. My guess is that this is tolerable, and I don't think we have a very good solution short of merging vector tiles before rendering.
queryRenderedFeatures will have some additional inaccuracies because the "shape" of collision boxes isn't fully determined until render time. While it would be possible to mimic the render-time calculations when the query happens, I'm hoping we can get away with ignoring them. Even without the pitch-scaling changes, the collisions boxes aren't always shaped correctly on pitched maps, and we seem to be able to live with it.

ChrisLoer · 2017-05-01T19:09:34Z

293cd08 fixes a bug that was contributing a lot of unnecessary label flicker (I was setting draw uniforms in the wrong place, so that they could be unpredictably shared between tiles).

While debugging that issue, I noticed another design issue that we might want to think about. Because the collision detection is based off the "least scaled" item in a tile, label density can change a lot between a tile that has an item with low scaling vs a tile without any items that have low scaling.

Look at the tiles that have Highway 2 (Santa Monica Blvd) in them, vs. the more distant tiles that don't have any highway shields.

Because my test style gives these highway shields low scaling (i.e. bigger size relative to the map when they're in the distance), the tiles that include the highway shields get pretty empty (following the conservative algorithm). However, the tiles above without any shields get denser label placement even though they're further away.

This could just be something style designers need to be aware of, or we could change the code to share the same conservative collision scaling factor across all visible tiles (this would trade lower overall label density in exchange for more uniformity of label density).

…hat won't render well. Hardwire to value of 1.5 for viewport-pitch-aligned line labels. Depending on the size of the viewport, this means something like "start hiding line labels at the top of the screen as you approach a pitch of 50 degrees".

… text scaling behavior.

…ne collision features if the map isn't pitched.

asheemmamoowala · 2017-05-24T22:53:30Z

src/source/image_source.js

-            gl.bindTexture(gl.TEXTURE_2D, this.tile.texture);
-            gl.texSubImage2D(gl.TEXTURE_2D, 0, 0, 0, gl.RGBA, gl.UNSIGNED_BYTE, image);
+        for (const w in this.tiles) {
+            const tile = this.tiles[w];


This is creating multiple textures in the wrapped coordinates. Is it possible to create just one texture and reuse it in all the wrapped tiles ?

OK, I took a stab at sharing the texture between tiles -- it passes tests, but I could definitely use a review to see if the approach makes sense. All the image/texture binding stuff just looks like a magic incantation to me.

anandthakker · 2017-05-25T16:33:13Z

src/source/image_source.js

    }

    prepare() {
-        if (!this.tile || !this.image) return;
+        if (this.tiles.length === 0 || !this.image) return;


this.tiles is an object, not an array, so I think you need Object.keys(this.tiles).length === 0

Oops, good catch.

anandthakker · 2017-05-25T16:41:38Z

src/source/query_features.js

-                    resultFeatures.push(tileFeatures[f]);
+                for (const tileFeature of tileFeatures) {
+                    if (!tileFeature.id || !wrappedIDFeatures[tileFeature.id]) {
+                        wrappedIDFeatures[tileFeature.id] = true;


If I'm reading this right, it looks like in the case where features do not have an id and there are two tiles w/ same wrapped id, the wrappedIDFeatures object will look like { 'undefined': true } after the first of those two tiles' results are collected. Then, if the results from the second tile have features that aren't duplicates, they'd be skipped.

If the features don't have IDs, they'll get duplicated (because of the !tileFeature.id check). If the feature has an ID, it should get included once. Say the new feature has ID "1" and wrappedIDFeatures looks like { 'undefined': true }. wrappedIDFeatures[1] will still be undefined. Am I missing something?

Do you have any opinion on whether it might make sense to just change the behavior here to "duplicate on wrapped tiles"? The current behavior is publicly documented (https://www.mapbox.com/mapbox-gl-js/api/#map#queryrenderedfeatures) so I'm thinking the answer is "no", but on the other hand users of the API can already see the same feature returned multiple times across tile boundaries.

@jfirebaugh Pointed me at #2321: we definitely intend to avoid duplicates. I pushed changes to get rid of the !tileFeature.id case by using the grid key from feature_index as a unique identifier for de-duplication.

anandthakker · 2017-05-25T17:07:17Z

src/source/video_source.js

@@ -107,7 +107,7 @@ class VideoSource extends ImageSource {
    // setCoordinates inherited from ImageSource

    prepare() {
-        if (!this.tile || this.video.readyState < 2) return; // not enough data for current position
+        if (this.tiles.length === 0 || this.video.readyState < 2) return; // not enough data for current position


See comment in ImageSource

ChrisLoer · 2017-05-25T18:43:51Z

Benchmarks look reasonable to me, although I'm not sure what the expected variability is:

benchmark	master `010f425`	cloer_text_pitch_scale `bdb217a`
map-load	216 ms	102 ms
style-load	70 ms	42 ms
buffer	977 ms	1,106 ms
fps	59 fps	60 fps
frame-duration	4.2 ms, 0% > 16ms	3.6 ms, 0% > 16ms
query-point	1.92 ms	2.09 ms
query-box	76.89 ms	83.43 ms
geojson-setdata-small	5 ms	2 ms
geojson-setdata-large	70 ms	99 ms

lbud · 2017-05-25T21:18:49Z

src/source/video_source.js

@@ -107,7 +107,7 @@ class VideoSource extends ImageSource {
    // setCoordinates inherited from ImageSource

    prepare() {
-        if (!this.tile || this.video.readyState < 2) return; // not enough data for current position
+        if (Object.keys(this.tiles).length === 0 || this.video.readyState < 2) return; // not enough data for current position


The CanvasSource class also inherits from ImageSource and will need this change in its prepare method as well.

I updated CanvasSource, but I didn't see an equivalent of debug/image.html for testing it -- do we have any working examples?

I pushed a simple debug/canvas.html. Looks like it's rendering fine.

Awesome thanks @ChrisLoer!

- Necessary because pitched/wrapped tiles now have different CollisionTiles - Introduces potential for more flicker while crossing dateline at zoom level 0 - To preserve the ImageSource/VideoSource wrapping behavior, we can now bind the same image/video to multiple (wrapped) tiles - query_features#mergeRenderedFeatureLayers adds a fair amount of extra work to preserve existing query behavior with wrapped tiles: i.e. don't return a feature twice if it's returned from both tiles. - Flip the tile order of results in box-cutting-antimeridian-z0. This was previously based on the order provided by SourceCache#getIDs, which was only strictly defined for zoom level. It's now defined by query_features#sortTilesIn

This eliminates precision-related disagreement between collision box colors and whether symbols actually show.

…inzoom, etc. This resolves an iOS rendering issue where right at the edge of a collision some vertices could be hidden while others showed. It's possible the underlying issue could still be exposed when the un-rounded perspective_zoom_adjust value is almost exactly at a multiple of .1, but if so it will be much less common.

ChrisLoer added the under development label Apr 5, 2017

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

ChrisLoer force-pushed the cloer_text_pitch_scale branch from d203ef7 to 86af564 Compare April 9, 2017 06:42

ChrisLoer force-pushed the cloer_text_pitch_scale branch from d67741d to 08fdd72 Compare April 13, 2017 19:17

ChrisLoer changed the base branch from dds-text-size to master April 13, 2017 19:18

ChrisLoer mentioned this pull request Apr 25, 2017

Rename circle-pitch-scale to circle-pitch-scaling #4165

Closed

ChrisLoer force-pushed the cloer_text_pitch_scale branch from 08fdd72 to 76e9b50 Compare April 25, 2017 21:55

ChrisLoer force-pushed the cloer_text_pitch_scale branch from 4fd8643 to 35127d3 Compare April 26, 2017 01:47

ChrisLoer force-pushed the cloer_text_pitch_scale branch from ef96da6 to bed53c9 Compare April 27, 2017 19:08

ChrisLoer force-pushed the cloer_text_pitch_scale branch from 293cd08 to f45f304 Compare May 1, 2017 19:25

ChrisLoer added 5 commits May 24, 2017 14:17

Render tests for icon and text pitch scaling.

1593dce

Update existing pitched map text rendering tests to match new pitched…

88b28eb

… text scaling behavior.

Fix type info in comments: Coordinate -> TileCoord

648fc0e

Small performance enhancement: don't add "pitch padding" boxes for li…

71ca322

…ne collision features if the map isn't pitched.

ChrisLoer force-pushed the cloer_text_pitch_scale branch from 1575bf3 to 4309634 Compare May 24, 2017 21:19

asheemmamoowala reviewed May 24, 2017

View reviewed changes

asheemmamoowala approved these changes May 24, 2017

View reviewed changes

anandthakker reviewed May 25, 2017

View reviewed changes

asheemmamoowala approved these changes May 25, 2017

View reviewed changes

lbud suggested changes May 25, 2017

View reviewed changes

ChrisLoer added 5 commits May 25, 2017 15:01

Add pitched-collision render tests, including case of wrapped tiles.

ff95426

Tiny shader logic simplification.

ecfeb4e

Make debug collision boxes use same fade texture as the symbols.

d3b83f1

This eliminates precision-related disagreement between collision box colors and whether symbols actually show.

ChrisLoer force-pushed the cloer_text_pitch_scale branch from 6131a7b to 6f06f78 Compare May 25, 2017 22:03

Add simple CanvasSource debug page.

4b0235a

ChrisLoer force-pushed the cloer_text_pitch_scale branch from 6d441c4 to 4b0235a Compare May 25, 2017 22:28

lbud approved these changes May 25, 2017

View reviewed changes

ChrisLoer merged commit 10eb02b into master May 25, 2017

ChrisLoer mentioned this pull request Jun 3, 2017

Improve readability of text-pitch-alignment: "viewport" text #3057

Closed

jfirebaugh deleted the cloer_text_pitch_scale branch June 14, 2017 21:37

nickidlugash mentioned this pull request Aug 2, 2017

Reduce density for labels based on distance from camera #5086

Open

jmunowitch mentioned this pull request Sep 14, 2017

Upgrade to Mapbox-gl-js 0.40.0 archesproject/arches#2410

Closed

1ec5 mentioned this pull request Apr 10, 2018

[ios] Change MGLAnnotationView.scalesWithViewingDistance default value to NO mapbox/mapbox-gl-native#11636

Merged

1ec5 mentioned this pull request Jul 19, 2019

Distant line-placed labels are illegible when top padding is large mapbox/mapbox-gl-native#15167

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attenuate label size scaling with distance #4547

Attenuate label size scaling with distance #4547

ChrisLoer commented Apr 5, 2017 •

edited

Loading

ChrisLoer commented Apr 13, 2017

ChrisLoer commented Apr 13, 2017

nickidlugash commented Apr 13, 2017

ChrisLoer commented Apr 13, 2017

ChrisLoer commented Apr 13, 2017

nickidlugash commented Apr 13, 2017

ChrisLoer commented Apr 13, 2017

ChrisLoer commented Apr 14, 2017 •

edited

Loading

kkaefer commented Apr 18, 2017

ChrisLoer commented Apr 24, 2017

ChrisLoer commented Apr 25, 2017

kkaefer commented Apr 26, 2017

ChrisLoer commented Apr 26, 2017

ChrisLoer commented Apr 26, 2017 •

edited

Loading

ChrisLoer commented Apr 27, 2017

mb12 commented Apr 27, 2017 •

edited

Loading

ChrisLoer commented Apr 27, 2017

ChrisLoer commented Apr 27, 2017

ChrisLoer commented Apr 29, 2017

ChrisLoer commented May 1, 2017

asheemmamoowala May 24, 2017

ChrisLoer May 24, 2017

anandthakker May 25, 2017

ChrisLoer May 25, 2017

anandthakker May 25, 2017

ChrisLoer May 25, 2017

ChrisLoer May 25, 2017

anandthakker May 25, 2017

ChrisLoer commented May 25, 2017

lbud May 25, 2017

ChrisLoer May 25, 2017

ChrisLoer May 25, 2017

lbud May 25, 2017

Attenuate label size scaling with distance #4547

Attenuate label size scaling with distance #4547

Conversation

ChrisLoer commented Apr 5, 2017 • edited Loading

ChrisLoer commented Apr 13, 2017

ChrisLoer commented Apr 13, 2017

nickidlugash commented Apr 13, 2017

ChrisLoer commented Apr 13, 2017

ChrisLoer commented Apr 13, 2017

nickidlugash commented Apr 13, 2017

ChrisLoer commented Apr 13, 2017

ChrisLoer commented Apr 14, 2017 • edited Loading

kkaefer commented Apr 18, 2017

ChrisLoer commented Apr 24, 2017

ChrisLoer commented Apr 25, 2017

kkaefer commented Apr 26, 2017

ChrisLoer commented Apr 26, 2017

ChrisLoer commented Apr 26, 2017 • edited Loading

ChrisLoer commented Apr 27, 2017

mb12 commented Apr 27, 2017 • edited Loading

ChrisLoer commented Apr 27, 2017

ChrisLoer commented Apr 27, 2017

ChrisLoer commented Apr 29, 2017

ChrisLoer commented May 1, 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

ChrisLoer commented May 25, 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

ChrisLoer commented Apr 5, 2017 •

edited

Loading

ChrisLoer commented Apr 14, 2017 •

edited

Loading

ChrisLoer commented Apr 26, 2017 •

edited

Loading

mb12 commented Apr 27, 2017 •

edited

Loading