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 1-pixel flickering between tiles on dark styles #12145

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

mpulkki-mapbox
Copy link
Contributor

@mpulkki-mapbox mpulkki-mapbox commented Aug 9, 2022

This pull request fixes the flickering issue between tiles (#11858) by extruding vertices of globe tiles slightly towards the neighbouring tiles. Based on empirical testing I found 0.5 pixels to be enough to stitch seams on all of my test devices using different pixel ratios. It should be noted that the texture sampling is unaffected even though tiles are scaled up by a tiny amount. Any possible extra fragments will have their uvs clamped to range [0, 1].

Fixes #11858

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
  • document any changes to public APIs
  • 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 pixel flickering between tiles on darker styles in globe view.</changelog>

@mpulkki-mapbox mpulkki-mapbox self-assigned this Aug 9, 2022
@mpulkki-mapbox mpulkki-mapbox force-pushed the mpulkki-fix-globe-tile-seams branch 2 times, most recently from 704f05c to 0c3f040 Compare August 9, 2022 08:55
@mpulkki-mapbox mpulkki-mapbox marked this pull request as ready for review August 9, 2022 08:56
@mpulkki-mapbox mpulkki-mapbox force-pushed the mpulkki-fix-globe-tile-seams branch 2 times, most recently from 329af1c to c6768ae Compare August 9, 2022 10:51
Copy link
Contributor

@akoylasar akoylasar left a comment

Choose a reason for hiding this comment

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

LGTM + some minor comments

const tileHeight = n - s;
const toUv = 1.0 / GLOBE_VERTEX_GRID_SIZE;
const x = tileWidth * toUv;
const y = -tileHeight / GLOBE_LATITUDINAL_GRID_LOD_TABLE[latitudinalLod];
Copy link
Contributor

Choose a reason for hiding this comment

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

how about toUvX and we could also refactor 1 / GLOBE_LATITUDINAL_GRID_LOD_TABLE(..) to toUvY

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 think it's unnecessary to have a separate variable toUv anymore

const xScale = padding / tileWidth + 1;
const yScale = padding / tileHeight + 1;
const padMatrix = [xScale, 0, 0, 0, yScale, 0, -0.5 * padding / x, 0.5 * padding / y, 1];

Copy link
Contributor

@akoylasar akoylasar Aug 10, 2022

Choose a reason for hiding this comment

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

The code here has now become more complicated, perhaps we could add a comment along the lines of:

// latLon = gridMatrix * (p_grid + scale * (p_grid - grid_center)) which implies
// latLon = grdiMatrix * (p_grid + scale * p_grid - scale * grid_center)
// and the part in parentheses is another transformation which results in:
// latLon = gridMatrix * padMatrix * p_grid

Copy link
Contributor Author

@mpulkki-mapbox mpulkki-mapbox Aug 10, 2022

Choose a reason for hiding this comment

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

This is fairly trivial transformation matrix composition. Perhaps I could add a comment or two about coordinate spaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed I was trying to capture the transformation from spaces as I think that might not be trivial at all for a person encountering it for the first time.

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.

Looks good to me! I wonder if we should add a more specific render test that has white gaps in pre-PR rendering?

@mourner mourner merged commit b58b38d into main Aug 15, 2022
@mourner mourner deleted the mpulkki-fix-globe-tile-seams branch August 15, 2022 15:25
@mpulkki-mapbox
Copy link
Contributor Author

Looks good to me! I wonder if we should add a more specific render test that has white gaps in pre-PR rendering?

The greenish looking render test globe-tile-padding is supposed to catch any gaps between tiles. It does not pass (at least locally) without this change.

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.

Potential 1-pixel flickering issue between tiles on darker styles when using globe
3 participants