Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Do not render tiles from disabled layers #6345

Merged
merged 1 commit into from
Sep 20, 2016
Merged

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Sep 15, 2016

When a source has a minzoom and maxzoom, tiles from it should be be rendered when they are outside the range. However, when building the list of tiles to render, we are not skipping them correctly.

@kkaefer kkaefer added the bug label Sep 15, 2016
@mention-bot
Copy link

@kkaefer, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @ansis and @tmpsantos to be potential reviewers.

@kkaefer
Copy link
Contributor Author

kkaefer commented Sep 15, 2016

Missing tests

@tmpsantos
Copy link
Contributor

Missing tests

👍 after adding utests

@kkaefer kkaefer changed the title Do not render tiles from disabled sources Do not render tiles from disabled layers Sep 16, 2016
@kkaefer
Copy link
Contributor Author

kkaefer commented Sep 16, 2016

This actually turned out to be a different problem:

So far, we didn't properly disable layers that are outside the zoom range. This means that we rendered layers that should not have been rendered, albeit we didn't make any attempt to load tiles for those layers. However, when zooming in/out, existing tiles might already have been loaded in the source which continued to be rendered. In most cases they weren't actually visible because either the matrices weren't updated, or the clip IDs weren't set so that they would be "rendered" off-screen and clipped completely. In any case, we did way too much work.

@kkaefer
Copy link
Contributor Author

kkaefer commented Sep 16, 2016

There are a number of follow up issues that I found while working on this:

  • When no layer of a source is visible anymore, we are currently disabling the source, but we don't evict tiles that are still stored in that source.

@kkaefer
Copy link
Contributor Author

kkaefer commented Sep 16, 2016

Urgh, the color is off by one on Linux. On Mac, I get #C1DEB7, but on Linux, it's #C1DEB6.

@jfirebaugh
Copy link
Contributor

So far, we didn't properly disable layers that are outside the zoom range.

I thought #5828 was supposed to fix that?

@kkaefer
Copy link
Contributor Author

kkaefer commented Sep 17, 2016

#5828 didn't resolve this fully. It didn't actually stop rendering the layers, it just stopped updating them. This lead to a situation where they were still rendered, but their position matrix and stencil mask wasn't updated anymore. In most situations, this resulted either in a matrix that was off-screen, or a stencil mask that was all zeros, but we still issued all of the draw calls.

This patch introduces an enabled field that is updated when recalculate is called on the layer and we read this field when iterating through the layers, e.g. when building the render list.

@jfirebaugh
Copy link
Contributor

Do we need to introduce more state (enabled member), or can we just call needsRendering() in more places?

@kkaefer
Copy link
Contributor Author

kkaefer commented Sep 20, 2016

Changed to call needsRendering() directly.

So far, we didn't properly disable layers that are outside the zoom range. This means that we rendered layers that should not have been rendered, albeit we didn't make any attempt to load tiles for those layers. However, when zooming in/out, existing tiles might already have been loaded in the source which continued to be rendered. In most cases they weren't actually visible because either the matrices weren't updated, or the clip IDs weren't set so that they would be "rendered" off-screen and clipped completely. In any case, we did way too much work.
@kkaefer kkaefer merged commit 1e8f9c7 into master Sep 20, 2016
@kkaefer kkaefer deleted the 6345-disabled-sources branch September 20, 2016 12:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants