Always preserve layer draw order when terrain is enabled #10258

karimnaaji · 2021-01-05T23:55:56Z

Terrain currently may not preserve draw order, it is a side-effect of an optimization that caches per-tile draped renders on interactive frames. See below for a visual representation of the issue.

Before / After

bug.mov

fix.mov

Terrain render cache & performance

The render cache optimization caches the result from offscreen framebuffer render during interactive frames. This is done only while there is user interaction or camera movement as animation changes are hardly noticeable during such frames. In cached mode, it renders all possible draped results in a single tile texture and reuses the result on subsequent frames. For interactive frames, it is done by batching all draped layers in a single texture and then drawing all other non-draped layers, while non-interactive frames preserve draw order by always interleaving and not caching results. As mentioned in #10186, it is critical for terrain performance as it highly reduces the number of draw call overhead.

Approach

During a frame, this PR proposes to change the terrain render cache to drape all layers in a cached result only until reaching the end of the batch, instead of grouping them always. Later in the frame, it then simply switches to regular draped/undraped interleaved mode.

This does not impact performance for base styles, as an observation we can make is most styles have draped layers at the bottom of the stack, and non-draped layers with a higher draw order (this is tested in all mapbox basemap styles by calculating the render cache efficiency which answers the question: How many layers are appropriately placed for the render cache to be optimal? this results in a 100% cache efficiency for all mapbox base maps and no performance regression in such situations).

Programmatically added draped layers are usually not as numerous as basemap draped layers such that the performance impact of not caching them remains minimal, for the tradeoff of correctness in layer ordering.

An alternative approach would be to allocate each possibly renderable batch in between undraped layers for a render cache entry. It does not seem worthwhile due to the observations above and the complexity involved with allowing multiple cache entries per tile.

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
[n/a] document any changes to public APIs
post benchmark scores (Checked through render cache efficiency on base map styles)
manually test the debug page
[n/a] tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
tagged @mapbox/gl-native if this PR includes shader changes or needs a native port, cc @astojilj @mpulkki-mapbox
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>Improve terrain support by always preserving layer draw order</changelog>

…er cache - Revise terrain render loop to be more explicit and easier to follow from the painter render passes: - Remove any painter modifications from render() function - Extract terrain draw depth to reduce branching and add explicit call from painter.render() - Make it explicit that all rendering of terrain happens in the translucent render pass - Update terrain.render() to work as follow: - In render cache mode: . Draw all layers until reaching first non-draped layer . Switch to interleaved render mode (not cached) for the remainder of the layer stack - In non render cache mode: . Draw all layers in interleaved render mode - Add debug/10186 sample code for simple reproduction case

- Add mapbox/satellite-streets-v11 style - Add back mistakenly removed fill-extrusion layer

karimnaaji · 2021-01-07T00:03:30Z

The results of 3d benchmark suite, which shows no performance impact on the set of fixtures tested:

mpulkki-mapbox · 2021-01-07T07:45:44Z

It's great that you're looking into optimizing and improving the render cache! As you mentioned the chosen approach for grouping all drapeable layers into a single big batch was made purely from the rendering performance point of view. Rendering artefacts of this implementation are being masked by enabling the cache only during user interaction (which might not be very effective).

I believe this approach is the right way to pursue especially if the drop in cache efficiency is minimal compared to the current implementation. I have few questions though:

This does not impact performance for base styles, as an observation we can make is most styles have draped layers at the bottom of the stack, and non-draped layers with a higher draw order (this is tested in all mapbox basemap styles by calculating the render cache efficiency which answers the question: How many layers are appropriately placed for the render cache to be optimal? this results in a 100% cache efficiency for all mapbox base maps and no performance regression in such situations).

Some special cases can be found where the efficiency is < 100% with the streets-style. For example the layer tulle-oneway-arrow-blue breaks the cache efficiency at 30% when zooming in towards the map. I would expect to see the "cul-de-sac" symbol layer interfering the efficiency as well. One location where this can be observed can be found at https://localhost:9966/debug/terrain-debug.html#15.92/37.706061/-122.43163/132/45. (The following artefact in cul-de-sac rendering is fixed by this PR.)

These special cases might be insignificant when it comes to the overall performance of the render cache efficiency. Have you been able to measure the average efficiency for different camera paths? This information would be very valuable in gl-native side as well!
An alternative approach would be to allocate each possibly renderable batch in between undraped layers for a render cache entry. It does not seem worthwhile due to the observations above and the complexity involved with allowing multiple cache entries per tile.

Would it be possible to choose the largest batch out of multiple candidates?

src/render/painter.js

astojilj · 2021-01-07T13:21:10Z

If draw order is preserved always, do we need forceRenderCached and two rendering modes?

I'm concerned about performance. Used this for benchmark (3440 x 1440 external display on MacBook Pro (15-inch, 2018), Radeon Pro 560X 4 GB).

main branch (forceDrapeFirst = true): 60 FPS (this is used when panning map)
main branch (forceDrapeFirst = false): 17 FPS (static mode render frame)
karim/fix-10186 (forceRenderCached = true): 21 FPS (when panning map; could it be also static/only one mode?)

I think this is a way to go. However don't think we can enable it until panning is 60 FPS. Unless we want to force layers reorder until reaching user defined layer always, we can try address this by having precomputed heuristics about style layers that are safe to be reordered (safe as in not producing artifacts) for each style.

What style is used in before/after videos?

karimnaaji · 2021-01-07T17:56:51Z

Thanks for testing @astojilj, what happens is that streets-v11 has a symbol layer named 'tunnel-oneway-arrow-blue' that only appears at very high zooms (so that this layer becomes interleaved at high zooms which breaks the assumption about the draped layers being nicely packed..). The render cache efficiency becomes ~30% at these zooms, this went through my testing due to not considering hidden layers in the cache efficiency, so thanks for finding that! (which is also what @mpulkki-mapbox has found in 1.)

An alternative approach would be to allocate each possibly renderable batch in between undraped layers for a render cache entry. It does not seem worthwhile due to the observations above and the complexity involved with allowing multiple cache entries per tile.

Would it be possible to choose the largest batch out of multiple candidates?

Yes due to the above it might be worth investigating that option as well.

So I guess we're left with these alternative approaches for draw order correctness (during interactive/moving frames):

(1) Cache each renderable batch of sequential draped
(2) Cache draped first until reaching user defined layers
(3) Cache largest renderable batch of sequential draped

While this approach is:

(4) Cache draped layers until reaching first non draped

I think (2) is worth looking at, as this issue is most immediate and strongly visible for user defined layers that are not part of the style (programmatically added). We can investigate (1) and (3) otherwise.

precomputed heuristics about style layers that are safe to be reordered (safe as in not producing artifacts)

That's a good idea but I wonder how hard it would become to assess which artifacts are strong enough to reject the layer from being reordered, unless there is some kind of user defined control for it.

What style is used in before/after videos?

The example from the video is from https://docs.mapbox.com/mapbox-gl-js/example/visualize-population-density/, but we really have that issue for any layer user-defined, while style layer that are in-place have minimal visual impact (cul-de-sac example, one way arrow...).

karimnaaji · 2021-01-07T23:24:27Z

@astojilj @mpulkki-mapbox here are some more details about what approach (1) or (3) may look like, it's a list of cacheable draped layer batches (for the set of styles streets-v11, satellite-streets-v11, light-v10, dark-v10, outdoors-v11, mapbox/cjerxnqt3cgvp2rmyuxbeqme7, mapbox-map-design/ckhqrdj3q0q1y19ko8t70qtqu, mapbox-map-design/ckhqrf2tz0dt119ny6azh975y, mapbox-map-design/ckhqrbxlc1awj19svtb92m0bd):

streets-v11, layer count: 113

batch 0: {start: 0, end: 26}
batch 1: {start: 30, end: 33}
batch 2: {start: 35, end: 55}
batch 3: {start: 57, end: 59}
batch 4: {start: 63, end: 78}
batch 5: {start: 80, end: 86}
batch 6: {start: 88, end: 93}

