Disable terrain render cache during style property transitions

[mpulkki-mapbox]

The terrain feature uses render cache for optimizing rendering performance by caching intermediate rendering results of tiles in cache textures when certain conditions are met. The cache is supposed to get invalidated on style changes but the code is not currently taking transitions into account. Terrain class correctly listens for style data events but those are sent only once at the beginning of transitions.

The following snippet changes water color correctly in Streets-style when the transition duration is set to zero.

map.painter.terrain.forceRenderCached = true;
map.style.stylesheet.transition = { duration: 0, delay: 0 };
map.setPaintProperty("water", "fill-color", ["rgba", 117, 207, 0, 1]);

But the render cache does not get invalidated properly when a transition duration is set.

map.painter.terrain.forceRenderCached = true;
map.style.stylesheet.transition = { duration: 500, delay: 0 };
map.setPaintProperty("water", "fill-color", ["rgba", 117, 207, 0, 1]);

This pull request fixes the cache invalidation by disabling the render cache for the duration of style property transitions. The PR does not fix cache issues caused by re-evalution of paint properties (#10302). Six new render tests have been added (most of which fails before the patch and passes after it) for validating the fix.

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• manually test the debug page
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix style property transitions not invalidating the terrain render cache</changelog>

[astojilj]

There's similar implementation used for crossfade below:

mapbox-gl-js/src/terrain/terrain.js

Lines 830 to 836 in 5db8469

	_shouldDisableRenderCache(): boolean {
	// Disable render caches on dynamic events due to fading.
	const isCrossFading = id => {
	const layer = this._style._layers[id];
	const isHidden = !layer.isHidden(this.painter.transform.zoom);
	const crossFade = layer.getCrossfadeParameters();
	const isFading = !!crossFade && crossFade.t !== 1;

should we add this.style.hasTransitions() there instead?

[karimnaaji]

Yes I agree, I also think that would be a better place to consolidate this logic.

[mpulkki-mapbox]

Agreed. I've updated the PR by moving the introduced logic to _shouldDisableRenderCache. I was initially concerned if some of the transition events could be missed because style.update() was called before painter.render() leading to "transition" flags being clear before rendering of the frame was done. But this works correctly as render cache contents gets updated in the very first frame after transitions have finished.

[karimnaaji]

One nit to maybe consolidate logic in _shouldDisableRenderCache, otherwise LGTM.

[karimnaaji]

Yes I agree, I also think that would be a better place to consolidate this logic.

[mpulkki-mapbox]

I noticed a possible bug where isHidden was negated twice leading to wrong result. Hidden layers are now properly skipped when determining whether caching should be disabled. @astojilj can you confirm this?

[astojilj]

Great find. This relates to raster fade on terrain and good to recheck the details (a192ebb) - there is render cache there that seems to be passing now, too.

Edit: please ignore this, wrong about it.

[astojilj]

LGTM