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

Fix incorrect textPixelRatio argument #6135

Merged
merged 2 commits into from
Feb 13, 2018
Merged

Fix incorrect textPixelRatio argument #6135

merged 2 commits into from
Feb 13, 2018

Conversation

anandthakker
Copy link
Contributor

Closes #6075

textPixelRatio is used to scale tile coordinates to viewport coordinates; the value that was being passed here, EXTENT / tileSize, was the reciprocal of the ratio needed -- we were passing 16 instead of 0.0625.

As a result, the corners of the viewport-projected collision box that we reconstructed here,

const x1 = blocking.x1 * tileToViewport + projectedPoint.point.x;
const y1 = blocking.y1 * tileToViewport + projectedPoint.point.y;
const x2 = blocking.x2 * tileToViewport + projectedPoint.point.x;
const y2 = blocking.y2 * tileToViewport + projectedPoint.point.y;
, were 16^2 times farther from the anchor than they should have been. With icon-offset: [0, 0], this didn't matter, because the incorrectly large box would contain the (much smaller) correct box. But when there's an offset, it too gets scaled too much, and so we run out of luck.

Anand Thakker added 2 commits February 12, 2018 16:30
Closes #6075

`textPixelRatio` is used to scale tile coordinates to viewport
coordinates; the value that was being passed here, EXTENT / tileSize,
was the reciprocal of the ratio needed -- we were passing 16 instead of
0.0625.

As a result, the corners of the viewport-projected collision box that we
reconstructed here,
https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325,
were 16^2 times farther from the anchor than they should have been. With
`icon-offset: [0, 0]`, this didn't matter, because the incorrectly large
box would contain the (much smaller) correct box.  But when there's an
offset, it too gets scaled too much, and so we run out of luck.
Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ Good catch @anandthakker. This textPixelRatio has always been incorrect, but it used to be masked by us inverting the tileToViewport parameter. When I fixed that in #5913, I missed this.

@ChrisLoer
Copy link
Contributor

Although this change is the simplest fix, it's worth noting that on gl-native we skipped this issue entirely by storing viewport coordinates in the GridIndex and just comparing the viewport-projected query directly against those coordinates instead of re-projecting tile coordinates the way we do in gl-js.

@anandthakker anandthakker merged commit db66c68 into master Feb 13, 2018
@anandthakker anandthakker deleted the fix-6075 branch February 13, 2018 12:38
pathmapper pushed a commit to pathmapper/mapbox-gl-js that referenced this pull request Feb 13, 2018
* Add failing test for issue mapbox#6075

* Fix incorrect textPixelRatio argument

Closes mapbox#6075

`textPixelRatio` is used to scale tile coordinates to viewport
coordinates; the value that was being passed here, EXTENT / tileSize,
was the reciprocal of the ratio needed -- we were passing 16 instead of
0.0625.

As a result, the corners of the viewport-projected collision box that we
reconstructed here,
https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325,
were 16^2 times farther from the anchor than they should have been. With
`icon-offset: [0, 0]`, this didn't matter, because the incorrectly large
box would contain the (much smaller) correct box.  But when there's an
offset, it too gets scaled too much, and so we run out of luck.
pathmapper pushed a commit to pathmapper/mapbox-gl-js that referenced this pull request Feb 13, 2018
* Add failing test for issue mapbox#6075

* Fix incorrect textPixelRatio argument

Closes mapbox#6075

`textPixelRatio` is used to scale tile coordinates to viewport
coordinates; the value that was being passed here, EXTENT / tileSize,
was the reciprocal of the ratio needed -- we were passing 16 instead of
0.0625.

As a result, the corners of the viewport-projected collision box that we
reconstructed here,
https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325,
were 16^2 times farther from the anchor than they should have been. With
`icon-offset: [0, 0]`, this didn't matter, because the incorrectly large
box would contain the (much smaller) correct box.  But when there's an
offset, it too gets scaled too much, and so we run out of luck.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants