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 visible grid around tiles #10594

Merged
merged 6 commits into from
Dec 18, 2024
Merged

Fix visible grid around tiles #10594

merged 6 commits into from
Dec 18, 2024

Conversation

Nekzuris
Copy link
Contributor

@Nekzuris Nekzuris commented Dec 11, 2024

This properly fixes #10589 by removing the tile overlap that was causing white borders with transparent tiles.

The tile overlap was added as a dirty fix to #3053, it doesn't even fully resolved the issue anyway.
This fix uses the CSS rule mix-blend-mode: plus-lighter, applied only to the background layer and is only needed to fix this bug in Chrome.
A future version of Chrome might make this CSS rule unnecessary (but leaving it shouldn't cause the borders to reappear).

Tested on Chrome and Firefox with normal and fractional zoom levels, with regular and transparent layers.
Not tested on macOS Safari or mobile.

@tyrasd tyrasd added the map-renderer An issue with how things are rendered in the map label Dec 11, 2024
tyrasd
tyrasd previously requested changes Dec 11, 2024
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

Hmm… Nice idea, but there are still corner cases where it does not work: To reproduce on Chrome 129 on Ubuntu, set browser zoom to 110%:

image

And, more importantly, now even non-transparent tile layers show the seam under the above mentioned conditions (this works fine on the development branch):

image


I think the +epsilon workaround is still needed to account for rounding errors associated with fractional browser zoom settings.

To improve the rendering of the small overlap for tile layers with transparency, I tried to play around with a few other combinations of CSS blend modes, background values, etc. but could unfortunately not find a solution that preserves the rendering of the non transparent tiles. 😒

As transparent imagery layers are a rather rare occurence1, I'm leaning towards closing this as an unfortunate wontfix / known bug. Except if you or someone is able to find a better solution?!

Footnotes

  1. some overlays use an alpha channel, but as long as it is only used as a mask ("1 bit transparency"), these also work fine with the current solution.

@Nekzuris
Copy link
Contributor Author

It is unfortunate that it doesn't work for 110% zoom in Chrome but it's fine with 125%, 150%, 175%, 200%.

The current solution with +epsilon to scale up by 1px and create an overlap is fundamentally flawed because it messes with tiles sharpness and will never work for transparent layers.
It's also only needed to fix a rendering bug in Chrome and doesn't even completely fix the issue at regular zoom levels and 100% browser zoom: https://www.openstreetmap.org/edit#map=16/48.36450/7.14242&background=MAPNIK
image

Value 90% and 80% are also problematic with both epsilon or plus-lighter, but 75% is fine.
So with my testing, the +epsilon only improves the situation for 110% in Chrome.
While removing it in favor of plus-lighter :

  • fixes everything at 100%
  • makes perfect zooming transitions
  • supports transparent layers
  • but slightly degrades experience at 110% in Chrome

The solution mix-blend-mode: plus-lighter was also considered the best by Leaflet#8891

I found that not applying plus-lighter at 110% improves the situation, as well as 80% and 67%.
90% is still bad in Chrome and might be improved with specific epsilon value in JS, but I don't think that's a good idea.

@Nekzuris Nekzuris requested a review from tyrasd December 12, 2024 13:32
@Nekzuris
Copy link
Contributor Author

Nekzuris commented Dec 12, 2024

I tried to compile in a comprehensive way everything I've tested to remove the grid:
0.002 normal is the current solution
0 plus-lighter is the Leaflet solution
0 normal is an exception (only included if plus-lighter isn't perfect)

Firefox

Browser zoom epsilon mix-blend-mode normal zoom fractional zoom zoom transition transparent layer
any 0.002 normal ✔️ ✔️
any 0 normal or plus-lighter ✔️ ✔️ ✔️ ✔️

Chrome

Browser zoom epsilon mix-blend-mode normal zoom fractional zoom zoom transition transparent layer
50% 0.002 normal
0.006 normal ✔️ ✔️
0 plus-lighter ✔️ ✔️ ✔️ ✔️
67% 0.002 normal ✔️ 〰️
0.003 normal ✔️ ✔️
0 plus-lighter or normal *
75% 0.002 normal 〰️
0.004 normal ✔️ ✔️
0 plus-lighter ✔️ ✔️ ✔️ ✔️
80% 0.002 normal 〰️ 〰️
0.003 normal ✔️ ✔️
0 plus-lighter
0 normal 〰️ 〰️ 〰️ 〰️
90% 0.002 normal
0.005 normal 〰️ 〰️
0 plus-lighter or normal
100% 0.002 normal 〰️ 〰️
0.003 normal ✔️ ✔️
0 plus-lighter ✔️ ✔️ ✔️ ✔️
110% 0.002 normal ✔️ ✔️
0 plus-lighter
0 normal 〰️ 〰️ 〰️ 〰️
125% 0.002 normal 〰️ 〰️
0.003 normal ✔️ ✔️
0 plus-lighter ✔️ ✔️ ✔️ ✔️
150-400% 0.002 normal ✔️ ✔️
0 plus-lighter ✔️ ✔️ ✔️ ✔️
* normal is a bit better 〰️ means okayish

TLDR: This PR with 3 CSS exceptions is a clear improvement over the current solution, the only annoying regression is 110% in Chrome but it's minor. We could go further with more logic in JS to keep epsilon in some cases but I don't like it.

@Nekzuris
Copy link
Contributor Author

Visual before vs after

current zoom 100%

zoom.old.100.mp4

new zoom 100%

zoom.new.100.mp4

current zoom 110%

zoom.old.110.mp4

new zoom 110%

zoom.new.110.mp4

I chose Carto because it's where the grid is most noticeable.

Here is what it looks like with real imagery at zoom 18

current vs new 100%

(click for full resolution)

current vs new 110%

The thin black lines currently visible at 100% are now at 110%.

I think most users will only really notice and appreciate the new flawless zoom transition.

@tyrasd tyrasd dismissed their stale review December 13, 2024 14:36

addressed in follow-up comments (see above)

@tyrasd
Copy link
Member

tyrasd commented Dec 13, 2024

Thanks for the in-depth analysis and detailed tests!

The current solution with +epsilon to scale up by 1px and create an overlap is fundamentally flawed because it messes with tiles sharpness and will never work for transparent layers.

Good point about the loss in sharpness with the existing workaround. I'm tending towards accepting the 0 plus-lighter implementation, but what do you think about using a dynamic epsilon? That is only set to a non-zero value when the workaround is really "needed": i.e. Chromium-based browser and window.devicePixelRatio is one of the "problematic" values.

@Nekzuris
Copy link
Contributor Author

I did my initial test with all the logic in the tileSizeAtZoom function, but it had a noticeable performance impact. I thought this function was only called for rendering tiles, but it's called hundreds of times even when the data layer is hidden, not sure if that's normal.

This is a better implementation, but maybe you have an even better idea.
And unlike the exceptions in CSS, this will still prevent good zooming transitions and transparent layers when the chromium bug is fixed.

@Nekzuris

This comment was marked as resolved.

@Nekzuris
Copy link
Contributor Author

While testing for Leaflet here, I found that plus-lighter can introduce some blur on Chrome, but it's nothing to worry about because iD is already blurry all the time due to sub-pixel translations.
Sub-pixel precision is apparently needed (see #2849), and trying to fix it for the map would create a noticeable sharpness difference when switching between normal and fractional zoom levels.

Again, this is only an issue on Chrome, Firefox handles both sub-pixel translations and plus-lighter just fine.

@Nekzuris
Copy link
Contributor Author

After further testing with non-standard pixel ratios, I found that the plus-lighter trick only works with zoom levels that are multiples of 25 (75%, 100%, 125%...).

So instead of a few exceptions, I changed it to be applied only at those specific pixel ratios where it’s been confirmed to work well.
And for all the other case we still use the dynamic epsilon with 0.003 by default.

@tordans
Copy link
Collaborator

tordans commented Dec 18, 2024

Just to put this out there: We could also decide to not support zoomed in UIs and leave it at that.
Or, consider adding a notice for zoomed in UIs for those users that zoom in but forgot about it.
TBH I am not sure it the added complexity of the workarounds and their added maintenance burden are worth it compared to the issue and how many users are likely affected (not many).

@Nekzuris
Copy link
Contributor Author

I originally made this PR to support transparent tiles and improve zoom transitions (which are currently awful for everyone), and fix the thin black lines visible with light backgrounds on Chrome.

The situation seems complex due to all the incremental improvements, but I think the final solution isn't that hard to understand.

If you ignore all other pixel ratios, simply remove epsilon and apply plus-lighter with no condition. That would improve things for the majority, but it would certainly be a regression for some.

There are many valid reasons to not have your zoom set to 100%, especially with high DPI screens, which are much more common now.

This mess is entirely caused by poor rendering in Chrome, and the only maintenance I foresee is removing all this when the bug is fixed.

Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

Thanks again! This seems to be the best we can do for now.

Below, I've added a few inline links to this PR for future reference.

css/80_app.css Outdated Show resolved Hide resolved
modules/renderer/tile_layer.js Show resolved Hide resolved
@tyrasd tyrasd merged commit 1a7cf41 into openstreetmap:develop Dec 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map-renderer An issue with how things are rendered in the map
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Borders around tiles at specific zoom levels
3 participants