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

Globe: Tile borders fix #4868

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kubapelc
Copy link
Collaborator

The aim of this PR is to fix #4799 and #4803. In its current state, it fixes both issues, but I have uncovered other related problems.

Both issues are fixed by simply re-enabling stencil testing for the fill layer, which causes fill geometry to be clipped to the bounds of its tile. This however introduces seams in the ocean at zoom levels around 7:

Seams in the ocean

What is happening is that the ocean geometry is a simple quad, but the stencil mask is a curved shape that extended above the ocean quad (because it's min granularity was set to 4), and the ocean's further edge lies below the stencil mask edge, leaving some pixels above it still marked in stencil as belonging to this tile, but not filling them with pixels.

These seams are then simply fixed by tweaking the stencil mask granularity so that it is never higher than fill granularity. This however introduces other, more subtle artifacts:

More subtle seams

These are in turn caused by the ocean geometry being more detailed than the stencil mask. These seams are generated around shores and islands, where the ocean geometry has more vertices, causing it to be more curved than the (now flat) stencil mask. So the ocean is projected above the stencil mask, causing pixels below it to not be filled. These pixels cannot be filled by the nearer ocean tile (which presumably has the same curvature at the given edge), because that tile's stencil mask does not cover these pixels.

I tried fixing this by generating extra geometry at tile edges that extends to the planet's center, but this is also problematic, because there is no good way to determine what geometry is actually the tile's edge, because vector tiles tend to have extra border geometry beyond the regular tile extends. I'm not sure a heurestic would do here.

The original approach of not using stencil at all is also not good, because a tranparent fill layer could be drawn two or more times near tile edges due to this border geometry.

I don't yet know how to best solve this, so I am making this a draft PR for now.

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.65%. Comparing base (92b86d5) to head (9be9834).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4868      +/-   ##
==========================================
- Coverage   87.77%   86.65%   -1.12%     
==========================================
  Files         265      265              
  Lines       37946    37945       -1     
  Branches     2407     2548     +141     
==========================================
- Hits        33306    32881     -425     
- Misses       3579     3946     +367     
- Partials     1061     1118      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Collaborator

HarelM commented Oct 21, 2024

Does changing the granularity can cause a performance issue?
Otherwise, looks good.

@HarelM HarelM requested a review from ibesora October 21, 2024 13:07
@kubapelc
Copy link
Collaborator Author

Does changing the granularity can cause a performance issue?

In theory it can, but stencil granularity got decreased from 4x4 quad grid to 1x1 grid, it is now drawing 2 instead of 32 triangles per tile. But both meshes are so tiny that it probably has no effect.

@kubapelc
Copy link
Collaborator Author

The problem with solving this is that in globe rendering we can't simply use stencil masks to mark some pixels as belonging to a given tile and only render the tile to those pixels, because the geometry of the stencil mask will in general never match the geometry of the underlying fill layer. Either the mask is more tesselated than the fill layer (first imagethe PR) which causes gaps at the ends of tiles, or the stencil mask is less tesselated than the fill layer (second image the PR - this seems like a very common occurence, fill layers tend to be complex).

The old solution was not to use stencil at all for fill layers, but that lead to the ocean stripes in the issue. Worse, it would possibly lead to some translarent layers being drawn twice at tile borders in some maps. So the old solution should not be used.

There are 3 possible solutions, I think:

  1. do not draw fill layers with the tile extents in the stencil mask: instead use stencil to make sure any pixel is drawn over at most one by a given fill layer, thus fixing the double transparency problem and also ensuring there are no seams between tiles by always drawing all of their border geometry. This will not work with the facebook tiles in Ocean stripes over continent #4799 though.

The problem is that the facebook tiles rely on maplibre (or possibly mapbox) cutting off tile geometry that is beyond the normal extend at some point, and they place some extra stripes beyond this point. But the cutoff point is not defined anywhere in the spec.

  1. use stencil masks, but make sure both stencil masks and fill layers are tesselated enough for seams not to appear between tiles. This would slow down tile loading somewhat, since every zoom level would now need to be subdivided to granularity of around 8 (zoom levels 7 and above skip subdivision entirely as of now). I tried interacting with the map and it seems this subdivision does not slow tile loading down noticeably, but I haven't done any proper measaurement yet. I think it might not be too bad, since most fill layer geometry is very fine and triangles that are not intersected by the subdivision grid are just copied, so most triangles should hit this fast path. This is also the simplest solution, since only two numbers would need to be changed.

  2. Do solution 1, but programatically clip tile geometry somewhere beyond tile borders to fix facebook's tiles. This is something I would need to implement and it would introduce extra processing during loading of tiles. I'm not sure about the performance impact.

I'm not sure what solutuon to pick. Option 1 is definitely the most performant, and I tend to think option 2 is the best, since it is the simplest. I will try to make a benchmark to test the performance of 2 on a real world map.

@HarelM
Copy link
Collaborator

HarelM commented Oct 26, 2024

@ibesora see if you can help here, I'm not sure I fully understand the possible solution to this or deeply understand the problem.

@kubapelc
Copy link
Collaborator Author

I've benchmarked option 2 (set minimal subdivision of all fill to 4, which should be enough to fix the seams). Loading MapTiler map of a city at zoom 11 takes on average:

  • 2602 ms (current state, no subdivision at this zoom level)
  • 2696 ms (option 2, subdivision granularity 4)

@birkskyum birkskyum added the globe Globe related issues label Nov 1, 2024
@birkskyum birkskyum added this to the 5.0 milestone Nov 5, 2024
@kubapelc
Copy link
Collaborator Author

I think our options are to either do option 1) and somehow convince Facebook not to include invalid geometry beyond the borders of their tiles, or do 2) and suffer a hit to tile load times at high zooms everywhere where globe will be used. Option 3 would also likely result in comparable performance hit as option 2.

