reduce symbol overlap when zooming out quickly #8628

ansis · 2019-08-13T21:24:31Z

fix #8627

When zooming out quickly labels could overlap for a lengthly period of time and produce and unwanted visual effect.

This PR reduces this overlap by:

reducing the time between collision calculations when zooming out quickly
reducing the fade duration when zooming out quickly

The collisions are discovered more quickly and eliminated more quickly.

Before:
https://bl.ocks.org/ansis/6d6aa89b8a5dbddb5e2031a29451c767

After:
https://bl.ocks.org/ansis/12665c0c885b6af9ed81c44fa7177078

After without automatic zooming
https://bl.ocks.org/ansis/6ad5fb3402d4ddec926b52af7f3f73fe

Launch Checklist

briefly describe the changes in this PR
post benchmark scores
manually test the debug page

asheemmamoowala

I played with these changes and found that on a macbook pro touchpad it still shows the overlap when zooming out fast. I'm not sure that this would be easy to reproduce on other platforms where the input devices are calibrated differently.

While this change performs well with the current mapbox streets style, would it cause jank on heavier styles, or style that use more dense tiles? In that case, should it be an option to configure this behavior?

asheemmamoowala · 2019-08-14T05:36:42Z

src/symbol/placement.js

+    }
+
+    zoomAdjustment(zoom: number) {
+        // When zooming out quickly labels can overlap each quickly. This


nit: typo can overlap each quickly

asheemmamoowala · 2019-08-14T05:39:42Z

src/symbol/placement.js

+        // When zooming out quickly labels can overlap each quickly. This
+        // adjustment is used to reduce the fade duration when zooming out quickly and
+        // to reduce the interval between placement calculations. Discovering the
+        // collisions more quickly and fading them more quickly reduces the unwanted effect.


Discovering the collisions more quickly and fading them more quickly reduces the unwanted effect.

nit: This part of the comment can be removed, or make it clearer. It is not clear how zoomAdjustment results in discovering the collisions more quickly.

It is not clear how zoomAdjustment results in discovering the collisions more quickly.

+1 to this - it took me reading through all the code to understand how the adjustment works. Perhaps merging the "Discovering" sentence into the previous one would make it clearer, something like:

"This adjustment is applied when zooming out quickly to reduce the interval between placement calculations so we can discover & correct label collisions sooner, and also to reduce the fade duration so that overlapping labels fade out more quickly."

Just a suggestion, not sure if that wording would make it any clearer to someone else.

As is, it seems relatively clear to me that "reducing the interval between placement calculations" discovers collisions more quickly and "reducing the fade duration" fades them more quickly. I agree with @vakila that keeping the order of the clauses in the last sentence parallel to the previous sentence would improve clarity. I think shorter and simpler sentences are generally easier to follow than longer ones.

// When zooming out quickly, labels can overlap each other. This // adjustment is used to reduce the interval between placement calculations // and to reduce the fade duration when zooming out quickly. Discovering the // collisions more quickly and fading them more quickly reduces the unwanted effect.

Thanks for all the suggestions on how to make this better! I've committed chloe's version with shorter sentences.

chloekraw · 2019-08-14T07:11:40Z

Thanks for investigating this, @ansis!

I played with these changes and found that on a macbook pro touchpad it still shows the overlap when zooming out fast. I'm not sure that this would be easy to reproduce on other platforms where the input devices are calibrated differently.

@asheemmamoowala I noticed the same thing using my macbook and touchpad, although I also found it to be a noticeable improvement from previous behavior.

@ansis, assuming the same approach works on native, could you open up a pr there too and allow us to test this change on mobile, where the reported behavior is a bit more urgent?

While this change performs well with the current mapbox streets style, would it cause jank on heavier styles, or style that use more dense tiles? In that case, should it be an option to configure this behavior?

Not sure about the first question, but on the second:

My understanding is this PR is a stopgap measure that reduces the incidence of the reported behavior and that it will be replaced with a root cause fix. It wouldn't make sense to expose a public API for a temporary measure, unless we have reason to believe a root cause fix is not really possible.
Even so: as a user, this is the type of decision I would expect Mapbox to get right, not something I'd like to configure and spend cycles testing with every style change. If we find that this causes a significant amount of jank on heavier styles, my inclination would be to not merge this PR.

ansis · 2019-08-14T18:23:18Z

I pushed another fix that improves this slightly further. I was using the zoom of when the placement is finished instead of started to do the adjustment which sometimes lead to regular fading after a fast zoom. This seems to help especially for zooms that load new tiles

While this change performs well with the current mapbox streets style, would it cause jank on heavier styles, or style that use more dense tiles? In that case, should it be an option to configure this behavior?

On -js, placement work is limited to 2ms each frame. This change means that we might have slightly more frames that pay that 2ms price. So if those new placements are pushing some frames over their frame budget, there were probably already many other frames that were already too long.

I played with these changes and found that on a macbook pro touchpad it still shows the overlap when zooming out fast. I'm not sure that this would be easy to reproduce on other platforms where the input devices are calibrated differently.

Yep, the limiting factors are:

placement isn't fast enough to do it instantly
we want to do some fading (some others do no fading)
we want to do placement at fractional zooms
we do not know if symbols will be in parent tiles

Looking closely there are also sometimes some frames where I see the effect but labels aren't actually colliding. Sometimes it's just increased density from zooming contrasting with a blank background before being replaced with a tile with lower density data.

In that case, should it be an option to configure this behavior?

My understanding is this PR is a stopgap measure that reduces the incidence of the reported behavior and that it will be replaced with a root cause fix.

It's a bit of both. The "real" fix would be cheap placement that we can run on every frame. But I think we would still need the fading acceleration here to avoid the effect.

Even so: as a user, this is the type of decision I would expect Mapbox to get right, not something I'd like to configure and spend cycles testing with every style change.

I completely agree. We shouldn't expose this.

asheemmamoowala · 2019-08-14T23:14:47Z

On -js, placement work is limited to 2ms each frame. This change means that we might have slightly more frames that pay that 2ms price. So if those new placements are pushing some frames over their frame budget, there were probably already many other frames that were already too long.

With the latest changes, I am seeing significant jank when zooming out fast over Tokyo. This is even worse when viewing a pitched map. This is likely to affect flyTo/easeTo animations as well.

we want to do some fading (some others do no fading)

For smooth animations, fading is important, but if the user is zooming really fast - what is the value of fading - especially if the trade off is between some fading - that causes jank + overlapping symbols, vs no fading - no jank, no overlap?

Even so: as a user, this is the type of decision I would expect Mapbox to get right, not something I'd like to configure and spend cycles testing with every style change.

I completely agree. We shouldn't expose this.

👍

vakila

@ansis The code changes look good to me (though I have a question/comment below), but in the examples/debug page I'm afraid I'm not reliably able to observe different behavior vs. the original. Here's what I'm seeing as I watch the zoom loop in the "after" example:

Firefox & Chrome on Ubuntu Thinkpad: Occasionally the labels disappear faster than in the original, as intended, but in many cases they seem to spend just as long on the screen as in the original. In other words, the collision/fade speed seems to vary from run to run of the zoom out loop (though I have no idea why that is), and only sometimes is actually noticeably faster than the original.
Chrome on (old, bad) Samsung Android phone: Tiles/labels load so slowly in general that the map is too sparse to notice any change in the looping example. And if I load the default debug page or the non-auto-zoom example and wait for everything to load before zooming, I can't seem to manually zoom out fast enough to appreciate the effect. So in either case I can't really notice any difference between the two versions.

That said, if this change sometimes improves the desktop experience & has no effect on mobile, I don't see any harm in merging it. Assuming there are no negative effects on perf benchmarks.

vakila · 2019-08-19T22:00:30Z

src/symbol/placement.js

+        // When zooming out quickly labels can overlap each quickly. This
+        // adjustment is used to reduce the fade duration when zooming out quickly and
+        // to reduce the interval between placement calculations. Discovering the
+        // collisions more quickly and fading them more quickly reduces the unwanted effect.


It is not clear how zoomAdjustment results in discovering the collisions more quickly.

+1 to this - it took me reading through all the code to understand how the adjustment works. Perhaps merging the "Discovering" sentence into the previous one would make it clearer, something like:

"This adjustment is applied when zooming out quickly to reduce the interval between placement calculations so we can discover & correct label collisions sooner, and also to reduce the fade duration so that overlapping labels fade out more quickly."

Just a suggestion, not sure if that wording would make it any clearer to someone else.

vakila · 2019-08-19T22:00:43Z

src/style/style.js

@@ -1215,7 +1215,7 @@ class Style extends Evented {
            this.pauseablePlacement.continuePlacement(this._order, this._layers, layerTiles);

            if (this.pauseablePlacement.isDone()) {
-                this.placement = this.pauseablePlacement.commit(browser.now());
+                this.placement = this.pauseablePlacement.commit(browser.now(), transform.zoom);


If the zoom value is coming from the same transform that we passed to the PausablePlacement constructor in L1204, why do we need to add it as an argument for the commit method (here as well on Placement)? Same question for the stillRecent method as well. What am I missing?

If the zoom value is coming from the same transform that we passed to the PausablePlacement constructor in L1204, why do we need to add it as an argument for the commit method (here as well on Placement)? Same question for the stillRecent method as well. What am I missing?

Thanks for catching this. The two values were different (one is when placement starts, the other is when placement ends). But using the one from when it starts is more correct.

stillRecent needs the latest zoom so that it can calculate how much the zoom has changed.

awulkan · 2019-08-22T07:06:48Z

Occasionally the labels disappear faster than in the original, as intended, but in many cases they seem to spend just as long on the screen as in the original.

@vakila For whatever it's worth, for me it's noticeably and consistently faster than the original. Using the latest Chrome and FireFox version on a PC with these specs: i7 8700, GTX 1060 3GB, 16GB DDR4.

- remove bug that eliminated all fading (left over from debugging) - make the formula match -native - only make placement more frequent after zooming has stopped

ansis · 2019-09-25T16:35:51Z

That said, if this change sometimes improves the desktop experience & has no effect on mobile, I don't see any harm in merging it. Assuming there are no negative effects on perf benchmarks.

Thanks for testing this on mobile @vakila !

I've pushed a couple adjustments.

improved comment
formula matches -native now (linear instead of exponential)
it only makes placement more frequent after zooming has stopped to avoid adding potential jank during zooming

If anyone could take a second look that would be great! I think this should be ready

asheemmamoowala

Between @vakila and I we have tested on Linux Firefox and Chrome, an older Android device, and a recent Macbook with Chrome's CPU throttling.

On recent hardware it shows a clear improvement in fading out labels faster when zooming out quickly. On older hardware there is not a consistent improvement, but also no degradation.

Let's merge this and track further improvements separately.

ansis requested a review from mourner August 13, 2019 21:24

chloekraw added this to the release-queso milestone Aug 13, 2019

asheemmamoowala reviewed Aug 14, 2019

View reviewed changes

vakila self-requested a review August 19, 2019 16:31

asheemmamoowala assigned ansis Aug 19, 2019

ansis mentioned this pull request Aug 19, 2019

[core] Fix placement for updated buckets mapbox/mapbox-gl-native#15308

Merged

vakila reviewed Aug 19, 2019

View reviewed changes

mourner modified the milestones: release-queso, release-ristretto Aug 22, 2019

vakila assigned ahk and vakila Sep 16, 2019

vakila mentioned this pull request Sep 17, 2019

[DNM] Further experiments with fading adjustments during zoom-out (#8627) #8764

Closed

3 tasks

kkaefer modified the milestones: release-ristretto, release-s Sep 24, 2019

ansis added 3 commits September 25, 2019 11:07

fix #8627

0e26fbd

use the zoom from when the placement started, not the commit zoom

5590c8b

improve zoom fading in several ways:

26cd733

- remove bug that eliminated all fading (left over from debugging) - make the formula match -native - only make placement more frequent after zooming has stopped

ansis force-pushed the fix-fade-zoom-out-8627 branch from d48e1f6 to 26cd733 Compare September 25, 2019 16:29

asheemmamoowala approved these changes Sep 26, 2019

View reviewed changes

ansis merged commit a0b5c3b into master Sep 30, 2019

kkaefer mentioned this pull request Oct 10, 2019

Symbols canned up when switching between high and low zoom levels mapbox/mapbox-gl-native#12356

Closed

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade mapbox-gl from 1.3.1 to 1.5.0 dyna-dot/iClient-JavaScript-s#9

Open

asheemmamoowala deleted the fix-fade-zoom-out-8627 branch January 8, 2020 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reduce symbol overlap when zooming out quickly #8628

reduce symbol overlap when zooming out quickly #8628

ansis commented Aug 13, 2019

asheemmamoowala left a comment

asheemmamoowala Aug 14, 2019

asheemmamoowala Aug 14, 2019

vakila Aug 19, 2019

chloekraw Aug 23, 2019

ansis Sep 25, 2019

chloekraw commented Aug 14, 2019

ansis commented Aug 14, 2019

asheemmamoowala commented Aug 14, 2019

vakila left a comment

vakila Aug 19, 2019

vakila Aug 19, 2019

ansis Sep 25, 2019

awulkan commented Aug 22, 2019 •

edited

Loading

ansis commented Sep 25, 2019

asheemmamoowala left a comment

reduce symbol overlap when zooming out quickly #8628

reduce symbol overlap when zooming out quickly #8628

Conversation

ansis commented Aug 13, 2019

Launch Checklist

asheemmamoowala 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chloekraw commented Aug 14, 2019

ansis commented Aug 14, 2019

asheemmamoowala commented Aug 14, 2019

vakila 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

awulkan commented Aug 22, 2019 • edited Loading

ansis commented Sep 25, 2019

asheemmamoowala left a comment

Choose a reason for hiding this comment

awulkan commented Aug 22, 2019 •

edited

Loading