improve pitched legibility of labels that follow lines by ansis · Pull Request #4781 · mapbox/mapbox-gl-js

ansis · 2017-06-02T23:08:35Z

This improves legibility of labels that follow lines in pitched views. The previous approach used the limited information in the shader to calculate put the glyph in approximatelyright place. The new approach does this more accurately by doing it on the cpu where we have access to the entire line geometry.

The core part of the pr is src/symbol/projection.js which does two things:

calculates the matrices used for projecting symbols
the find-the-correct-point-along-the-line-for-this-glyph calculations

See the comment in src/symbol/projection.js for an overview of the new approach.

This pr also gets rid of the the sliding-glyphs-for-each-segment-that-get-toggled-on-and-off approach, which simplifies things a lot.

This fixes:

garbled text near sharp corners
upside down labels caused by perspective sloping
glyph stretching and squishing
letter spacing with pitch-scaling

before / after

Benchmarks

Frame duration is about 1ms slower in benchmarks. In full page maps it's about 1-2 ms slower, and a bit more in pitched full page maps.

benchmark	master `943dc39`	along-line-label-legibility `b5b6f9a`
map-load	243 ms	138 ms
style-load	122 ms	119 ms
buffer	1,075 ms	1,069 ms
fps	60 fps	60 fps
frame-duration	4.2 ms, 0% > 16ms	5.2 ms, 0% > 16ms
query-point	1.55 ms	1.41 ms
query-box	73.66 ms	73.58 ms
geojson-setdata-small	12 ms	5 ms
geojson-setdata-large	137 ms	130 ms

Launch Checklist

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

@ChrisLoer @kkaefer @anandthakker

ansis · 2017-06-02T23:27:06Z

and a couple wip shots:

ChrisLoer · 2017-06-02T23:49:00Z

This looks SOO good!

I won't have a chance to get into too much review until Monday, but to give you an idea for how the old max-camera-distance filter might still be relevant, look at:

14.27/34.004/-118.4845/-46.6/60 with viewport aligned road labels:

ChrisLoer · 2017-06-05T16:29:25Z


+        const glyphOffsetArrayStart = this.glyphOffsetArray.length;
+
+        const labelAngle = Math.abs((labelAnchor.angle + placementAngle) % Math.PI);


Maybe add a comment that labelAngle is over a 180 degree range relative to line orientation? I assumed 360 degrees at first and got confused reading the code.

ChrisLoer · 2017-06-05T17:14:32Z

-        if (anchor === 'viewport') {
-            const sinA = Math.sin(-this.transform.angle);
-            const cosA = Math.cos(-this.transform.angle);
+        const angle = inPixelUnits ?


It's not immediately obvious here what inPixelUnits has to do with the anchor angle -- I had to go back and look at the use of getGlCoordMatrix in draw_symbol to puzzle it out. Maybe just an explanatory comment would help?

ChrisLoer

This is great, looks beautiful, and fixes so many problems! My only nits were about commenting.

There is one significant architectural concern left, though: what do we do about collision boxes for line features? The pitch-scaling changes account for line labels getting bigger as they go into the distance, but they don't account for growth due to the effective increase in letter-spacing that this PR introduces. If you use a style with viewport-pitch-aligned road labels, it's pretty easy to get collisions in the distance for vertically oriented roads.

In updateLineLabels, if we deferred the addGlyph calls until we has processed the whole label, we could calculate something like an "expansion ratio" and pass that in as a fourth element of a_projected_pos. We could feed that into the collision_adjustment calculation, although it would have the same problem that the incidence_stretch logic has: it would grow the collision boxes in both x and y direction when we really just want them to expand in the x direction. We could try a similar solution: calculate an approximation of the shape at placement time, and at render time use the expansion factor to handle the difference. (Or: maybe just doing it at placement time is good enough)

ChrisLoer · 2017-06-05T17:41:20Z

+ *
+ * ## GL coordinate space
+ * At the end of everything, the vertex shader needs to produce a position in GL coordinate space,
+ * which is (-1, 1) at the top left and (1, -1) in the bottom left.


(1, -1) should be bottom right?

ChrisLoer · 2017-06-05T17:47:28Z

+ *      - map pixel space           pitch-alignment=map         rotation-alignment=map
+ *      - rotated map pixel space   pitch-alignment=map         rotation-alignment=viewport
+ *      - viewport pixel space      pitch-alignment=viewport    rotation-alignment=*
+ * 2. the the label follows a line, find the point along the line that is the correct distance from the anchor.


"the the label" -> "if the label"

ChrisLoer · 2017-06-05T18:00:47Z

+        const anchorPos = [symbol.anchorX, symbol.anchorY, 0, 1];
+        vec4.transformMat4(anchorPos, anchorPos, posMatrix);
+
+        // Don't bother calculating the correct point for invisible labels. Move them offscreen.


Maybe add a note here that we have to move them offscreen instead of just dropping them because we're not updating the paired/non-dynamic buffers?

ChrisLoer · 2017-06-05T18:16:27Z

+    highp float distance_ratio = u_pitch_with_map ?
+        camera_to_anchor_distance / u_camera_to_center_distance :
+        u_camera_to_center_distance / camera_to_anchor_distance;
+    highp float perspective_ratio = 0.5 + 0.5 * clamp(distance_ratio, 0.1, 10.0);


Just out of curiosity, what case is the clamp addressing? Or is it just a way of saying "we expect a distance ratio of 10 to be effectively infinite"?

I was seeing things like this and thought it was an infinite/precision issue. Since I'm not seeing it now I think it might have been something else (broken inversion). Removing

ChrisLoer · 2017-06-05T18:28:15Z

-    float fontScale = u_is_text ? size / 24.0 : size;
+    vec4 projectedPoint = u_matrix * vec4(a_pos, 0, 1);
+    highp float camera_to_anchor_distance = projectedPoint.w;
+    highp float distance_ratio = u_pitch_with_map ?


The corresponding logic in projection.js is easier to understand, but here the inversion of distance_ratio is pretty hard to figure out since it's further removed from the rest of the projection logic. I would add a comment saying something like "If the label is pitched with the map, layout is done in pitched space, which makes labels in the distance smaller relative to viewport space. We counteract part of that effect by multiplying by the perspective ratio. If the label isn't pitched with the map, we do layout in viewport space, which makes labels in the distance larger relative to the features around them. We counteract part of that effect by dividing by the perspective ratio."

ChrisLoer · 2017-06-05T18:28:56Z

-        gl_Position = u_matrix * vec4(a_pos, 0, 1) + vec4(extrude, 0, 0);
-    }
+    highp float distance_ratio = u_pitch_with_map ?
+        camera_to_anchor_distance / u_camera_to_center_distance :


Same comment as for symbol_icon

ChrisLoer · 2017-06-05T18:51:50Z

Example of broken collision detection for vertically-oriented viewport-pitch-aligned road labels:

ansis · 2017-06-06T16:25:13Z

@ChrisLoer yeah, the collision boxes are a problem. I just tried adding an adjustment to maxScale to account for this and it seems like it mostly works: 6a789ec. Do you think this is good enough to last until we rewrite this?

ChrisLoer

The hack looks surprisingly OK when I play around with it. But unless I'm missing something I think we need to apply rotation to the offsets before we calculate the adjustment factors? Kind of disconcerting that I can be pretty sure it's broken but the results still look OK...

ChrisLoer · 2017-06-06T17:56:04Z

@@ -75,7 +75,7 @@ class CollisionFeature {
        // We calculate line collision boxes out to 150% of what would normally be our


150% -> 300% now, right? Because we've got n/2 padding boxes on either side and they're spaced out twice as far apart? Although in practice we often don't get that far because the line geometry doesn't let us.

ChrisLoer · 2017-06-06T17:59:51Z

+            const yStretch2 = yStretch;
+            const xSqr = box.offsetX * box.offsetX;
+            const ySqr = box.offsetY * box.offsetY;
+            const yStretchSqr = ySqr * yStretch2 * yStretch2;


I don't think yStretch2 adds much clarity here, it could just be yStretchSqr = ySqr * yStretch * yStretch.

ChrisLoer · 2017-06-06T18:18:55Z

+            // Adjust the max scale by (approximatePitchedLength / approximateRegularLength)
+            // to compensate for this.
+            const yStretch2 = yStretch;
+            const xSqr = box.offsetX * box.offsetX;


Don't these offsets need to have a rotation applied to them? I think as is this only works for labels that are relatively horizontal relative to tile orientation.

Yeah, definitely

ChrisLoer · 2017-06-06T18:25:12Z

This is a nit, and it could go in another PR, but maybe it would be better if we detected the orientation of a line label based on the beginning and the end of the label instead of the orientation of the middle line segment? The current approach can lead to labels that look "upside down" even though technically the segment their anchor is on is "right side up":

ChrisLoer · 2017-06-06T18:46:15Z

+        // If the current segment doesn't have enough remaining space, iterate forward along the line.
+        // Since all the glyphs are sorted by their distance from the anchor you never need to iterate backwards.
+        // This way line vertices are projected at most once.
+        while (offsetX >= segmentDistance + previousDistance && Math.abs(vertexIndex) < numVertices) {


This while loop exits if we reach the end of the line (vertexIndex >= numVertices), but we keep adding glyphs onto whatever the last segment was. This leads to some funky behavior:

Ignoring collisions (because collision boxes only go as long as the line)

Road labels extending past the end of the road

Road labels wrapping back on themselves (see Ocean Park Blvd in the screen capture)... although this one might be better addressed by a max-curvature property.

We should hide the label instead if we hit this limit.

ChrisLoer · 2017-06-07T18:36:41Z

With an eye on mapbox/mapbox-gl-native#9009 (comment), we should try to see if we can stay under the 8 vertex attribute limit when we make these changes on the JS side.

since we should move towards treating a_size as a regular data-driven property we might not want to pack into it.

Do we have a choice? On this PR a_data is using 40 bits and a_size is using 32 bits, and I believe the only extra space is 16 bits in a_projected_pos. If we keep a_size and try to get rid of a_data, we'd have 40 - 16 -> 24 bits to pack in somewhere else. We can drop a few bits of precision from the offset in a_pos_offset (dropping from 1/64 precision to 1/8 saves 6 bits and doesn't look too noticeably different although the render tests do change), and a few bits from the anchor position. You could also go back to dropping 8 bits of precision from segment_angle (but that'd be a bummer, because the low precision angles meant that labels sometimes didn't quite line up with their lines).

ansis · 2017-06-12T16:16:32Z

@ChrisLoer I fixed the collision box rotation bug and packed all the symbol vertex data into 8 attributes. I think flipping the label based on start and endpoints instead of midpoints can be handled in another pr.

ChrisLoer

Awesome!

I filed #4825 for the text orientation issue, and #4826 for the "labels can flip back on themselves issue".

Out of curiosity, why did you pack the label angle into the third part of a_projected_pos instead of just adding a fourth item to the vector? Do we think there's a performance benefit? I was a little sad to see the increased label angle precision go away (and a little surprised that text-writing-mode/chinese didn't have to be reverted after the angle change)

ansis · 2017-06-12T20:56:15Z

I just rebased on master which includes the recent *-offset fix (#4779). The new implementation is here for alongLine labels and here for horizontal labels.

Out of curiosity, why did you pack the label angle into the third part of a_projected_pos instead of just adding a fourth item to the vector? Do we think there's a performance benefit? I was a little sad to see the increased label angle precision go away (and a little surprised that text-writing-mode/chinese didn't have to be reverted after the angle change)

I'm not sure, I guess the impulse was to just pack everything as tightly as possible instead of using an extra 4 bytes. Should I switch to using a fourth item?

my remaining items:

fix the x component of text-offset for labels that follow lines
fix failing query test

ansis · 2017-06-12T21:10:31Z

@ChrisLoer I also made a query test less sensitive to avoid a failure. The test shouldn't be less sensitive but I don't think we can fix this properly before working on all the collision stuff.

The correct position of a glyph along is now found by doing the work on the CPU where we have access to the entire line geometry. For performance, as much work as possible is left to be done in the shaders. This includes all the work for point horizontal labels. ref #4718

Apply pitch scaling to the font size before calculating the glyph's position.

to fit within the minimum number of supported vertex attributes

This also changes how we iterate along the line to find the correct point. The previous algorithm sorted the quads by offset (during bucket creation) and then iterated along the line a single time, continuing from the previous point. The new approach is simpler. It iterates for each glyph independently and doesn't require them to be sorted. It's a tiny bit slower but it's also easier to work with. For example, implementing text-offset changes the sorted order you'd need for the previous algorithm. As we explore other improvements like #4825 #4826 this will be easier to work with. We can always optimize again later when things settle.

port mapbox/mapbox-gl-js#4781 This improves legibility of labels that follow lines in pitched views. The previous approach used the limited information in the shader to calculate put the glyph in approximatelyright place. The new approach does this more accurately by doing it on the cpu where we have access to the entire line geometry.

ansis requested review from ChrisLoer and anandthakker June 2, 2017 23:09

This was referenced Jun 3, 2017

Map pitch not accounted for in text orientation #4376

Closed

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

Closed

ChrisLoer reviewed Jun 5, 2017

View reviewed changes

ChrisLoer suggested changes Jun 5, 2017

View reviewed changes

ChrisLoer mentioned this pull request Jun 5, 2017

Label legibility improvements: pitch-scaling and pitched line-following mapbox/mapbox-gl-native#9009

Merged

10 tasks done

ChrisLoer suggested changes Jun 6, 2017

View reviewed changes

ansis mentioned this pull request Jun 7, 2017

Fix behaviour when using keep-text-upright in combination with text-offset places text #2350

Closed

ansis force-pushed the along-line-label-legibility branch 4 times, most recently from ac0998d to d2ea30b Compare June 12, 2017 16:04

ChrisLoer mentioned this pull request Jun 12, 2017

Base label "keep-upright" orientation based on start and end of label #4825

Closed

ansis force-pushed the along-line-label-legibility branch from d2ea30b to 75f0765 Compare June 12, 2017 20:31

ChrisLoer mentioned this pull request Jun 12, 2017

Implement max-curvature property for labels #4826

Open

ChrisLoer approved these changes Jun 12, 2017

View reviewed changes

ansis force-pushed the along-line-label-legibility branch from 75f0765 to 28d553f Compare June 12, 2017 20:39

ansis force-pushed the along-line-label-legibility branch from 28d553f to ef07398 Compare June 12, 2017 21:11

ansis added 8 commits June 13, 2017 12:46

fix letter spacing for pitch-scaled labels

ae19df2

Apply pitch scaling to the font size before calculating the glyph's position.

expand angle math to make it clearer

26e16e0

fix comments

a4d2bd3

add another hack to make collision boxes work labels in pitched views

8136395

hide labels that don't fit on the line

da9a759

pack symbol data into 8 attributes

42c7375

to fit within the minimum number of supported vertex attributes

ansis force-pushed the along-line-label-legibility branch from ef07398 to ce09894 Compare June 13, 2017 16:48

ansis mentioned this pull request Jun 13, 2017

use higher precision angles for glyphs #4829

Closed

ansis merged commit ce09894 into master Jun 13, 2017

jfirebaugh deleted the along-line-label-legibility branch June 14, 2017 21:28

jfirebaugh mentioned this pull request Jun 14, 2017

Improve pitched legibility of labels that follow lines mapbox/mapbox-gl-native#9272

Closed

This was referenced Jun 19, 2017

WIP: Pitched label legibility #4682

Closed

text-pitch-alignment: viewport w/ text-rotation-alignment: map doesn't work for point features #4860

Closed

Fix issue #4860: apply map rotation to unpitched point labels. #4864

Merged

ChrisLoer mentioned this pull request Aug 3, 2017

Glyph x-offset calculation incorrect for vertical glyphs #5090

Closed

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


		const glyphOffsetArrayStart = this.glyphOffsetArray.length;

		const labelAngle = Math.abs((labelAnchor.angle + placementAngle) % Math.PI);

		@@ -75,7 +75,7 @@ class CollisionFeature {
		// We calculate line collision boxes out to 150% of what would normally be our

Conversation

ansis commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

before / after

Benchmarks

Launch Checklist

Uh oh!

ansis commented Jun 2, 2017

Uh oh!

ChrisLoer commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisLoer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisLoer Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisLoer commented Jun 5, 2017

Uh oh!

ansis commented Jun 6, 2017

Uh oh!

ChrisLoer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisLoer commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisLoer commented Jun 7, 2017

Uh oh!

ansis commented Jun 12, 2017

Uh oh!

ChrisLoer left a comment

Choose a reason for hiding this comment

Uh oh!

ansis commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ansis commented Jun 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ansis commented Jun 2, 2017 •

edited

Loading

ChrisLoer Jun 5, 2017 •

edited

Loading

ansis commented Jun 12, 2017 •

edited

Loading