Also there is an implicit option 4: do nothing. Facebook tiles remain broken, there is still a visible border around tiles, tile loading performance does not degrade, stencil for fill layers remains disabled, possibly leading to overdraw issues in the future.

@HarelM
Copy link
Collaborator

HarelM commented Nov 11, 2024

Is it possible to introduce some flag to support this fix? This way the user has control if they need accuracy or performance...?
What do you mean by doing nothing? Does it mean we keep the stripes on the ocean bugs?

@kubapelc
Copy link
Collaborator Author

What do you mean by doing nothing? Does it mean we keep the stripes on the ocean bugs?

I mean not merging this PR. It would mean we keep the stripes, but they will only appear in that one tile source from Facebook that is used in the original issue #4799 We would not introduce the seams round Alaska that are visible on the screenshots in the PR post.

Is it possible to introduce some flag to support this fix?

It is, but it would need to be a flag on the source.

This is overall a pretty hard problem to solve. Doing option 2 or 3 (preprocessing tiles either by removing geometry beyond tile borders or by subdivision) slows down map loads. I was thinking about fixing this with some creative stencil usage (similar to option 1) instead, but it has to have the following properties:

  • A tile mask with extended borders is drawn, any given tile only fills pixels inside this mask. Note that neighbouring tile masks will overlap. Without this we would get the ocean stripes bug.
  • Any pixel must be drawn over by at most one tile at a time. Without this we would get visible banding at tile edges in any transparent fill layer due to tiles always having a bit of border geometry that extends into its neighbours.
  • Any pixel must be able to be drawn over repeatedly by the same tile. This is needed because some layers might overdraw some area multiple times by a transparent color for shading. I think MapTiler maps do this.

Setting up stencil in a way that does this is possible I think, but I'm afraid it would slow down rendering way too much because of state changes. We would need to draw some stencil-only masks after every fill tile draw, which means switching shaders and other rendering state, potentially hundreds of times per frame.

@HarelM
Copy link
Collaborator

HarelM commented Nov 13, 2024

While there might be issues with the facebook tiles, the following issue is a regression issue as far as I understand, and it requires fixing I believe:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
globe Globe related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ocean stripes over continent
4 participants