change tile loading logic to match native

[mollymerp]

this changes the tile retaining logic in source cache to load not-previously-loaded parent tiles if a tile and its children are not found. its a rough port of the update renderables algorithm that native uses https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/algorithm/update_renderables.hpp

this change is required to support sparse raster tile sets and to make the raster-masking render test pass #5105

Launch Checklist

• briefly describe the changes in this PR
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page

[anandthakker]

@kkaefer @mollymerp I have couple clarifying questions (left in the diff) that apply across both JS & native implementation

[anandthakker]

Hm, I don't follow what's happening in this branch -- why do we only get the first child tile?

[kkaefer]

Overzoomed children are different from regular children. While regular children increase the z by one, and enumerate all four quadrants, an "overzoomed child" is represented with the same z/x/y as its parent, except that the overzoomedZ value is increased by one. We're using overzoomed children to indicate that it's using the same data as the non-overzoomed tile, except "overzoomed" to a particular zoom level. This overzoom information is used while parsing the tile and placing labels.

[anandthakker]

What is the function of parentIsLoaded in this condition? It looks like parentIsLoaded is set to the value of the previous iteration's tile.isLoaded() -- I don't see why, e.g., if the z5 parent is loaded, we would then want to add the z4 tile.

[anandthakker]

(Maybe related: I find the name other than parentIsLoaded to be pretty ambiguous, since it's in a context where we're iterating up the parent chain... there are lots of parents and children here!)

[kkaefer]

"Loaded" is kind of a misnomer here; it refers to the fact that we attempted to load the tile. This condition means that "we attempted to load, but we don't have the tile (!tile), indicating a load or parse failure, which necessitates loading the parent tile.

[anandthakker]

Ah, okay. So is it right to say that the purpose of this loop is:

• ascend the ancestors of the "ideal" tile until we reach:
  • a renderable one.
  • the parent of the last ancestor we attempted and failed to load (such attempts would have been initiated previous frames), and initiate loading it.
  • minCoveringZoom
• mark for retention any currently loading ancestors between the "ideal" zoom and its renderable ancestor (or minCoveringZoom)

Proposed variable name change: parentIsLoaded => previousTileFailed

[mollymerp]

@anandthakker thanks for the thoughtful questions. this isn't working yet (as discovered by un-ignoring the associated native render test), and I'm sure I fudged something up in the port over from native. My next step was to add unit tests to try and figure out where I'm running into problems.

I was also confused by these variables and why they're being used this way. gl-js doesn't have a concept of hasTriedOptional and I tried implementing something similar but it didn't fix the render test.

      bool parentHasTriedOptional = tile->hasTriedOptional();
      bool parentIsLoaded = tile->isLoaded();

@kkaefer could you share insight into anands questions? maybe that will help me figure out what I'm missing.

[kkaefer]

Optional requests only exist in GL Native. Marking a request as "optional" indicates to the FileSource that it shouldn't go to great lengths to retrieve the data. In practice this means that an "optional" request will only look up existing information in the database, but won't go out to the server to fetch it. I experimented last week with removing this distinction again and changing the logic to only do all or nothing requests.

Currently, we use optional requests in native to load parent tiles while the actual tile we need is loading. However, this isn't something we can do in GL JS, and I'm considering removing it from GL native as well since it means that we'll have to parse more data (thus consuming more CPU) that we only show for a very short period of time, and it further delays showing the ideal data we downloaded.

When the ideal data can't be loaded, or parsing it fails, we're always making a "required" request (the default and only option on GL JS).

[mollymerp]

I still have a few more tests to add @kkaefer but this is ready for a more thorough review

[kkaefer]

Reviewed this, and overall it's 👍 and fixes the underlying issue. As you noted, it's still missing a few tests. The native side has a large number of tests, but they might not all be applicable since we don't have a distinction of optional vs. required tiles.

[mollymerp]

Thanks, @kkaefer – I think I got all the applicable tests ported over from native. Could you 👀 ✅ when you get a chance?

[mollymerp]

curiously, the raster-masking render test is passing on this PR without any of the actual masking code 💭 https://9317-8629417-gh.circle-artifacts.com/0/root/mapbox-gl-js/test/integration/render-tests/index.html

[kkaefer]

Can we add flow types for these?

[kkaefer]

curiously, the raster-masking render test is passing on this PR

This could be because we're rendering into the depth buffer for raster tiles:

painter.depthMask(true);

// Change depth function to prevent double drawing in areas where tiles overlap.
gl.depthFunc(gl.LESS);

This means that we're rendering into the depth buffer for those tiles, and subsequent (parent) tiles read from the depth buffer and discard those fragments. Ideally we should remove this (now that we're generating correct masks) to reduce writes to the depth buffer.