Preserve depth buffer between fill-extrusion layers + optimize render order #5101

lbud · 2017-08-04T19:54:15Z

This PR changes the way we render fill-extrusion layers:

Optimizes render order by rendering all fill-extrusion textures first, so that we don't have to restore any framebuffer contents (this optimization is outlined in Optimize Extrusion drawing mapbox-gl-native#9286 but not ticketed in gl-js)
Preserves depth buffer contents between different fill-extrusion layers in the same render pass so that multiple fill-extrusion layers can be rendered on the same map (fixes Correct depth rendering for multiple fill extrusions #4134)

Before + after (the blue layer is on top of the green layer in the render stack):

Technically this is a "breaking" rendering change ⚠️ , though I think we can all agree that it was already broken…

One odd quirk of this PR is the changes made in b155913: on my live maps, this worked whether clearing the depth buffer before binding or rendering any textures, or after rendering and unbinding all textures. However, all fill-extrusion tests were failing to render any fill-extrusions at this point. I'm not sure why clearing the depth buffer only once a texture (color target) is bound fixes the issue for the render tests, because it seems to me the depth clearing should work once the depth renderbuffer is attached whether or not there is a render target bound… 🤷‍♀️

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
manually test the debug page
corresponding mapbox-gl-native PR

jfirebaugh

Sharing a depth buffer between the 3D layers is an elegant solution to #4134 -- kudos to @kkaefer for the idea and @lbud for the implementation.

I'd like to see some refactoring of the draw process to integrate this more cleanly. It seems like it would make the most sense to treat the 3D pass as a full-fledged third rendering pass, preceding the existing opaque and translucent passes, and allowing any layer type to render during it, so that fill extrusions aren't so much of a special case.

Concretely, what that would mean is:

Introduce an enum type export type RenderPass = '3d' | 'opaque' | 'translucent', replace isOpaquePass: boolean with renderPass: RenderPass
Merge draw_fill_extrusion_texture.js into draw_fill_extrusion.js, and do the appropriate thing for the current pass. (BTW, if fill-extrusion-opacity is 1, can it copy the texture over in the opaque pass?)
Refactor/clean up render, renderPass, render3d -- I'd suggest just inlining the latter two into render. On the native side, I did something similar in Refactor Painter away mapbox-gl-native#9541, and though it makes for a long render method, I think it's clearer in the end because it eliminates the complex looping and conditional logic based on which render pass it is.

The benefits would be:

Future 3d layer types could hook into the 3d pass
It would get us back to a single draw function per layer type
Less special-case code for extrusions in Painter itself

I'm leaving a couple of smaller comments inline, but ☝️ is the big one.

jfirebaugh · 2017-08-04T21:50:20Z

src/style/style.js

@@ -231,6 +233,7 @@ class Style extends Evented {
        const layers = deref(this.stylesheet.layers);

        this._order = layers.map((layer) => layer.id);
+        this._order3D = layers.filter((layer) => layer.type === 'fill-extrusion').map((layer) => layer.id);


Could we compute this from _order when needed, rather than adding additional state?

jfirebaugh · 2017-08-04T21:53:15Z

src/render/draw_fill_extrusion_texture.js

+    array.emplaceBack(1, 1);
+    const buffer = Buffer.fromStructArray(array, Buffer.BufferType.VERTEX);
+
+    const vao = new VertexArrayObject();


I see that this isn't new code, but is there a way to avoid creating a new VAO for each render?

jfirebaugh · 2017-08-04T22:06:51Z

src/render/painter.js

-        if (this.viewportTexture) {
-            this.gl.deleteTexture(this.viewportTexture);
-            this.viewportTexture = null;
+        for (let i = 0; i < this.viewportTextures.length; i++) {


Use a for ... of loop here.

lbud · 2017-08-05T16:42:36Z

if fill-extrusion-opacity is 1, can it copy the texture over in the opaque pass?)

No — the opaque pass relies on fragments being able to retain their place through subsequent draw calls based on their depth. Since we can't write to gl_FragDepth, the full viewport would be assigned the same depth value and would block all layers underneath:

jfirebaugh · 2017-08-08T22:20:02Z

src/render/draw_circle.js

@@ -11,7 +11,8 @@ import type TileCoord from '../source/tile_coord';
 module.exports = drawCircles;

 function drawCircles(painter: Painter, sourceCache: SourceCache, layer: CircleStyleLayer, coords: Array<TileCoord>) {
-    if (painter.isOpaquePass) return;
+    const pass = 'translucent';
+    if (painter.renderPass !== pass) return;


In places (x4) where there's no conditionals involved in computing pass, just compare directly against the string literal.

jfirebaugh · 2017-08-08T22:23:33Z

src/render/painter.js

+            let sourceCache;
+            let coords = [];
+
+            for (let i = 0; i < layerIds.length; i++) {


Are there ways we could simplify the control flow in the body of this loop? For example, move checks such as layer.paint['fill-extrusion-opacity'] === 0 into has3DPass and set this._prerenderedTextures[layer.id] = null up front for every layer? Or even eliminate has3Dpass, have the layers without a 3D pass simply do nothing, and have the 3D layers do the texture setup inside their render functions? Not sure what exactly is kosher performance-wise, but this code seems like it could be simpler.

have the 3D layers do the texture setup inside their render functions

Conceptually I don't prefer this — as it is, the render functions are mostly just responsible for rendering whatever they have to render to whatever target is bound, and the painter is responsible for managing render targets and switching between them. Especially if we add more types with 3d passes I'd prefer to have the render target management for all 3D types happen in the painter's 3D pass loop. I did some simplification in 677c261.

jfirebaugh · 2017-08-08T22:31:44Z

src/render/painter.js

+                        // texture and clear the depth buffer a few lines down.
+                    }
+
+                    let renderTarget = this.viewportTextures.pop();


It looks like this.viewportTextures will always be an empty array. Am I missing somewhere where textures get released back (say when the layer is removed from the style)?

Instead of having a this._prerenderedTextures map keyed on layer ID, could fill extrusion layers have a prerenderedTexture property?

Am I missing somewhere where textures get released back (say when the layer is removed from the style)?

Ah, I forgot this — added in 0651dcf.

In order for fill extrusion layers to have a prerenderedTexture property, we'd have to make fbos/textures members of style layers, right? Which feels weird…

jfirebaugh · 2017-08-08T22:37:55Z

src/render/painter.js


-        let sourceCache;
-        let coords = [];
+                if (layer.source !== (sourceCache && sourceCache.id)) {


(I know this bit of code is duplicated in similar form in all three of the passes -- not ideal, but okay for now IMO. Hopefully as part of #4875 we can bring over the system in native where the coordinates are precomputed once for all layers ahead of time.)

kkaefer · 2017-08-07T10:50:07Z

src/render/draw_fill_extrusion_texture.js

+    const vao = new VertexArrayObject();
+    vao.bind(gl, program, buffer);
+    gl.drawArrays(gl.TRIANGLE_STRIP, 0, 4);
+}


Nit: missing newline

kkaefer · 2017-08-09T15:29:04Z

src/render/render_texture.js

+
+    attachToFramebuffer() {
+        const gl = this.gl;
+        gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, this.texture, 0);


Ideally, we'd be using one framebuffer for each layer (with different color attachments, and the same depth attachment), rather than detaching/reattaching textures to the framebuffer. The reason for this is that it's expensive to "validate" that a framebuffer has correctly bound attachments. It looks like we don't do that yet in JS; in Native, we're running glCheckFramebufferStatus. Therefore, the API in Native is modeled so that you can't change the attachments of a framebuffer after creation.

http://www.songho.ca/opengl/gl_fbo.html claims that the opposite of what I said is true:

Framebuffer object (FBO) provides an efficient switching mechanism; detach the previous framebuffer-attachable image from a FBO, and attach a new framebuffer-attachable image to the FBO. Switching framebuffer-attachable images is much faster than switching between FBOs.

However, further research suggests that this is outdated information, and no longer true for modern GPUs:

Intel built-in GPUs

Switching an entire FBO is more efficient than switching individual surfaces one at a time.

PowerVR

For optimal performance, [...] attachments should not be added or removed once the FBO has been created.

nVidia

Do not create a single FBO and then swap out attachments on it. This causes lots of validation in the driver, which in turn leads to poor performance.

ARM/Mali also has a very good article on framebuffer handling and how it interacts with buffer swapping that also confirms the behavior we've seen of PowerVR GPUs which prompted mapbox/mapbox-gl-native#9286

👀 interesting — thank you for the links @kkaefer! 🛠

@kkaefer I implemented this in 0651dcf. We still have to detach/reattach the depth renderbuffer between framebuffers, so I do wonder about performance gains — before there was one framebuffer with an always-attached renderbuffer and we switched texture attachments, and now there are many framebuffers with their own texture but we switch renderbuffer attachments, so I assume they still do validation every time we attach the depth rbo… 🤔

Hmm, why are we attaching the depth renderbuffer for every render? Can we leave the same renderbuffer attached to multiple framebuffers?

Oh, interesting — okay, I guess I had conceptually assumed that since gl.framebufferRenderbuffer attaches a renderbuffer to a framebuffer, that attachment was unique to the bound framebuffer. It seems instead based on what you're implying (and manual testing confirms this) that that attaches a renderbuffer to whatever is bound at the gl.FRAMEBUFFER attachment point at the time of the render?
~~To me this doesn't read super clearly either way —~~

attach a renderbuffer object to a framebuffer object

glFramebufferRenderbuffer attaches the renderbuffer specified by renderbuffer as one of the logical buffers of the currently bound framebuffer object

~~but it does seem to work when I only attach the depth RBO on the first fill-extrusion draw ( 1143dc4 )…~~

Spoke too soon above — this appeared to work for two layers, but without reattaching the renderbuffer with framebufferRenderbuffer per fbo, subsequent fbos are only reading from the depth rbo but not writing to them. So as soon as a third layer is introduced you can see how this goes wonky, reading depths from the first but not the second previously rendered layer:

render order: yellow, pink, then blue

hmm, what could work is upon creation of a framebuffer, attach both the depth RBO + color texture to the FBO, with every FBO having a different texture, but the same depth RBO?

Right — this is how it currently works 👍

asheemmamoowala · 2017-08-28T20:20:58Z

src/render/render_texture.js

+        this.vao = new VertexArrayObject();
+    }
+
+    attachRenderbuffer(depthRbo: WebGLRenderbuffer) {


The name of this method and detachRenderBuffer is a bit misleading. Can we make DEPTH_ATTACHMENT a parameter, or change to attachDepthBuffer/detachDepthBuffer ?

jfirebaugh

Your assessments sound reasonable to me. One last question and I think this is ready to go.

jfirebaugh · 2017-08-28T23:44:01Z

src/render/painter.js

            }
+
+            if (this.depthRboAttached) gl.bindRenderbuffer(gl.RENDERBUFFER, null);


_setup3DRenderbuffer always unbinds the renderbuffer, so is this necessary?

Given that we're binding/unbinding framebuffers, I believe we shouldn't be unbinding renderbuffers manually, since this state is part of the FBO.

…er layers + preserve depth buffer between extrusion layers

Port of mapbox/mapbox-gl-js#5101: adds a new render pass `Pass3D` before any other rendering wherein we render layers with 3D passes (fill-extrusion layers) to offscreen framebuffers, sharing a depth renderbuffer between those layers in order to render 3D space correctly. Those framebuffers are saved on the RenderLayers and copied back to the map during the translucent pass. Rendering to offscreen framebuffers before we do any clear + draw means we can avoid expensive framebuffer restores.

VNuhaan · 2017-09-25T19:20:36Z

I have a problem with the new render order.

I have a base map at low detail and an overlay with high detail. With this change all low detail buildings are drawn on top and thru the high detail map.

Is it possible to add an option to select how the extrusion layer interacts with other extrusions? Like: use z-axis drawing order, use layer drawing order, draw all extrusions.

For me its not logical anymore how the drawing works now. In the past I used an UNION ALL to combine extrusions to have a correct drawing, now everything is mixed up.

lbud requested review from kkaefer and jfirebaugh August 4, 2017 20:41

jfirebaugh reviewed Aug 4, 2017

View reviewed changes

lbud force-pushed the preserve-extrusion-depth branch 2 times, most recently from 1af1936 to 31f6369 Compare August 5, 2017 16:28

jfirebaugh reviewed Aug 8, 2017

View reviewed changes

kkaefer reviewed Aug 9, 2017

View reviewed changes

lbud force-pushed the preserve-extrusion-depth branch from 1143dc4 to bb51e2f Compare August 28, 2017 18:56

asheemmamoowala reviewed Aug 28, 2017

View reviewed changes

lbud force-pushed the preserve-extrusion-depth branch from bb51e2f to 4642796 Compare August 28, 2017 20:37

jfirebaugh approved these changes Aug 28, 2017

View reviewed changes

jfirebaugh mentioned this pull request Aug 29, 2017

Per-tile glyph and icon atlases #5190

Merged

6 tasks

lbud mentioned this pull request Aug 30, 2017

Preserve depth buffer between fill-extrusion layers mapbox/mapbox-gl-native#9894

Closed

Lauren Budorick added 3 commits August 30, 2017 11:35

Optimize extrusion rendering by rendering all textures before any oth…

6ead486

…er layers + preserve depth buffer between extrusion layers

Remove unnecessary rbo unbind + rename render_texture methods

bb1f273

lint

4bbef4e

lbud force-pushed the preserve-extrusion-depth branch from 6060264 to 4bbef4e Compare August 30, 2017 18:36

lbud mentioned this pull request Aug 30, 2017

Update gl-js SHA + ignore fill-extrusion-multiple test mapbox/mapbox-gl-native#9895

Merged

lbud merged commit c48a56b into master Aug 30, 2017

lbud deleted the preserve-extrusion-depth branch August 30, 2017 19:48

lbud mentioned this pull request Sep 7, 2017

Preserve depth buffer between 3D layers + optimize render order mapbox/mapbox-gl-native#9931

Merged

jfirebaugh mentioned this pull request Sep 12, 2017

Refactor texture-related classes #5277

Closed

mollymerp mentioned this pull request Sep 14, 2017

add raster-dem source type and hillshade layer type #5286

Merged

19 tasks

kekscom mentioned this pull request Sep 14, 2017

Inconsistent transparency for multiple fill-extrusions #5288

Closed

jmunowitch mentioned this pull request Sep 14, 2017

Upgrade to Mapbox-gl-js 0.40.0 archesproject/arches#2410

Closed

kkaefer mentioned this pull request Sep 22, 2017

Heatmaps layer #5253

Merged

36 tasks

lbud mentioned this pull request Oct 13, 2017

overlay with extrusion not possible #5457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve depth buffer between fill-extrusion layers + optimize render order #5101

Preserve depth buffer between fill-extrusion layers + optimize render order #5101

lbud commented Aug 4, 2017 •

edited

jfirebaugh left a comment

jfirebaugh Aug 4, 2017

jfirebaugh Aug 4, 2017

jfirebaugh Aug 4, 2017

lbud commented Aug 5, 2017

jfirebaugh Aug 8, 2017

jfirebaugh Aug 8, 2017

lbud Aug 26, 2017

jfirebaugh Aug 8, 2017

lbud Aug 26, 2017

jfirebaugh Aug 8, 2017

kkaefer Aug 7, 2017

kkaefer Aug 9, 2017

lbud Aug 16, 2017

lbud Aug 26, 2017

kkaefer Aug 26, 2017

lbud Aug 28, 2017 •

edited

kkaefer Aug 28, 2017

lbud Aug 28, 2017 •

edited

asheemmamoowala Aug 28, 2017

jfirebaugh left a comment

jfirebaugh Aug 28, 2017

kkaefer Aug 29, 2017

VNuhaan commented Sep 25, 2017 •

edited

		}

		if (this.depthRboAttached) gl.bindRenderbuffer(gl.RENDERBUFFER, null);

Preserve depth buffer between fill-extrusion layers + optimize render order #5101

Preserve depth buffer between fill-extrusion layers + optimize render order #5101

Conversation

lbud commented Aug 4, 2017 • edited

Launch Checklist

jfirebaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud commented Aug 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud Aug 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud Aug 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VNuhaan commented Sep 25, 2017 • edited

lbud commented Aug 4, 2017 •

edited

lbud Aug 28, 2017 •

edited

lbud Aug 28, 2017 •

edited

VNuhaan commented Sep 25, 2017 •

edited