Framebuffer refactor #5793

lbud · 2017-12-02T00:28:38Z

Primary changes:

Refactors all render calls that render to offscreen framebuffers (fill-extrusion, heatmap, hillshade) into one offscreen pass (fixes Render to heatmap framebuffer in separate pass #5803)
Makes Context responsible for all framebuffer + renderbuffer creation
Introduces Framebuffer class, responsible for managing its color and/or depth attachment states, retaining size info, and cleaning up its attachments with gl.delete* calls when destroyed
- Framebuffers still need to be bound and unbound (by setting context.bindFramebuffer) in render code in order to specify the intended rendering target, but do not need to be manually bound in order to change their attachments: attachment values handle the necessary bindings when set
Eliminates RenderTexture class

Minor changes:

Moves GL extensions stored on the painter to the context object
~~Caches tile-sized framebuffers on the painter when unloaded (from hillshade layers) for reuse in new loaded hillshade tiles~~
Updates heatmap-radius/antimeridian render test (Framebuffer refactor #5793 (comment))

Benchmark results: https://bl.ocks.org/anonymous/raw/3d9a8aa509b5dc8d0c1d97c6e5f6110f/

Refs #145.

jfirebaugh

Good direction here. Aside from a few places the type annotations could be improved, the main thing I'm wondering about is whether we can make the ownership conventions clearer. Can the the Framebuffer itself own the attachments and be responsible for releasing them in a Framebuffer#destroy method comparable to Texture#destroy and VertexArrayObject#destroy? Or does the need for shared attachments prevent that?

jfirebaugh · 2017-12-04T20:37:41Z

src/gl/value.js


-    constructor(context: Context) {
+    constructor(context: Context, parent: ?any) {


Instead of adding an any-typed parameter and member in the base class, override the constructor in ColorAttachment and DepthAttachment.

jfirebaugh · 2017-12-04T20:57:48Z

src/source/tile.js

@@ -87,6 +87,7 @@ class Tile {
    needsHillshadePrepare: ?boolean
    request: any;
    texture: any;
+    fbo: any;


fbo: Framebuffer

lbud · 2017-12-05T20:52:50Z

Note that the test updated in 3577b7c changed because it had been incorrectly overdrawing before: drawHeatmap was previously using painter.isOpaquePass, which we deprecated a little while ago in favor of a painter.pass enum.

mourner · 2017-12-05T21:10:21Z

@lbud the heatmap drawing changes look great to me!

lbud · 2017-12-05T22:20:49Z

Can the the Framebuffer itself own the attachments and be responsible for releasing them in a Framebuffer#destroy method comparable to Texture#destroy and VertexArrayObject#destroy? Or does the need for shared attachments prevent that?

Yes, good idea — addressed this in 470e764. Per the WebGL spec, "If the renderbuffer has already been deleted the call has no effect," so shared renderbuffer attachments won't cause a problem (confirmed on a debug map).

jfirebaugh · 2017-12-06T23:43:01Z

src/style/style_layer.js

        return false;
    }

-    resize(gl: WebGLRenderingContext) { // eslint-disable-line
+    resize() { // eslint-disable-line


eslint-disable-line was probably for the unused parameter and can be removed too.

jfirebaugh · 2017-12-07T00:59:06Z

src/render/draw_fill_extrusion.js

@@ -20,9 +21,11 @@ function draw(painter: Painter, source: SourceCache, layer: FillExtrusionStyleLa
        return;
    }

-    if (painter.renderPass === '3d') {
+    if (painter.renderPass === 'offscreen') {


One rule of thumb that I try to follow is that if you have a series of conditionals, it's good for comprehensibility if you can make the bodies of each branch parallel in form (same principle as parallelism in grammar). Here, for instance, I would rename setupFramebuffer to drawExtrusionFramebuffer and move the code just above and below the call into the function, so that the two branch bodes are both a single drawExtrusion*(...) expression.

jfirebaugh · 2017-12-07T01:02:07Z

src/style/style_layer.js

@@ -42,7 +42,7 @@ class StyleLayer extends Evented {
    _transitioningPaint: Transitioning<any>;
    +paint: mixed;

-    viewportFrame: ?RenderTexture;
+    viewportFrame: ?Framebuffer;


Can this declaration be pushed down hierarchy, into FillExtrusionStyleLayer?

jfirebaugh · 2017-12-07T01:16:17Z

src/source/raster_dem_tile_source.js

@@ -116,6 +116,10 @@ class RasterDEMTileSource extends RasterTileSource implements Source {

    unloadTile(tile: Tile) {
        if (tile.demTexture) this.map.painter.saveTileTexture(tile.demTexture);
+        if (tile.fbo) {
+            this.map.painter.saveTileFramebuffer(tile.fbo);


How much of a performance difference does caching framebuffers make? If it's not too big a difference, I think it would be good to keep things simple and create a fresh framebuffer for each new DEM tile.

…have framebuffers destroy their own attachments

mourner · 2018-01-10T16:09:27Z

@lbud are you planning to do a similar refactor (consolidating offscreen rendering into an "offscreen" pass) on the gl-native side?

lbud · 2018-01-10T18:56:01Z

@mourner we should use the same process (an offscreen pass to render to all offscreen framebuffers) on native. I haven't yet started something like this there, although at the moment fill-extrusion layers are the only offscreen layer type on master, so if I were to do so right now it would just be a minor refactor (moving extrusion depth buffer setup into render_fill_extrusion.cpp and renaming "3d" to "offscreen"). I know you and @mollymerp have heatmap/hillshade branches in progress — not sure how far along you are; if it's convenient for you to render those in the 3d pass feel free to make those changes, but if not it shouldn't be a big deal to refactor afterward as we did in gl-js.

lbud requested a review from jfirebaugh December 2, 2017 00:45

lbud force-pushed the state-framebuffers branch from 17d1dc0 to a541fae Compare December 2, 2017 02:03

jfirebaugh reviewed Dec 4, 2017

View reviewed changes

lbud force-pushed the state-framebuffers branch from 3577b7c to 880b481 Compare December 5, 2017 21:53

lbud requested a review from jfirebaugh December 6, 2017 22:32

jfirebaugh reviewed Dec 7, 2017

View reviewed changes

jfirebaugh approved these changes Dec 7, 2017

View reviewed changes

Lauren Budorick added 7 commits December 7, 2017 11:03

Create Framebuffer class, remove RenderTexture

b91fce0

Tweak naming in draw_hillshade

6669b08

Refactor all offscreen passes, move painter extensions into context, …

b8fcd75

…have framebuffers destroy their own attachments

Use tile framebuffer caching for hillshades

98b9169

cleanups

a87018b

Update heatmap test

6280f13

Address review comments

3a5a0aa

lbud force-pushed the state-framebuffers branch from 6a2216c to 3a5a0aa Compare December 7, 2017 19:09

lbud merged commit 22ffd3f into master Dec 7, 2017

lbud deleted the state-framebuffers branch December 7, 2017 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framebuffer refactor #5793

Framebuffer refactor #5793

lbud commented Dec 2, 2017 •

edited

Loading

jfirebaugh left a comment

jfirebaugh Dec 4, 2017

jfirebaugh Dec 4, 2017

lbud commented Dec 5, 2017

mourner commented Dec 5, 2017

lbud commented Dec 5, 2017

jfirebaugh Dec 6, 2017

jfirebaugh Dec 7, 2017

jfirebaugh Dec 7, 2017

jfirebaugh Dec 7, 2017 •

edited

Loading

mourner commented Jan 10, 2018

lbud commented Jan 10, 2018


		constructor(context: Context) {
		constructor(context: Context, parent: ?any) {

Framebuffer refactor #5793

Framebuffer refactor #5793

Conversation

lbud commented Dec 2, 2017 • edited Loading

jfirebaugh left a comment

Choose a reason for hiding this comment

jfirebaugh Dec 4, 2017

Choose a reason for hiding this comment

jfirebaugh Dec 4, 2017

Choose a reason for hiding this comment

lbud commented Dec 5, 2017

mourner commented Dec 5, 2017

lbud commented Dec 5, 2017

jfirebaugh Dec 6, 2017

Choose a reason for hiding this comment

jfirebaugh Dec 7, 2017

Choose a reason for hiding this comment

jfirebaugh Dec 7, 2017

Choose a reason for hiding this comment

jfirebaugh Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

mourner commented Jan 10, 2018

lbud commented Jan 10, 2018

lbud commented Dec 2, 2017 •

edited

Loading

jfirebaugh Dec 7, 2017 •

edited

Loading