6768 overdraw fix

[ryanhamley]

Launch Checklist

• briefly describe the changes in this PR
  • This partially solves Fix vector tile overdrawing with overlapping tiles from different zoom levels #6768 by treating 404'd tiles as renderable empty tiles. This is similar to how GL-Native treats these tiles as seen here. This is a compromise position between not retaining parent tiles for tiles that don't exist, which produced a noticeable flash at each zoom level as tiles drew in, and creating a mask for vector tiles similar to the one used with raster tiles, which forced circle features to be cut off at tile boundaries. Several variations of these approaches were attempted without achieving satisfactory results, but not retaining parent tiles came the closest; the trade off was a flash at every zoom level for every tile set which seems unacceptable as a solution to a more limited bug.
  • This proposed solution does not introduce a flash of tile loading, cut off features or allow the overdraw to be indefinitely displayed on the screen, but it does produce a flash of overdraw at new zoom levels. This flash goes away once the tiles are cached. See this example.
  • Eventually, GL-Native's tile loading/caching logic will need to be ported over to GL-JS and that may improve this functionality a bit more.
• manually test the debug page

[ryanhamley]

@mollymerp Do you have anything to add to this? Anything I'm forgetting to mention?

[jfirebaugh]

This is similar to how GL-Native treats these tiles as seen here.

Are you sure this is how native works? I was under the impression that it also overzooms parent tiles when receiving a 204/404 and the same issue exists there. (Note that a tile 404 actually does not produce Reason::NotFound. For historical reasons, tiles that 404 are treated as "no content".)

I wonder if there are many cases of users relying on the current behavior of overzooming parent tiles when a 204/404 occurs. It isn't as important for vector sources as it is for raster sources, where heterogenous maximum zoom levels are common, but there's probably cases where it's expected -- #6783 was a recent example, and possibly #5013.

[mollymerp]

@jfirebaugh thanks for the clarification around the 404 handling – TIL.

In that code you linked, the 404s then do not cause reponse->error to be set at all, and the noContent flag causes data to be a nullptr for that tile , but eventually the empty tile still gets marked as loaded = true; renderable = true which prevents the updateRenderables algorithm from retaining the empty tile's parent.

[mollymerp]

I think this is the relevant change in native where we started rendering errored/empty tiles mapbox/mapbox-gl-native#10011

this part of the code seems to have a history of a fix for one issue introducing another issue.

trueee 😅

I do see what you mean about users relying on parent tile overzooming when tiles 404. maybe the check we discussed before (don't retain parent tiles if at least one child successfully loads and one child 404s/errors) will have fewer side-effects than this change

[ryanhamley]

Yeah I can confirm actually that #5013 is worse with this solution. The overzoomed tiles fail completely 😞 I assume the same is true for the other issue John pointed out. So this actually isn't going to work. We didn't anticipate this use case.

[mollymerp]

picking this back up in favor of #7051. per chat, this is what native does, and we should keep behavior consistent cross-platform. note that we'll still want to revise our strategy if/when we implement server-side 204 responses for no content.

[mollymerp]

note that #5013 was an issue with how the tileset was generated, overzooming at ZL > tileset maxzoom still works w this PR.

[jfirebaugh]

Let's see if we can write a render test for this. Here's a sketch:

• Add a subdirectory to test/integration/tiles named sparse or something like that. Add a 0/0/0 vector tile that covers the world with a solid feature. (Or contains... anything really.)
• Add an integration test that renders a fill layer at z1 using a source pointing to this "tileset", with a contrasting background color.
• The expected result should be the background color: the server will send a 404 for z1 tiles, and we don't want to try loading z0. (Test without this PR applied, to make sure the test is failing as expected.)

There might be a reason why a render test of this type doesn't work, but let's give it a shot. It would be nice for maintaining parity in behavior between js and native.

[ryanhamley]

@jfirebaugh What makes you say that a render test like the one you described may not work? The test I have set up produces the expected output of just a background color, but it does this with or without the fix. I'm unable to produce a failing test.

This is the test I'm running for reference:

{
  "version": 8,
  "metadata": {
    "test": {
      "height": 256
    }
  },
  "center": [
    13.418056,
    52.499167
  ],
  "zoom": 1,
  "sources": {
    "mapbox": {
      "type": "vector",
      "tiles": [
        "local://tiles/sparse/{z}-{x}-{y}.mvt"
      ]
    }
  },
  "layers": [
    {
      "id": "water-layer",
      "source": "mapbox",
      "source-layer": "water",
      "type": "fill",
      "paint": {
          "fill-color": "blue"
      }
    },
    {
      "id": "background-layer",
      "type": "background",
      "paint": {
        "background-color": "yellow"
      }
    }
  ]
}

The sparse tile set is a 0/0/0 tile and I've confirmed that this is attempting to load 1/0/0 and 1/1/0, both of which are resulting in a 404 error.

[jfirebaugh]

What makes you say that a render test like the one you described may not work?

It's out of the norm for render tests, which tends to make things tricky. Not being able to produce a failing test is a common way for this to manifest. It means you'll have to debug the "expected to be failing" case to see why it's not failing. In this case, we expect it to be failing because it will load and render the parent tile, so the first thing to check is whether it's actually loading the parent tile. Or is it, say, indicating that all tiles are loaded as soon as the z1 tile fails, and stopping there?

Sometimes when you figure out why it's not failing, it's something you can fix with a tweak to the test case or the harness. But sometimes it's not, and you have to settle for a gl-js-specific unit test (or if even that's a disproportionate effort, manual testing).

[jfirebaugh]

Actually, the issue here may just be that your layer order is swapped. The background layer should be first.

[ryanhamley]

I did need to reverse the layer order, but there was also an inconsistency between src/util/ajax#getArrayBuffer and test/ajax_stubs#getArrayBuffer. The stubbed version was not returning an error object with a status property which is what the library is using to check for 404 status so I patched that and confirmed that the test fails without this PR and passes with it.

Without PR

With PR

All other tests pass except for render-tests/mixed-zoom which seems to be testing some label placement when zooming in. @ChrisLoer looks like you wrote this test. Can you elaborate on exactly what it was this is testing so we can figure out how to fix it?

Without PR

With PR

[ChrisLoer]

Can you elaborate on exactly what it was this is testing so we can figure out how to fix it?

The key thing that test case was focusing on was collision detection in the case where symbols are being included from both the overdrawn child and from the parent. The basic approach is to place both tiles but rely on the CrossTileSymbolIndex to detect duplicates between parent and child and hide the duplicate parent symbols:

• There shouldn't be any double-draw in the overlapping area (visually, you'd notice double-draw as labels looking bolder as they crossed the tile border)
• Parent symbols can show in the child tile area, as long as nothing else is there to collide them out
• Collision boxes/circles should be seamless at tile boundaries

[mollymerp]

🎉

[danielfornies]

Hi guys, just tested this, with tiles generated via tippecanoe which only go until zoom level 14, while I use the map until zoom level 18. After zoom level 15 the tiles aren't rendered anymore.
What should I do to overcome this? I could invoke tippecanoe forcing it until zoom level 18, but that's also a lot of extra data I don't really need (zoom level 14 already contains all the details)
Let me know, thanks!
Daniel

[mollymerp]

@danielfornies you need to se tthe maxzoom on your source to 14.

[danielfornies]

@danielfornies you need to se tthe maxzoom on your source to 14.

Omg, silly me, it was just that, thanks!

[shayke]

Hello,
We are using a custom tileserver which serves multiple mbtiles for the same source
(Users can choose which mbtiles they want to include since it's local).
Is there a workaround other than putting maxzoom for the source?
For example I have 2 mbtiles served from the same source:

1. planet - z0-z7
2. usa - z0-z14

If setting maxzoom 7 then mapbox won't request tiles which are available from usa
If setting maxzoom 14 then mapbox won't use the parent tile for z8-z14 for places which I don't have usa tiles (for example europe).

[shayke]

Hello,
We are using a custom tileserver which serves multiple mbtiles for the same source
(Users can choose which mbtiles they want to include since it's local).
Is there a workaround other than putting maxzoom for the source?
For example I have 2 mbtiles served from the same source:

1. planet - z0-z7
2. usa - z0-z14

If setting maxzoom 7 then mapbox won't request tiles which are available from usa
If setting maxzoom 14 then mapbox won't use the parent tile for z8-z14 for places which I don't have usa tiles (for example europe).

@mollymerp Any pointers here please? This prevents me from upgrading mapbox from 0.47 and I'm not really sure how I could tackle this.