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

Disable sky after maximum projection transition to mercator #11215

Merged
merged 5 commits into from
Nov 4, 2021

Conversation

karimnaaji
Copy link
Contributor

@karimnaaji karimnaaji commented Nov 4, 2021

For non-mercator projection, this PR disables the sky layer after the end range of all projections that aren't mercator (as suggested in #11124 (comment)). This ensures that the sky is disabled before the end of sheer adjustment range where projections are set to mercator at high zoom levels.

Fixes #11207

The motivation is to give more thoughts on whether we could use it as a background fill for empty projection space. Some options that we should consider then:

  • Use a gl.clear using the background color to fill space where tiles aren't drawn
  • Leverage the sky layer below the horizon line for that use case
  • Transition to alpha = 0 below the horizon line when a sky is available (as mentioned by @arindam1993)
  • Some other options?

Launch Checklist

  • briefly describe the changes in this PR
  • document any changes to public APIs
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

@karimnaaji karimnaaji added the skip changelog Used for PRs that do not need a changelog entry label Nov 4, 2021
@karimnaaji karimnaaji marked this pull request as ready for review November 4, 2021 00:34
@arindam1993
Copy link
Contributor

This looks good, but I think we need to add in this
a9ddf15
to prevent this #11207 , cauze there might still be situations where the frustum culling conflicts causing flickering in and out.

Copy link
Contributor

@arindam1993 arindam1993 left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

src/geo/transform.js Outdated Show resolved Hide resolved
@karimnaaji karimnaaji merged commit 92284d3 into main Nov 4, 2021
@karimnaaji karimnaaji deleted the karim/projection-sky branch November 4, 2021 21:53
ansis pushed a commit that referenced this pull request Nov 4, 2021
* Disable sky below maximum projection transition to mercator

* Revert change

* Better naming

* Apply Ansis suggestion

* Renaming
SnailBones pushed a commit that referenced this pull request Nov 5, 2021
* Disable sky below maximum projection transition to mercator

* Revert change

* Better naming

* Apply Ansis suggestion

* Renaming
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Sky behaviour when projection is set
3 participants