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 missing ocean tiles #12279

Merged
merged 8 commits into from
Oct 18, 2022
Merged

Fix missing ocean tiles #12279

merged 8 commits into from
Oct 18, 2022

Conversation

avpeery
Copy link
Contributor

@avpeery avpeery commented Oct 5, 2022

Fixes #11975. Similar logic to PR #12203, but epsilon value is only applied when globe projection and to the left bound (see definition for mat4.ortho here)

Test with codesandbox on different devices, many thanks!

  • Already tested on Macbook Pro Monterey 12.6 (chrome, safari, firefox), and iPhone 12 Pro (Safari)

Launch Checklist

  • 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
  • manually test the debug page
  • 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 missing ocean tiles when globe is enabled.</changelog>

@avpeery avpeery added the skip changelog Used for PRs that do not need a changelog entry label Oct 5, 2022
@karimnaaji
Copy link
Contributor

@avpeery I haven't been able to reproduce from my machine with this new epsilon value. Before moving this forward, it would be worth doing two things:

@@ -260,7 +260,8 @@ export class Terrain extends Elevation {
this._sourceTilesOverlap = {};
this.proxySourceCache = new ProxySourceCache(style.map);
this.orthoMatrix = mat4.create();
mat4.ortho(this.orthoMatrix, 0, EXTENT, 0, EXTENT, 0, 1);
const epsilon = this.painter.transform.projection.name === 'globe' ? .015 : 0; // Experimentally the smallest value to avoid rendering artifacts (https://github.com/mapbox/mapbox-gl-js/issues/11975)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to check for terrain here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not seeing the missing tile with terrain (no globe) with this fix, but will test out on various devices! I also hit a failed render test when I removed the globe check (no significant difference though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a codesandbox with this change

@avpeery avpeery added bug 🐞 and removed skip changelog Used for PRs that do not need a changelog entry labels Oct 15, 2022
@avpeery avpeery marked this pull request as ready for review October 15, 2022 00:15
@avpeery avpeery requested a review from a team as a code owner October 15, 2022 00:15
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.

LGTM. Tested on both chrome iPhone se and macOS.

Copy link
Contributor

@SnailBones SnailBones left a comment

Choose a reason for hiding this comment

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

Looks good to me! Tested on Linux, Windows and Android.

@avpeery avpeery merged commit 01e539d into main Oct 18, 2022
@avpeery avpeery deleted the avpeery/fix-missing-ocean-tile branch October 18, 2022 00:10
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.

Draping artifacts on ocean tiles
3 participants