Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full screen symbol flicker on pitched map with text-translate #6363

Closed
jfirebaugh opened this issue Mar 19, 2018 · 2 comments
Closed

Full screen symbol flicker on pitched map with text-translate #6363

jfirebaugh opened this issue Mar 19, 2018 · 2 comments
Labels
Milestone

Comments

@jfirebaugh
Copy link
Contributor

flicker

http://jsbin.com/hinivotoqi/edit?html,output

Rotate / pan / zoom enough (mostly rotating), and you should see full-screen flashes the same color as the text. The presence of the text-translate property seems to be key -- without it, I haven't been able to reproduce any flickering.

cc @malwoodsantoro @ChrisLoer

@ChrisLoer
Copy link
Contributor

I put in a fix for behavior very similar to this in #6060 (I just saw the behavior as I was testing, this is different from #6041 which was the main object of that PR):

highp float collision_perspective_ratio = clamp(
0.5 + 0.5 * (u_camera_to_center_distance / camera_to_anchor_distance),
0.0, // Prevents oversized near-field circles in pitched/overzoomed tiles
4.0);

In that case, what was happening was that for a long vertically-oriented label, the perspective ratio was calculated based on the anchor, but the actual circle was well above the anchor. So when the anchor was way below the screen, the circle would get a giant perspective ratio, and since it was closer to the viewport it was able to actually expand to fill the viewport.

In this case, I think what's happening is that the translation is getting exaggerated for anchor points that are well below the bottom of the viewport, because pixelsToTileUnits is only an approximation once you're pitched (say you're at the bottom of the viewport on a 60 degree pitched map, and the tiles are currently showing at their ideal zoom level: you'll get a pixel to tile unit ratio of 8, but in fact the ratio at that point will be much less).

gl.uniformMatrix4fv(program.uniforms.u_matrix, false, painter.translatePosMatrix(coord.posMatrix, tile, translate, translateAnchor));
const s = pixelsToTileUnits(tile, 1, painter.transform.zoom);
const labelPlaneMatrix = symbolProjection.getLabelPlaneMatrix(coord.posMatrix, pitchWithMap, rotateWithMap, painter.transform, s);
const glCoordMatrix = symbolProjection.getGlCoordMatrix(coord.posMatrix, pitchWithMap, rotateWithMap, painter.transform, s);
gl.uniformMatrix4fv(program.uniforms.u_gl_coord_matrix, false, painter.translatePosMatrix(glCoordMatrix, tile, translate, translateAnchor, true));

u_matrix gets a translation based on the pixelsToTileUnits approximation, while u_gl_coord_matrix gets a translation done directly in viewport units (that 'true' passed into translatePosMatrix means inViewportUnits). In the shader, the perspective ratio is based off of u_matrix (the anchor), but the actual layout of the glyphs is done with u_gl_coord_matrix. This discrepancy is what allows the calculation to go berserk (the giant perspective ratio happens as you get close to the plane of the camera, but as long as everything is in sync, it will never grow large enough to enter the viewport).

I just looked back to my discussion with @ansis about this in January, and found this prophetic quote:

But I’m wondering why we don’t ever see that cause a problem with other shaders on pitched/overzoomed tiles (or whether maybe we do, it’s just hard to notice). And then I’m theorizing that it just doesn’t come up for point labels because the y value of the anchor is usually not that far off from the y value of the glyph center.

I think the best fix may be just to clamp the perspective ratio every time we generate it in a shader. It's a weird kind of late-stage fix, but it has the advantage of being easy to reason about.

ChrisLoer added a commit that referenced this issue Mar 20, 2018
This fixes issue #6363, but also guards against a class of bugs:

We previously relied on the assumption that the perspective ratio was calculated with the same anchor point as the item we were drawing. If that assumption was violated, it was possible for overzoomed items far outside of the viewport to grow larger faster than they moved away from the viewport. They could eventually grow large enough to cover the viewport entirely, causing brief "flashes" during map animations.

This commit does _not_ clamp the perspective ratios on the CPU side, to avoid extra work in the inner loop of very performance sensitive collision/projection code. On the collision side, anything far outside of the viewport should be collided/ignored anyway, and on the projection side, we have a filter requiring that the anchor of the label be within the viewport before the projection logic begins.
ChrisLoer added a commit that referenced this issue Mar 20, 2018
This fixes issue #6363, but also guards against a class of bugs:

We previously relied on the assumption that the perspective ratio was calculated with the same anchor point as the item we were drawing. If that assumption was violated, it was possible for overzoomed items far outside of the viewport to grow larger faster than they moved away from the viewport. They could eventually grow large enough to cover the viewport entirely, causing brief "flashes" during map animations.

This commit does _not_ clamp the perspective ratios on the CPU side, to avoid extra work in the inner loop of very performance sensitive collision/projection code. On the collision side, anything far outside of the viewport should be collided/ignored anyway, and on the projection side, we have a filter requiring that the anchor of the label be within the viewport before the projection logic begins.
@ChrisLoer ChrisLoer added this to the 0.45.0 milestone Mar 20, 2018
@ChrisLoer
Copy link
Contributor

Fixed in #11487.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants