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

Revert tile loading changes from #11988 #12075

Merged
merged 7 commits into from
Aug 2, 2022
Merged

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented Jul 8, 2022

Fixes #12069

Reopens https://github.com/mapbox/gl-js-team/issues/443 #1

EDIT2: I've updated this PR to revert the changes in #11988. The new test remains to verify that we're not introducing a regression, while I've added the old test to ignores. I'm working on a more robust and performant solution to that issue to be included in the following release.

This issue: When a tile in globe-to-mercator transition is placed such that the bottom tile corners are just above the view, but the bottom edge of the tile intersects with the view (this is especially possible at extreme lattitudes due to the increasingly curved latitude lines/tile boundaries), the tile is not rendered.

Logic to handle this in Globe by calculating the bounding box of the arc was introduced in #11508. This bug was introduced by an and an attempt to support this in the transition in #11988 (ported from https://github.com/mapbox/mapbox-gl-native-internal/pull/3450) by interpolating the arc extremum toward the midpoint of the corresponding edge in Mercator.

The issue is probably that the arc extremum is always interpolated toward the center of the tile edge. The extreme values of the arc only correspond with the center in the case that a tile edge is centered in the viewport.

As the logic is the same in @mapbox/gl-native, this issue likely occurs there too.

This can be fixed by taking the maximum values between the Mercator midpoint and arc extremums instead of averaging the two.

EDIT: #11988 is actually not working as intended by using interpolated tile corners to generate the AABB. Correcting this results in inadequate tile coverage, likely from an issue of mismatched units (possibly related to the changes to pixelSpaceConversion/_transform._pixelsPerMercatorPixel in #11951, which has not yet been ported to Native.)

Previous behavior on left, behavior after fix on right:

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/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'

@SnailBones SnailBones added the skip changelog Used for PRs that do not need a changelog entry label Jul 8, 2022
@SnailBones SnailBones changed the title Use max instead of interpolating arc midopint Fix missing tiles when corners are offscreen during globe transition Jul 11, 2022
@SnailBones SnailBones marked this pull request as draft July 11, 2022 22:09
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

👍

@mourner
Copy link
Member

mourner commented Jul 12, 2022

Looks like the added render test is drawn slightly differently — not sure if it's antialiasing difference between platforms or something else, but we can probably bump the diff threshold since for the failing case, it would be a strong 0.3 difference anyway.

@SnailBones SnailBones changed the title Fix missing tiles when corners are offscreen during globe transition Revert tile loading changes from #11988 Aug 2, 2022
@SnailBones SnailBones marked this pull request as ready for review August 2, 2022 18:22
@SnailBones SnailBones requested a review from mourner August 2, 2022 18:42
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

Approving to unblock the release, I'll reopen https://github.com/mapbox/gl-js-team/issues/443. We should double check whether #11988 was missing something from the port from native to confirm.

@karimnaaji karimnaaji merged commit dcc5a2b into main Aug 2, 2022
@karimnaaji karimnaaji deleted the aidan/globe-tile-cover-fix branch August 2, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 release blocker ⛔ skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing tiles at the top of the screen in globe view
3 participants