-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(carto): Support picking with MVT tile-relative coordinates #8926
fix(carto): Support picking with MVT tile-relative coordinates #8926
Conversation
Let's hold this PR until next week, to get a review from @felixpalmer as well. |
@@ -151,6 +151,12 @@ export default class VectorTileLayer< | |||
getPickingInfo(params) { | |||
const info = super.getPickingInfo(params); | |||
|
|||
// MVT tiles use tile-relative coordinates, handled by MVTLayer#getPickingInfo. | |||
if (this.state.mvt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this fixes the issue, I notice that it code path below will mean that binaryToGeojson
is invoked twice. We could do better, either by invoking super.super.getPickingInfo
here (https://stackoverflow.com/a/49056678) or modifying the MVTLayer
to move isWGS84
to state and overriding it in VectorTileLayer
. @donmccurdy could you give this a try in a follow-up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, thanks! Opened a followup PR in #8932.
49a78e7
to
b76cc26
Compare
Changes:
test/modules/carto/layers/vector-tile-layer.spec.ts
test/modules/carto/layers/carto-tile-layer.spec.ts
Related: