-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Description
If you look at a dense map in the distant field, you can find places where the collision boxes for two labels clearly overlap, but both labels are still showing. Look at "Kenneth Hahn State Recreational Area" and "VIEW PARK-WINDSOR HILLS":
The collision boxes themselves look right, and if you look at their rendering code, you'll see they adjust their size by:
highp float collision_perspective_ratio = 0.5 + 0.5 * (u_camera_to_center_distance / camera_to_anchor_distance); |
On the other hand, the code in CollisionIndex
is using the opposite ratio (p[3]
is the same as camera_to_anchor_distance
):
mapbox-gl-js/src/symbol/collision_index.js
Line 359 in 79f0a84
perspectiveRatio: 0.5 + 0.5 * (p[3] / this.transform.cameraToCenterDistance) |
Why is this not completely broken? Well, we end up dividing by the perspective ratio instead of multiplying by it:
mapbox-gl-js/src/symbol/collision_index.js
Lines 62 to 66 in 79f0a84
const tileToViewport = textPixelRatio * projectedPoint.perspectiveRatio; | |
const tlX = collisionBox.x1 / tileToViewport + projectedPoint.point.x; | |
const tlY = collisionBox.y1 / tileToViewport + projectedPoint.point.y; | |
const brX = collisionBox.x2 / tileToViewport + projectedPoint.point.x; | |
const brY = collisionBox.y2 / tileToViewport + projectedPoint.point.y; |
But 1/(.5 + .5 * y/x)
is not the same as (.5 + .5 * x/y)
! It is however a close enough approximation when x and y are relatively near each other that we didn't notice! 😅 😬
Dividing by tileToViewport
always should have felt fishy -- it turned out to mostly work because we inverted the textPixelRatio
as well. Inversions all the way down!
I've got a fix that un-inverts everything and makes "collision detection" + "collision debug boxes" + "actually rendered labels" all be consistent (and I believe correct against the expected "attentuates 50% of the effect of perspective scaling" behavior). I'm working through the associated test changes.
Same problem exists in gl-native.
/cc @ansis @jfirebaugh