Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #10763 #10767

Merged
merged 6 commits into from
Jun 15, 2021
Merged

Fix #10763 #10767

merged 6 commits into from
Jun 15, 2021

Conversation

karimnaaji
Copy link
Contributor

@karimnaaji karimnaaji commented Jun 11, 2021

Fixes #10763 "Map flickers when optimizeForTerrain is disabled, fog is enabled, and a GeoJSON layer is present"

This flickering issue was visible in draw order priority mode, as we split terrain rendering in interleaved render batches between draped/non-draped layers. Currently, fog is applied without considering alpha in terrain_raster.fragment.glsl, and this works when terrain is rendered as a single draw call, but not when we split terrain rendering in several batches and compositing is required.

In the example provided in #10763 , we were rendering fog as many times as we had render batches without correct compositing (e.g. alpha blended), which led to various rendering artifacts when any of these batch had empty space in the tile, refer below with the overly saturated render due to multiple pass of fog applied:

Screen Shot 2021-06-10 at 5 16 39 PM

The issue is fixed by accounting for alpha in fog in the draw terrain raster pass.

This PR also simplifies some of logic no longer required by removing unnecessary forceRenderCached and checks against options.zooming || options.moving || options.rotating to have consistent code path on still frames.

cc @jczaplew

changelog: <changelog>Fix a bug where the map flickers with fog when optimizeForTerrain is disabled</changelog>

@jczaplew
Copy link
Contributor

Thank you for such a quick remedy @karimnaaji!

Copy link
Contributor

@rreusser rreusser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for the additional cleanup! I just had a small background question about the render cache, but nothing blocking from my point of view. 👏

src/shaders/_prelude_fog.fragment.glsl Show resolved Hide resolved
@@ -401,10 +398,6 @@ export class Terrain extends Elevation {
this._emptyDEMTextureDirty = !this._initializing;
}

const options = this.painter.options;
this.renderCached = (options.zooming || options.moving || options.rotating || !!this.forceRenderCached) && !this._invalidateRenderCache;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this logic still exist, just elsewhere? If so, I didn't succeed in tracking it down.

Copy link
Contributor Author

@karimnaaji karimnaaji Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the logic for this._invalidateRenderCache is kept. The rest is not necessary anymore and adds complexity to the number of possible code paths, it's a reason why the flicker is only visible during moving frames in #10763.

To provide some context, we launched v2 having two terrain render modes being fairly implicit:

  • One render mode using the render cache during interactive frames, not preserving draw order
  • One render mode using interleaved draw order otherwise (no caching even for large draped sequences)

The reason being that interleaved rendering was too expensive and honoring draw order on still fames was a fine workaround. The issue with this approach is that still and moving fames were not rendering layers in the same order, leading to noticeable differences when the user stopped moving the camera for example.

In #10258, we moved away from this behavior to have an explicit opt-in render mode with optimizerForTerrain, allowing that decision to be based on the needs of the user depending on whether draw order is important, instead of being implicit to the renderer.

In draw order priority, we still do our best to identify sequences of draped batches to render them cached. Now, this particular check 'options.zooming || options.moving || options.rotating' is what determines still vs moving frames, but we can simply render cache always as it doesn't have any effect anymore: with or without draw order priority we are always rendering the same frames once the render mode choice is made.

@karimnaaji karimnaaji merged commit 9db7de7 into main Jun 15, 2021
@karimnaaji karimnaaji deleted the karim/10763 branch June 15, 2021 15:35
rreusser pushed a commit that referenced this pull request Jun 17, 2021
* Fix #10763

* Simplify some code path, remove extraneous code

* Remove debug code

* Add render test

* Address review comment

* Leave render test operation handler for gl-native
rreusser added a commit that referenced this pull request Jun 17, 2021
* Increase strictness of style API validation. (#10779)

* Remove strictly-increasing fog range validation (#10772)

* Fix #10763 (#10767)

Co-authored-by: Saman Bemel-Benrud <[email protected]>
Co-authored-by: Karim Naaji <[email protected]>
SnailBones pushed a commit to SnailBones/mapbox-gl-js that referenced this pull request Jul 15, 2021
* Fix mapbox#10763

* Simplify some code path, remove extraneous code

* Remove debug code

* Add render test

* Address review comment

* Leave render test operation handler for gl-native
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map flickers when optimizeForTerrain is disabled, fog is enabled, and a GeoJSON layer is present
3 participants