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

Align projection matrix to pixel grids to draw crisp raster tiles #6026

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Jan 19, 2018

This changes our projection matrix to be aligned to the pixel grid to fix blurry raster tiles at integer zoom levels (#4552).

While this change works great

before:
expected

after:
actual

and produces the same level of crispness as a Leaflet map of the same raster tiles, it also changes the behavior of some of our APIs: it is no longer possible to roundtrip a point through our transform object and get the same result back, since all transformations are now aligned to this pixel grid. It also slightly shifts rendering of all other items (shapes, lines, symbols) by up to half a pixel, which results in hundreds of rendering "errors".

One way to solve this would be to use the pixel-aligned projection matrix only for raster (and terrain?) tiles. However, this means that our raster tiles might be shifted by up to half a pixel compared to the rest of the data we are drawing on the screen. In reality, I doubt that this will be noticeable, but something we should consider before merging this.

A notable discovery is that pixel alignment of our map depends on whether the viewport has even or odd dimensions.

@kkaefer
Copy link
Member Author

kkaefer commented Jan 20, 2018

Update: changed this PR so that it only pixel-grid-aligns raster tiles. On my machine, this produces perfectly crisp output on regular and retina screens in all browsers and for all viewport dimensions.

@kkaefer
Copy link
Member Author

kkaefer commented Jan 20, 2018

Testing-wise, it'd be cool to have a few people with different GPUs test this change. For this purpose, I've checked in a http://localhost:9966/debug/raster-streets.html#3/0/0 file. Use (shift-)double-click to zoom in(out) to remain on integer zoom levels (or manually manipulate the hash).

@kkaefer kkaefer force-pushed the blurry-raster-tiles branch from 918f7ec to e5dbcd4 Compare January 20, 2018 00:44
@kkaefer
Copy link
Member Author

kkaefer commented Jan 20, 2018

Also added the ability to draw crisp rasters at 90 and 270 degrees rotation + tests.

Copy link
Contributor

@lbud lbud left a comment

Choose a reason for hiding this comment

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

Looks sharp to me! (GPU: Intel Iris Graphics 550 1536 MB)

if (this._posMatrixCache[posMatrixKey]) {
return this._posMatrixCache[posMatrixKey];
const cache = aligned ? this._alignedPosMatrixCache : this._posMatrixCache;
if (!aligned && cache[posMatrixKey]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 would we not want to use the alignedPosMatrixCache result if it's available?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good catch. That’s an artifact from a different approach I tried.

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

Nice fix!

@ryanbaumann
Copy link
Contributor

cc @pdgoodman @aag6z

@ibesora
Copy link
Contributor

ibesora commented Jan 23, 2018

Amazing work @kkaefer
Time to ditch Leaflet!

@jaapster
Copy link

Thanks @kkaefer!

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.

6 participants