Update minZoom check to ignore zoomOffset and rounding

[rory-orennia]

This fixes an issue where minZoom isn't actually minZoom because we round or ceil the current zoom. So you get scenarios like this:

Extent Set?	minZoom	currentZoom	display?
yes	10	9.000001	yes
no	10	9.499999	no
no	10	9.5	yes

After this PR:

Extent Set?	minZoom	currentZoom	display?
yes	10	9.000001	no
yes	10	10	yes
no	10	9.499999	no
no	10	9.5	no
no	10	10	yes

Closes #9806

Change List

• Changes getTileIndices to use Math.floor instead of Math.round or Math.ceil

[felixpalmer]

Bear in mind that this would be a breaking change as apps would load different tiles for the same viewport. So this is not a change we would want to make lightly.

I didn't write the original code, but I believe the zoom rounding done in a way as to match what the basemap libraries are doing. Would you be able to update your table to show which level of tiles the maplibre and Google basemaps are loading in those scenarios? As loading new tiles always leads to some features "popping in", it makes sense that deck does it at the same point as the basemap to reduce the noise

[rory-orennia]

@felixpalmer You're right.. that is how Google Map Tiles work in Deck.GL... 9.5 zoom fetches Z=10 tiles when running this example code locally https://deck.gl/examples/google-maps:

Unfortunately (or fortunately if you like things to mean what they should), that is not how MapBoxGL works. As you can see here:

I guess we need some way of changing this behaviour based on what you're using as your map container? I'm assuming this is actually a MapLibreGL vs MapBoxGL thing and not really a Google Tiles vs MapBox Tiles issue. Maybe we can add a way for you to modify this behaviour in the layer properties, or automatically when using MapboxOverlay?

I'll take a stab at implementing whatever you think is the best approach

[Pessimistress]

I agree the current behavior is confusing, though the rounding logic is a specific one that took us several iterations to reach. I think we can improve the behavior without making a breaking change:

• use the limits to validate the current zoom before rounding (whether to show/hide)
• use the limits to clamp the zoom after rounding (which level to load)

[rory-orennia]

@Pessimistress If I'm understanding your comment correctly, you're proposing this:

Extent Set?	minZoom	currentZoom	display?	level to load
yes	10	9.000001	no	10
yes	10	10.000001	yes	11
no	10	9.499999	no	10
no	10	9.5	no	10
no	10	10	yes	10
no	10	10.5	yes	11

or in other words: "use minZoom to mean minZoom, but match MapLibre Z level calculations for which tile to grab"

I think that makes sense for MapLibre, but it still creates the problem in MapboxGL where if you're passed the minZoom issue, you're still 0.5 out of sync because zoom 13.5 will grab basemap tile 13 and TileLayer tile 14. I don't think there's any way to make the logic work for both map hosts without having some logic switch. The easiest non-breaking change would likely be to add this:

// I'm adding Deck.Gl and MapLibre in here just to be less confusing to someone looking at this
// when they are using MapLibre or Deck.GL and don't realize they're basically the same thing
export type TileIndexSystem = 'Deck.GL' | 'MapLibre' | 'Mapbox';

export type TileLayerOptions = {
  ...
  tileIndexSystem: TileIndexSystem;
}

export function getTileIndices({
  ...
  tileIndexSystem = 'Deck.GL'
}: {
  ...
  tileIndexSystem: TileIndexSystem;
}) {
  if(tileIndexSystem === 'Mapbox') {
    // Do the logic originally proposed in this PR
  } else {
    // Do the existing logic, or also add the minZoom vs fetch difference proposed by Pessimistress
  }
}

We'd then need to update the documentation to explain what this does and why it's necessary.

Thoughts?

[felixpalmer]

Agree with @Pessimistress.

@rory-orennia regarding extent. If this is true the tile will always load, even when currentZoom is less than minZoom - so you can remove that from the table.

The visibility test is then improved to directly compare currentZoom >= minZoom, so that with minZoom = 10 the layer is visible for currentZoom = 10.001 but hidden for currentZoom = 9.999.

For the level to load, we keep the current behavior (i.e. GMaps style).

I don't think it is a good idea to change the tiling loading behavior based on the basemap behavior as it will be confusing for a developer to change the basemap and have different data load.

[rory-orennia]

@felixpalmer I don't think this is an issue with changing basemap, it's an issue if you use MapboxOverlay with Mapbox Standard vs the built in map host. So I don't see this as an issue where a user flips a toggle to select a different basemap and now the loading behaviour is different. I'm guessing 99.99% of the apps out there have a single map host, and then some of them have the ability to use other base map styles inside that single host. I don't really see why someone would build an app that had the options of "Use Mapbox" vs "Use MapLibre" vs "Use Carto" other than a demo or POC app.

Without some toggle to this logic, how would you ever make this hypothetical app work:

• I'm using Mapbox Overlay with a Mapbox standard tileset
• I have multiple TileLayer data sets that highlight hazardous roads (frequent icy conditions, bad sun glare at sunset/sunrise, etc.)
• I zoom the map to 9.5
• Basemap loads Z=9
• TileLayer loads Z=10
• Now we have a glowing highlight in the middle of nowhere
• Zoom into 10
• Ohh, now that road loaded on the basemap and it makes sense again

The minzoom fix (don't load any TileLayer until zoom is >= minzoom) fixes the most egregious error in my use case, but I'll likely have to fork or pnpm patch this project to make it line up with Mapbox Standard if I want it to be perfectly in sync (like it is for MapLibre right now) for some of our data sets.

[felixpalmer]

CARTO literally has a UI control that let's users switch between basemaps, including different providers. But I totally see you have a valid use-case and it would be a shame if you had to resort to forking over something like this

Perhaps a way out would be to allow zoomOffset to be non-integer. It already lets you shift the zoom of the tiles fetched and feels like a potential solution to this issue. Would this work for you? This line:

Math.floor(viewport.zoom + Math.log2(TILE_SIZE / tileSize)) + zoomOffset

and any other using zoomOffset would need to be adjusted to place zoomOffset inside the rounding/flooring function, but then you would be able to pass zoomOffset: 0.5 and have it work. Could you try this out and confirm if it solves your issue?

[rory-orennia]

It's not a basemap issue, it's a map host issue. If you're injecting Deck.GL into MapboxGL JS vs using Deck.GL on it's own, you get different tile fetching based on your zoom level. Doesn't matter if those tiles come from Mapbox or OpenMapTiles or Carto.

Being able to zoomOffset by 0.5 would definitely fix the issue, as long as the new code for minZoom ignores that offset. So if minzoom: 10 and currentZoom:9.9, it shouldn't fetch tiles Z=10 and display them because you still haven't hit the minzoom. I'm not sure if that would be confusing to people?

Say we set zoomOffset to 1.5 and minZoom to 10, which of these would be what we want?

CurrentZoom	minZoom uses offset	minZoom ignores offset
8	Hide	Hide
8.5	Show 10	Hide
9	Show 10	Hide
9.5	Show 11	Hide
10	Show 11	Show 11
10.5	Show 12	Show 12

It feels a little weird that you can't ever fetch Z=10 tiles in the 'ignores' case. But I can see the logic of that as "don't show me tiles until my zoom>=minZoom, and once we're showing tiles, use my offset". Without that 'ignores offset' then in MapboxGL, the 0.5 offset means we're back to square one where tiles show up at zoom=9.5 even though minZoom=10.

[felixpalmer]

It feels like minZoom ignores offset is the better option - it is consistent with how the props are described in the docs.

It feels a little weird that you can't ever fetch Z=10 tiles in the 'ignores' case.

I think this is fine, as the user has to explicitly set the prop.

In general the mental model should be that:

• min/maxZoom specify when the tiles are shown as the zoom goes from 0-MAX_ZOOM, and
• zoomOffset specifies which tiles will be loaded.

Note this is assuming the suggestion above is implemented

[rory-orennia]

@felixpalmer Yeah I agree with you. I'll update this PR code and title to reflect the changes we've settled on and then your team can give it a review

[rory-orennia]

The new code has been pushed. It makes the following changes:

1. the check for minZoom now directly uses viewport.zoom instead of z. This means we're ignoring the rounding and the zoomOffset when we check if you're past the minZoom value
2. zoomOffset handling was moved inside the Math.round() and Math.ceil() call. The code didn't lock zoomOffset down to an integer like it does with minZoom and maxZoom, so you could put zoomOffset=0.5 and now you'd be passing z=4.5 around the code which is unexpected. With this moved inside the Math.round it means we can exactly match MapBoxGL with a zoomOffset=-0.5

[coveralls]

coverage: 91.145% (+0.001%) from 91.144%
when pulling 079ad41 on rory-orennia:minzoom-fix
into 5da7942 on visgl:master.