satellite-streets-v11, layer count: 71

batch 0: {start: 0, end: 9}
batch 1: {start: 13, end: 28}
batch 2: {start: 32, end: 40}
batch 3: {start: 42, end: 46}
batch 4: {start: 48, end: 53}

light-v10, layer count: 71

batch 0: {start: 0, end: 70}

dark-v10, layer count: 85

batch 0: {start: 0, end: 70}

outdoors-v11, layer count: 123

batch 0: {start: 0, end: 32}
batch 1: {start: 36, end: 62}
batch 2: {start: 64, end: 66}
batch 3: {start: 69, end: 86}
batch 4: {start: 88, end: 94}
batch 5: {start: 96, end: 102}

mapbox/cjerxnqt3cgvp2rmyuxbeqme7, layer count: 61

batch 0: {start: 0, end: 11}
batch 1: {start: 14, end: 17}
batch 2: {start: 19, end: 32}
batch 3: {start: 35, end: 42}

mapbox-map-design/ckhqrdj3q0q1y19ko8t70qtqu, layer count: 74

batch 0: {start: 0, end: 56}

mapbox-map-design/ckhqrf2tz0dt119ny6azh975y, layer count: 81

batch 0: {start: 0, end: 66}

mapbox-map-design/ckhqrbxlc1awj19svtb92m0bd, layer count: 74

batch 0: {start: 0, end: 56}

It is not immediately obvious that we would benefit to go with (1) for correctness of draw order, as some of these styles may still require a lot of batches. This also does not look promising for (3) as well, as the largest batch may not be large large enough to warrant caching when the style is really interleaved.

For (4) here's the updated render cache efficiency from this PR which confirms only rendering the first batch isn't possible for performance, it's now inclusive of hidden layers that may usually appear at high zooms (higher is better):

streets-v11: 31.76470588235294%
satellite-streets-v11: 20.833333333333336%
light-v10: 100%
dark-v10: 100%
mapbox/cjerxnqt3cgvp2rmyuxbeqme7: 31.57894736842105%
mapbox-map-design/ckhqrdj3q0q1y19ko8t70qtqu: 100%
mapbox-map-design/ckhqrf2tz0dt119ny6azh975y: 100%
mapbox-map-design/ckhqrbxlc1awj19svtb92m0bd: 100%

For (2) Always drape first until reaching user defined layers, I wonder if this warrants minimal control, what do you think of extending addLayer like so if we were to choose that approach:

addLayer(..., optimizeDrawOrder?: boolean) // name TBD, maybe allowCaching?

Having this flag defaulting to true (current behavior), and otherwise prevents this layer to be moved/reordered during caching phases, allowing user defined layers draw order to be honored, while not damaging performance for other in-place style layers.

ivovandongen · 2021-01-08T14:48:24Z

@karimnaaji I'm a little surprised there are so many layers breaking the draping in strict order btw. Is this something that is needed? Or should we also look into the styles and see if this can be resolved?

karimnaaji · 2021-01-08T19:32:50Z

@karimnaaji I'm a little surprised there are so many layers breaking the draping in strict order btw.

Yes I believe streets and outdoors are the worst case scenario at z+17 from the styles I have tested, some non-draped layers become interleaved at these zooms due to the hidden property being dependent on the zoom, at low zoom these layers have a nice ordering of sequential draped then non-draped.

Is this something that is needed? Or should we also look into the styles and see if this can be resolved?

The major use case this is needed for is user defined layers. For basemap styles, we've worked with map design to make a few styles nicely ordered and optimized for use with the render cache drape first approach in interactive mode (3D Terrain RGB Outdoors, 3D Terrain RGB Outdoors, 3D Terrain Satellite). While it's possible to fix it in the style, it has two main disadvantages:

Non curated mapbox styles still need reordering, I could imagine to introduce an optimize for 3d kind of option in studio that automatically reorders styles to not break draw order, or even layer scoped flag.
Does not work for programmatically added layers (only layers that are part of the style would have correct draw order) this is related to the suggestion of option (2) from above, some of the examples we've seen are usually weather based layers, overlay polygons on top of our base maps, or custom layers, some of the main examples I've seen where this breaks through:
- https://twitter.com/i/status/1337083656934883329
- https://vintage-mapper.glitch.me/
- sf isochrones (road labels)
- weather data

re: approach (1) and (3): I prototyped both of these approaches (reminder that it's Cache each renderable batch of sequential draped and Cache largest renderable batch of sequential draped).

Caching each renderable batch is better than only caching the largest one, and while this fixes draw order issue, it does not come close to main performance in both cases but is better than this PR. On a very large viewport (Intel Iris Plus Graphics 655), streets-v11 z+17 (one of the worst case):

(1): 40 FPS
(main): 60 FPS
(4): 30 FPS

@mpulkki-mapbox , @astojilj , @ivovandongen Considering the above I'd like to discuss more proposition (2) Always drape first until reaching user defined layers, for programmatic styles add a user defined flag controlling whether or not to drape first. There might be other options that improves draw order that I have not considered, so please let me know!

karimnaaji · 2021-01-11T22:29:29Z

cc @ansis @mourner @arindam1993 for feedback on the potential solution targeting user defined layers (especially concerns around style reloading/diffing).

arindam1993 · 2021-01-12T05:07:28Z

This may not be a popular opinion, but I'm always in favor of correctness and determinism even if there is a slight performance impact.

I think (1) (multiple fbo's for contiguous sets of drapeable layers) is the way to go, the performance impact is the least, and the path to optimizing it in a style is very clear. We can totally provide warning messages with exact layer-ids and recommend pushing them to the top.
Our current streets/outdoors styles arent great with terrain anyways, and we can account for this optimization when developing terrain specific styles.

I also don't think we should distinct programmatically added layers from initially defined layers, we should always treat the style as a single declarative thing. And in a use case like studio there isn't really a distinction between whats user defined and whats not.

astojilj · 2021-01-12T16:14:53Z

@karimnaaji , @arindam1993

It is good to address here approach for existing styles as well as discussing set of rules to integrate to Studio for later (as @arindam1993 mentioned) terrain specific styles.

I might overemphasize the importance of use case "User new to Mapbox, wants to have high performance terrain map". So, I'll focus on existing vanilla styles with one user defined layer.

precomputed heuristics about style layers that are safe to be reordered (safe as in not producing artifacts)

That's a good idea but I wonder how hard it would become to assess which artifacts are strong enough to reject the layer from being reordered, unless there is some kind of user defined control for it.

In this case, I was referring to no visual artifacts, order correct results. You might have understood it as acceptable artifacts.

Heuristics would cover this knowledge for existing styles:

E.g. tunnel symbols (tunnel-oneway-arrow-blue, tunnel-oneway-arrow-white) come with significant performance penalty here as we have theoretically have drape-immediate-drape-immediate-drape framebuffer switches and expensive GPU load store. In streets style (no translate) can tunnel symbols be safely reordered to be after road lines and together with road symbols? I have never seen tunnel symbol under road lines or road symbols (collision check doesnt allow it) - could it happen with vanilla styles?

Same screenshot: beforementioned turning-feature-outline we can resolve (drape symbol while preventing cut on border issue).

Next performance issue:

We have a lot of breaks of drape-immediate sequences here and golf, road and bridges don't appear in the same place on map. Seems safe to group symbols together.

cc @mapbox/map-design-studio

Performance cost is subjective. For some cases 30 FPS is acceptable, for others only 90+. This is also directly related to CPU consumption/battery life/how warm is device when using map/ too. We must have option to enable 90+ FPS while keeping user defined layers rendered correctly.

addLayer(..., optimizeDrawOrder?: boolean) // name TBD, maybe allowCaching?

Seems complicated to handle it with layer granularity.
Regardless what we do about heuristics mentioned above, I'd prefer MapOptions.optimizeForTerrain that would reorder all except user defined layers. For user defined layers (custom, canvas) we will render correctly but warn user that performance would be better if layer is inserted after layer "id of the last draped layer".
As there's explicit flag, we would render it the same way when camera is moving and in stabile mode. For that we just need to fix cut on border artifacts for cul-de-sac when it is draped.

karimnaaji · 2021-01-13T02:10:57Z

@arindam1993

I think (1) (multiple fbo's for contiguous sets of drapeable layers) is the way to go, the performance impact is the least, and the path to optimizing it in a style is very clear. We can totally provide warning messages with exact layer-ids and recommend pushing them to the top.

Definitely worth looking more at this option, something that may come up is making higher use of texture space. As we would be caching draped batches instead of all draped for a single tile (tile <=> 1 framebuffer in main, whereas we would have tile <=> number of batch framebuffers). We're already quite tight on framebuffer memory on mobile so we'd have to be careful on that.

@astojilj

In this case, I was referring to no visual artifacts, order correct results. You might have understood it as acceptable artifacts.

Yes, that's how I understood this, this makes sense to approach it as you proposed. Especially if we provide some sort of warning to determine best placement.

Seems complicated to handle it with layer granularity.
Regardless what we do about heuristics mentioned above, I'd prefer MapOptions.optimizeForTerrain that would reorder all except user defined layers. For user defined layers (custom, canvas) we will render correctly but warn user that performance would be better if layer is inserted after layer "id of the last draped layer".

Thinking more about that I agree, I like the option you proposed MapOptions.optimizeForTerrain:

MapOptions.optimizeForTerrain == true:
- use render cache for a single batch with drape-first-always method for performance priority
MapOptions.optimizeForTerrain == false:
- use render cache for sequential batches of draped method for draw order priority

As there's explicit flag, we would render it the same way when camera is moving and in stabile mode. For that we just need to fix cut on border artifacts for cul-de-sac when it is draped.

👍

arindam1993 · 2021-01-13T02:16:13Z

We can also use one large framebuffer woth each stack tiled inside it, this would take away the cost of switching framebuffers constantly.

@mpulkki-mapbox aren't you doing something like this in native?

Minor syntax tweak

This should be addressed in separate PR, needs to take into account video source type and feature state changes in order to invalidate the render cache.

karimnaaji · 2021-01-19T20:38:11Z

@astojilj @arindam1993 @mpulkki-mapbox , updated the PR as per discussion and suggestions to have optimizeForTerrain map option as well as method (1). The optimization method is now explicit through this option instead of depending on the map state, and is set to cache sequential batches of draped layers while using draw order priority.

Other work from native will help improve batching performance in draw order priority:

Improved FBO placement and allocation through atlasing in 4k textures instead of 1k from @mpulkki-mapbox
Drape potential offenders to maximize draped batch sizes and reduce penalty of interleaving symbols from @astojilj

karimnaaji · 2021-01-19T20:40:03Z

debug/terrain-debug.html

@@ -177,11 +178,87 @@
            ]
        }
    });
+
+    var highlightLayer = {


Added terrain-debug demo with a new custom layer for manual testing and debugging.

karimnaaji · 2021-01-19T20:40:39Z

debug/10186.html

@@ -0,0 +1,131 @@
+<!DOCTYPE html>


This is a simple example with very few layers to test draw ordering.

karimnaaji · 2021-01-19T20:41:32Z

src/terrain/draw_terrain_raster.js

@@ -174,6 +174,8 @@ function drawTerrainRaster(painter: Painter, terrain: Terrain, sourceCache: Sour
 }

 function drawTerrainDepth(painter: Painter, terrain: Terrain, sourceCache: SourceCache, tileIDs: Array<OverscaledTileID>) {
+    assert(painter.renderPass === 'offscreen');


Moved depth drawing in offscreen pass to reduce switching framebuffer from main as suggested by @astojilj .

karimnaaji · 2021-01-19T20:44:42Z

src/terrain/terrain.js

@@ -380,7 +398,8 @@ export class Terrain extends Elevation {
        }

        const options = this.painter.options;
-        this.drapeFirst = (options.zooming || options.moving || options.rotating || !!this.forceDrapeFirst) && !this._invalidateRenderCache;
+        this.renderCached = (options.zooming || options.moving || options.rotating || !!this.forceRenderCached) && !this._invalidateRenderCache;


We could remove check against options.zooming || options.moving || options.rotating. I opted to leave as is in this PR, invalidation of cache will need to happen on video update event and feature state to make that work properly. I'll create a follow-up ticket for this.

karimnaaji · 2021-01-19T20:46:01Z

src/terrain/terrain.js

@@ -580,34 +598,17 @@ export class Terrain extends Elevation {
        program.setTerrainUniformValues(context, uniforms);
    }

-    // If terrain handles layer rendering (rasterize it), return true.
-    renderLayer(layer: StyleLayer, _?: SourceCache): boolean {


Removed this method and replaced it with batched rendering in the main render loop, the reason is to make the render commands and drawn layer range more explicit while reducing potential branching.

nice, this is much clearer 💯

src/terrain/terrain.js

src/style/style.js

src/render/painter.js

src/style/style.js

arindam1993 · 2021-01-19T21:16:36Z

src/terrain/terrain.js

        style.on('data', this._onStyleDataEvent.bind(this));
+        style.on('neworder', this._checkRenderCacheEfficiency.bind(this));


Do we really need this neworder event? I think its fine to not have it

I would also prefer without, but this event is only necessary when order changes after setting the style in terrain, I'll look into ways to bypass that but we might have to keep this one.

arindam1993

Just one major nit with.hasLayerType looping over all layers constantly, other than that changes looking v good @karimnaaji !

@arindam1993

Thanks @arindam1993

arindam1993

LGTM, nice work!

karimnaaji requested review from mpulkki-mapbox and arindam1993 January 5, 2021 23:57

karimnaaji added 12 commits January 5, 2021 15:59

Update 3d playground with more styles to test from

1ff96f1

Add helper function to calculate render cache efficiency

45ff31e

Update render test baseline with correct terrain drawn order

ef041ad

Update 3d playground example

3876a6b

- Add mapbox/satellite-streets-v11 style - Add back mistakenly removed fill-extrusion layer

Add render tests

2198dfd

layerEnd -> lastDrawnLayerIndex

fdd8871

Self review: Comment + renaming

ffa8ff8

Check render cache efficiency on style order changes

a29f4a6

Add more information on render cache efficiency warning

b3250a4

Fix wrong variable naming and unit test to not warn

d7251fc

Add unit tests

de39709

karimnaaji force-pushed the karim/fix-10186 branch from 5929701 to de39709 Compare January 6, 2021 00:00

asheemmamoowala linked an issue Jan 6, 2021 that may be closed by this pull request

Enabling terrain may not preserve draw order of layers #10186

Closed

astojilj reviewed Jan 7, 2021

View reviewed changes

src/render/painter.js Outdated Show resolved Hide resolved

Calculate list of sequential draped render batches

d9925cb

karimnaaji added 10 commits January 15, 2021 17:25

Don't render empty batches

52f803c

Fix bug not clearing raster fade if any source is not RasterTileSource

6be8ad5

Minor syntax tweak

Use draped render batches in terrain.render()

a675920

Simplify render loop

678ffed

Move terrain depth render earlier to reduce framebuffer switches

253d823

Update terrain-debug to add custom layer for debugging

f6eb4eb

Add map.options.optimizeForTerrain and update unit tests

188e699

Update render tests to account for map.options.optimizeForTerrain

0c90763

Fix Flow + Lint + render tests

b6feebe

Remove extraneous flag

d0419ac

karimnaaji requested a review from astojilj January 19, 2021 15:53

karimnaaji added 3 commits January 19, 2021 10:09

Add documentation and update comments

76cfd59

Add warning and guidance on low render cache efficiency

3fcf092

Revert using cached mode always

e1701d8

This should be addressed in separate PR, needs to take into account video source type and feature state changes in order to invalidate the render cache.

karimnaaji commented Jan 19, 2021

View reviewed changes