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

Render tile masks in order #1989

Closed
wants to merge 1 commit into from
Closed

Render tile masks in order #1989

wants to merge 1 commit into from

Conversation

TimSylvester
Copy link
Collaborator

Tile masks were being rendered into the stencil buffer in whatever order they ended up in RenderTiles. When multiple sources are present, this can result in lower-detail tiles blocking higher-detail ones from rendering.

@TimSylvester TimSylvester added bug Something isn't working metal OpenGL Issues related to the OpenGL renderer backend labels Jan 2, 2024
@TimSylvester TimSylvester self-assigned this Jan 2, 2024
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1098b2) 85.68% compared to head (44fc0f8) 85.69%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1989   +/-   ##
=======================================
  Coverage   85.68%   85.69%           
=======================================
  Files         569      569           
  Lines       28049    28052    +3     
=======================================
+ Hits        24035    24040    +5     
+ Misses       4014     4012    -2     

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

Copy link

github-actions bot commented Jan 2, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0%    +880  +0.0%    +296    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-1989-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +19% +21.6Mi  +400% +23.9Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-1989-compared-to-legacy.txt

Copy link

github-actions bot commented Jan 2, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0%     +96  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-1989-compared-to-main.txt

@louwers
Copy link
Collaborator

louwers commented Jan 2, 2024

This is to fix #1981 right? I am testing with maptiler-planet source from the MapTiler Outdoors style and a custom vector source with a single triangle. This is what is rendering...

this branch

out

826f17b (commit before 2e489eb)

out-826f17b65931cf4623711fc2e625b9ecebb408a5

Prior to Metal project (ios-v5.13.0)

out

MapLibre GL JS

image

@TimSylvester
Copy link
Collaborator Author

This is to fix #1981 right?

I recognize the triangle from the Slack discussion, but I didn't see the issue number. Probably it's related, but a separate issue from the "Pastel" style linked there. Is a custom vector source tile-clipped? It doesn't seem like it should participate in the stencil masking at all.

@TimSylvester TimSylvester marked this pull request as draft January 2, 2024 23:14
@TimSylvester TimSylvester marked this pull request as ready for review January 3, 2024 21:35
@TimSylvester
Copy link
Collaborator Author

This seems to improve things, and makes sense to me. But looking way back, tiles have always been rendered to the stencil buffer in RenderTiles order which I'm pretty sure hasn't changed.

https://github.com/maplibre/maplibre-native/blob/71f6da27228c6b95ccd0e5d9e7229eba41724012/src/mbgl/renderer/paint_parameters.cpp

So I'm a little worried that I'm totally missing something here. Perhaps the order in which sources are listed matters, and it only worked before if they were in order of increasing max-zoom?

@sjg-wdw
Copy link
Collaborator

sjg-wdw commented Jan 3, 2024

Might be a good idea to try a bunch of the styles visually and see if there are problems.

@mwilsnd
Copy link
Collaborator

mwilsnd commented Jan 4, 2024

One observation I made - clipping masks were rendered during the opaque pass but the raster layer in the pastel style is translucent. That feels like it shouldn't work, since high zoom vector source tiles are possibly being rendered both in front and behind the raster layer. But everything is in the translucent pass. It feels like the most correct solution then would be to detect when visiting a new RenderItem with a different source and re-rendering the clipping mask for that source.

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

On zoom level >= 14 the behavior is a bit different still (see comment), but this fixes the problem with MapTiler Outdoors.

Let's focus on extending the render tests instead of manual checking.

@TimSylvester
Copy link
Collaborator Author

... detect when visiting a new RenderItem with a different source and re-rendering the clipping mask for that source.

I had been thinking along similar lines. Re-considering what are the correct criteria for rebuilding the mask reminded me that the way we decide that has changed, and that may have been a mistake.

Specifically, a layer reuses the mask if the tiles are a subset of what's there, rather than set equality. If the tiles don't overlap that makes sense, but if there can be overlaps then sometimes a subset should be considered not equivalent.

That was done because we were rebuilding the mask several times per frame in some cases, when some layers didn't produce any data for one tile or another. But that has since changed to account for prefetch tiles, see #1871.

@TimSylvester TimSylvester mentioned this pull request Jan 4, 2024
@TimSylvester TimSylvester deleted the stencil-order branch June 14, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working metal OpenGL Issues related to the OpenGL renderer backend
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants