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 querySourceFeatures returning bad results on zooms past maxZoom #7061

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 1, 2018

Fixes #2868. Fixes #3722.

I also removed the reparseOverscaled source option because it doesn't seem useful — we always want to reparse overscaled tiles for correct layout values, and we don't need to worry about performance because parsing overscaled tiles is very fast anyway (there's only a few on the screen and they're small).

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mourner mourner requested a review from ansis August 1, 2018 11:23
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

The coordinate fix looks good!

@@ -248,7 +246,7 @@ class Transform {
this.pointCoordinate(new Point(this.width, this.height), z),
this.pointCoordinate(new Point(0, this.height), z)
];
return tileCover(z, cornerCoords, options.reparseOverscaled ? actualZ : z, this._renderWorldCopies)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work for raster sources past their max zoom level? I think the previous behaviour was that it used the existing tiles from the max zoom level. Does this end up creating new tiles?

@mourner mourner force-pushed the no-reparse-overscaled branch from 6222c85 to 326eed6 Compare August 2, 2018 16:45
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

👍

@mourner mourner merged commit 3ffa20a into master Aug 2, 2018
@mourner mourner deleted the no-reparse-overscaled branch August 2, 2018 17:05
@mourner
Copy link
Member Author

mourner commented Aug 2, 2018

I also noticed (after the PR) that #4990 inadvertently contains the same fix. Sorry for not noticing earlier @asheemmamoowala!

@ralphking
Copy link

ralphking commented Aug 13, 2018

I'm not sure that this fixes #3722

it sets reparseOverscaled to true, which seemingly does the opposite as the geometries coming back at zoom levels greater than 16 are still wrong

pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 18, 2019
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.

3 participants