Fix issue #5112 (line labels can render incorrectly on overzoomed tiles) #5120
Conversation
|
Building tests was tricky using a GeoJSON source. For the single glyph special case I ended up just finding a vector tile that could reproduce the issue and I used the vector tile in the test case. Is that a reasonable approach? |
There was a problem hiding this comment.
@ChrisLoer strategy looks good to me! Couple detail questions inline
src/symbol/projection.js
Outdated
| const w = pos[3]; | ||
| return { | ||
| point: new Point(pos[0] / w, pos[1] / w), | ||
| distanceToCamera: w |
There was a problem hiding this comment.
Suggested rename: signedDistanceFromCamera
src/symbol/projection.js
Outdated
| const tileSegmentEnd = lineVertexArray.get(symbol.lineStartIndex + symbol.segment + 1); | ||
| const projectedVertex = project(tileSegmentEnd, posMatrix); | ||
| // We know the anchor will be in the viewport, but the end of the line segment may be | ||
| // past the plane of the camera, in which case we can use a point at any arbitrary (closer) |
There was a problem hiding this comment.
Suggested edit: "past the plane of the camera" => "behind the plane of the camera"
src/symbol/projection.js
Outdated
| @@ -276,11 +290,19 @@ function placeGlyphsAlongLine(symbol, | |||
| return {}; | |||
| } | |||
|
|
|||
| function projectTruncatedLineSegment(previousTilePoint: Point, currentTilePoint: Point, previousProjectedPoint: Point, minimumLength: number, projectionMatrix: mat4) { | |||
| const projectedUnitVertex = project(previousTilePoint.add(previousTilePoint.sub(currentTilePoint)._unit()), projectionMatrix).point; | |||
There was a problem hiding this comment.
Would it be possible for previousTilePoint + unitVector(previousTilePoint - currentTilePoint) to fall behind the camera, if previousTilePoint happened to be within one unit of the camera plane? If so, would that cause a similar problem to the one we're solving here?
There was a problem hiding this comment.
🤔 That's a good question. I think the answer is "not under any reasonable conditions," but let me try to reason why:
- Any point that's near the plane of the camera is very far "away" (in x/y terms) from the viewport
- We're only placing glyphs whose anchor is within the viewport
- For a label to reach from within the viewport to a line vertex at the edge of camera plane, it would need to be very long (much longer than could be shown on one screen)
- A label with
text-pitch-alignment: mapwould be the best candidate for being way longer than expected, because it would get stretched out more and more the closer it got to the plane of the camera. But that case wouldn't be affected by the "plane of the camera" logic, because it would be projected using a tile-based matrix (see logic ingetLabelPlaneMatrix). - Therefore any label we're placing should fit on the line before the hypothetical vertex that's right at the edge of the camera plane, and we should never reach a point where we try to project past the hypothetical vertex.
I guess another way of thinking about it is: if we needed to draw a glyph that was on a line just next to the plane of the camera, we'd be very close to needing to draw a glyph that was just behind the plane of the camera, and we'd be hosed either way.
There was a problem hiding this comment.
👍 sounds legit to me -- thanks for spelling it out!
Adds special handling for (rare) case that line geometry projects to a point behind the plane of the camera.
Without the fix: - The "viewport-overzoomed" test would show "Figueroa St." jumbled in the lower half of the screen - The "viewport-overzoomed-single-glyph" test would show the letter "C" upside down
Adds special handling for (rare) case that line geometry projects to a point behind the plane of the camera. Fixes issue #5112.
Before merging the PR, I want to build tests for the two cases here (multiple glyph and single-glyph labels). In development, I've been testing with the coordinates @asheemmamoowala provided in #5112, and modifying the style to make single-glyph labels.
/cc @asheemmamoowala @ansis @kkaefer