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

Animations of circle layer extremely sluggish in version 1.30 #8701

Closed
thunderkid opened this issue Aug 28, 2019 · 23 comments · Fixed by #8913
Closed

Animations of circle layer extremely sluggish in version 1.30 #8701

thunderkid opened this issue Aug 28, 2019 · 23 comments · Fixed by #8913
Assignees
Labels
needs information 🙏 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@thunderkid
Copy link

I am animating the appearance and disappearance of ~2000 circles over 10 seconds using a data driven circle-layer. In version 1.2.1, this was running fine at ~35fps, with the circles updating continuously. In version 1.3.0 the circles no longer update continuously - they update in batches only about once every 2 seconds, giving a very sluggish/jerky experience. (NB: If you just look at the fps, it's now reporting ~50fps, so by that metric it's faster than before, but that's because it's no longer doing the hard work of actually updating the circles. As soon as you look at the map as opposed to the fps meter, you see how bad the performance is now.)

mapbox-gl-js version: 1.3.0

browser: Chrome 76

Steps to Trigger Behavior

  1. Add a data-driven circle layer.
  2. Animate the underlying data so that, say, 2000 circles appear randomly with a lifetime of 1 second each, over a 10 second interval.
  3. Watch how extremely sluggish it is compared to MapboxGL version 1.2.1.

Expected Behavior

Animation at least as smooth as version 1.2.1.

Actual Behavior

Animation dramatically less smooth than version 1.2.1.

@mourner mourner added performance ⚡ Speed, stability, CPU usage, memory usage, or power usage release blocker ⛔ needs information 🙏 labels Aug 28, 2019
@mourner
Copy link
Member

mourner commented Aug 28, 2019

@thunderkid oh no! This is likely because of changes in #8633 and #8673. Can you provide a minimal live (e.g. JSFiddle) test case? That would make it much easier to get to the bottom of this and do a quick fix.

@asheemmamoowala asheemmamoowala added this to the release-queso milestone Aug 29, 2019
@arindam1993 arindam1993 self-assigned this Aug 30, 2019
@arindam1993
Copy link
Contributor

arindam1993 commented Aug 31, 2019

@thunderkid I tried to create a reproducible example based on your description. Here are the fiddles:
1.2.1: https://jsfiddle.net/5sg4b3co/
1.3.0: https://jsfiddle.net/sd7rk0hu/
Based on your description, my hunch was that you may be calling map.setData every time you add or remove a point, and in that case 1.3.0 does cause a regression, since it floods queue in the worker.

However, the general best-practice when animating is to call map.setData in a requestAnimationFrame loop, which throttles the map updates to the same rate as which the browser's frame rate. You can still update your data at any frequency you want.
Demo fiddle with workaround:
https://jsfiddle.net/sd7rk0hu/2/

@mourner @asheemmamoowala , not sure if this is a release-blocker, given it might be a bad implementation.

@thunderkid
Copy link
Author

@arindam1993
No, I'm not calling map.setData for every point added - my animation loop uses requestAnimationFrame and it's pretty robust. I've double checked this by logging every call of setData, and it's only being called at the browser frame rate.

I'm afraid I can't get your fiddles to tell me much - the last one doesn't load at all.

My animation is a bit more complex than yours. For each event I add a circle with text in the center, and over ~1 second it expands by a factor of ~1.5 and simultaneously fades, then disappears. It looks nice and smooth on v1.2.1, but not on 1.3.0.

It's a fairly complex project so hard for me to produce a small fiddle for it. I was hoping someone else would have something smaller and cleaner for stress testing. I could probably produce a couple of animated gifs to show you what it looks like, but the degradation is pretty bad!

@asheemmamoowala
Copy link
Contributor

@thunderkid animated gifs would be a good start.

Can you say more about how many circles are in your source, and how frequently the events occur that add circles? How many layers are referring to the source, and what is the complexity of the expressions in the layer properties?

@thunderkid
Copy link
Author

@asheemmamoowala
So here are a couple of gifs to give you an idea of the degradation:
Here it is with MapboxGL 1.2.1:
SmallQ1000MapboxGL1 2 1
And here it is with MapboxGL 1.3.0:
SmallQ1000MapboxGL1 3 0
(I noticed that the latter gif is < half the size of the former, which is a striking measure of the information loss!)

In this example I'm adding 1000 circles over a 10 second interval. Each circle expands for 0.5 seconds, then fades for 0.4 seconds. So on average around 100 circles are active at any moment. There are two layers referring to the data source - a text layer which doesn't do much:

    'type': 'symbol',
    'layout': {
        'text-field': '{qcval}',
        'text-size': 11,
    },

and a circle layer which gets the radius and opacity which are dynamically driven by the animation of the data source, updated by requestAnimationFrame. (It also gets the color, which in this example is always just red):

    'type': 'circle',
    'paint': {
        'circle-radius': ['get', 'rad'],
        'circle-color':  ['get', 'color'],
        'circle-opacity': ['get', 'opacity'],
        }
    }

Hope that helps.

@kkaefer
Copy link
Member

kkaefer commented Sep 2, 2019

I've created a similar fiddle based on earthquake data:

I've tried this in Firefox 70, Chrome 76, and Safari 12, all on 4-core Macbook 15" running macOS, but was unable to spot any differences. They all get sluggish after a few seconds, but I can't tell that 1.2.1 is less sluggish than 1.3.0. @thunderkid, are you able to share a reduced test case with actual code (like a jsfiddle test page) that exhibits what you're observing?

@thunderkid
Copy link
Author

@kkaefer: Thanks! That's a nice clean example and it runs fine on my systems too under 1.3.0. My own code uses React & Mobx and has other non-animating layers so it will take a bit of work to figure out what's interfering with the animation, but I'll try to build a simple test case.

@sunaviation
Copy link

I fully agree with @thunderkid that 1.2.1 is less sluggish than 1.3.0

Environment

MacOS Catalina 10.15 Beta

browsers:
Chrome 76.0.3809.132
Safari 13.0 (15608.2.5)

mapbox-gl-js version:
1.3.0 compared with 1.2.1

latest turf version:
5.1.6

jQuery:
3.4.1

Steps to Trigger Behavior

parse large geojson files

$.getJSON('assets/geos/airports.geojson', function (data) {     // 52.875 objects
$.getJSON('assets/geos/navaids.geojson', function (data) {      // 11.118 objects

points = geoJSONAdNa;

while moving the mouse @turf/nearest-point is using: takes a reference point and a FeatureCollection of Features with Point geometries and returns the point from the FeatureCollection closest to the reference.
Once the nearesat point has been found, parse the array of coordinates and text, draw a circle on the parsed coord position, also draw a line between nearest position and the mouse pointer.

    targetPoint = turf.point([e.lngLat.lng, e.lngLat.lat]);
    points = geoJSONAdNa;
    nearest = turf.nearestPoint(targetPoint, points);
    map.getSource("ancorcoord").setData(nearest);
    map.getSource("ancorcoorddest").setData(nearest);
    snappedcurrent = [nearest.geometry.coordinates,targetPoint.geometry.coordinates];
    map.getSource("snappedlinecurrent").setData(snappedLinecurrent());

Actual Behavior

I've created two versions based on my project on clpa.tunayschuster.com.
same code, only mapboxGL version replaced 1.2.1 <-> 1.3.0

1.2.1: https://clpa.tunayschuster.com/performance-test-mapboxgl_1.2.1.html
1.3.0: https://clpa.tunayschuster.com/performance-test-mapboxgl_1.3.0.html

Behavior less smooth than version 1.2.1.

@kkaefer
Copy link
Member

kkaefer commented Sep 7, 2019

@sunaviation thank you so much for this great example. I can finally reproduce it locally with this example.

@mmuelder
Copy link

mmuelder commented Sep 13, 2019

I can confirm this issue.
We are animating a circle (layer) by using setData with only one data point every 10/20/30 ms (makes no difference). Currently we're on v 1.0.0 and its super smooth. Now I tried to move up to v 1.3.0 and the performance is very poor. On a pc you can barely see the difference but on any mobile device (even newer iphones or a nokia 8.1) I tested, the performance was horrible. No matter if I use setInterval or requestAnimationFrame, it's always super laggy and updates the layer only every other second, if at all. You cannot call it an animation anymore. So right now we cannot update to v 1.3.0 :(

@mourner
Copy link
Member

mourner commented Sep 13, 2019

@mmuelder would you help us by sharing a minimal reproducible live test case? This turned out to be quite a difficult issue to crack, so the more test cases we have, the sooner we will find a solution.

@mmuelder
Copy link

mmuelder commented Sep 13, 2019

@mourner it was pretty hard but it looks like I found a test case which replicates our problem:

https://jsfiddle.net/L0omq42a/11/ <- mapbox gl js v1.3.0
https://jsfiddle.net/L0omq42a/12/ <- mapbox gl js v1.0.0

If I open the 1.3.0 version, the circle lags behind the camera view so far, that its basically out of view all the time.
If I open the 1.0.0 version, the circle moves so fast that it is always visible.

(On a Nokia 8.1. Again, on a desktop PC the difference is barely noticeable)

I also found that it was apparently dependent of the map positon/zoom level. The more details/buildings/roads are visible and have to be rendered, the bigger the difference between the two versions was.

@msbarry
Copy link
Contributor

msbarry commented Oct 25, 2019

I'm seeing this problem as well for animating a simple line layer. The map needs to be at least a few tiles to start seeing the problem.

https://jsfiddle.net/erhpzwaj/ <- mapbox gl js 1.2.1 (works fine)
https://jsfiddle.net/b9hp0jv1/ <- mapbox gl js 1.3.0 (janky)

This is a blocker for my use-case to upgrade to 1.3.0 or later.

@sunaviation
Copy link

I can confirm it. The performance issues are not only based on the circle layers. I guess it depends to setData. While moving the mouse instance member .getSource .setData (data) is called permanently.

map.on('mousemove', function (e) {
[..]
    map.getSource("snappedlinecurrent").setData(snappedLinecurrent());

In the current examples only layer line is used. Draw a line between the nearest lat/lng and the mouse pointer.

1.2.1: https://clpa.tunayschuster.com/performance-test-mapboxgl_1.2.1.html <- smooth
1.3.0: https://clpa.tunayschuster.com/performance-test-mapboxgl_1.3.0.html <- sluggish

@thunderkid
Copy link
Author

I've just checked against today's release of mapboxgl 1.5.0. Unfortunately this release is still very jerky.

@msbarry
Copy link
Contributor

msbarry commented Oct 26, 2019

I think the issue is with responses getting queued up in the main thread. If I change the logic in new actor implementation to run pending callbacks synchronously, the main thread will process ~20 response messages in a single animation frame, whereas when they get deferred with MessageChannel they get spread out across ~20 frames, including letting renders slip in between them, further spreading out the responses and leading to partial renders where one tile shows new data and another shows old data.

Do responses need to get queued in the main thread? Do workers ever tell main thread to cancel? If not it might be sufficient to only queue them up in the workers.

@msbarry msbarry mentioned this issue Oct 27, 2019
4 tasks
@msbarry
Copy link
Contributor

msbarry commented Nov 1, 2019

I have a potential fix in #8913 which seems to resolve the issue on my setup - could anyone who has been seeing this issue let me know if this fixes it for them: https://cdn.jsdelivr.net/gh/msbarry/mapbox-gl-js@build-fix-8701/dist/mapbox-gl.js

@sunaviation
Copy link

sunaviation commented Nov 1, 2019

You did it! - it's fixed. Everything is running smoothly as before (like mapbox gl js 1.2.1)

https://clpa.tunayschuster.com/performance-test-mapboxgl_1.2.1.html <- 1.2.1 (works fine)
https://clpa.tunayschuster.com/performance-test-mapboxgl_1.3.0.html <- 1.3.0 (janky)
https://clpa.tunayschuster.com/performance-test-mapboxgl_1.6.0-dev.html <- 1.6.0-dev (fixed !)

Thanks a lot!

edit:
on Safari Version 13.0.3 (15608.3.10.1.4) it is still janky
but on Google Chrome Version 78.0.3904.87 (Official Build) (64-bit) it works smoothly again.

environment:
MacBook Pro (15-inch, 2016), Catalina 10.15.1

@thunderkid
Copy link
Author

thunderkid commented Nov 1, 2019

@msbarry Is your fix available on npm as a @nextnext or something? My typescript imports don't like the cdn. (And @next on npm still resolves to 1.5.0-beta.1)

@msbarry
Copy link
Contributor

msbarry commented Nov 1, 2019

@thunderkid you should be able to temporarily grab the mapbox-gl dependency from https://github.com/msbarry/mapbox-gl-js.git#build-fix-8701 to test this out.

@thunderkid
Copy link
Author

@msbarry Yes, the sluggishness now seems to be fixed. (On chrome at least. I haven't tested safari.)

@kkaefer
Copy link
Member

kkaefer commented Nov 6, 2019

@sunaviation: I can confirm that this mostly fixes the issue on Chrome. Both Safari and Firefox don't seem to be affected by this issue because the 1.2.1 version is already very janky and I couldn't observe a noticeable difference between 1.2.1, 1.3+, and @msbarry's patch.

@mmuelder
Copy link

mmuelder commented Nov 6, 2019

@msbarry our example seems to be fixed as well, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs information 🙏 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants