[core] fix flickering caused by incorrect ClipIDs #12982

mollymerp · 2018-09-27T02:49:11Z

fix #12981

need to add unit test and possibly render tests

LukasPaczos · 2018-09-27T10:41:59Z

I tested this one quickly and can't reproduce issues mentioned in #11877 🚀

mb12 · 2018-09-27T14:04:37Z

@mollyperp Can you please kindly clarify the following?

Is it possible to explain why/how incorrect clip id results in flickering visual artifacts? Is the flicker result of an over zoomed parent tile overlapping onto some other tile because it has not been clipped?
Also is it possible to why clipping is needed for correct rendering ( at a logical level) and how it works?

mourner · 2018-09-27T16:49:43Z

@mollymerp I'm wondering — could this also fix #12452? That problem also seems related to clip ids (a flicker of buffers outside of tiles on zoom).

mollymerp · 2018-09-27T18:58:07Z

@mourner yep looks like it could be the same issue. will check and let you know.

mollymerp · 2018-09-27T19:10:05Z

@mourner just confirmed this change also fixes #12452 🎉

sansumbrella · 2018-10-01T18:10:10Z

src/mbgl/algorithm/generate_clip_ids_impl.hpp

@@ -64,7 +64,7 @@ void ClipIDGenerator::update(std::vector<std::reference_wrapper<Renderable>> ren
        uint8_t count = 1;
        for (auto& it : renderables) {
            auto& renderable = it.get();
-            if (!renderable.used) {
+            if (!renderable.used || !renderable.needsClipping) {


Any feeling on !(renderable.used && renderable.needsClipping) instead? It has the same results as the above expression for all four bool, bool permutations.

Interesting nit @sansumbrella — I'm curious about your reasoning behind this comment? In general I find code reviews to be most helpful when they express suggestions about improving functionality, efficiency, or readability, rather than simply suggesting a different way to write the same thing (with the same or, theoretically, slightly worse performance) — unless there's some other motivation behind this suggestion, in which case please share such reasoning while reviewing!

I personally prefer how the existing code is with (!renderable.used || !renderable.needsClipping). The logic is easier to understand when glancing at it.

Sorry to parachute in earlier; this is a simple readability question.

I find compound conditionals a little head-scratchy no matter how they are written, and a series of ands is easier for me to read than a series of !ors. But as @lbud pointed out, the short-circuit in the !ors sequence works in favor of the common case here—where we expect to perform clipping.

I'll provide more context on my reviews in the future; and will also introduce myself so I'm not a stranger jumping on your work. I'm excited to see this fix, since I saw the issue in the sales/customer slack channels.

kkaefer

Confirmed that the test produces the overflowing ref values that we've identified as the cause for the failed clipping before this patch is applied:

{ Renderable{ 7/36/49+0, mask=00000011,ref=00000100 },
  Renderable{ 7/36/48+0, mask=00000011,ref=00000011 },
  Renderable{ 7/35/48+0, mask=00000011,ref=00000001 },
  Renderable{ 7/35/49+0, mask=00000011,ref=00000010 },
  Renderable{ 7/37/48+0, mask=00000011,ref=00000101 },
  Renderable{ 7/37/49+0, mask=00000011,ref=00000110 } }

Notice how the ref value goes all the way to 00000110, overflowing the mask 00000011.

kkaefer · 2018-10-02T14:33:56Z

Bug was introduced in #10198

we missed a check for RenderTile.needsClipping when generating the new ClipIDs for some tiles, resulting in incorrect clipping that caused flickering in some cases

…enerator

mollymerp · 2018-10-02T19:51:58Z

@kkaefer thanks for the review – can you also 👍 the change in 62061e1 before I merge?

friedbunny

Confirmed that this fixes all of the issues we were seeing in the iOS examples — thanks! (I couldn’t help myself with a tiny changelog nit. 🧐)

friedbunny · 2018-10-02T20:52:45Z

platform/ios/CHANGELOG.md

@@ -10,6 +10,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
 * Added an `MGLSymbolStyleLayer.symbolZOrder` property for forcing point features in a symbol layer to be layered in the same order that they are specified in the layer’s associated source. ([#12783](https://github.com/mapbox/mapbox-gl-native/pull/12783))
 * Fixed a crash when a style layer `*-pattern` property evaluates to nil for a particular feature. ([#12896](https://github.com/mapbox/mapbox-gl-native/pull/12896))
 * Fixed an issue with view annotations (including the user location annotation) and the GL map lagging relative to each other. ([#12895](https://github.com/mapbox/mapbox-gl-native/pull/12895))
+* Fixed an issue where fill and line layers would occasionally flicker on zoom due to a bug in `ClipIDGenerator` ([#12982](https://github.com/mapbox/mapbox-gl-native/pull/12982))


Small nit: since these changelogs are meant to be end-developer facing, it’s usually not necessary to mention internal implementation details. In this case, I’d suggest lopping off the “due to a bug in ClipIDGenerator” part, as that’s only relevant to us.

ansis

62061e1 looks good to me

mollymerp added the Core The cross-platform C++ core, aka mbgl label Sep 27, 2018

mollymerp requested a review from kkaefer September 27, 2018 02:50

friedbunny added the rendering label Sep 27, 2018

mourner mentioned this pull request Sep 27, 2018

Drawing of layers changes randomly with zoom in and zoom out #12452

Closed

friedbunny added the needs changelog Indicates PR needs a changelog entry prior to merging. label Sep 27, 2018

mollymerp force-pushed the flicker-fix branch from d84d233 to 8c00aba Compare October 1, 2018 17:18

sansumbrella reviewed Oct 1, 2018

View reviewed changes

kkaefer mentioned this pull request Oct 2, 2018

MGLComputedShapeSource renders clipped features #12995

Closed

kkaefer approved these changes Oct 2, 2018

View reviewed changes

Molly Lloyd added 3 commits October 2, 2018 10:45

[core] fix flickering caused by incorrect ClipIDs

a90d310

we missed a check for RenderTile.needsClipping when generating the new ClipIDs for some tiles, resulting in incorrect clipping that caused flickering in some cases

[core] don't prefetch AnnotationTiles

8d5b29a

[test] add test checking that unclipped tiles are not used by ClipIDG…

865f66c

…enerator

mollymerp force-pushed the flicker-fix branch from 8c00aba to 96cf0d0 Compare October 2, 2018 18:25

[core] only generate clip ids for clipped layers

62061e1

mollymerp force-pushed the flicker-fix branch from 96cf0d0 to 62061e1 Compare October 2, 2018 18:39

mollymerp requested a review from friedbunny October 2, 2018 19:37

friedbunny removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Oct 2, 2018

friedbunny approved these changes Oct 2, 2018

View reviewed changes

[ios, macos, android, node] update changelogs

dde5b05

mollymerp force-pushed the flicker-fix branch from 9f29530 to dde5b05 Compare October 2, 2018 21:00

ansis approved these changes Oct 3, 2018

View reviewed changes

mollymerp merged commit f4ffcaa into master Oct 3, 2018

mollymerp mentioned this pull request Oct 3, 2018

cherry pick #12982 to release-gazpacho #13025

Merged

mollymerp deleted the flicker-fix branch October 3, 2018 20:15

This was referenced Oct 9, 2018

[Android] LineLayer not appearing on map #12990

Closed

Mapbox Android Demo app StyleLineIdentityPropertyActivity cause incorrect map rendering #11686

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] fix flickering caused by incorrect ClipIDs #12982

[core] fix flickering caused by incorrect ClipIDs #12982

mollymerp commented Sep 27, 2018

LukasPaczos commented Sep 27, 2018

mb12 commented Sep 27, 2018

mourner commented Sep 27, 2018

mollymerp commented Sep 27, 2018

mollymerp commented Sep 27, 2018

sansumbrella Oct 1, 2018

lbud Oct 1, 2018

dsuresh-ap Oct 1, 2018

sansumbrella Oct 2, 2018

kkaefer left a comment

kkaefer commented Oct 2, 2018

mollymerp commented Oct 2, 2018

friedbunny left a comment

friedbunny Oct 2, 2018 •

edited

Loading

ansis left a comment

[core] fix flickering caused by incorrect ClipIDs #12982

[core] fix flickering caused by incorrect ClipIDs #12982

Conversation

mollymerp commented Sep 27, 2018

LukasPaczos commented Sep 27, 2018

mb12 commented Sep 27, 2018

mourner commented Sep 27, 2018

mollymerp commented Sep 27, 2018

mollymerp commented Sep 27, 2018

sansumbrella Oct 1, 2018

Choose a reason for hiding this comment

lbud Oct 1, 2018

Choose a reason for hiding this comment

dsuresh-ap Oct 1, 2018

Choose a reason for hiding this comment

sansumbrella Oct 2, 2018

Choose a reason for hiding this comment

kkaefer left a comment

Choose a reason for hiding this comment

kkaefer commented Oct 2, 2018

mollymerp commented Oct 2, 2018

friedbunny left a comment

Choose a reason for hiding this comment

friedbunny Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

ansis left a comment

Choose a reason for hiding this comment

friedbunny Oct 2, 2018 •

edited

Loading