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 tile cover during globe-to-Mercator transition #12137

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

SnailBones
Copy link
Contributor

Closes https://github.com/mapbox/gl-js-team/issues/443

Follow up to #11988 and #12075.

Solves several issues with #11988, most critically an issue in mercatorTileCornersInCameraSpace where camera position in Mercator now multiplied by mercatorScale, now correctly multiplied bynumTiles .

I've verified that this function is working as intended with the AABB visualization introduced in #12131
image

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix occasional missing tiles at bottom of screen during globe-Mercator transition</changelog>

@SnailBones SnailBones changed the title Fix tile cover during transition Fix tile cover during globe-to-Mercator transition Aug 4, 2022
@SnailBones SnailBones requested a review from mourner August 4, 2022 23:00
@akoylasar
Copy link
Contributor

is there a test that validating the fix?

@SnailBones
Copy link
Contributor Author

SnailBones commented Aug 9, 2022

is there a test that validating the fix?

@akoylasar Yes, the test here validates the fix. Previously this test was ignored since tiles were missing at the bottom (see #11988).

@avpeery
Copy link
Contributor

avpeery commented Aug 13, 2022

I am seeing that tile coverage at bottom of globe is resolved! However, tile coverage missing during mercator-to-globe transition at the north pole still:
Screen Shot 2022-08-12 at 5 11 59 PM

Screen Shot 2022-08-12 at 5 13 36 PM

Copy link
Contributor

@avpeery avpeery left a comment

Choose a reason for hiding this comment

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

LGTM, tested a bunch with the AABB visualizer tool and fixes the issue and adds the render test! Nice work @SnailBones! As mentioned, I noticed a pole coverage issue near poles; however, may not be related to the fix in this PR, but maybe should confirm if it could be fixed here? Otherwise, looks good

@SnailBones
Copy link
Contributor Author

Many thanks for the thorough testing @avpeery! Since #12167) is an issue in pole coloring, not tile coverage, I think we're safe to merge in this PR and should address that separately.

@avpeery avpeery self-requested a review August 15, 2022 18:27
Copy link
Contributor

@avpeery avpeery left a comment

Choose a reason for hiding this comment

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

Sounds good @SnailBones !

@SnailBones SnailBones merged commit 31936f3 into main Aug 15, 2022
@SnailBones SnailBones deleted the aidan/globe-tile-transition branch August 15, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants