Return early from draw calls on layers with constant opacity=0 #5429

lbud · 2017-10-09T13:03:24Z

We're completing full draw calls for layers even when their opacity is set to zero. This cuts back on render passes by bailing early from draw functions if a layer's opacity is constant at 0. One common use case for this is switching very quickly between several visualization layers without having to reparse buckets/re-download tiles.

Locally I replaced the Streets style in the Paint benchmark with a version where half of the layers are set to opacity=0 — obviously this example is extremely contrived, but

anandthakker

Nice!

This made me wonder how we handle visibility: none, and it looks like that's done via the StyleLayer#isHidden() method. I wonder if it makes sense to fold these checks into isHidden() for each layer type?

anandthakker · 2017-10-09T13:09:35Z

src/render/draw_circle.js

@@ -12,6 +12,7 @@ module.exports = drawCircles;

 function drawCircles(painter: Painter, sourceCache: SourceCache, layer: CircleStyleLayer, coords: Array<TileCoord>) {
    if (painter.renderPass !== 'translucent') return;
+    if (layer.paint['circle-opacity'] === 0 && layer.paint['circle-stroke-opacity'] === 0) return;


With an eye towards #3044, we should use layer.isPaintValueFeatureConstant() and layer.getPaintValue() instead of directly accessing layer.paint

lbud · 2017-10-09T14:37:44Z

Good idea @anandthakker — moved these in 09ed3de.

jfirebaugh · 2017-10-09T15:16:30Z

I wonder if it makes sense to fold these checks into isHidden() for each layer type?

I seem to recall that's what we used to do, but we had reason to stop. Can you please review the history and see why?

lbud · 2017-10-09T15:39:53Z

Good call @jfirebaugh — looks like this was removed in #3120. It seems like the cause of that underlying bug was that we were also trying to be smart about the alpha channel in the color, which I think isn't necessary here — users trying to show/hide layers with opacity would do so with explicit -opacity properties. From that PR:

The latest commit removes all opacity tests from the StyleLayer#isHidden method. This is unlikely to cause performance regressions in core styles because we don't often have 0 opacity layers in core styles. It's possible a 3rd party style may see a perf regression due to this change.

¯_(ツ)_/¯

jfirebaugh · 2017-10-09T18:37:43Z

Yeah, that's the one I was thinking of. In addition to that, we also tell people to use opacity 0 if they don't want the layer to be visible, but do want it to be queryable with queryRenderedFeatures. I believe including an opacity check in isHidden would break that, because isHidden affects whether the tiles are loaded. So I think we should short circuit in drawing, but not change isHidden.

…ing for invisible features * Add query tests for invisible features + render test for visible circle-stroke with zero-opacity circle

lbud · 2017-10-10T11:15:27Z

Okay, this now short-circuits only in draw functions with isOpacityZero methods per StyleLayer subclass. I've also added two query tests for invisible features (one opacity=0 and one visibility=none) and a render test for circles with strokes but zero-opacity fills, since that's a slightly weird edge case here.

mourner · 2017-10-10T13:50:36Z

@lbud let's also run the bench for a style with no opacity=0 layers since this may affect performance (many additional getPaintValue calls)

lbud · 2017-10-10T14:12:24Z

@mourner with the regular Streets style there's no statistically significant change. Here are bench results from #Paint — ran it several times, always ± just a bit in either direction:

Full bench run:

@anandthakker and I discussed this yesterday as well — we could theoretically just use computed layer.paint[<type>-opacity], but as he noted in #5429 (comment) we'd like to get away from doing that.

jfirebaugh · 2017-10-10T19:04:39Z

src/render/draw_background.js

@@ -9,6 +9,8 @@ import type StyleLayer from '../style/style_layer';
 module.exports = drawBackground;

 function drawBackground(painter: Painter, sourceCache: SourceCache, layer: StyleLayer) {


Change layer: StyleLayer to layer: BackgroundStyleLayer...

jfirebaugh · 2017-10-10T19:05:10Z

src/render/draw_raster.js

@@ -12,6 +12,7 @@ module.exports = drawRaster;

 function drawRaster(painter: Painter, sourceCache: SourceCache, layer: StyleLayer, coords: Array<TileCoord>) {


...layer: RasterStyleLayer...

jfirebaugh · 2017-10-10T19:05:33Z

src/style/style_layer.js

@@ -41,6 +41,7 @@ class StyleLayer extends Evented {
    _layoutFunctions: {[string]: boolean};

    +createBucket: (parameters: BucketParameters) => Bucket;
+    +isOpacityZero: (zoom: number, property?: string) => boolean;


...then you can remove this...

jfirebaugh · 2017-10-10T19:07:27Z

src/style/style_layer/symbol_style_layer.js

@@ -48,6 +48,13 @@ class SymbolStyleLayer extends StyleLayer {
        return (new SymbolBucket((parameters: any)): any);
    }

+    isOpacityZero(zoom: number, property?: string) {


...and make property non-optional.

👍 thanks @jfirebaugh!

Return early from draw calls on layers with constant opacity=0

26d73a8

anandthakker reviewed Oct 9, 2017

View reviewed changes

Move zero-opacity check to StyleLayer subclasses

09ed3de

lbud force-pushed the bail-zero-opacity branch from 9856917 to 09ed3de Compare October 9, 2017 14:36

Lauren Budorick added 2 commits October 9, 2017 16:42

lint

49f2d3c

fix flow

045aa53

Use this rather than super + add noop for SymbolStyleLayer

36af605

explain symbol noop

5b1de97

* Short-circuit in draw fns rather than in isHidden to preserve query…

c82332c

…ing for invisible features * Add query tests for invisible features + render test for visible circle-stroke with zero-opacity circle

jfirebaugh approved these changes Oct 10, 2017

View reviewed changes

Use appropriate StyleLayer subclasses in draw fns

c592eb6

lbud merged commit 454a891 into master Oct 11, 2017

lbud deleted the bail-zero-opacity branch October 11, 2017 09:50

jmunowitch mentioned this pull request Oct 11, 2017

Upgrade to Mapbox-gl-js 0.41.0 archesproject/arches#2551

Closed

ChrisLoer mentioned this pull request Oct 24, 2017

circle layers with invisible strokes don't render in 0.41.0 #5496

Closed

renovate bot mentioned this pull request Feb 2, 2018

Update dependency mapbox-gl to ^0.44.0 - autoclosed oSoc17/rideaway-frontend#72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return early from draw calls on layers with constant opacity=0 #5429

Return early from draw calls on layers with constant opacity=0 #5429

lbud commented Oct 9, 2017

anandthakker left a comment

anandthakker Oct 9, 2017

lbud commented Oct 9, 2017

jfirebaugh commented Oct 9, 2017

lbud commented Oct 9, 2017

jfirebaugh commented Oct 9, 2017

lbud commented Oct 10, 2017

mourner commented Oct 10, 2017

lbud commented Oct 10, 2017

jfirebaugh Oct 10, 2017

jfirebaugh Oct 10, 2017

jfirebaugh Oct 10, 2017

jfirebaugh Oct 10, 2017

lbud Oct 11, 2017

		@@ -9,6 +9,8 @@ import type StyleLayer from '../style/style_layer';
		module.exports = drawBackground;

		function drawBackground(painter: Painter, sourceCache: SourceCache, layer: StyleLayer) {

		@@ -12,6 +12,7 @@ module.exports = drawRaster;

		function drawRaster(painter: Painter, sourceCache: SourceCache, layer: StyleLayer, coords: Array<TileCoord>) {

Return early from draw calls on layers with constant opacity=0 #5429

Return early from draw calls on layers with constant opacity=0 #5429

Conversation

lbud commented Oct 9, 2017

anandthakker left a comment

Choose a reason for hiding this comment

anandthakker Oct 9, 2017

Choose a reason for hiding this comment

lbud commented Oct 9, 2017

jfirebaugh commented Oct 9, 2017

lbud commented Oct 9, 2017

jfirebaugh commented Oct 9, 2017

lbud commented Oct 10, 2017

mourner commented Oct 10, 2017

lbud commented Oct 10, 2017

jfirebaugh Oct 10, 2017

Choose a reason for hiding this comment

jfirebaugh Oct 10, 2017

Choose a reason for hiding this comment

jfirebaugh Oct 10, 2017

Choose a reason for hiding this comment

jfirebaugh Oct 10, 2017

Choose a reason for hiding this comment

lbud Oct 11, 2017

Choose a reason for hiding this comment